Changeset 2499 in vbox

Timestamp:

May 4, 2007 4:49:18 PM (18 years ago)

Author:

vboxsync

Message:

XPCOM/IPC/DConnect: Attempted to fix a race condition when deleting instance wrappers introduced by changeset:20849.

Location:

trunk/src/libs/xpcom18a4/ipc/ipcd/extensions/dconnect/src

Files:

• ipcDConnectService.cpp (modified) (8 diffs)
• ipcDConnectService.h (modified) (1 diff)

trunk/src/libs/xpcom18a4/ipc/ipcd/extensions/dconnect/src/ipcDConnectService.cpp

@@ -239 +239 @@
       nsRefPtr <ipcDConnectService> dConnect (ipcDConnectService::GetInstance());
       if (dConnect)
-          dConnect->DeleteInstance(this);
+        dConnect->DeleteInstance(this);
       else
-          NS_NOTREACHED("ipcDConnectService has gone before DConnectInstance");
+        NS_NOTREACHED("ipcDConnectService has gone before DConnectInstance");
       delete this;
       return 0;
@@ -2223 +2223 @@
   // disregarding the reference counter
     
+#ifdef IPC_LOGGING
+  const char *name;
+  aData->InterfaceInfo()->GetNameShared(&name);
+  LOG(("ipcDConnectService: WARNING: deleting unreleased "
+       "instance=%p iface=%p {%s}\n", aData, aData->RealInstance(), name));
+#endif
+
   delete aData;
   return PL_DHASH_NEXT;
@@ -2424 +2431 @@
 
 void
-ipcDConnectService::DeleteInstance(DConnectInstance *wrapper)
-{
+ipcDConnectService::DeleteInstance(DConnectInstance *wrapper,
+                                   PRBool locked /* = PR_FALSE */)
+{
+  if (!locked)
+    PR_Lock(mLock);
+
 #ifdef IPC_LOGGING
   const char *name;
@@ -2433 +2444 @@
 #endif
 
-  {
-    nsAutoLock lock (mLock);
-
-    mInstances.Remove(wrapper->GetKey());
-    mInstanceSet.Remove(wrapper);
-  }
+  mInstances.Remove(wrapper->GetKey());
+  mInstanceSet.Remove(wrapper);
+
+  if (!locked)
+    PR_Unlock(mLock);
 }
 
@@ -2648 +2658 @@
 struct PruneInstanceMapForPeerArgs
 {
+  ipcDConnectService *that;
   PRUint32 clientID;
-  DConnectInstanceMap &instances;
-  DConnectInstanceSet &instanceSet;
+  nsVoidArray &wrappers;
 };
 
@@ -2663 +2673 @@
   if (args && args->clientID == aData->Peer())
   {
-#ifdef IPC_LOGGING
-    const char *name;
-    aData->InterfaceInfo()->GetNameShared(&name);
-    LOG(("PruneInstanceMapForClient(): instance=%p iface=%p {%s} peer=%d\n",
-         aData, aData->RealInstance(), name, args->clientID));
-#endif
-    args->instances.Remove(aKey);
-    args->instanceSet.Remove(aData);
-    /* ignore the reference counter: the client is officially dead */
-    delete aData;
+    // ignore the reference counter: the client is officially dead
+    args->that->DeleteInstance(aData, PR_TRUE /* locked */);
+    // collect the instance for future destruction
+    if (!args->wrappers.AppendElement(aData))
+    {
+      NS_NOTREACHED("Not enough memory");
+      // bad but what to do
+      delete aData;
+    }
   }
   return PL_DHASH_NEXT;
@@ -2697 +2706 @@
            "pruning all instances created for peer %d...\n", aClientID));
 
-      nsAutoLock lock (mLock);
-
-      // make sure we have released all instances
-      PruneInstanceMapForPeerArgs args = { aClientID, mInstances, mInstanceSet };
-      mInstances.EnumerateRead(PruneInstanceMapForPeer, (void *)&args);
+      nsVoidArray wrappers;
+
+      {
+        nsAutoLock lock (mLock);
+
+        // make sure we have removed all instances from instance maps
+        PruneInstanceMapForPeerArgs args = { this, aClientID, wrappers };
+        mInstances.EnumerateRead(PruneInstanceMapForPeer, (void *)&args);
+      }
+
+      // destruct all instances outside the lock because it will release
+      // the real objects which may need to make asynchronous use our service
+      for (PRInt32 i = 0; i < wrappers.Count(); ++i)
+        delete ((DConnectInstance *) wrappers[i]);
     }
   }
@@ -2921 +2939 @@
        peer, release->instance));
 
+  DConnectInstance *wrapper = release->instance;
+
+  nsAutoLock lock (mLock);
+
   // make sure we've been sent a valid wrapper
-  if (CheckInstanceAndAddRef(release->instance))
-  {
-    // release twice: once to balance CheckInstanceAndAddRef() and once
-    // to satisfy the RELEASE request (the instance is no more needed by
-    // this peer)
-    release->instance->Release();
-    release->instance->Release();
+  if (mInstanceSet.Contains(wrapper))
+  {
+    nsrefcnt count = wrapper->AddRef();
+    count = wrapper->Release();
+    if (count == 1)
+    {
+      // we are the last one who holds a (fake) reference, remove the
+      // instace from instance maps while still under the lock
+      DeleteInstance(wrapper, PR_TRUE /* locked */);
+      // leave the lock before calling the destructor because it will release
+      // the real object which may need to make asynchronous use our service
+      lock.unlock();
+      delete wrapper;
+    }
   }
   else
   {
-    NS_NOTREACHED("instance wrapper not found");
+    // it is possible that the client disconnection event handler has released
+    // all client instances before the RELEASE message sent by the client gets
+    // processed here. Just give a warning
+    LOG(("ipcDConnectService::OnRelease: WARNING: instance wrapper not found"));
   }
 }

trunk/src/libs/xpcom18a4/ipc/ipcd/extensions/dconnect/src/ipcDConnectService.h

@@ -233 +233 @@
 
   // public only for DConnectInstance::Release()
-  NS_HIDDEN_(void)     DeleteInstance(DConnectInstance *);
+  NS_HIDDEN_(void)     DeleteInstance(DConnectInstance *, PRBool locked = PR_FALSE);
   // public only for DConnectStub::CallMethod()
   NS_HIDDEN_(PRBool)   CheckInstanceAndAddRef(DConnectInstance *);