Last Comment Bug 338069 - Potential deadlock in nsObserverList::RemoveObserver
: Potential deadlock in nsObserverList::RemoveObserver
Status: RESOLVED FIXED
: fixed1.8.0.5, fixed1.8.1
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-15 16:11 PDT by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2006-06-14 20:32 PDT (History)
4 users (show)
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (2.32 KB, patch)
2006-05-15 16:12 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
darin.moz: review+
darin.moz: superreview+
roc: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-15 16:11:03 PDT
Even after bug 334523, there is still a potential deadlock. The problem is that we do a QueryInterface while holding mLock, and if the observer is a JS object, then it can run any arbitrary code.

In https://bugzilla.novell.com/show_bug.cgi?id=173986 we seem to be hitting this; the stack trace shows that while running the JS we do a JS GC which releases stuff which deadlocks trying to reenter RemoveObserver. Unfortunately I can't reproduce this myself so I can't yet verify that my fix fixes that issue, but I still think it's worth doing. Hopefully we'll get this verified on the Novell side shortly.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-15 16:12:00 PDT
Created attachment 222112 [details] [diff] [review]
fix

Obvious fix --- move the QI to before we grab the lock.
Comment 2 Darin Fisher 2006-05-15 16:23:31 PDT
This is 1.8 branch only, right?  The locks don't exist on the trunk anymore.
Comment 3 Darin Fisher 2006-05-15 16:24:47 PDT
Dupe of bug 334523 (which I fixed for 1.5.0.4)?
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-15 19:34:54 PDT
(In reply to comment #2)
> This is 1.8 branch only, right?  The locks don't exist on the trunk anymore.

Yes.

(In reply to comment #3)
> Dupe of bug 334523 (which I fixed for 1.5.0.4)?

No. Our builds have that patch.

Well, our stack does look like attachment 218886 [details]. I think perhaps you diagnosed bug 334523 incorrectly. Your comment there says "If RemoveObserver results in an observer being destroyed, then that could result in another call to RemoveObserver" and you prevented that keeping the observer alive until the lock is released. However these stacks seem to indicate that the transition to JS from RemoveObserver is because of a QI call, not because of a destructor call.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-15 19:36:45 PDT
I think we should avoid both QI *and* destructor calls while holding mLock, so we need both patches.
Comment 6 Darin Fisher 2006-05-16 00:29:31 PDT
Comment on attachment 222112 [details] [diff] [review]
fix

ok, that makes sense.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-16 11:15:21 PDT
Comment on attachment 222112 [details] [diff] [review]
fix

I'm adding 1.8.1+ on the grounds that Darin knew when he r+ed that this was intended for the 1.8 branch only...
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-24 19:33:06 PDT
checked in.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-05-24 19:33:42 PDT
Comment on attachment 222112 [details] [diff] [review]
fix

This probably should go into 1.8.0.5.
Comment 10 Brendan Eich [:brendan] 2006-05-25 09:00:22 PDT
roc, could you please attach the stack from that Novell bug?  I can't access it (I signed up for a bugzilla.novell.com account, but still no joy).  Thanks,

/be
Comment 11 Daniel Veditz [:dveditz] 2006-06-12 12:03:05 PDT
Comment on attachment 222112 [details] [diff] [review]
fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-14 20:32:37 PDT
Fixed on 1.8.0 branch.

Note You need to log in before you can comment on or make changes to this bug.