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

RESOLVED FIXED in mozilla1.6alpha

Status

()

--
minor
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: darin.moz, Assigned: darin.moz)

Tracking

Trunk
mozilla1.6alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

15 years ago
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
(Assignee)

Updated

15 years ago
Severity: normal → minor
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6alpha
(Assignee)

Comment 1

15 years ago
btw, patches welcome... i might not get to this right away.
Created attachment 132784 [details] [diff] [review]
patch to try

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

/be
(Assignee)

Comment 3

15 years ago
patch looks good.. i'll try it out tomorrow hopefully.
Created attachment 132811 [details] [diff] [review]
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
(Assignee)

Comment 5

15 years ago
Created attachment 132843 [details] [diff] [review]
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.
Created attachment 132848 [details] [diff] [review]
proposed for checkin
Attachment #132784 - Attachment is obsolete: true
Attachment #132811 - Attachment is obsolete: true
Comment on attachment 132848 [details] [diff] [review]
proposed for checkin

Requesting r+sr=darin.

/be
Attachment #132848 - Flags: review?(darin)
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
(Assignee)

Comment 9

15 years ago
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!
Attachment #132848 - Flags: superreview+
Attachment #132848 - Flags: review?(darin)
Attachment #132848 - Flags: review+
(Assignee)

Comment 10

15 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.