Bug 350132 - Deadlock in JS/XPCOM proxy, r=brendan

This commit is contained in:
benjamin@smedbergs.us 2007-08-16 13:51:50 -07:00
parent 900c4f4e4b
commit ee8b81eaac
5 changed files with 201 additions and 98 deletions

View File

@ -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"

View File

@ -342,6 +342,12 @@ nsProxyObject::nsProxyObject(nsIEventTarget *target, PRInt32 proxyType,
{
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();
@ -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<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;
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<nsISomeInterface>(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<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;
}
@ -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!");
}

View File

@ -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

View File

@ -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<nsISomeInterface> 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<nsIDHashKey, nsProxyEventClass> mProxyClassMap;
PRMonitor *mProxyCreationMonitor;
PRLock *mProxyCreationLock;
};
#define NS_XPCOMPROXY_CLASSNAME "nsProxyObjectManager"

View File

@ -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<nsProxyObject> 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<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.");
}
@ -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<nsIInterfaceInfo> 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<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;
// 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;
}