Recent telemetry has this runnable in the #7 spot: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLQuery.cpp?q=%22AvailableRunnable%22&redirect_type=direct#20 It doesn't look too hard to redirect this event into the correct queue; I think for offscreen canvases, we'll have to just accept that it's going in the SystemGroup, though.

This change will make labeling AvailableRunnable simpler, as we'll only have to modify one location.

If we have an associated canvas element, the query should go in the queue of the associated document. If not, the system group should be fine, as the query can't touch content. I'm unsure of this justification because this is code that I'm not very familiar with...Jeff, in what circumstances do we have a WebGLContext that doesn't have an associated canvas element? Can we use WebGL in a worker...? If so, is there an associated global/document that we could attach ourselves to?

Comment on attachment 8890042 [details] [diff] [review] part 2 - label WebGLQuery's AvailableRunnable Review of attachment 8890042 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLQuery.cpp @@ +63,5 @@ > > void > WebGLQuery::DispatchAvailableRunnable() > { > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); Is there any reason to do this instead of just using new? @@ +70,5 @@ > + canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other, > + runnable.forget()); > + return; > + } > + SystemGroup::Dispatch("AvailableRunnable", TaskCategory::Other, I would feel safer doing a normal (unlabeled) dispatch in this case.

Comment on attachment 8890040 [details] [diff] [review] part 1 - centralize AvailableRunnable dispatching Review of attachment 8890040 [details] [diff] [review]: ----------------------------------------------------------------- Prefer keeping implementation details out of the header. ::: dom/canvas/WebGLQuery.cpp @@ +61,5 @@ > LinkedListElement<WebGLQuery>::removeFrom(mContext->mQueries); > } > > +void > +WebGLQuery::DispatchAvailableRunnable() static void AvailableRunnable::Dispatch(WebGLQuery*) ::: dom/canvas/WebGLQuery.h @@ +49,5 @@ > > // WebGLRefCountedObject > void Delete(); > > + void DispatchAvailableRunnable(); Doesn't need to be a member. ::: dom/ipc/ContentParent.cpp @@ +2762,5 @@ > Unused << SendFlushMemory(nsDependentString(aData)); > } > // listening for remotePrefs... > else if (!strcmp(aTopic, "nsPref:changed")) { > + // We know prefs are ASCII here. Accidental EOL whitespace here

Comment on attachment 8890042 [details] [diff] [review] part 2 - label WebGLQuery's AvailableRunnable Review of attachment 8890042 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLQuery.cpp @@ +63,5 @@ > > void > WebGLQuery::DispatchAvailableRunnable() > { > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); I agree with billm. Use new. @@ +65,5 @@ > WebGLQuery::DispatchAvailableRunnable() > { > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); > + > + if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) { Assigning in a sub-expression is bad enough, but declaring? Absolutely not. @@ +66,5 @@ > { > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); > + > + if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) { > + canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other, Add a WebGLContext::GetOwnerDoc, and have it assert if there's not canvas element for now.

Thanks for the review! (In reply to Jeff Gilbert [:jgilbert] from comment #5) > @@ +65,5 @@ > > WebGLQuery::DispatchAvailableRunnable() > > { > > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); > > + > > + if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) { > > Assigning in a sub-expression is bad enough, but declaring? Absolutely not. At least with declaring, you don't have possible =/== confusion. But will change. > @@ +66,5 @@ > > { > > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); > > + > > + if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) { > > + canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other, > > Add a WebGLContext::GetOwnerDoc, and have it assert if there's not canvas > element for now. Given the "Get" prefix, I'm assuming you expect it to be written like: nsIDocument* WebGLContext::GetOwnerDoc() { MOZ_ASSERT(mCanvasElement); if (!mCanvasElement) { return nullptr; } return mCanvasElement->OwnerDoc(); } Is that a correct assumption, or are you suggesting to dispense with the !mCanvasElement check entirely?

(In reply to Nathan Froyd [:froydnj] from comment #6) > Thanks for the review! > > (In reply to Jeff Gilbert [:jgilbert] from comment #5) > > @@ +65,5 @@ > > > WebGLQuery::DispatchAvailableRunnable() > > > { > > > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); > > > + > > > + if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) { > > > > Assigning in a sub-expression is bad enough, but declaring? Absolutely not. > > At least with declaring, you don't have possible =/== confusion. But will > change. > > > @@ +66,5 @@ > > > { > > > + RefPtr<AvailableRunnable> runnable = MakeAndAddRef<AvailableRunnable>(this); > > > + > > > + if (mozilla::dom::HTMLCanvasElement* canvas = mContext->GetParentObject()) { > > > + canvas->OwnerDoc()->Dispatch("AvailableRunnable", TaskCategory::Other, > > > > Add a WebGLContext::GetOwnerDoc, and have it assert if there's not canvas > > element for now. > > Given the "Get" prefix, I'm assuming you expect it to be written like: > > nsIDocument* > WebGLContext::GetOwnerDoc() > { > MOZ_ASSERT(mCanvasElement); > if (!mCanvasElement) { > return nullptr; > } > return mCanvasElement->OwnerDoc(); > } > > Is that a correct assumption, or are you suggesting to dispense with the > !mCanvasElement check entirely? That sounds good.

Thanks for the explanation. I went with billm's suggestion of continuing to dispatch to the current thread.

revised the patches according to reviewer's comments and wait for the test result from treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=954664240e0d2a0eaf4f279a451fc945afd7662f

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/98b144a76336 part 1 - centralize AvailableRunnable dispatching; r=jgilbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a28654493547 part 2 - label WebGLQuery's AvailableRunnable; r=jgilbert,billm

https://hg.mozilla.org/mozilla-central/rev/98b144a76336 https://hg.mozilla.org/mozilla-central/rev/a28654493547