Last Comment Bug 771074 - Add API to allow off-main-thread consumers to hold strong references to main-thread-only objects
: Add API to allow off-main-thread consumers to hold strong references to main-...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks: 773610 808146 850253
  Show dependency treegraph
 
Reported: 2012-07-05 01:56 PDT by Bobby Holley (busy)
Modified: 2013-05-05 18:47 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Introduce pointer wrapping mechanism for hold references to pointers off-main-thread. v1 (5.24 KB, patch)
2012-07-05 02:20 PDT, Bobby Holley (busy)
benjamin: review+
Details | Diff | Review
Fix URL classifier proxies. v1 (5.88 KB, patch)
2012-07-05 02:21 PDT, Bobby Holley (busy)
no flags Details | Diff | Review

Description Bobby Holley (busy) 2012-07-05 01:56:48 PDT
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 (busy) 2012-07-05 02:20:01 PDT
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.
Comment 2 Bobby Holley (busy) 2012-07-05 02:21:03 PDT
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 3 Benjamin Smedberg [:bsmedberg] 2012-07-10 12:06:47 PDT
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 (busy) 2012-07-10 13:34:55 PDT
(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 [:bsmedberg] 2012-07-10 14:04:14 PDT
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 (busy) 2012-07-13 05:56:18 PDT
Pushed the API to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/784d3448dfe3
Comment 7 Bobby Holley (busy) 2012-07-13 06:00:00 PDT
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 (busy) 2012-07-13 06:00:55 PDT
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 (busy) 2012-07-13 06:13:24 PDT
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 [:khuey] (khuey@mozilla.com) 2012-07-13 06:27:15 PDT
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 (busy) 2012-07-13 06:38:59 PDT
(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
Comment 12 Bobby Holley (busy) 2012-07-13 07:47:40 PDT
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] 2012-07-13 20:23:24 PDT
https://hg.mozilla.org/mozilla-central/rev/93f24c8511e7

Note You need to log in before you can comment on or make changes to this bug.