Closed Bug 400450 Opened 12 years ago Closed 9 years ago

Deadlock in nsProxyObject::Release

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 650674

People

(Reporter: mook, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/proxy/src/nsProxyObjectManager.cpp&rev=1.64&mark=182,227,231#182
nsProxyObjectManager::GetProxyForObject locks mProxyCreationLock and calls LockedFind

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/proxy/src/nsProxyEvent.cpp&rev=1.97&mark=431,480#430
a nsProxyEventObject gets deleted

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/proxy/src/nsProxyEventPrivate.h&rev=1.54&mark=161,196#161
that nsProxyEventObject has a member nsCOMPtr<nsProxyObject> which gets deleted too

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/proxy/src/nsProxyEvent.cpp&rev=1.97&mark=375,377#373
nsProxyObject::Release() attempts to acquire the same lock in step 1

I managed to hit this attempting to write a test that spun up lots of threads and get proxies.
We're violating the invariant specified at http://mxr.mozilla.org/mozilla/source/xpcom/proxy/src/nsProxyEventObject.cpp#70

You'll need to move the "delete peo;" call out of the lock somehow.

Please please submit your testcase... we need tests for the proxying stuff.
Sorry for the delay.

Attached testcase forces a deadlock by timing things via application of locks.  See the top of the file for a small diagram.

Unfortuantely adding PR logging managed to break the fragile testcase.
Attachment #300290 - Flags: review?(benjamin)
Comment on attachment 300290 [details] [diff] [review]
testcase to show deadlock

please land this, tests are auto-approved during the freeze
Attachment #300290 - Flags: review?(benjamin) → review+
Umm, landing this will (hopefully!) cause hangs in the tests and therefore turn things orange, correct?  Or am I totally misunderstanding what tests get run and which don't?
I don't see how this test would ever get run automatically: you didn't add it to any "make check" target.
Ah, yeah, you're right, of course :)
Keywords: checkin-needed
Checking in xpcom/proxy/tests/Makefile.in;
/cvsroot/mozilla/xpcom/proxy/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.26; previous revision: 1.25
done
RCS file: /cvsroot/mozilla/xpcom/proxy/tests/proxy-create-threadsafety.cpp,v
done
Checking in xpcom/proxy/tests/proxy-create-threadsafety.cpp;
/cvsroot/mozilla/xpcom/proxy/tests/proxy-create-threadsafety.cpp,v  <--  proxy-create-threadsafety.cpp
initial revision: 1.1
done
Keywords: checkin-needed
hrm, i think i had a tree w/ this patched somewhere. oh well :)
Depends on: 380228
Couldn't the delete just be wrapped in an unlock? This seems too simple, so I'm assuming I'm missing something.

    for (peo = mFirst; peo; peo = peo->mNext) {
        if (peo->GetClass()->GetProxiedIID().Equals(aIID)) {
            *aResult = static_cast<nsISupports*>(peo->mXPTCStub);
            peo->LockedAddRef();
            // Can't delete peo's while locked
            {
              nsAutoUnlock unlock(pom->GetLock());
              delete newpeo;
            }
            return NS_OK;
        }
    }
No. the mFirst/mNext chain is guarded by the lock, and you'd potentially race on it.
We're on our way out. Aren't we done with the mFirst/mNext?
Would changing the lock to a monitor create too much of a performance hit?
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 650674
You need to log in before you can comment on or make changes to this bug.