Changeset 40020 in vbox for trunk/src/VBox/Main/src-client

Timestamp:

Feb 7, 2012 4:02:30 PM (13 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

76142

Message:

GuestCtrl/Main: Fixed some locking issues + callback cleanup hangs; fixed a memory leak on callback creation.

Location:

trunk/src/VBox/Main/src-client

Files:

• GuestCtrlImpl.cpp (modified) (7 diffs)
• GuestImpl.cpp (modified) (2 diffs)

trunk/src/VBox/Main/src-client/GuestCtrlImpl.cpp

@@ -116 +116 @@
     /* puContextID is optional. */
 
-    int rc;
+    int rc = VERR_NOT_FOUND;
+
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
     /* Create a new context ID and assign it. */
@@ -139 +141 @@
     if (RT_SUCCESS(rc))
     {
-        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
         /* Add callback with new context ID to our callback map. */
         mCallbackMap[uNewContextID] = *pCallback;
@@ -154 +154 @@
 
 /**
+ * Destroys the formerly allocated callback data. The callback then
+ * needs to get removed from the callback map via callbackRemove().
  * Does not do locking!
  *
@@ -173 +175 @@
             it->second.cbData = 0;
         }
-
-        /* Remove callback context (not used anymore). */
-        mCallbackMap.erase(it);
-    }
+    }
+}
+
+/**
+ * Removes a callback from the callback map.
+ * Does not do locking!
+ *
+ * @param   uContextID
+ */
+void Guest::callbackRemove(uint32_t uContextID)
+{
+    callbackDestroy(uContextID);
+
+    mCallbackMap.erase(uContextID);
 }
 
@@ -1708 +1720 @@
                     vrc = callbackInit(&callback, VBOXGUESTCTRLCALLBACKTYPE_EXEC_START, pProgress);
                     if (RT_SUCCESS(vrc))
-                    {
-                        /* Allocate and assign payload. */
-                        callback.cbData = sizeof(CALLBACKDATAEXECSTATUS);
-                        PCALLBACKDATAEXECSTATUS pData = (PCALLBACKDATAEXECSTATUS)RTMemAlloc(callback.cbData);
-                        AssertReturn(pData, E_OUTOFMEMORY);
-                        RT_BZERO(pData, callback.cbData);
-                        callback.pvData = pData;
-                    }
-
-                    if (RT_SUCCESS(vrc))
                         vrc = callbackAdd(&callback, &uContextID);
 
@@ -2012 +2014 @@
             }
 
-            /* The callback isn't needed anymore -- just was kept locally. */
-            callbackDestroy(uContextID);
+            {
+                AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+                /* The callback isn't needed anymore -- just was kept locally. */
+                callbackRemove(uContextID);
+            }
 
             /* Cleanup. */
@@ -2207 +2213 @@
                 rc = handleErrorHGCM(vrc);
 
-            /* The callback isn't needed anymore -- just was kept locally. */
-            callbackDestroy(uContextID);
+            {
+                AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+                /* The callback isn't needed anymore -- just was kept locally. */
+                callbackRemove(uContextID);
+            }
 
             /* Cleanup. */

trunk/src/VBox/Main/src-client/GuestImpl.cpp

@@ -118 +118 @@
     LogFlowThisFunc(("\n"));
 
+    /* Enclose the state transition Ready->InUninit->NotReady */
+    AutoUninitSpan autoUninitSpan(this);
+    if (autoUninitSpan.uninitDone())
+        return;
+
 #ifdef VBOX_WITH_GUEST_CONTROL
-    /* r=poetzsch: Not sure if this is really right. Please note that
-     * IGuest::uninit is called twice (which I also consider a bug). So the
-     * test for uninitDone should be always first! */
     /* Scope write lock as much as possible. */
     {
@@ -144 +146 @@
             callbackDestroy(it->first);
 
-        /* Clear process map. */
+        /* Clear process map (remove all callbacks). */
         mGuestProcessMap.clear();
     }
 #endif
-
-    /* Enclose the state transition Ready->InUninit->NotReady */
-    AutoUninitSpan autoUninitSpan(this);
-    if (autoUninitSpan.uninitDone())
-        return;
 
 #ifdef VBOX_WITH_DRAG_AND_DROP