850247 - Remove Off-Main-Thread XPCWrappedJS refcounting from nsRequestObserverProxy

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Make nsRequestObserverProxy proxy events to the main thread only. v1

I'm doing this so that I can safely make this class use nsMainThreadPtr{Handle,Holder}.
There's no theoretical reason why we couldn't support other threads here, but it
would mean implementing that functionality in the PtrHolders, renaming them, and
weakening their assertions/invariants. AFAICT the consumers here don't actually have
a reason to proxy events anywhere but the main thread, so I'm just going to do this
rather than writing code to support something that we don't do (and thus can't test).

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make nsRequestObserverProxy hold onto its context. v1

Both the consumers just do this manually. The context here is sometimes an
XPCWrappedJS, so passing it around with nsCOMPtrs off-main-thread can't
happen anymore.

We add assertions in OnStartRequest/OnStopRequest to catch any consumers
that might try to change contexts midstream.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Move nsRequestObserverProxy to nsMainThreadPtr{Holder,Handle}. v1

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 723985 [details] [diff] [review]
Part 1 - Make nsRequestObserverProxy proxy events to the main thread only. v1

Review of attachment 723985 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIRequestObserverProxy.idl
@@ -21,5 @@
>      /**
>       * Initializes an nsIRequestObserverProxy.
>       *
>       * @param observer - receives observer notifications on the other thread
> -     * @param target - may be NULL indicating the calling thread's event target

I think you need a uuid change?

update the documentation in the idl to indicate that this happens on main thread rather than "another thread" as it says now

r+ with that

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 723986 [details] [diff] [review]
Part 2 - Make nsRequestObserverProxy hold onto its context. v1

Review of attachment 723986 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/public/nsIRequestObserverProxy.idl
@@ +17,2 @@
>   */
>  [scriptable, uuid(7df8845f-938a-4437-9ea4-b11b850048f1)]

uuid bump

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 723987 [details] [diff] [review]
Part 3 - Move nsRequestObserverProxy to nsMainThreadPtr{Holder,Handle}. v1

Review of attachment 723987 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsRequestObserverProxy.cpp
@@ -118,5 @@
> -
> -nsRequestObserverProxy::~nsRequestObserverProxy()
> -{
> -    if (mObserver) {
> -        // order is crucial here... we must be careful to clear mObserver

deleting code like that just makes ya feel good :)

Comment 7

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=eb7aa78fec9b

Comment 8

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0e02cd1e8d6f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/02f9c504991c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb8548b4582

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e02cd1e8d6f
https://hg.mozilla.org/mozilla-central/rev/02f9c504991c
https://hg.mozilla.org/mozilla-central/rev/5eb8548b4582