The default bug view has changed. See this FAQ.

ContentProcessParent uses thread observers unsafely

RESOLVED FIXED in mozilla11

Status

()

Core
IPC
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: drexler)

Tracking

Trunk
mozilla11
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

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.

Comment 2

6 years ago
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.
Whiteboard: [good first bug]
(Assignee)

Comment 3

5 years ago
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.
Attachment #576314 - Flags: review?(josh)

Comment 4

5 years ago
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
Attachment #576314 - Flags: review?(josh)
(Assignee)

Comment 5

5 years ago
Created attachment 576336 [details] [diff] [review]
Safe use of thread observers v2

Thanks...made the adjustments as suggested.
Attachment #576314 - Attachment is obsolete: true
Attachment #576336 - Flags: feedback?(josh)

Updated

5 years ago
Attachment #576336 - Flags: review?(benjamin)
Attachment #576336 - Flags: feedback?(josh)
Attachment #576336 - Flags: feedback+

Updated

5 years ago
Duplicate of this bug: 700598
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+

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Assignee: nobody → andrew.quartey
http://hg.mozilla.org/integration/mozilla-inbound/rev/a02f77397320
Keywords: checkin-needed
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/a02f77397320
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.