Closed
Bug 221331
Opened 21 years ago
Closed 21 years ago
nsAutoLock: bogus deadlock warning if unlock/lock are used.
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.6alpha
People
(Reporter: darin.moz, Assigned: darin.moz)
Details
Attachments
(2 files, 2 obsolete files)
3.59 KB,
patch
|
Details | Diff | Splinter Review | |
8.12 KB,
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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•21 years ago
|
Severity: normal → minor
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6alpha
Assignee | ||
Comment 1•21 years ago
|
||
btw, patches welcome... i might not get to this right away.
Comment 2•21 years ago
|
||
Some NS_TRACE_MALLOC_XXX junk here too, from another bug timeless will recall. /be
Assignee | ||
Comment 3•21 years ago
|
||
patch looks good.. i'll try it out tomorrow hopefully.
Comment 4•21 years ago
|
||
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•21 years ago
|
||
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•21 years ago
|
||
Attachment #132784 -
Attachment is obsolete: true
Attachment #132811 -
Attachment is obsolete: true
Comment 7•21 years ago
|
||
Comment on attachment 132848 [details] [diff] [review] proposed for checkin Requesting r+sr=darin. /be
Attachment #132848 -
Flags: review?(darin)
Comment 8•21 years ago
|
||
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•21 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•21 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
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•