771074 - Add API to allow off-main-thread consumers to hold strong references to main-thread-only objects

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

We seem to be fairly good about proxying calls to XPCWrappedJS-implemented callbacks and listeners to the main thread. When we do this, we usually start with the callback on the main thread, then stick it in a runnable (usually with an nsCOMPtr) that we dispatch onto thread X. When that runnable runs and wants to fire the callback, it instantiates a runnable of a different class (usually an inner class), stores the callback (again with an nsCOMPtr) and dispatches it to the main thread.

The rub here is that we end up calling AddRef/Release on the XPCWrappedJS off-main-thread. This is a problem, because XPCWrappedJS has special implementations of AddRef/Release that call into cycle collector maps. Off-main-thread Release is easy to handle, because we can just detect it and convert it into a proxied release. But AddRef has to be synchronous, so our only option in the status quo is to acquire a lock. This, in turn, requires _all_ the XPConnect cycle collector machinery to be lock-protected, which is undesirable.

Every problem in computer science can be solved with a layer of indirection. So that's the approach I'm going to use here.

I introduced a new XPCOM template class called nsMainThreadPtrHolder<T> (I'm open to better/shorter naming suggestions). This class holds a strong reference to its internal T*, and is reference-counted itself in a thread-safe manner. I also introduced nsMainThreadPtrHandle<T> to avoid the cumbersome verbiage of dealing with nsRefPtr<nsMainThreadPtrHolder<T> >.

so consumers that want to pass a callback around off-main-thread can instantiate a Holder, and pass that instead. The holder may be AddRef/Release'd by anyone, but may only be constructed (AddRefing the underlying pointer) on the main thread. It also has an additional safety mechanism of asserting main-threadedness when getting the underlying pointer. So when used correctly, it guarantees that off-main-thread callers treat the handle as opaque.

There are a number of places in the tree where we need to fix this, so I'm going to start by getting review on the API to minimize code churn.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Introduce pointer wrapping mechanism for hold references to pointers off-main-thread. v1

Attaching the API for bsmedberg's perusal. Any/all suggestions welcome.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Fix URL classifier proxies. v1

Attaching an example of the API in action. Once the API is finalized, I'll work on the getting this reviewed.

Comment 3

[Benjamin Smedberg]

Comment on attachment 639268 [details] [diff] [review]
Introduce pointer wrapping mechanism for hold references to pointers off-main-thread. v1

>diff --git a/xpcom/glue/nsProxyRelease.h b/xpcom/glue/nsProxyRelease.h

>+template<class T>
>+class nsMainThreadPtrHolder

nit, standard style is to put the public API first and the private second

>+  public:

nit, put this at column 0


>+  T* get() {
>+    // Nobody should be touching the raw pointer off-main-thread.
>+    MOZ_ASSERT(NS_IsMainThread());

I think that we should be using this rarely-enough that this can be a release-mode assert as well.

r=me with those changes

Comment 4

[Bobby Holley (:bholley)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> I think that we should be using this rarely-enough that this can be a
> release-mode assert as well.
> 

I considered this, but this is already a helper class for people honestly trying to do the right thing, and thus presumably they're testing it out on debug builds. These asserts are designed as debugging aids, and so IMO they should be debug-only. I'm adding release-mode asserts where it matters to us (XPCWrappedJS::{AddRef,Release}), which I think should be sufficient.

I don't feel that strongly, though, so I can make them release-mode if you want. Thoughts?

Comment 5

[Benjamin Smedberg]

I generally think that everything should be fatal which doesn't obviously hurt perf, so I'd love to keep it fatal.

Comment 6

[Bobby Holley (:bholley)]

Pushed the API to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/784d3448dfe3

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 639269 [details] [diff] [review]
Fix URL classifier proxies. v1

I split off fixing the consumers into bug 773610 to avoid milestone weirdness with the pending uplift.

Comment 8

[Bobby Holley (:bholley)]

Kyle - whoever it was that was trying to AddRef XPCWrappedJS off-main-thread, can you tell them that this API is now landed and ready?

Comment 9

[Bobby Holley (:bholley)]

Annnd, backed out for windows template bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb30728cb99

Log is here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13491920&tree=Mozilla-Inbound#error0

I don't have a recent windows VM handy. Kyle, any idea what I'm doing wrong?

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The macros give you AddRef/Release with the stdcall calling convention on Windows, but your declarations use the cdecl (default) calling convention.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> The macros give you AddRef/Release with the stdcall calling convention on
> Windows, but your declarations use the cdecl (default) calling convention.

Thanks kyle. I've updated it to use NS_IMETHOD_(nsrefcnt) instead of nsrefcnt and pushed to try build-only:
https://tbpl.mozilla.org/?tree=Try&rev=2954a12fae4a

Comment 12

[Bobby Holley (:bholley)]

Windows builds haven't died yet, and XPCOM is built first. I'm guessing this is fixed. Repushing:

http://hg.mozilla.org/integration/mozilla-inbound/rev/93f24c8511e7

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/93f24c8511e7