Closed Bug 613360 Opened 13 years ago Closed 12 years ago

ContentProcessParent uses thread observers unsafely


(Core :: IPC, defect)

Not set





(Reporter: bent.mozilla, Assigned: drexler)



(Whiteboard: [good first bug])


(1 file, 1 obsolete file)

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.
Yes, it only needs the after-processed-event notification.
Relevant classes:

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.
Whiteboard: [good first bug]
Attached patch Safe use of thread observers (obsolete) — Splinter Review
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.
Attachment #576314 - Flags: review?(josh)
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 (
- we can replace the SetObserver in ActorDestroy with RemoveObserver
Attachment #576314 - Flags: review?(josh)
Thanks...made the adjustments as suggested.
Attachment #576314 - Attachment is obsolete: true
Attachment #576336 - Flags: feedback?(josh)
Attachment #576336 - Flags: review?(benjamin)
Attachment #576336 - Flags: feedback?(josh)
Attachment #576336 - Flags: feedback+
Comment on attachment 576336 [details] [diff] [review]
Safe use of thread observers v2

Oh how I hate thread observers!
Attachment #576336 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Assignee: nobody → andrew.quartey
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.