Changeset 4863 in vbox for trunk/src/libs/xpcom18a4/ipc/ipcd/extensions

Timestamp:

Sep 17, 2007 6:05:36 PM (17 years ago)

Author:

vboxsync

Message:

XPCOM/IPC/DConnect: Lock the whole Release() method of stub objects to provide atomicity of releasing the last stub reference and removing this stub from the set of existing stubs (to avoid misuse of stubs by other threads just before they are deleted which may happen on SMP machines).

Location:

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

Files:

• ipcDConnectService.cpp (modified) (13 diffs)
• ipcDConnectService.h (modified) (4 diffs)

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

@@ -991 +991 @@
   DConnectStub *mCachedISupports;
   
-  // stack of reference counter values returned by AddRef made in CreateStub
-  // (access must be protected using the ipcDConnectService::stubLock())
+  // stack of reference counter values (protected by
+  // ipcDConnectService::StubLock())
   nsDeque mRefCntLevels;
 };
@@ -1005 +1005 @@
   // time it passes an object over IPC.
 
+  // NOTE: this function is to be called from DConnectInstance::CreateStub only!
+
   nsRefPtr <ipcDConnectService> dConnect (ipcDConnectService::GetInstance());
   NS_ASSERTION(dConnect, "no ipcDConnectService (uninitialized?)");
   if (!dConnect)
     return 0;
-  
-  nsAutoLock stubLock (dConnect->StubLock()); 
-  
+
+  // dConnect->StubLock() must be already locked here by
+  // DConnectInstance::CreateStub
+
   nsrefcnt count = AddRef();
   mRefCntLevels.Push((void *) count);
@@ -1031 +1034 @@
 
   if (mDisconnected)
-      return NS_ERROR_NOT_INITIALIZED;
+    return NS_ERROR_NOT_INITIALIZED;
+
+  // we also need the stub lock which protects DConnectStub::mRefCntLevels and
+  // ipcDConnectService::mStubs
+  nsAutoLock stubLock (mStubLock);
 
   DConnectStub *stub = nsnull;
@@ -1624 +1631 @@
 #endif    
     
-  nsRefPtr <ipcDConnectService> dConnect (ipcDConnectService::GetInstance());
-  if (dConnect)
-  {
-#ifdef NS_DEBUG
-    {
-      nsAutoLock stubLock (dConnect->StubLock());
-      NS_ASSERTION(mRefCntLevels.GetSize() == 0, "refcnt levels are still left");
-    }
-#endif    
-    dConnect->DeleteStub(this);
-  }
-
   // release the cached nsISupports instance if it's not the same object
   if (mCachedISupports != 0 && mCachedISupports != this)
@@ -1653 +1648 @@
 DConnectStub::Release()
 {
-  nsrefcnt count = PR_AtomicDecrement((PRInt32 *)&mRefCnt);
-  NS_LOG_RELEASE(this, count, "DConnectStub");
+  nsrefcnt count;
 
   nsRefPtr <ipcDConnectService> dConnect (ipcDConnectService::GetInstance());
   if (dConnect)
   {
+    // lock the stub lock on every release to make sure that once the counter
+    // drops to zero, we delete the stub from the set of stubs before a new
+    // request to create a stub on other thread tries to find the existing
+    // stub in the set (wchich could otherwise AddRef the object after it had
+    // Released to zero and pass it to the client right before its
+    // destruction).
     nsAutoLock stubLock (dConnect->StubLock());
-      
+
+    count = PR_AtomicDecrement((PRInt32 *)&mRefCnt);
+    NS_LOG_RELEASE(this, count, "DConnectStub");
+
     // mRefCntLevels may already be empty here (due to the "stabilize" trick below)
     if (mRefCntLevels.GetSize() > 0)
@@ -1675 +1678 @@
         mRefCntLevels.Pop();
 
+        if (0 == count)
+        {
+          // this is the last reference, remove from the set before we leave
+          // the lock, to provide atomicity of these two operations
+          dConnect->DeleteStub (this);
+
+          NS_ASSERTION(mRefCntLevels.GetSize() == 0, "refcnt levels are still left");
+        }
+
+        // leave the lock before sending a message
         stubLock.unlock();
       
@@ -1692 +1705 @@
       }
     }
+  }
+  else
+  {
+    count = PR_AtomicDecrement((PRInt32 *)&mRefCnt);
+    NS_LOG_RELEASE(this, count, "DConnectStub");
   }
   
@@ -2322 +2340 @@
 ipcDConnectService::ipcDConnectService()
  : mLock(NULL)
+ , mStubLock(NULL)
  , mDisconnected(PR_TRUE)
- , mStubLock(NULL)
  , mStubQILock(NULL)
 {
@@ -2383 +2401 @@
     return NS_ERROR_OUT_OF_MEMORY;
 
+  mStubLock = PR_NewLock();
+  if (!mStubLock)
+    return NS_ERROR_OUT_OF_MEMORY;
+
   if (!mStubs.Init())
     return NS_ERROR_OUT_OF_MEMORY;
@@ -2389 +2411 @@
   if (NS_FAILED(rv))
     return rv;
-
-  mStubLock = PR_NewLock();
-  if (!mStubLock)
-    return NS_ERROR_OUT_OF_MEMORY;
 
   mStubQILock = PR_NewLock();
@@ -2621 +2639 @@
 #endif
 
-  nsAutoLock lock (mLock);
-
-  // this method is intended to be called only from DConnectStub destructor.
+  // this method is intended to be called only from DConnectStub::Release().
   // the stub object is not deleted when removed from the table, because
   // DConnectStub pointers are not owned by mStubs.
@@ -2629 +2645 @@
 }
 
+// not currently used
+#if 0
 PRBool
 ipcDConnectService::FindStubAndAddRef(PRUint32 peer, const DConAddr instance,
                                       DConnectStub **stub)
 {
-  nsAutoLock lock (mLock);
+  nsAutoLock stubLock (mStubLock);
 
   PRBool result = mStubs.Get(DConnectStubKey::Key(peer, instance), stub);
@@ -2640 +2658 @@
   return result;
 }
+#endif
 
 NS_IMPL_THREADSAFE_ISUPPORTS3(ipcDConnectService, ipcIDConnectService,

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

@@ -228 +228 @@
 
   NS_HIDDEN_(nsresult) CreateStub(const nsID &, PRUint32, DConAddr, DConnectStub **);
+#if 0
   NS_HIDDEN_(PRBool)   FindStubAndAddRef(PRUint32, const DConAddr, DConnectStub **);
+#endif
   // public only for DConnectStub::~DConnectStub()
   NS_HIDDEN_(void)     DeleteStub(DConnectStub *);
@@ -269 +271 @@
   nsCOMPtr<nsIInterfaceInfoManager> mIIM;
 
-  // lock to protect access to instance and stub sets and the disconnected flag
+  // lock to protect access to instance sets and the disconnected flag
   PRLock *mLock;
 
@@ -279 +281 @@
   DConnectInstanceSet mInstanceSet;
 
+  // lock to protect access to mStubs and DConnectStub::mRefCntLevels
+  // (also guards every DConnectStub::Release call to provide atomicity)
+  PRLock *mStubLock;
+
   // table of remote object stubs allocated to communicate with peer's instances
   DConnectStubMap mStubs;
@@ -288 +294 @@
   PRUint32 mSelfID;
   
-  // global lock to protect access to DConnectStub internal data
-  PRLock *mStubLock;
-
   // global lock to protect access to protect DConnectStub::QueryInterface()
   // (we cannot use mStubLock because it isn't supposed to be held long,