400450 - Deadlock in nsProxyObject::Release

Status: RESOLVED DUPLICATE of bug 650674

(Core :: XPCOM, defect)

Description

[Mook (songbird dead account)]

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.

Comment 1

[Benjamin Smedberg]

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.

Comment 2

[:Mook]

Attachment: testcase to show deadlock

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.

Comment 3

[Benjamin Smedberg]

Comment on attachment 300290 [details] [diff] [review]
testcase to show deadlock

please land this, tests are auto-approved during the freeze

Comment 4

[:Mook]

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?

Comment 5

[Benjamin Smedberg]

I don't see how this test would ever get run automatically: you didn't add it to any "make check" target.

Comment 6

[Mook (songbird dead account)]

Ah, yeah, you're right, of course :)

Comment 7

[Reed Loden [:reed]]

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

Comment 8

[timeless]

hrm, i think i had a tree w/ this patched somewhere. oh well :)

Comment 9

[David Bradley]

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;
        }
    }

Comment 10

[Benjamin Smedberg]

No. the mFirst/mNext chain is guarded by the lock, and you'd potentially race on it.

Comment 11

[David Bradley]

We're on our way out. Aren't we done with the mFirst/mNext?

Comment 12

[David Bradley]

Would changing the lock to a monitor create too much of a performance hit?