Closed
Bug 482938
Opened 16 years ago
Closed 16 years ago
nsPipe uses nsAutoMonitor without using nsAutoMonitor::New/DestroyMonitor
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
Details
(Keywords: fixed1.9.1)
Attachments
(1 file)
896 bytes,
patch
|
benjamin
:
review+
shaver
:
approval1.9.1+
|
Details | Diff | Splinter Review |
With my patch in bug 475441 applied, I've observed "potential deadlock" warnings running video/audio mochitests, warning about a media decoder lock and the media cache lock being acquired in inconsistent order. However, the "previous" stack trace shows a call path that terminates at nsPipe::OnPipeException grabbing the nsPipe monitor, which is definitely not one of the monitors the warning is about.
I think this happens because nsPipe uses nsAutoMonitor to acquire and release its PRMonitor, but it releases the monitor using PR_DestroyMonitor instead of nsAutoMonitor::DestroyMonitor. So nsAutoMonitor's potential-deadlock is never informed of the pipe's monitor going away, and if we're unlucky enough to reallocate a new monitor at the same address, it gets confused. The attached patch fixes the spurious warnings for me. It may also fix other spurious warnings. I've noticed a few other places in the code (security/manager) that have the same problem and I'll file a separate bug for that.
Attachment #367014 -
Flags: review?(benjamin)
Ugh, we should really guard against this somehow, either by asserting or biting the bullet and making nsLock and nsMonitor as described in the XXX comment above _hash_free_entry in nsAutoLock.cpp...
Assignee | ||
Comment 2•16 years ago
|
||
Yeah, we absolutely should.
Assignee | ||
Comment 3•16 years ago
|
||
Chris, you should probably be aware of this deadlock detection stuff.
Updated•16 years ago
|
Attachment #367014 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs landing]
Assignee | ||
Comment 4•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee | ||
Comment 5•16 years ago
|
||
Comment on attachment 367014 [details] [diff] [review]
fix
Very small and safe patch that avoid spurious warnings, so would be useful on 1.9.1 branch
Assignee | ||
Updated•16 years ago
|
Attachment #367014 -
Flags: approval1.9.1?
Comment 6•16 years ago
|
||
Comment on attachment 367014 [details] [diff] [review]
fix
agreed, a=shaver
Attachment #367014 -
Flags: approval1.9.1? → approval1.9.1+
Updated•16 years ago
|
Whiteboard: [needs 191 approval]
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 191 landing]
Assignee | ||
Comment 7•16 years ago
|
||
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•