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

RESOLVED FIXED in mozilla16

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 639268 [details] [diff] [review]
Introduce pointer wrapping mechanism for hold references to pointers off-main-thread. v1

Attaching the API for bsmedberg's perusal. Any/all suggestions welcome.
Attachment #639268 - Flags: review?(benjamin)
Created attachment 639269 [details] [diff] [review]
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 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
Attachment #639268 - Flags: review?(benjamin) → review+
(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?
I generally think that everything should be fatal which doesn't obviously hurt perf, so I'd love to keep it fatal.
Pushed the API to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/784d3448dfe3
Blocks: 773610
No longer blocks: 770840
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.
Attachment #639269 - Attachment is obsolete: true
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?
Summary: Prevent XPCWrappedJS from being AddRef/Released off-main-thread → Add API to allow off-main-thread consumers to hold strong references to main-thread-only objects
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?
The macros give you AddRef/Release with the stdcall calling convention on Windows, but your declarations use the cdecl (default) calling convention.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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
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
https://hg.mozilla.org/mozilla-central/rev/93f24c8511e7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Blocks: 808146
Blocks: 850253
You need to log in before you can comment on or make changes to this bug.