If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

label AvailableRunnable

RESOLVED FIXED in Firefox 57

Status

()

Core
Canvas: WebGL
P3
normal
RESOLVED FIXED
3 months ago
a month ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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.
(Reporter)

Updated

3 months ago
No longer blocks: 1321812
(Reporter)

Updated

3 months ago
Blocks: 1321812

Updated

3 months ago
Whiteboard: [gfx-noted]
Priority: -- → P3
(Reporter)

Comment 1

2 months ago
Created attachment 8890040 [details] [diff] [review]
part 1 - centralize AvailableRunnable dispatching

This change will make labeling AvailableRunnable simpler, as we'll only
have to modify one location.
Attachment #8890040 - Flags: review?(jgilbert)
(Reporter)

Comment 2

2 months ago
Created attachment 8890042 [details] [diff] [review]
part 2 - label WebGLQuery's AvailableRunnable

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?
Attachment #8890042 - Flags: review?(wmccloskey)
Attachment #8890042 - Flags: review?(jgilbert)
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.
Attachment #8890042 - Flags: review?(wmccloskey) → review+
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
Attachment #8890040 - Flags: review?(jgilbert) → review+
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.
Attachment #8890042 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 6

2 months ago
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?
Flags: needinfo?(jgilbert)
(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.
Flags: needinfo?(jgilbert)
(Reporter)

Comment 8

2 months ago
Created attachment 8895095 [details] [diff] [review]
part 2 - label WebGLQuery's AvailableRunnable

Thanks for the explanation.  I went with billm's suggestion of continuing to
dispatch to the current thread.
Attachment #8895095 - Flags: review?(jgilbert)
Attachment #8895095 - Flags: review+
(Reporter)

Updated

2 months ago
Attachment #8890042 - Attachment is obsolete: true
Attachment #8895095 - Flags: review?(jgilbert) → review+
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

Comment 10

a month ago
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

Comment 11

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98b144a76336
https://hg.mozilla.org/mozilla-central/rev/a28654493547
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.