482938 - nsPipe uses nsAutoMonitor without using nsAutoMonitor::New/DestroyMonitor

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, we absolutely should.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Chris, you should probably be aware of this deadlock detection stuff.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/a8880afe4027

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 367014 [details] [diff] [review]
fix

agreed, a=shaver

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fb419a744938