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

RESOLVED FIXED in mozilla16

Status

()

defect
RESOLVED FIXED
7 years ago
6 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)

Assignee

Description

7 years ago
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.
Assignee

Comment 1

7 years ago
Attaching the API for bsmedberg's perusal. Any/all suggestions welcome.
Attachment #639268 - Flags: review?(benjamin)
Assignee

Comment 2

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

Comment 3

7 years ago
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+
Assignee

Comment 4

7 years ago
(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

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

Updated

7 years ago
Blocks: 773610
Assignee

Updated

7 years ago
No longer blocks: 770840
Assignee

Comment 7

7 years ago
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
Assignee

Comment 8

7 years ago
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?
Assignee

Updated

7 years ago
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
Assignee

Comment 9

7 years ago
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.
Assignee

Comment 11

7 years ago
(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
Assignee

Comment 12

7 years ago
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Blocks: 850253
You need to log in before you can comment on or make changes to this bug.