diff --git a/xpcom/glue/nsAutoLock.h b/xpcom/glue/nsAutoLock.h index 57993ba0f473..0cd4dbbfb419 100644 --- a/xpcom/glue/nsAutoLock.h +++ b/xpcom/glue/nsAutoLock.h @@ -250,6 +250,24 @@ public: } }; +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" diff --git a/xpcom/proxy/src/nsProxyEvent.cpp b/xpcom/proxy/src/nsProxyEvent.cpp index 5fa94bed1827..bee1415f2ee5 100644 --- a/xpcom/proxy/src/nsProxyEvent.cpp +++ b/xpcom/proxy/src/nsProxyEvent.cpp @@ -342,6 +342,12 @@ nsProxyObject::nsProxyObject(nsIEventTarget *target, PRInt32 proxyType, { MOZ_COUNT_CTOR(nsProxyObject); +#ifdef DEBUG + nsCOMPtr 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(); @@ -358,24 +364,43 @@ nsProxyObject::~nsProxyObject() 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); - pom->Release(); + nsAutoUnlock unlock(pom->GetLock()); delete this; + pom->Release(); return 0; } @@ -398,15 +423,14 @@ nsProxyObject::QueryInterface(REFNSIID aIID, void **aResult) 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; @@ -414,39 +438,58 @@ nsProxyObject::LockedFind(REFNSIID aIID, void **aResult) for (peo = mFirst; peo; peo = peo->mNext) { if (peo->GetClass()->GetProxiedIID().Equals(aIID)) { *aResult = static_cast(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; - nsISomeInterface* newInterface; - rv = mRealObject->QueryInterface(aIID, (void**) &newInterface); - if (NS_FAILED(rv)) - return rv; + // Both GetClass and QueryInterface call out to XPCOM, so we unlock for them + { + nsProxyObjectManager* pom = nsProxyObjectManager::GetInstance(); + nsAutoUnlock unlock(pom->GetLock()); - peo = new nsProxyEventObject(this, pec, - already_AddRefed(newInterface), &rv); - if (!peo) { - NS_RELEASE(newInterface); - return NS_ERROR_OUT_OF_MEMORY; + 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; + + newpeo = new nsProxyEventObject(this, pec, + already_AddRefed(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(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(peo->mXPTCStub); + *aResult = static_cast(newpeo->mXPTCStub); return NS_OK; } @@ -457,8 +500,8 @@ nsProxyObject::LockedRemove(nsProxyEventObject *peo) for (i = &mFirst; *i; i = &((*i)->mNext)) { if (*i == peo) { *i = peo->mNext; - break; + return; } } + NS_ERROR("Didn't find nsProxyEventObject in nsProxyObject chain!"); } - diff --git a/xpcom/proxy/src/nsProxyEventObject.cpp b/xpcom/proxy/src/nsProxyEventObject.cpp index 3933c8304577..892617d2335e 100644 --- a/xpcom/proxy/src/nsProxyEventObject.cpp +++ b/xpcom/proxy/src/nsProxyEventObject.cpp @@ -67,11 +67,9 @@ nsProxyEventObject::nsProxyEventObject(nsProxyObject *aParent, 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. @@ -82,30 +80,41 @@ nsProxyEventObject::~nsProxyEventObject() // 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"); - 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; + --mRefCnt; + NS_LOG_RELEASE(this, mRefCnt, "nsProxyEventObject"); + + 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 diff --git a/xpcom/proxy/src/nsProxyEventPrivate.h b/xpcom/proxy/src/nsProxyEventPrivate.h index 61b30e8f27db..80ecbb011fa6 100644 --- a/xpcom/proxy/src/nsProxyEventPrivate.h +++ b/xpcom/proxy/src/nsProxyEventPrivate.h @@ -114,7 +114,15 @@ public: 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; @@ -176,6 +184,8 @@ public: already_AddRefed aRealInterface, nsresult *rv); + // AddRef, but you must be holding the global POM lock + nsrefcnt LockedAddRef(); friend class nsProxyObject; private: @@ -278,9 +288,9 @@ public: 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; @@ -292,7 +302,7 @@ private: static nsProxyObjectManager* mInstance; nsHashtable mProxyObjectMap; nsClassHashtable mProxyClassMap; - PRMonitor *mProxyCreationMonitor; + PRLock *mProxyCreationLock; }; #define NS_XPCOMPROXY_CLASSNAME "nsProxyObjectManager" diff --git a/xpcom/proxy/src/nsProxyObjectManager.cpp b/xpcom/proxy/src/nsProxyObjectManager.cpp index 0ff8e12501f5..d29a53925d37 100644 --- a/xpcom/proxy/src/nsProxyObjectManager.cpp +++ b/xpcom/proxy/src/nsProxyObjectManager.cpp @@ -103,7 +103,7 @@ NS_IMPL_THREADSAFE_ISUPPORTS1(nsProxyObjectManager, nsIProxyObjectManager) nsProxyObjectManager::nsProxyObjectManager() : mProxyObjectMap(256, PR_FALSE) { - mProxyCreationMonitor = PR_NewMonitor(); + mProxyCreationLock = PR_NewLock(); mProxyClassMap.Init(256); } @@ -111,8 +111,8 @@ nsProxyObjectManager::~nsProxyObjectManager() { mProxyClassMap.Clear(); - if (mProxyCreationMonitor) - PR_DestroyMonitor(mProxyCreationMonitor); + if (mProxyCreationLock) + PR_DestroyLock(mProxyCreationLock); nsProxyObjectManager::mInstance = nsnull; } @@ -193,29 +193,41 @@ nsProxyObjectManager::GetProxyForObject(nsIEventTarget* aTarget, nsProxyEventKey rootKey(realObj, realEQ, proxyType); - // Enter the Grand Monitor here. - nsAutoMonitor mon(mProxyCreationMonitor); - - nsCOMPtr root = (nsProxyObject*) mProxyObjectMap.Get(&rootKey); - if (!root) { - root = new nsProxyObject(aTarget, proxyType, realObj); - if (!root) - return NS_ERROR_OUT_OF_MEMORY; - - mProxyObjectMap.Put(&rootKey, root); + { + 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; + + // 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, newRoot); + return newRoot->LockedFind(aIID, aProxyObject); } - return root->LockedFind(aIID, aProxyObject); } void -nsProxyObjectManager::Remove(nsProxyObject *aProxy) +nsProxyObjectManager::LockedRemove(nsProxyObject *aProxy) { nsCOMPtr 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."); } @@ -224,31 +236,42 @@ nsProxyObjectManager::Remove(nsProxyObject *aProxy) nsresult nsProxyObjectManager::GetClass(REFNSIID aIID, nsProxyEventClass **aResult) { - nsAutoMonitor mon(mProxyCreationMonitor); - - nsProxyEventClass *pec; - mProxyClassMap.Get(aIID, &pec); - if (!pec) { - nsIInterfaceInfoManager *iim = - xptiInterfaceInfoManager::GetInterfaceInfoManagerNoAddRef(); - if (!iim) - return NS_ERROR_FAILURE; - - nsCOMPtr ii; - nsresult rv = iim->GetInfoForIID(&aIID, getter_AddRefs(ii)); - if (NS_FAILED(rv)) - return rv; - - pec = new nsProxyEventClass(aIID, ii); - if (!pec) - return NS_ERROR_OUT_OF_MEMORY; - - if (!mProxyClassMap.Put(aIID, pec)) { - delete pec; - return NS_ERROR_OUT_OF_MEMORY; + { + nsAutoLock lock(mProxyCreationLock); + if (mProxyClassMap.Get(aIID, aResult)) { + NS_ASSERTION(*aResult, "Null data in mProxyClassMap"); + return NS_OK; } } + nsIInterfaceInfoManager *iim = + xptiInterfaceInfoManager::GetInterfaceInfoManagerNoAddRef(); + if (!iim) + return NS_ERROR_FAILURE; + + nsCOMPtr 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; + + // 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.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; }