Closed Bug 482938 Opened 13 years ago Closed 13 years ago

nsPipe uses nsAutoMonitor without using nsAutoMonitor::New/DestroyMonitor

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file)

Attached patch fixSplinter 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...
Yeah, we absolutely should.
Chris, you should probably be aware of this deadlock detection stuff.
Attachment #367014 - Flags: review?(benjamin) → review+
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/a8880afe4027
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
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
Comment on attachment 367014 [details] [diff] [review]
fix

agreed, a=shaver
Attachment #367014 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 approval]
Whiteboard: [needs 191 landing]
You need to log in before you can comment on or make changes to this bug.