613360 - ContentProcessParent uses thread observers unsafely

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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]

Yes, it only needs the after-processed-event notification.

Comment 2

[Josh Matthews [:jdm]]

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]]

Attachment: 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]]

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]]

Attachment: Safe use of thread observers v2

Thanks...made the adjustments as suggested.

Comment 7

[Benjamin Smedberg]

Comment on attachment 576336 [details] [diff] [review]
Safe use of thread observers v2

Oh how I hate thread observers!

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a02f77397320

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a02f77397320