221331 - nsAutoLock: bogus deadlock warning if unlock/lock are used.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Darin Fisher]

SUMMARY
-------

nsAutoLock does not properly pop its lock from the "stack" when unlock is
called.  this can lead to a bogus deadlock warning.  see below...


DETAILS
-------

Brendan Eich wrote:

> Darin Fisher wrote:
>
>> brendan,
>>
>> is there a bug filed (i couldn't find one) about the fact that
nsAutoLock::unlock does not prevent "potential deadlock" assertions from wrongly
firing?
>>
>> e.g.,
>>
>> static PRLock *gLock1, *gLock2;
>>
>> void a()
>> {
>>   nsAutoLock lock(gLock1);
>>   // ...
>> }
>>
>> void b()
>> {
>>   nsAutoLock lock(gLock1);
>>   // ...
>>
>>   lock.unlock();
>>   c();
>>   lock.lock();
>>  
>>   // ...
>> }
>>
>> void c()
>> {
>>   nsAutoLock lock(gLock2);
>>   a();
>> }
>>
>> void d()
>> {
>>   c(); // c() is not only called from b()
>> }
>>
>> in this example, assume d() is called first followed by b() at some later
time.  nsAutoLockBase::nsAutoLockBase will warn of a potential deadlock between
these two locks.  but, there is no deadlock since gLock1 is not held while
calling c().  however, the nsAutoLock allocated in b() doesn't know that!
>>
>> i hit this bug while converting some legit code over from raw
PR_Lock/PR_Unlock to nsAutoLock.  i could work around it by inverting the logic
in b() to avoid the unlock/lock pair, but i would prefer not too since it would
add extra complexity to that code.
>
>
>
> No bug on file yet, feel free to fix.  I'll r= fast -- this is all #ifdef
DEBUG only, and in XPCOM, so my r+sr= is as good as the next guy's ;-).
>
> Suggest you take unlock and lock out of line, into nsAutoLock.cpp, and make
unlock temporarily pop the nsAutoLockBase from top of stack, and lock re-push
it.  Hmm, what if this base being unlocked is not at top of stack?  Stack should
be shallow enough that you can search, so we don't have to doubly-link _a la_
PRCList.h.  That way we can keep the single-linked mDown pointer.  You'll just
have to grovel over the stack like so:
>
>    nsAutoLockBase* base = (nsAutoLockBase*) PR_GetThreadPrivate(LockStackTPI);
>    nsAutoLockBase** basep = nsnull;
>    while (base != this) {
>        basep = &base->mDown;
>        base = *basep;
>    }
>    if (!basep)
>        PR_SetThreadPrivate(LockStackTPI, mDown);
>    else
>        *basep = mDown;
>
>
> for unlock, and like so:
>
>    nsAutoLockBase* base = (nsAutoLockBase*) PR_GetThreadPrivate(LockStackTPI);
>    nsAutoLockBase** basep = nsnull;
>    while (base != mDown) {
>        basep = &base->mDown;
>        base = *basep;
>    }
>    if (!basep)
>        PR_SetThreadPrivate(LockStackTPI, this);
>    else
>        *basep = this;
>
>
> for lock.  Or something like that -- I'm typing fast!
>
> /be

Comment 1

[Darin Fisher]

btw, patches welcome... i might not get to this right away.

Comment 2

[Brendan Eich [:brendan]]

Attachment: patch to try

Some NS_TRACE_MALLOC_XXX junk here too, from another bug timeless will recall.

/be

Comment 3

[Darin Fisher]

patch looks good.. i'll try it out tomorrow hopefully.

Comment 4

[Brendan Eich [:brendan]]

Attachment: slightly better nsAutoLock.h patch

Hide after releasing mLock in unlock(), not before -- no need to lengthen
#ifdef DEBUG critical sections for thread-local code and data (LockStackTPI,
etc.).

/be

Comment 5

[Darin Fisher]

Attachment: necko patch to be applied once this bug is fixed

so, the nsAutoLock changes seem to do the trick.  the patch i am attaching here
makes nsIOThreadPool not work around this bug anymore.	this section of code is
what prompted me to file this bug.

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed for checkin

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 132848 [details] [diff] [review]
proposed for checkin

Requesting r+sr=darin.

/be

Comment 8

[Brendan Eich [:brendan]]

BTW, I intend to factor the stack tracing stuff out of NS_TRACE_MALLOC and build
it early, so we can use it here.  So I do intend to clean up the XXX stuff in
the proposed patch, soon.

/be

Comment 9

[Darin Fisher]

Comment on attachment 132848 [details] [diff] [review]
proposed for checkin

r+sr=darin

one nit: 

+			   const void *callsite2, PRUint32* index2p,

|const void* callsite2| for consistency.

i'll make that change and check this in.  thx for the quick patch brendan!

Comment 10

[Darin Fisher]

landed "proposed for checkin" patch, and checked in "necko patch to be applied..."

brendan: i presume you plan to clean up the _XXX macros at some later time (in
that other bug).

marking FIXED.