Bug 350132 - Deadlock in JS/XPCOM proxy, r=brendan
authorbenjamin@smedbergs.us
Thu, 16 Aug 2007 13:51:50 -0700
changeset 4730 a517616ec4853774cb7c84a0e14fcdfede8ef418
parent 4729 49318d49f0c142537fe4b8c8f69c264bf6c25ad8
child 4731 56a3bd89c42926a5bf81ec2d7127acad9c945785
push idunknown
push userunknown
push dateunknown
reviewersbrendan
bugs350132
milestone1.9a8pre
Bug 350132 - Deadlock in JS/XPCOM proxy, r=brendan
xpcom/glue/nsAutoLock.h
xpcom/proxy/src/nsProxyEvent.cpp
xpcom/proxy/src/nsProxyEventObject.cpp
xpcom/proxy/src/nsProxyEventPrivate.h
xpcom/proxy/src/nsProxyObjectManager.cpp
--- a/xpcom/glue/nsAutoLock.h
+++ b/xpcom/glue/nsAutoLock.h
@@ -245,16 +245,34 @@ public:
      void unlock() {
         PR_ASSERT(mLocked);
         PR_Unlock(mLock);
         mLocked = PR_FALSE;
         Hide();
     }
 };
 
+class nsAutoUnlock : nsAutoUnlockBase
+{
+private:
+    PRLock *mLock;
+     
+public:
+    nsAutoUnlock(PRLock *lock) : 
+        nsAutoUnlockBase(lock),
+        mLock(lock)
+    {
+        PR_Unlock(mLock);
+    }
+
+    ~nsAutoUnlock() {
+        PR_Lock(mLock);
+    }
+};
+
 #include "prcmon.h"
 #include "nsError.h"
 #include "nsDebug.h"
 
 class NS_COM_GLUE nsAutoMonitor : public nsAutoLockBase {
 public:
 
     /**
--- a/xpcom/proxy/src/nsProxyEvent.cpp
+++ b/xpcom/proxy/src/nsProxyEvent.cpp
@@ -337,50 +337,75 @@ nsProxyObject::nsProxyObject(nsIEventTar
                              nsISupports *realObject) :
   mProxyType(proxyType),
   mTarget(target),
   mRealObject(realObject),
   mFirst(nsnull)
 {
     MOZ_COUNT_CTOR(nsProxyObject);
 
+#ifdef DEBUG
+    nsCOMPtr<nsISupports> canonicalTarget = do_QueryInterface(target);
+    NS_ASSERTION(target == canonicalTarget,
+                 "Non-canonical nsISupports passed to nsProxyObject constructor");
+#endif
+
     nsProxyObjectManager *pom = nsProxyObjectManager::GetInstance();
     NS_ASSERTION(pom, "Creating a proxy without a global proxy-object-manager.");
     pom->AddRef();
 }
 
 nsProxyObject::~nsProxyObject()
 {
     // Proxy the release of mRealObject to protect against it being deleted on
     // the wrong thread.
     nsISupports *doomed = nsnull;
     mRealObject.swap(doomed);
     NS_ProxyRelease(mTarget, doomed);
 
     MOZ_COUNT_DTOR(nsProxyObject);
 }
 
-NS_IMPL_THREADSAFE_ADDREF(nsProxyObject)
+NS_IMETHODIMP_(nsrefcnt)
+nsProxyObject::AddRef()
+{
+    nsAutoLock lock(nsProxyObjectManager::GetInstance()->GetLock());
+    return LockedAddRef();
+}
 
 NS_IMETHODIMP_(nsrefcnt)
 nsProxyObject::Release()
 {
-    nsrefcnt count;
+    nsAutoLock lock(nsProxyObjectManager::GetInstance()->GetLock());
+    return LockedRelease();
+}
+
+nsrefcnt
+nsProxyObject::LockedAddRef()
+{
+    ++mRefCnt;
+    NS_LOG_ADDREF(this, mRefCnt, "nsProxyObject", sizeof(nsProxyObject));
+    return mRefCnt;
+}
+
+nsrefcnt
+nsProxyObject::LockedRelease()
+{
     NS_PRECONDITION(0 != mRefCnt, "dup release");
-    count = PR_AtomicDecrement((PRInt32*) &mRefCnt);
-    NS_LOG_RELEASE(this, count, "nsProxyObject");
-    if (count)
-        return count;
+    --mRefCnt;
+    NS_LOG_RELEASE(this, mRefCnt, "nsProxyObject");
+    if (mRefCnt)
+        return mRefCnt;
 
     nsProxyObjectManager *pom = nsProxyObjectManager::GetInstance();
-    NS_ASSERTION(pom, "Deleting a proxy without a global proxy-object-manager.");
+    pom->LockedRemove(this);
 
-    pom->Remove(this);
+    nsAutoUnlock unlock(pom->GetLock());
+    delete this;
     pom->Release();
-    delete this;
 
     return 0;
 }
 
 NS_IMETHODIMP
 nsProxyObject::QueryInterface(REFNSIID aIID, void **aResult)
 {
     if (aIID.Equals(GetIID())) {
@@ -393,72 +418,90 @@ nsProxyObject::QueryInterface(REFNSIID a
         *aResult = static_cast<nsISupports*>(this);
         AddRef();
         return NS_OK;
     }
 
     nsProxyObjectManager *pom = nsProxyObjectManager::GetInstance();
     NS_ASSERTION(pom, "Deleting a proxy without a global proxy-object-manager.");
 
-    nsAutoMonitor mon(pom->GetMonitor());
-
+    nsAutoLock lock(pom->GetLock());
     return LockedFind(aIID, aResult);
 }
 
 nsresult
 nsProxyObject::LockedFind(REFNSIID aIID, void **aResult)
 {
-    // This method is only called when the global monitor is held.
+    // This method is only called when the global lock is held.
     // XXX assert this
 
     nsProxyEventObject *peo;
 
     for (peo = mFirst; peo; peo = peo->mNext) {
         if (peo->GetClass()->GetProxiedIID().Equals(aIID)) {
             *aResult = static_cast<nsISupports*>(peo->mXPTCStub);
-            peo->AddRef();
+            peo->LockedAddRef();
             return NS_OK;
         }
     }
 
-    nsProxyEventClass *pec;
-    nsresult rv = nsProxyObjectManager::GetInstance()->GetClass(aIID, &pec);
-    if (NS_FAILED(rv))
-        return rv;
+    nsProxyEventObject *newpeo;
+
+    // Both GetClass and QueryInterface call out to XPCOM, so we unlock for them
+    {
+        nsProxyObjectManager* pom = nsProxyObjectManager::GetInstance();
+        nsAutoUnlock unlock(pom->GetLock());
+
+        nsProxyEventClass *pec;
+        nsresult rv = pom->GetClass(aIID, &pec);
+        if (NS_FAILED(rv))
+            return rv;
 
-    nsISomeInterface* newInterface;
-    rv = mRealObject->QueryInterface(aIID, (void**) &newInterface);
-    if (NS_FAILED(rv))
-        return rv;
+        nsISomeInterface* newInterface;
+        rv = mRealObject->QueryInterface(aIID, (void**) &newInterface);
+        if (NS_FAILED(rv))
+            return rv;
 
-    peo = new nsProxyEventObject(this, pec, 
-                already_AddRefed<nsISomeInterface>(newInterface), &rv);
-    if (!peo) {
-        NS_RELEASE(newInterface);
-        return NS_ERROR_OUT_OF_MEMORY;
+        newpeo = new nsProxyEventObject(this, pec, 
+                       already_AddRefed<nsISomeInterface>(newInterface), &rv);
+        if (!newpeo) {
+            NS_RELEASE(newInterface);
+            return NS_ERROR_OUT_OF_MEMORY;
+        }
+
+        if (NS_FAILED(rv)) {
+            delete newpeo;
+            return rv;
+        }
     }
 
-    if (NS_FAILED(rv)) {
-        delete peo;
-        return rv;
+    // Now that we're locked again, check for races by repeating the
+    // linked-list check.
+    for (peo = mFirst; peo; peo = peo->mNext) {
+        if (peo->GetClass()->GetProxiedIID().Equals(aIID)) {
+            delete newpeo;
+            *aResult = static_cast<nsISupports*>(peo->mXPTCStub);
+            peo->LockedAddRef();
+            return NS_OK;
+        }
     }
 
-    peo->mNext = mFirst;
-    mFirst = peo;
+    newpeo->mNext = mFirst;
+    mFirst = newpeo;
 
-    NS_ADDREF(peo);
+    newpeo->LockedAddRef();
 
-    *aResult = static_cast<nsISupports*>(peo->mXPTCStub);
+    *aResult = static_cast<nsISupports*>(newpeo->mXPTCStub);
     return NS_OK;
 }
 
 void
 nsProxyObject::LockedRemove(nsProxyEventObject *peo)
 {
     nsProxyEventObject **i;
     for (i = &mFirst; *i; i = &((*i)->mNext)) {
         if (*i == peo) {
             *i = peo->mNext;
-            break;
+            return;
         }
     }
+    NS_ERROR("Didn't find nsProxyEventObject in nsProxyObject chain!");
 }
-
--- a/xpcom/proxy/src/nsProxyEventObject.cpp
+++ b/xpcom/proxy/src/nsProxyEventObject.cpp
@@ -62,55 +62,64 @@ nsProxyEventObject::nsProxyEventObject(n
       mRealInterface(aRealInterface),
       mNext(nsnull)
 {
     *rv = InitStub(aClass->GetProxiedIID());
 }
 
 nsProxyEventObject::~nsProxyEventObject()
 {
-    // This destructor is always called within the POM monitor.
+    // This destructor must *not* be called within the POM lock
     // XXX assert this!
 
-    mProxyObject->LockedRemove(this);
-
     // mRealInterface must be released before mProxyObject so that the last
     // release of the proxied object is proxied to the correct thread.
     // See bug 337492.
     mRealInterface = nsnull;
 }
 
 //
 // nsISupports implementation...
 //
 
-NS_IMPL_THREADSAFE_ADDREF(nsProxyEventObject)
+NS_IMETHODIMP_(nsrefcnt)
+nsProxyEventObject::AddRef()
+{
+    nsAutoLock lock(nsProxyObjectManager::GetInstance()->GetLock());
+    return LockedAddRef();
+}
+
+nsrefcnt
+nsProxyEventObject::LockedAddRef()
+{
+    ++mRefCnt;
+    NS_LOG_ADDREF(this, mRefCnt, "nsProxyEventObject", sizeof(nsProxyEventObject));
+    return mRefCnt;
+}
 
 NS_IMETHODIMP_(nsrefcnt)
 nsProxyEventObject::Release(void)
 {
-    nsAutoMonitor mon(nsProxyObjectManager::GetInstance()->GetMonitor());
+    {
+        nsAutoLock lock(nsProxyObjectManager::GetInstance()->GetLock());
+        NS_PRECONDITION(0 != mRefCnt, "dup release");
+
+        --mRefCnt;
+        NS_LOG_RELEASE(this, mRefCnt, "nsProxyEventObject");
 
-    nsrefcnt count;
-    NS_PRECONDITION(0 != mRefCnt, "dup release");
-    // Decrement atomically - in case the Proxy Object Manager has already
-    // been deleted and the monitor is unavailable...
-    count = PR_AtomicDecrement((PRInt32 *)&mRefCnt);
-    NS_LOG_RELEASE(this, count, "nsProxyEventObject");
-    if (0 == count) {
-        mRefCnt = 1; /* stabilize */
-        //
-        // Remove the proxy from the hashtable (if necessary) or its
-        // proxy chain.  This must be done inside of the proxy lock to
-        // prevent GetNewOrUsedProxy(...) from ressurecting it...
-        //
-        NS_DELETEXPCOM(this);
-        return 0;
+        if (mRefCnt)
+            return mRefCnt;
+
+        mProxyObject->LockedRemove(this);
     }
-    return count;
+
+    // call the destructor outside of the lock so that we aren't holding the
+    // lock when we release the object
+    NS_DELETEXPCOM(this);
+    return 0;
 }
 
 NS_IMETHODIMP
 nsProxyEventObject::QueryInterface(REFNSIID aIID, void** aInstancePtr)
 {
     if( aIID.Equals(GetClass()->GetProxiedIID()) )
     {
         *aInstancePtr = static_cast<nsISupports*>(mXPTCStub);
--- a/xpcom/proxy/src/nsProxyEventPrivate.h
+++ b/xpcom/proxy/src/nsProxyEventPrivate.h
@@ -109,17 +109,25 @@ public:
 
     nsProxyObject(nsIEventTarget *destQueue, PRInt32 proxyType,
                   nsISupports *realObject);
  
     nsISupports*        GetRealObject() const { return mRealObject; }
     nsIEventTarget*     GetTarget() const { return mTarget; }
     PRInt32             GetProxyType() const { return mProxyType; }
 
+    // these are the equivalents of AddRef/Release, but must be called
+    // while holding the global POM lock
+    nsrefcnt LockedAddRef();
+    nsrefcnt LockedRelease();
+
+    // LockedFind should be called holding the POM lock. It will
+    // temporarily unlock the lock during execution.
     nsresult LockedFind(REFNSIID iid, void **aResult);
+
     void LockedRemove(nsProxyEventObject* aObject);
 
     friend class nsProxyObjectManager;
 private:
     ~nsProxyObject();
 
     PRInt32                   mProxyType;
     nsCOMPtr<nsIEventTarget>  mTarget;           /* event target */
@@ -171,16 +179,18 @@ public:
                                          nsXPTCVariant **fullParam,
                                          uint8 *outParamCount);
 
     nsProxyEventObject(nsProxyObject *aParent,
                        nsProxyEventClass *aClass,
                        already_AddRefed<nsISomeInterface> aRealInterface,
                        nsresult *rv);
 
+    // AddRef, but you must be holding the global POM lock
+    nsrefcnt LockedAddRef();
     friend class nsProxyObject;
 
 private:
     ~nsProxyEventObject();
 
     // Member ordering is important: See note in the destructor.
     nsProxyEventClass          *mClass;
     nsCOMPtr<nsProxyObject>     mProxyObject;
@@ -273,31 +283,31 @@ public:
     
     static nsProxyObjectManager *GetInstance();
     static PRBool IsManagerShutdown();
 
     static void Shutdown();
 
     nsresult GetClass(REFNSIID aIID, nsProxyEventClass **aResult);
 
-    void Remove(nsProxyObject* aProxy);
+    void LockedRemove(nsProxyObject* aProxy);
 
-    PRMonitor*   GetMonitor() const { return mProxyCreationMonitor; }
+    PRLock* GetLock() const { return mProxyCreationLock; }
 
 #ifdef PR_LOGGING
     static PRLogModuleInfo *sLog;
 #endif
 
 private:
     ~nsProxyObjectManager();
 
     static nsProxyObjectManager* mInstance;
     nsHashtable  mProxyObjectMap;
     nsClassHashtable<nsIDHashKey, nsProxyEventClass> mProxyClassMap;
-    PRMonitor   *mProxyCreationMonitor;
+    PRLock *mProxyCreationLock;
 };
 
 #define NS_XPCOMPROXY_CLASSNAME "nsProxyObjectManager"
 #define NS_PROXYEVENT_MANAGER_CID                 \
 { 0xeea90d41,                                     \
   0xb059,                                         \
   0x11d2,                                         \
  {0x91, 0x5e, 0xc1, 0x2b, 0x69, 0x6c, 0x93, 0x33} \
--- a/xpcom/proxy/src/nsProxyObjectManager.cpp
+++ b/xpcom/proxy/src/nsProxyObjectManager.cpp
@@ -98,26 +98,26 @@ protected:
 
 nsProxyObjectManager* nsProxyObjectManager::mInstance = nsnull;
 
 NS_IMPL_THREADSAFE_ISUPPORTS1(nsProxyObjectManager, nsIProxyObjectManager)
 
 nsProxyObjectManager::nsProxyObjectManager()
     : mProxyObjectMap(256, PR_FALSE)
 {
-    mProxyCreationMonitor = PR_NewMonitor();
+    mProxyCreationLock = PR_NewLock();
     mProxyClassMap.Init(256);
 }
 
 nsProxyObjectManager::~nsProxyObjectManager()
 {
     mProxyClassMap.Clear();
 
-    if (mProxyCreationMonitor)
-        PR_DestroyMonitor(mProxyCreationMonitor);
+    if (mProxyCreationLock)
+        PR_DestroyLock(mProxyCreationLock);
 
     nsProxyObjectManager::mInstance = nsnull;
 }
 
 PRBool
 nsProxyObjectManager::IsManagerShutdown()
 {
     return mInstance == nsnull;
@@ -188,70 +188,93 @@ nsProxyObjectManager::GetProxyForObject(
     if (po) {
         realObj = po->GetRealObject();
     }
 
     nsCOMPtr<nsISupports> realEQ = do_QueryInterface(aTarget);
 
     nsProxyEventKey rootKey(realObj, realEQ, proxyType);
 
-    // Enter the Grand Monitor here.
-    nsAutoMonitor mon(mProxyCreationMonitor);
+    {
+        nsAutoLock lock(mProxyCreationLock);
+                nsProxyObject *root =
+                    (nsProxyObject*) mProxyObjectMap.Get(&rootKey);
+        if (root)
+            return root->LockedFind(aIID, aProxyObject);
+    }
+
+    // don't lock while creating the nsProxyObject
+    nsProxyObject *newRoot = new nsProxyObject(aTarget, proxyType, realObj);
+    if (!newRoot)
+        return NS_ERROR_OUT_OF_MEMORY;
 
-    nsCOMPtr<nsProxyObject> root = (nsProxyObject*) mProxyObjectMap.Get(&rootKey);
-    if (!root) {
-        root = new nsProxyObject(aTarget, proxyType, realObj);
-        if (!root)
-            return NS_ERROR_OUT_OF_MEMORY;
+    // lock again, and check for a race putting into mProxyObjectMap
+    {
+        nsAutoLock lock(mProxyCreationLock);
+        nsProxyObject *root =
+            (nsProxyObject*) mProxyObjectMap.Get(&rootKey);
+        if (root) {
+            delete newRoot;
+            return root->LockedFind(aIID, aProxyObject);
+        }
 
-        mProxyObjectMap.Put(&rootKey, root);
+        mProxyObjectMap.Put(&rootKey, newRoot);
+        return newRoot->LockedFind(aIID, aProxyObject);
     }
-    return root->LockedFind(aIID, aProxyObject);
 }
 
 void
-nsProxyObjectManager::Remove(nsProxyObject *aProxy)
+nsProxyObjectManager::LockedRemove(nsProxyObject *aProxy)
 {
     nsCOMPtr<nsISupports> realEQ = do_QueryInterface(aProxy->GetTarget());
 
     nsProxyEventKey rootKey(aProxy->GetRealObject(), realEQ, aProxy->GetProxyType());
 
-    nsAutoMonitor mon(mProxyCreationMonitor);
-
     if (!mProxyObjectMap.Remove(&rootKey)) {
         NS_ERROR("nsProxyObject not found in global hash.");
     }
 }
 
 nsresult
 nsProxyObjectManager::GetClass(REFNSIID aIID, nsProxyEventClass **aResult)
 {
-    nsAutoMonitor mon(mProxyCreationMonitor);
+    {
+        nsAutoLock lock(mProxyCreationLock);
+        if (mProxyClassMap.Get(aIID, aResult)) {
+            NS_ASSERTION(*aResult, "Null data in mProxyClassMap");
+            return NS_OK;
+        }
+    }
 
-    nsProxyEventClass *pec;
-    mProxyClassMap.Get(aIID, &pec);
-    if (!pec) {
-        nsIInterfaceInfoManager *iim =
-            xptiInterfaceInfoManager::GetInterfaceInfoManagerNoAddRef();
-        if (!iim)
-            return NS_ERROR_FAILURE;
+    nsIInterfaceInfoManager *iim =
+        xptiInterfaceInfoManager::GetInterfaceInfoManagerNoAddRef();
+    if (!iim)
+        return NS_ERROR_FAILURE;
 
-        nsCOMPtr<nsIInterfaceInfo> ii;
-        nsresult rv = iim->GetInfoForIID(&aIID, getter_AddRefs(ii));
-        if (NS_FAILED(rv))
-            return rv;
+    nsCOMPtr<nsIInterfaceInfo> ii;
+    nsresult rv = iim->GetInfoForIID(&aIID, getter_AddRefs(ii));
+    if (NS_FAILED(rv))
+        return rv;
+
+    nsProxyEventClass *pec = new nsProxyEventClass(aIID, ii);
+    if (!pec)
+        return NS_ERROR_OUT_OF_MEMORY;
 
-        pec = new nsProxyEventClass(aIID, ii);
-        if (!pec)
-            return NS_ERROR_OUT_OF_MEMORY;
+    // Re-lock to put this class into our map. Before putting, check to see
+    // if another thread raced to put before us
+    nsAutoLock lock(mProxyCreationLock);
 
-        if (!mProxyClassMap.Put(aIID, pec)) {
-            delete pec;
-            return NS_ERROR_OUT_OF_MEMORY;
-        }
+    if (mProxyClassMap.Get(aIID, aResult)) {
+        NS_ASSERTION(*aResult, "Null data in mProxyClassMap");
+        delete pec;
+    }
+
+    if (!mProxyClassMap.Put(aIID, pec)) {
+        delete pec;
+        return NS_ERROR_OUT_OF_MEMORY;
     }
 
     *aResult = pec;
     return NS_OK;
 }
 
 /**
  * Helper function for code that already has a link-time dependency on