Last Comment Bug 613360 - ContentProcessParent uses thread observers unsafely
: ContentProcessParent uses thread observers unsafely
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla11
Assigned To: Andrew Quartey [:drexler]
:
Mentors:
: 700598 (view as bug list)
Depends on: 542341
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-18 14:55 PST by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2012-02-01 14:00 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Safe use of thread observers (4.17 KB, patch)
2011-11-22 14:56 PST, Andrew Quartey [:drexler]
no flags Details | Diff | Review
Safe use of thread observers v2 (4.23 KB, patch)
2011-11-22 15:49 PST, Andrew Quartey [:drexler]
benjamin: review+
josh: feedback+
Details | Diff | Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2010-11-18 14:55:47 PST
ContentProcessParent uses the nsIThreadObserver unsafely (added in bug 542341). Specifically, it unsets itself as the main thread observer regardless of whether another thread observer has been installed into the chain.

Perhaps it can be changed to use the new interface introduced in bug 612807.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-11-18 15:04:36 PST
Yes, it only needs the after-processed-event notification.
Comment 2 Josh Matthews [:jdm] 2011-04-10 11:52:59 PDT
Relevant classes: http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp
http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsIThreadInternal.idl#111

This would be a good bug for someone with C++ skills who is not afraid of learning about how we use XPCOM and IDL interfaces.  ContentParent currently sets itself as the sole observer of the main thread, and stores the last observer in mOldObserver.  We don't actually want to care about other observers, and the addObserver/removeObserver methods on nsIThreadInternal2 would be much better fits here.  All traces of mOldObserver should be removed from ContentParent.
Comment 3 Andrew Quartey [:drexler] 2011-11-22 14:56:44 PST
Created attachment 576314 [details] [diff] [review]
Safe use of thread observers

I made the required changes but i was a bit unsure when it came to handling OnDispatchedEvent since it's not really used. Feedback most welcome.
Comment 4 Josh Matthews [:jdm] 2011-11-22 15:18:05 PST
Comment on attachment 576314 [details] [diff] [review]
Safe use of thread observers

This is part of the way there, but we can do better. Some names have changed, and interfaces have been collapsed, so here's what we should do instead:
- where we currently use SetObserver in Init, we can instead use AddObserver (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsIThreadInternal.idl#94)
- we can replace the SetObserver in ActorDestroy with RemoveObserver
Comment 5 Andrew Quartey [:drexler] 2011-11-22 15:49:52 PST
Created attachment 576336 [details] [diff] [review]
Safe use of thread observers v2

Thanks...made the adjustments as suggested.
Comment 6 Josh Matthews [:jdm] 2011-11-27 10:10:29 PST
*** Bug 700598 has been marked as a duplicate of this bug. ***
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-12-01 08:21:25 PST
Comment on attachment 576336 [details] [diff] [review]
Safe use of thread observers v2

Oh how I hate thread observers!
Comment 9 Ed Morley [:emorley] 2011-12-02 12:05:08 PST
https://hg.mozilla.org/mozilla-central/rev/a02f77397320

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