Per comment in mozilla.dev.platform, for a number of safety reasons it makes sense to move away from raw pointers for Dispatch()/DispatchToMainThread(). Passing around 0-refcount objects is dangerous. Doing the "obvious" conversion pattern of: nsCOMPtr<nsIRunnable> runnable = new FooRunnable(...); target->Dispatch(runnable, flags); adds thread-races to the code, since now runnable may die either on Mainthread OR on the sending thread. This may not matter, but if (for example) the runnable holds MainThread-Release()-only objects (like JS callbacks), releasing them off MainThread is a Very Bad Thing. See "Runnables and thread-unsafe members" in m.d.platform, 14-Apr-2015. Note; the implications of touching this run deep....

Another possible small benefit: avoiding some amount of extra (threadsafe) AddRef/Release pairs.

Works (at least well enough to start up and run some webrtc tests). Haven't removed all the raw ptr interfaces (that will come in a later patch) or convert all the usages over (just did a couple, and part 2 will work on that). I'm sure there's some over-use of &&, and dangling ends (it is a WIP).

After a try, cleaned up some warnings-as-errors and a leak from Promise::MaybeReportRejected() (which wants to free stuff if DispatchToMainThread() fails in shutdown)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5583c51c5a2 looks like I'm past most of the warnings-as-errors/etc. I'll wait for more tests before I update the patch; nothing major changed thus far

Comment on attachment 8593388 [details] [diff] [review] Patch 1 - WIP conversion of Dispatch() and friends to already_AddRefed<> Review of attachment 8593388 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable, a couple comments below. I don't know whether folks would want to see |already_AddRefed<T>&&| parameters put into ns{COM,Ref}Ptrs and then .forget()'en or directly Move()'d when possible. Move()'ing seems slightly more performant (no need for unnecessary smart pointer destructor call). ::: dom/promise/Promise.cpp @@ +1144,5 @@ > new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport); > + nsRefPtr<AsyncErrorReporter> sacrifice = r; > + // If this fails, we know NS_DispatchToMainThread leaked on purpose, and we don't want that > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > + r->Release(); I think the comment could be a little more complete here; calling Release() directly on a smart pointer is not the most obvious thing in the world. Comments also end with periods. ::: dom/workers/WorkerPrivate.cpp @@ +2873,4 @@ > { > // May be called on any thread! > + nsRefPtr<WorkerControlRunnable> runnable(aWorkerControlRunnable); > + MOZ_ASSERT(runnable); Nit: keep the whitespace, and just swap the two lines, as you did below for DispatchDebuggerRunnable below. @@ +7309,5 @@ > return NS_ERROR_UNEXPECTED; > } > > + if (event) { > + workerRunnable = mWorkerPrivate->MaybeWrapAsWorkerRunnable(event.forget()); Couldn't we use Move(aRunnable) here, and then we don't need |event|? ::: dom/workers/WorkerThread.cpp @@ +187,2 @@ > > + nsresult rv = nsThread::Dispatch(runnable.forget(), NS_DISPATCH_NORMAL); This can just be Move(aWorkerRunnable), can't it? ::: xpcom/glue/nsThreadUtils.cpp @@ +174,5 @@ > nsCOMPtr<nsIThread> thread; > nsresult rv = NS_GetMainThread(getter_AddRefs(thread)); > if (NS_WARN_IF(NS_FAILED(rv))) { > + // NOTE: if you stop leaking here, adjust Promise::MaybeReportRejected(), > + // which assumes a leak here, or split into leaks and no-leaks versions This comment is kind of cryptic. ::: xpcom/threads/nsIEventTarget.idl @@ +38,1 @@ > void dispatch(in nsIRunnable event, in unsigned long flags); I wonder if we could figure out how to make nsIEventTarget::Dispatch the obvious implementation that forwarded to the already_AddRefed Dispatch...but nsIEventTarget::Dispatch would have to stay in the vtable somehow to not break JS... @@ +38,2 @@ > void dispatch(in nsIRunnable event, in unsigned long flags); > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); I think this should be [noscript]? ::: xpcom/threads/nsThreadPool.h @@ +32,5 @@ > private: > ~nsThreadPool(); > > void ShutdownThread(nsIThread* aThread); > nsresult PutEvent(nsIRunnable* aEvent); Do we need the pointer version of this anymore? It looks like we don't. Same question for nsEventQueue::PutEvent, etc.

So this makes it overall harder to leak runnables accidentally, in particular when the targets are off-main-thread. There still is a class of runnables that holds non-threadsafe things on other threads, and which are already AddRef'd. For example, JS callbacks, or mtransport/webrtc stuff tied to STS. For this limited subset of Runnables, this patch blocks one bad failure race, where someone stores it in a refptr/comptr across a Dispatch*() (see above and .platform). The other issue is if for any reason we have to release it without/before Dispatch*()ing it. MainThreadPtrHolder is of limited use here, since it has to be constructed on MainThread. One alternative is custom destructors to do NS_ProxyReleases as needed; this requires writing and maintaining code that likely never or rarely gets tested. And alternative is something like jib's bug 1154337, but that is overkill for runnables without this restriction (though we could make Dispatch/DispatchToMainThread take both). Perhaps we could instead modify MainThreadPtrHolder to allow creation from an already_AddRefed non-thread-safe object (avoiding the reason it currently has to be created on MainThread), and generalize it as ThreadSafePtrHolder or some such. It would allow creation with a raw pointer only on the target thread (if at all). Then just make these fields (like gUM's mOnSuccess/mOnFailure) be ThreadSafePtrHolder's, and .swap() them with the runnable's similar fields when setting up the runnable.

Comment on attachment 8593388 [details] [diff] [review] Patch 1 - WIP conversion of Dispatch() and friends to already_AddRefed<> Review of attachment 8593388 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall, but minusing because of a couple of points below. Also, I think bsmedberg has been opposed to this in the past (but I may be wrong.) Please get feedback from him as well. ::: dom/promise/Promise.cpp @@ +1144,5 @@ > new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport); > + nsRefPtr<AsyncErrorReporter> sacrifice = r; > + // If this fails, we know NS_DispatchToMainThread leaked on purpose, and we don't want that > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > + r->Release(); I don't think we should take this patch at all before making NS_DispatchToMainThread infallible. It's totally not worth adding these bad idioms to the tree, and more importantly, this pattern is outlawed in our static analyses so this won't even compile. ::: xpcom/threads/nsIEventTarget.idl @@ +38,2 @@ > void dispatch(in nsIRunnable event, in unsigned long flags); > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); You don't actually want two different virtual functions, right? Why not do something like this? %{C++ void Dispatch(already_AddRefed<...> ...); %} And just do the right thing in its implementation?

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5) > Comment on attachment 8593388 [details] [diff] [review] > Patch 1 - WIP conversion of Dispatch() and friends to already_AddRefed<> > > Review of attachment 8593388 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks reasonable, a couple comments below. I don't know whether folks would > want to see |already_AddRefed<T>&&| parameters put into ns{COM,Ref}Ptrs and > then .forget()'en or directly Move()'d when possible. Move()'ing seems > slightly more performant (no need for unnecessary smart pointer destructor > call). I prefer to Move(), but then we have to explicitly deal with any early returns to have a consistent behavior, and replace any hidden-returns (NS_ENSURE_*). So instead of: nsresult Dispatch(already_AddRefed<nsIRunnable>&& aEvent, uint32_t aFlags) { nsCOMPtr<nsIRunnable> event(aEvent); if (something) { return NS_ERROR_FAILURE; } return DispatchInternal(event.forget(), aFlags); } you have: nsresult Dispatch(already_AddRefed<nsIRunnable>&& aEvent, uint32_t aFlags) { if (something) { nsCOMPtr<nsIRunnable> event(aEvent); // or more efficient but scarier: aEvent.take()->Release(); return NS_ERROR_FAILURE; } return DispatchInternal(Move(aEvent), aFlags); } I recoded things from the second to the first (when it mattered due to early returns) to keep the patch simpler, but I could move things to the second pretty easily. I probably prefer the second (Move()) for efficiency, but the first is simpler to read and understand (and less chance of messing up with future changes). > ::: dom/promise/Promise.cpp > @@ +1144,5 @@ > > new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport); > > + nsRefPtr<AsyncErrorReporter> sacrifice = r; > > + // If this fails, we know NS_DispatchToMainThread leaked on purpose, and we don't want that > > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > > + r->Release(); > > I think the comment could be a little more complete here; calling Release() > directly on a smart pointer is not the most obvious thing in the world. > Comments also end with periods. Ah, the perennial discussion, comment style ;-) ok, np > ::: dom/workers/WorkerPrivate.cpp > @@ +2873,4 @@ > > { > > // May be called on any thread! > > + nsRefPtr<WorkerControlRunnable> runnable(aWorkerControlRunnable); > > + MOZ_ASSERT(runnable); > > Nit: keep the whitespace, and just swap the two lines, as you did below for > DispatchDebuggerRunnable below. MOZ_ASSERT() can't be called on already_AddRefed<>... > @@ +7309,5 @@ > > return NS_ERROR_UNEXPECTED; > > } > > > > + if (event) { > > + workerRunnable = mWorkerPrivate->MaybeWrapAsWorkerRunnable(event.forget()); > > Couldn't we use Move(aRunnable) here, and then we don't need |event|? See above; I'll check this one. > ::: dom/workers/WorkerThread.cpp > @@ +187,2 @@ > > > > + nsresult rv = nsThread::Dispatch(runnable.forget(), NS_DISPATCH_NORMAL); > > This can just be Move(aWorkerRunnable), can't it? Not if we've stuffed it in an nsCOMPtr/RefPtr... per above, if we don't stuff it in the no-errors path, then we can use Move(). > ::: xpcom/glue/nsThreadUtils.cpp > @@ +174,5 @@ > > nsCOMPtr<nsIThread> thread; > > nsresult rv = NS_GetMainThread(getter_AddRefs(thread)); > > if (NS_WARN_IF(NS_FAILED(rv))) { > > + // NOTE: if you stop leaking here, adjust Promise::MaybeReportRejected(), > > + // which assumes a leak here, or split into leaks and no-leaks versions > > This comment is kind of cryptic. I'll expand. > > ::: xpcom/threads/nsIEventTarget.idl > @@ +38,1 @@ > > void dispatch(in nsIRunnable event, in unsigned long flags); > > I wonder if we could figure out how to make nsIEventTarget::Dispatch the > obvious implementation that forwarded to the already_AddRefed Dispatch...but > nsIEventTarget::Dispatch would have to stay in the vtable somehow to not > break JS... A followup patch will remove (or semi-hide0 the rawptr dispatch (after fixing all the Dispatch(rawptr's)). > @@ +38,2 @@ > > void dispatch(in nsIRunnable event, in unsigned long flags); > > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); > > I think this should be [noscript]? Yes, forgot that. > ::: xpcom/threads/nsThreadPool.h > @@ +32,5 @@ > > private: > > ~nsThreadPool(); > > > > void ShutdownThread(nsIThread* aThread); > > nsresult PutEvent(nsIRunnable* aEvent); > > Do we need the pointer version of this anymore? It looks like we don't. > Same question for nsEventQueue::PutEvent, etc. Quite possibly; this was iterative when developing it. I'll try yanking it now (less of a problem than Dispatch(rawptr))

> This looks good overall, but minusing because of a couple of points below. ok, thanks > Also, I think bsmedberg has been opposed to this in the past (but I may be > wrong.) Please get feedback from him as well. yes, I noticed that last night, will do. > > ::: dom/promise/Promise.cpp > @@ +1144,5 @@ > > new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport); > > + nsRefPtr<AsyncErrorReporter> sacrifice = r; > > + // If this fails, we know NS_DispatchToMainThread leaked on purpose, and we don't want that > > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > > + r->Release(); > > I don't think we should take this patch at all before making > NS_DispatchToMainThread infallible. It's totally not worth adding these bad > idioms to the tree, and more importantly, this pattern is outlawed in our > static analyses so this won't even compile. Sure, ok, but note this means we'll need a doesn't-leak-on-failure variant of DispatchToMainThread(), since this code wants to not leak - and I've proved with a Try run it will fail (and leak without this) in Try. > ::: xpcom/threads/nsIEventTarget.idl > @@ +38,2 @@ > > void dispatch(in nsIRunnable event, in unsigned long flags); > > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); > > You don't actually want two different virtual functions, right? Why not do > something like this? > > %{C++ > void Dispatch(already_AddRefed<...> ...); > %} > > And just do the right thing in its implementation? Dispatch(rawptr) will die in a part 2 or 3 patch here (after the patch that changes everything to not do it), so I didn't care much.

Comment on attachment 8593388 [details] [diff] [review] Patch 1 - WIP conversion of Dispatch() and friends to already_AddRefed<> Benjamin - we noted you've had objections to avoid rawptrs for DispatchToMain in the past, so feedbacking you

(In reply to Randell Jesup [:jesup] from comment #9) > > ::: dom/promise/Promise.cpp > > @@ +1144,5 @@ > > > new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport); > > > + nsRefPtr<AsyncErrorReporter> sacrifice = r; > > > + // If this fails, we know NS_DispatchToMainThread leaked on purpose, and we don't want that > > > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > > > + r->Release(); > > > > I don't think we should take this patch at all before making > > NS_DispatchToMainThread infallible. It's totally not worth adding these bad > > idioms to the tree, and more importantly, this pattern is outlawed in our > > static analyses so this won't even compile. > > Sure, ok, but note this means we'll need a doesn't-leak-on-failure variant > of DispatchToMainThread(), since this code wants to not leak - and I've > proved with a Try run it will fail (and leak without this) in Try. No. The right thing to do is fix the code that's trying to dispatch things after dispatch isn't doing anything anymore. Let's not have leaking/non-leaking versions of the API.

(In reply to Randell Jesup [:jesup] from comment #9) > > ::: dom/promise/Promise.cpp > > @@ +1144,5 @@ > > > new AsyncErrorReporter(CycleCollectedJSRuntime::Get()->Runtime(), xpcReport); > > > + nsRefPtr<AsyncErrorReporter> sacrifice = r; > > > + // If this fails, we know NS_DispatchToMainThread leaked on purpose, and we don't want that > > > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > > > + r->Release(); > > > > I don't think we should take this patch at all before making > > NS_DispatchToMainThread infallible. It's totally not worth adding these bad > > idioms to the tree, and more importantly, this pattern is outlawed in our > > static analyses so this won't even compile. > > Sure, ok, but note this means we'll need a doesn't-leak-on-failure variant > of DispatchToMainThread(), since this code wants to not leak - and I've > proved with a Try run it will fail (and leak without this) in Try. I agree with Nathan on this. > > ::: xpcom/threads/nsIEventTarget.idl > > @@ +38,2 @@ > > > void dispatch(in nsIRunnable event, in unsigned long flags); > > > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); > > > > You don't actually want two different virtual functions, right? Why not do > > something like this? > > > > %{C++ > > void Dispatch(already_AddRefed<...> ...); > > %} > > > > And just do the right thing in its implementation? > > Dispatch(rawptr) will die in a part 2 or 3 patch here (after the patch that > changes everything to not do it), so I didn't care much. How can you kill that without killing the scriptability of that function?

> > > > + if (NS_FAILED(NS_DispatchToMainThread(sacrifice.forget()))) { > > > > + r->Release(); > > > > > > I don't think we should take this patch at all before making > > > NS_DispatchToMainThread infallible. It's totally not worth adding these bad > > > idioms to the tree, and more importantly, this pattern is outlawed in our > > > static analyses so this won't even compile. > > > > Sure, ok, but note this means we'll need a doesn't-leak-on-failure variant > > of DispatchToMainThread(), since this code wants to not leak - and I've > > proved with a Try run it will fail (and leak without this) in Try. > > I agree with Nathan on this. Sure. Just note that to avoid leaks, I'll need to do a DispatchToMainThreadDoesntLeakOnNoMainthread() or equivalent - I proved with my Try it will fail/leak without the above bit of code right now. > > > > ::: xpcom/threads/nsIEventTarget.idl > > > @@ +38,2 @@ > > > > void dispatch(in nsIRunnable event, in unsigned long flags); > > > > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); > > > > > > You don't actually want two different virtual functions, right? Why not do > > > something like this? > > > > > > %{C++ > > > void Dispatch(already_AddRefed<...> ...); > > > %} > > > > > > And just do the right thing in its implementation? > > > > Dispatch(rawptr) will die in a part 2 or 3 patch here (after the patch that > > changes everything to not do it), so I didn't care much. > > How can you kill that without killing the scriptability of that function? Can't I just replace it with the bit right above: [binaryname(DispatchFromScript)] void dispatch(in nsIRunnable event... such that people won't use it by mistake from c++?

> > Sure, ok, but note this means we'll need a doesn't-leak-on-failure variant > > of DispatchToMainThread(), since this code wants to not leak - and I've > > proved with a Try run it will fail (and leak without this) in Try. > > No. The right thing to do is fix the code that's trying to dispatch things > after dispatch isn't doing anything anymore. Let's not have > leaking/non-leaking versions of the API. Sorry, missed this comment. I'll try to see if I can figure out why they're doing this.

(In reply to Randell Jesup [:jesup] from comment #13) > > > > ::: xpcom/threads/nsIEventTarget.idl > > > > @@ +38,2 @@ > > > > > void dispatch(in nsIRunnable event, in unsigned long flags); > > > > > + [binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); > > > > > > > > You don't actually want two different virtual functions, right? Why not do > > > > something like this? > > > > > > > > %{C++ > > > > void Dispatch(already_AddRefed<...> ...); > > > > %} > > > > > > > > And just do the right thing in its implementation? > > > > > > Dispatch(rawptr) will die in a part 2 or 3 patch here (after the patch that > > > changes everything to not do it), so I didn't care much. > > > > How can you kill that without killing the scriptability of that function? > > Can't I just replace it with the bit right above: > [binaryname(DispatchFromScript)] void dispatch(in nsIRunnable event... > such that people won't use it by mistake from c++? Oh, if that's what you meant by "die" above, then yeah, I guess so.

I found where Promise DispatchToMainthread is failing (not surprising given the comments in the bugs that landed the code). It's happening in the ShutdownCollect() of the CC, called from nsCycleCollecter_shutdown(). (The final sequence is Promise::cycleCollection::Unlink() -> Promise::MaybeReportRejectedOnce() -> Promise::MaybeReportRejected()). So I guess the issue is for the promise people to fix the issue by finding a way to avoid these sitting unresolved up to the point of final collection, or find some way to know we should skip DispatchToMainthread (the irony is we *are* on MainThread, but at this point in shutdown we've nulled out the MainThread ptr. I'm not certain how easy this is at all; CCing a few people. I suggest we file a separate bug on it (and have that block the infallible-dispatchtomainthread landing). Alternatively, make NS_IsMainThread() continue to work in shutdown after we stop processing the event queue so we can know we can safely just Run() in DispatchToMainthread if it would fail do Dispatch(). Other threads (non-nsThread one presumes) trying to DispatchToMainthread at that point would still fail, and just leak the runnable. NI'ing people responsible for bug 958684 and bug 887687 that caused Promises to need this

Comment on attachment 8593388 [details] [diff] [review] Patch 1 - WIP conversion of Dispatch() and friends to already_AddRefed<> I'm unhappy with this pattern, but I've already said that I prefer the raw-pointer version in several bugs related to this. Ultimately this is Nathan's decision to make.

(In reply to Randell Jesup [:jesup] from comment #16) > So I guess the issue is for the promise people to fix the issue by finding a > way to avoid these sitting unresolved up to the point of final collection, > or find some way to know we should skip DispatchToMainthread (the irony is > we *are* on MainThread, but at this point in shutdown we've nulled out the > MainThread ptr. I'm not certain how easy this is at all; CCing a few > people. I suggest we file a separate bug on it (and have that block the > infallible-dispatchtomainthread landing). I'm not too familiar with Promise lifetime, but it looks like this code is expecting to fail when run in the CC. I'd think then that the fix to it failing in shutdown would be to guard the call to DispatchToMainThread with whatever check DispatchToMainThread is now. I don't entirely understand the thread utils code, but it seems to mostly hinge on whether the thread manager is still around.

Per discussion with bsmedberg, one option to preserve ease-of-use (similar to DispatchToMainthread(new foo(...))) would be a sprinkle of template/c++ magic, to allow: DispatchToMainthread(do_AddRef(new foo(...))); do_AddRef(rawptr) would add a ref and return already_AddRefed<> (as is obvious)

(In reply to Randell Jesup [:jesup] from comment #19) > Per discussion with bsmedberg, one option to preserve ease-of-use (similar > to DispatchToMainthread(new foo(...))) would be a sprinkle of template/c++ > magic, to allow: > DispatchToMainthread(do_AddRef(new foo(...))); > > do_AddRef(rawptr) would add a ref and return already_AddRefed<> (as is > obvious) That sounds great to me. I've often been tempted to add this functionality, just never got around to it.

(In reply to Randell Jesup [:jesup] from comment #19) > Per discussion with bsmedberg, one option to preserve ease-of-use (similar > to DispatchToMainthread(new foo(...))) would be a sprinkle of template/c++ > magic, to allow: > DispatchToMainthread(do_AddRef(new foo(...))); > > do_AddRef(rawptr) would add a ref and return already_AddRefed<> (as is > obvious) Over in bug 1116905, we discussed a MakeAndAddRef<T>(args...) template function for providing this sort of thing. Our objective there was to help make things nicer when getting rid of warts in MFBT, but if folks have pined for this technique, I'd be happy to see it added to XPCOM too.

(In reply to Randell Jesup [:jesup] from comment #16) > I found where Promise DispatchToMainthread is failing (not surprising given > the comments in the bugs that landed the code). It's happening in the > ShutdownCollect() of the CC, called from nsCycleCollecter_shutdown(). (The > final sequence is Promise::cycleCollection::Unlink() -> > Promise::MaybeReportRejectedOnce() -> Promise::MaybeReportRejected()). > > So I guess the issue is for the promise people to fix the issue by finding a > way to avoid these sitting unresolved up to the point of final collection, > or find some way to know we should skip DispatchToMainthread (the irony is > we *are* on MainThread, but at this point in shutdown we've nulled out the > MainThread ptr. I'm not certain how easy this is at all; CCing a few > people. I suggest we file a separate bug on it (and have that block the > infallible-dispatchtomainthread landing). > > Alternatively, make NS_IsMainThread() continue to work in shutdown after we > stop processing the event queue so we can know we can safely just Run() in > DispatchToMainthread if it would fail do Dispatch(). Other threads > (non-nsThread one presumes) trying to DispatchToMainthread at that point > would still fail, and just leak the runnable. > > NI'ing people responsible for bug 958684 and bug 887687 that caused Promises > to need this Bug 1083361 and friends should just make this go away. David, do we have a timeline for removing the deprecated bits?

This implementation of do_AddRef() seems to do the trick, and allows 'NS_DispatchToMainThread(do_AddRef(new MyRunnable(arg1, arg2)));'

We haven't discussed a timeline, plus I'm working on something seriously different at the moment. bz, do you have a clearer idea?

It's basically blocked on devtools UI adding support for the new setup. I know Nick has been at least looking into it... I'm pretty sure we have nothing resembling ETAs yet.

AFAIK, devtools product folks are prioritizing it within the rest of devtools work. No idea on an ETA.

Comment on attachment 8595141 [details] [diff] [review] Patch 2 - add do_AddRef() Review of attachment 8595141 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for taking so long to get to this. Why do you have the nsCOMPtr version? Since we intend for this to traffic only with concrete types coming in, using nsRefPtr in a two-arg template version and then relying on the compiler to convert should work just fine, I would think. I think if we make the argument T*&&, we'll prevent people from saying: T* bar = ... foo = do_AddRef(bar) which sounds like a good thing.

> Why do you have the nsCOMPtr version? Since we intend for this to traffic > only with concrete types coming in, using nsRefPtr in a two-arg template > version and then relying on the compiler to convert should work just fine, I > would think. Seems to; I'll roll that in. I started with nsCOMPtr was why. > I think if we make the argument T*&&, we'll prevent people from saying: > > T* bar = ... > foo = do_AddRef(bar) > > which sounds like a good thing. Trying that too, thanks. Looks happy

Comment on attachment 8596847 [details] [diff] [review] Patch 2 - add do_AddRef() Review of attachment 8596847 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsRefPtr.h @@ +528,5 @@ > /*****************************************************************************/ > > +template <class T> > +inline already_AddRefed<T> > +do_AddRef(T*&& aObj) I checked for myself, and sadly, this would accept |do_AddRef(&var)| (it happily doesn't accept do_AddRef(var)). Half a loaf is better than none, I suppose.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7) > I don't think we should take this patch at all before making > NS_DispatchToMainThread infallible. Making NS_DispatchToMainThread assert on failure would mean that it could not be used from destructors or unlink methods, without a WillDispatchToMainThreadSucceed() method. I don't like that kind of API. It certainly doesn't work well with threads and, even on the main thread only, a fallible method is tidier. I don't mind if it is a variant of an infallible method. Attempts to make dispatch infallible have been going since 2012-10-19 at least, suggesting that we can't expect that to succeed soon.

this was needed because of MSVC's handling of overloads, per Neil's comments in #developers

r? to John Daggett/khuey/froyd since it's unclear who should review this change; John Dagget and Kyle were the last to touch the code

there may be a better way that avoids it trying to do this too late, but this is safe

First step in converting the tree wholesale to already_AddRefed<>

Comment on attachment 8622872 [details] [diff] [review] Patch 0 - add do_AddRef() Carry forward r=froyd

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d87ac06664b All oranges are unrelated to this

Comment on attachment 8622881 [details] [diff] [review] Patch 5 - clean up ServiceWorkers and avoid leaks Review of attachment 8622881 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting review to nsm. I don't know if its safe to ignore the TeardownRunnable in the destructor.

> Redirecting review to nsm. I don't know if its safe to ignore the > TeardownRunnable in the destructor. I hope it is safe to ignore, since it's not running now... and if it did it would Assert ;-) That said, it should be reworked to shut down earlier. BTW, bug XXXXXX will be replaced with bug 1174381

Comment on attachment 8622892 [details] [diff] [review] Patch 8 - Don't leak runnables when MediaCache/FileBlockCache get shut down after XPCOM is in final shutdown Review of attachment 8622892 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/FileBlockCache.cpp @@ +78,5 @@ > + if (mainThread) { > + nsCOMPtr<nsIRunnable> event = new ShutdownThreadEvent(mThread); > + mainThread->Dispatch(event.forget(), NS_DISPATCH_NORMAL); > + } else { > + mThread->Shutdown(); // we're on Mainthread already - safe to do The comment above explains why this isn't supposed to be safe. Can you add a comment here explaining why it's OK to do this?

Comment on attachment 8622890 [details] [diff] [review] Patch 6 - fix problems with gfxFontInfoLoader shutdown sequence I claim that the font guys are the correct reviewer.

Comment on attachment 8622890 [details] [diff] [review] Patch 6 - fix problems with gfxFontInfoLoader shutdown sequence > --- a/gfx/thebes/gfxFontInfoLoader.cpp > +++ b/gfx/thebes/gfxFontInfoLoader.cpp > @@ -61,32 +61,30 @@ class AsyncFontInfoLoader : public nsRun > nsresult > FontInfoLoadCompleteEvent::Run() > { > gfxFontInfoLoader *loader = > static_cast<gfxFontInfoLoader*>(gfxPlatformFontList::PlatformFontList()); > > loader->FinalizeLoader(mFontInfo); > > - mFontInfo = nullptr; > return NS_OK; > } > > NS_IMPL_ISUPPORTS_INHERITED0(FontInfoLoadCompleteEvent, nsRunnable); > > // runs on separate thread > nsresult > AsyncFontInfoLoader::Run() > { > // load platform-specific font info > mFontInfo->Load(); > > // post a completion event that transfer the data to the fontlist > NS_DispatchToMainThread(mCompleteEvent); > - mFontInfo = nullptr; > > return NS_OK; > } Randell, what's the reasoning behind these two deletions?

> Randell, what's the reasoning behind these two deletions? Simply cleanup IIRC.... those will get released on runnable deletion anyways

Comment on attachment 8622890 [details] [diff] [review] Patch 6 - fix problems with gfxFontInfoLoader shutdown sequence Review of attachment 8622890 [details] [diff] [review]: ----------------------------------------------------------------- jdaggett's review is good enough for me.

Comment on attachment 8622895 [details] [diff] [review] Patch 9 - Modify DataChannel.cpp to use updated API Review of attachment 8622895 [details] [diff] [review]: ----------------------------------------------------------------- I think this patch will be nicer when we can use MakeAndAddRef, but that can be future work.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aca7f0f64a2d I think the W(4) failures are something I just hit due to the inbound rev I rebased to.

Comment on attachment 8622876 [details] [diff] [review] Patch 3 - fix leaks in Promise and ConsoleService Review of attachment 8622876 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/promise/Promise.cpp @@ +1217,5 @@ > // AsyncErrorReporter, otherwise if the call to DispatchToMainThread fails, it > + // will leak. See Bug 958684. So... don't use DispatchToMainThread() > + nsCOMPtr<nsIThread> mainThread = do_GetMainThread(); > + if (NS_WARN_IF(!mainThread)) { > + NS_WARNING("Trying to report rejected Promise after MainThread shutdown"); Will making this a warning ensure we turn tests orange? And shouldn't this be included in part 1? ::: xpcom/base/nsConsoleService.cpp @@ +297,5 @@ > if (r) { > + // avoid failing in XPCShell tests > + nsCOMPtr<nsIThread> mainThread = do_GetMainThread(); > + if (mainThread) { > + NS_DispatchToMainThread(r); Shouldn't this be r.forget()?

Comment on attachment 8622875 [details] [diff] [review] Patch 2 - Modify Dispatch IDL and code to deal with MSVC issues with overloaded templates Review of attachment 8622875 [details] [diff] [review]: ----------------------------------------------------------------- This should be rolled into part 1, I think? I would like to understand the nsIEventTarget inheritance thing and to see the commenting in nsIEventTarget fixed before r+. ::: xpcom/threads/LazyIdleThread.h @@ +44,5 @@ > NS_DECL_NSITHREAD > NS_DECL_NSITIMERCALLBACK > NS_DECL_NSITHREADOBSERVER > NS_DECL_NSIOBSERVER > + // missing from NS_DECL_NSIEVENTTARGET because MSVC Shouldn't we get this from inheriting from nsIEventTarget, though? I would expect the %C++ block from nsIEventTarget.idl to be picked up... ::: xpcom/threads/nsIEventTarget.idl @@ +36,2 @@ > /* until I can get rid of it */ > + /*void dispatch(in nsIRunnable event, in unsigned long flags);*/ This commenting-out/uncommenting things requires some cleanup. The interface needs a uuid bump, I think, even though you bumped it in the last patch.

Comment on attachment 8622873 [details] [diff] [review] Patch 1 - Convert Dispatch() and friends to already_AddRefed<> Review of attachment 8622873 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/nsEventQueue.h @@ +9,5 @@ > > #include <stdlib.h> > #include "mozilla/ReentrantMonitor.h" > #include "nsIRunnable.h" > +#include "nsCOMPtr.h" mozilla/AlreadyAddRefed.h, please. ::: xpcom/threads/nsIEventTarget.idl @@ +6,5 @@ > > #include "nsISupports.idl" > +/* for already_AddRefed */ > +%{C++ > +#include "nsCOMPtr.h" #include "mozilla/AlreadyAddRefed.h", please. @@ +38,2 @@ > void dispatch(in nsIRunnable event, in unsigned long flags); > + [noscript, binaryname(Dispatch)] void dispatchFromC(in alreadyAddRefed_nsIRunnable event, in unsigned long flags); This needs to go at the end of the IDL file, after isOnCurrentThread. The commented-out stuff also needs to go.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #54) > Comment on attachment 8622875 [details] [diff] [review] > Patch 2 - Modify Dispatch IDL and code to deal with MSVC issues with > overloaded templates > > Review of attachment 8622875 [details] [diff] [review]: > ----------------------------------------------------------------- > > This should be rolled into part 1, I think? I would like to understand the > nsIEventTarget inheritance thing and to see the commenting in nsIEventTarget > fixed before r+. > > ::: xpcom/threads/LazyIdleThread.h > @@ +44,5 @@ > > NS_DECL_NSITHREAD > > NS_DECL_NSITIMERCALLBACK > > NS_DECL_NSITHREADOBSERVER > > NS_DECL_NSIOBSERVER > > + // missing from NS_DECL_NSIEVENTTARGET because MSVC > > Shouldn't we get this from inheriting from nsIEventTarget, though? I would > expect the %C++ block from nsIEventTarget.idl to be picked up... It isn't... Why? no idea, see the xpidl source (ugh). Likely because %c++ stuff isn't parsed (just pushed out into the .h), and so isn't used to build the macros (and in fact might be tricky since it can include moderately arbitrary code). This entire patch is due to MSVC being evil and messing up the vtables per NeilAway's comments in #developers. Otherwise we didn't need the %C++ block to begin with, and none of all these workarounds. I hope we can remove them someday... > > ::: xpcom/threads/nsIEventTarget.idl > @@ +36,2 @@ > > /* until I can get rid of it */ > > + /*void dispatch(in nsIRunnable event, in unsigned long flags);*/ > > This commenting-out/uncommenting things requires some cleanup. The > interface needs a uuid bump, I think, even though you bumped it in the last > patch. Sure, though this "fixes" the previous patch so it compiles on Windows. I want to keep them separate patches... No big deal to rev the UUID though.

> ::: dom/promise/Promise.cpp > @@ +1217,5 @@ > > // AsyncErrorReporter, otherwise if the call to DispatchToMainThread fails, it > > + // will leak. See Bug 958684. So... don't use DispatchToMainThread() > > + nsCOMPtr<nsIThread> mainThread = do_GetMainThread(); > > + if (NS_WARN_IF(!mainThread)) { > > + NS_WARNING("Trying to report rejected Promise after MainThread shutdown"); > > Will making this a warning ensure we turn tests orange? And shouldn't this > be included in part 1? I may merge some of these for landing Making that an NS_ASSERTION will crash xpcshell tests everywhere; they leave dangling promises that get cleaned up late in shutdown. See http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rjesup@wgate.com-62ac7e3ead0e/try-linux-debug/try_ubuntu32_vm-debug_test-xpcshell-bm05-tests1-linux32-build923.txt.gz (with NS_ASSERTION there).

(In reply to Randell Jesup [:jesup] from comment #56) > (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #54) > > Comment on attachment 8622875 [details] [diff] [review] > > Patch 2 - Modify Dispatch IDL and code to deal with MSVC issues with > > overloaded templates > > > > Review of attachment 8622875 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This should be rolled into part 1, I think? I would like to understand the > > nsIEventTarget inheritance thing and to see the commenting in nsIEventTarget > > fixed before r+. > > > > ::: xpcom/threads/LazyIdleThread.h > > @@ +44,5 @@ > > > NS_DECL_NSITHREAD > > > NS_DECL_NSITIMERCALLBACK > > > NS_DECL_NSITHREADOBSERVER > > > NS_DECL_NSIOBSERVER > > > + // missing from NS_DECL_NSIEVENTTARGET because MSVC > > > > Shouldn't we get this from inheriting from nsIEventTarget, though? I would > > expect the %C++ block from nsIEventTarget.idl to be picked up... > > It isn't... Why? no idea, see the xpidl source (ugh). Likely because %c++ > stuff isn't parsed (just pushed out into the .h), and so isn't used to build > the macros (and in fact might be tricky since it can include moderately > arbitrary code). That can't be right. See, for instance: http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsITimer.idl#200 which does show up in dist/include/nsITimer.h in the declaration of nsITimer. I just added a %{C++ block to nsIEventTarget.idl for a Dispatch() overload and that worked too...

> > > > NS_DECL_NSIOBSERVER > > > > + // missing from NS_DECL_NSIEVENTTARGET because MSVC > > > > > > Shouldn't we get this from inheriting from nsIEventTarget, though? I would > > > expect the %C++ block from nsIEventTarget.idl to be picked up... > > > > It isn't... Why? no idea, see the xpidl source (ugh). Likely because %c++ > > stuff isn't parsed (just pushed out into the .h), and so isn't used to build > > the macros (and in fact might be tricky since it can include moderately > > arbitrary code). > > That can't be right. See, for instance: > > http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsITimer.idl#200 > > which does show up in dist/include/nsITimer.h in the declaration of > nsITimer. I just added a %{C++ block to nsIEventTarget.idl for a Dispatch() > overload and that worked too... Ok, memory refreshed (it's been a while....) The issue is that if Dispatch is virtual, MSVC messes up the vtbl ordering apparently and horrible things happen on Try. So the C++ block has a *non*-virtual Dispatch(), which means we need to add it to a lot of classes. This was the solution to the compiler screwup that Neil gave me (and it works). I'll add some comments.

Should cover the questions from the f+ review - new UUID, fix up ordering and commenting/etc

Comment on attachment 8629076 [details] [diff] [review] Patch 2 - Modify Dispatch IDL and code to deal with MSVC issues with overloaded templates Review of attachment 8629076 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. Thanks for walking me through the problems on IRC. ::: xpcom/threads/nsIEventTarget.idl @@ +20,4 @@ > /** > + * This must be non-virtual due to issues with MSVC 2013's ordering of > + * vtbls for overloads. With other platforms we can leave this virtual > + * and avoid adding lots of Dispatch() methods to classes inheriting this. As previously discussed on IRC, if subclasses of nsIEventTarget have to add their own declarations of this function, then there's effectively no value in having it, right? Please delete this. And please file a followup for getting rid of the raw-pointer Dispatch overload in the subclasses. @@ +87,5 @@ > + * @throws NS_ERROR_UNEXPECTED > + * Indicates that the thread is shutting down and has finished processing > + * events, so this event would never run and has not been dispatched. > + */ > + [binaryname(DispatchFromScript)] void dispatch(in nsIRunnable event, in unsigned long flags); This bit needs to remain in vtable order relative to the previous version of nsIEventTarget. So it needs to be moved back to where it was. Please commit these patches in an ordered and squashed fashion that actually bisects; it's not clear to me that part 1 compiles standalone, for instance... ::: xpcom/threads/nsThread.h @@ +28,5 @@ > NS_DECL_NSIEVENTTARGET > NS_DECL_NSITHREAD > NS_DECL_NSITHREADINTERNAL > NS_DECL_NSISUPPORTSPRIORITY > + // missing from NS_DECL_NSIEVENTTARGET because MSVC This is partly MSVC and partly the standard, AFAICT. Since we're removing the raw-pointer Dispatch overload, please remove this comment here and elsewhere.

> ::: xpcom/threads/nsIEventTarget.idl > @@ +20,4 @@ > > /** > > + * This must be non-virtual due to issues with MSVC 2013's ordering of > > + * vtbls for overloads. With other platforms we can leave this virtual > > + * and avoid adding lots of Dispatch() methods to classes inheriting this. > > As previously discussed on IRC, if subclasses of nsIEventTarget have to add > their own declarations of this function, then there's effectively no value > in having it, right? Please delete this. Anything using an nsCOMPtr<nsIThread> for thread->Dispatch(event, ...) will break without it, sorry. > And please file a followup for getting rid of the raw-pointer Dispatch > overload in the subclasses. Sure. > @@ +87,5 @@ > > + * @throws NS_ERROR_UNEXPECTED > > + * Indicates that the thread is shutting down and has finished processing > > + * events, so this event would never run and has not been dispatched. > > + */ > > + [binaryname(DispatchFromScript)] void dispatch(in nsIRunnable event, in unsigned long flags); > > This bit needs to remain in vtable order relative to the previous version of > nsIEventTarget. So it needs to be moved back to where it was. Sure. I moved it because I was asked to (IIRC) > > Please commit these patches in an ordered and squashed fashion that actually > bisects; it's not clear to me that part 1 compiles standalone, for > instance... Of course; they're as many small patches for reviewing purposes. > > ::: xpcom/threads/nsThread.h > @@ +28,5 @@ > > NS_DECL_NSIEVENTTARGET > > NS_DECL_NSITHREAD > > NS_DECL_NSITHREADINTERNAL > > NS_DECL_NSISUPPORTSPRIORITY > > + // missing from NS_DECL_NSIEVENTTARGET because MSVC > > This is partly MSVC and partly the standard, AFAICT. Since we're removing > the raw-pointer Dispatch overload, please remove this comment here and > elsewhere. See above, we can't remove it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81134eb682a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/2265e031ab97 https://hg.mozilla.org/integration/mozilla-inbound/rev/108445a3840e https://hg.mozilla.org/integration/mozilla-inbound/rev/85ecde08d394 https://hg.mozilla.org/integration/mozilla-inbound/rev/314c54546479 https://hg.mozilla.org/integration/mozilla-inbound/rev/b768d4e3b9b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/b2d47f4d2e52 https://hg.mozilla.org/integration/mozilla-inbound/rev/470d574f3f49

https://hg.mozilla.org/mozilla-central/rev/81134eb682a1 https://hg.mozilla.org/mozilla-central/rev/2265e031ab97 https://hg.mozilla.org/mozilla-central/rev/108445a3840e https://hg.mozilla.org/mozilla-central/rev/85ecde08d394 https://hg.mozilla.org/mozilla-central/rev/314c54546479 https://hg.mozilla.org/mozilla-central/rev/b768d4e3b9b4 https://hg.mozilla.org/mozilla-central/rev/b2d47f4d2e52 https://hg.mozilla.org/mozilla-central/rev/470d574f3f49

(In reply to Randell Jesup [:jesup] from comment #59) > > Ok, memory refreshed (it's been a while....) The issue is that if Dispatch > is virtual, MSVC messes up the vtbl ordering apparently and horrible things > happen on Try. So the C++ block has a *non*-virtual Dispatch(), which means > we need to add it to a lot of classes. This was the solution to the > compiler screwup that Neil gave me (and it works). I still don't understand. Why do we need the "Dispatch(nsIRunnable*, uint32_t)" to be virtual from the very beginning? It is just a simple inline wrapper of the virtual "Dispatch(already_AddRefed, uint32_t)", no?

OK, I know why. So if a function have the same name declared in the subclass, the function in superclass is not considered as a candidate function anymore. That doesn't even matter whether the function is virtual, actually.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #66) > OK, I know why. So if a function have the same name declared in the > subclass, the function in superclass is not considered as a candidate > function anymore. That doesn't even matter whether the function is virtual, > actually. You can use |using| to import the base version into the derived so that it gets considered for overload resolution, I think.

(In reply to :Ehsan Akhgari from comment #67) > (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #66) > > OK, I know why. So if a function have the same name declared in the > > subclass, the function in superclass is not considered as a candidate > > function anymore. That doesn't even matter whether the function is virtual, > > actually. > > You can use |using| to import the base version into the derived so that it > gets considered for overload resolution, I think. Yes, that is proved to work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b51cabec4e2 I'll file a bug and submit this patch for review.

Filed bug 1238404 and submitted the patch.