Closed Bug 613360 Opened 10 years ago Closed 9 years ago
Process Parent uses thread observers unsafely
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: 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]
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 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
Thanks...made the adjustments as suggested.
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+
Target Milestone: --- → mozilla11
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.