Closed Bug 1171228 Opened 9 years ago Closed 9 years ago

Expose WEBGL_debug_renderer_info extension to Web content

Categories

(Core :: Graphics, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- wontfix
firefox41 + fixed
firefox42 --- fixed

People

(Reporter: bugs, Assigned: jgilbert)

References

Details

(Keywords: dev-doc-complete, Whiteboard: webgl-extension, [gfx-noted])

Attachments

(3 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #742781 +++

We get external requests for Firefox to expose the WEBGL_debug_renderer_info extension like IE and Chrome do. eg. YouTube uses this info to log problematic hardware/driver combinations that are useful for debugging.

Bug 742781 exposes it for privileged content. This bug is about opening that up to Web content, potentially guarded by a domain whitelist.
We are going to lose battles to prevent fingerprinting :(
When dealing with some issues with Google Maps I was thinking about this and came up with the idea of making the request for this information prompt the user. That way when YouTube or the user detect a problem the website can prompt the user for the information. We can display the information that will be exposed to the website so the user can make a somewhat informed decision.

This allows websites to get information to help diagnose problems that they can forward on to us so that we can fix them while avoiding the problem where websites block hardware and we never discover the problem.
Flags: needinfo?(jgilbert)
Whiteboard: webgl-extension → webgl-extension, [gfx-noted]
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> When dealing with some issues with Google Maps I was thinking about this and
> came up with the idea of making the request for this information prompt the
> user. That way when YouTube or the user detect a problem the website can
> prompt the user for the information. We can display the information that
> will be exposed to the website so the user can make a somewhat informed
> decision.
> 
> This allows websites to get information to help diagnose problems that they
> can forward on to us so that we can fix them while avoiding the problem
> where websites block hardware and we never discover the problem.

I like this idea; Benjamin, somebody you can point us to for thoughts on the extra user intervention?  Just relaxing the chrome requirement is easy enough to do, but we're trying to test one more time if we can do this without the extra fingerprinting - even though we're being told that battle has already been lost.
Flags: needinfo?(jgilbert) → needinfo?(benjamin)
If this change lands, it would be helpful to uplift it to Beta so YouTube can help us isolate buggy GPUs.

If we don't want to expose WEBGL_debug_renderer_info to the entire web, maybe we could report it only in Firefox's pre-release channels or whitelist it for youtube.com.
I'm not a UX designer, but I don't feel like this is the kind of question we ought to be asking users. I don't think there's any way to present the choice to users in a way they could understand the tradeoffs or even why we're asking them to make a choice.
Flags: needinfo?(benjamin)
(In reply to Chris Peterson [:cpeterson] from comment #4)
> If this change lands, it would be helpful to uplift it to Beta so YouTube
> can help us isolate buggy GPUs.
> 
> If we don't want to expose WEBGL_debug_renderer_info to the entire web,
> maybe we could report it only in Firefox's pre-release channels or whitelist
> it for youtube.com.

I like both of these proposals.  Let's do a youtube.com whitelist and nightly+aurora+beta for now, and we can have another bug for the actual Release.

Kyle, Andrew, let's see if you guys can sort this one out.
Flags: needinfo?(kfung)
Flags: needinfo?(acomminos)
Okay we'll take a look at it
Flags: needinfo?(kfung)
Flags: needinfo?(acomminos)
Assignee: nobody → kfung
Here's the full version. We will be having a more concrete discussion about this soon.
Sorry for the confusion, but let's hold off on more complicated work here until we decide what it is we want.
Assignee: kfung → jgilbert
(In reply to Milan Sreckovic [:milan] from comment #6)
> I like both of these proposals.  Let's do a youtube.com whitelist and
> nightly+aurora+beta for now, and we can have another bug for the actual
> Release.

Milan, when you say want to do both a whitelist and pre-release channels, do you mean the intersection or the union of those conditions? (youtube AND pre-release) or (youtube OR pre-release)?

To get useful debug info from YouTube, I think we should implement the union (youtube OR pre-release). Google won't be able to fingerprint users by WEBGL_debug_renderer_info if our whitelist restricts WEBGL_debug_renderer_info to youtube.com. Our MSE whitelist also includes youtube-nocookie.com, but I think we should omit youtube-nocookie.com from the WEBGL_debug_renderer_info whitelist because users visiting that site clearly do not want to be tracked.

https://mxr.mozilla.org/mozilla-central/source/dom/media/mediasource/MediaSource.cpp#379
Flags: needinfo?(milan)
There is a "intent to implement" e-mail coming this week that will describe in details what we do now, and what may come later.  Let's wait for that.
Flags: needinfo?(milan)
I have posted an "Intent to implement and ship" email to dev-platform.
Attachment #8617080 - Flags: review?(dglastonbury)
Attachment #8617080 - Flags: superreview?(jst)
Attachment #8617628 - Flags: review?(dglastonbury)
Comment on attachment 8617080 [details] [diff] [review]
0001-Allow-unprivileged-WEBGL_debug_renderer_info.patch

Review of attachment 8617080 [details] [diff] [review]:
-----------------------------------------------------------------

Conditional r+ on StringValue(...) being sorted out.

::: dom/canvas/WebGLContextExtensions.cpp
@@ +159,5 @@
>          return gl->IsExtensionSupported(gl::GLContext::EXT_texture_compression_dxt1) &&
>                 gl->IsExtensionSupported(gl::GLContext::ANGLE_texture_compression_dxt3) &&
>                 gl->IsExtensionSupported(gl::GLContext::ANGLE_texture_compression_dxt5);
> +    case WebGLExtensionID::WEBGL_debug_renderer_info:
> +        {

Move on to previous line.

::: dom/canvas/WebGLContextState.cpp
@@ +73,5 @@
>      return JS::StringValue(str);
>  }
>  
> +static JS::Value
> +StringValue(JSContext* cx, const nsAString& str, ErrorResult& rv)

Didn't I move this function into WebGLContextUtils.h/.cpp? Please check and remove duplicate.
Attachment #8617080 - Flags: review?(dglastonbury) → review+
Attachment #8617628 - Flags: review?(dglastonbury) → review+
Depends on: 1175424
Johnny, is there any more information you need before you can sr+ Jeff's WEBGL_debug_renderer_info patch? With this change, YouTube will be able to collect and share data correlating GPUs to problems reported by YouTube users.
Flags: needinfo?(jst)
Comment on attachment 8617080 [details] [diff] [review]
0001-Allow-unprivileged-WEBGL_debug_renderer_info.patch

Nope, all good, just been digging myself out from under a pile of email after being on PTO and at Whistler. sr=jst
Flags: needinfo?(jst)
Attachment #8617080 - Flags: superreview?(jst) → superreview+
Good to land this?
Flags: needinfo?(jgilbert)
When talking to JeffG about this in Whistler we agreed that having some kind of white list of sites where this is exposed is the easiest way forward with this. That gets us youtube help without the problems that were brought up in the thread. It also allows us to police the usage to get feel for how things are done. The current patch does not have the white list.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> When talking to JeffG about this in Whistler we agreed that having some kind
> of white list of sites where this is exposed is the easiest way forward with
> this. That gets us youtube help without the problems that were brought up in
> the thread. It also allows us to police the usage to get feel for how things
> are done. The current patch does not have the white list.

The whitelist is (to me) non-trivial, and so it would be useful to let this land on Nightly for now, general access.

I think we should land (for Nightly, maybe Dev?) this now, even if we replace it.
Flags: needinfo?(jgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #21)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> > When talking to JeffG about this in Whistler we agreed that having some kind
> > of white list of sites where this is exposed is the easiest way forward with
> > this. That gets us youtube help without the problems that were brought up in
> > the thread. It also allows us to police the usage to get feel for how things
> > are done. The current patch does not have the white list.
> 
> The whitelist is (to me) non-trivial, and so it would be useful to let this
> land on Nightly for now, general access.

Would that be hidden behind #ifndef RELEASE_BUILD?  Otherwise, there is no difference between landing it on Nightly and shipping it.
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #22)
> (In reply to Jeff Gilbert [:jgilbert] from comment #21)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #19)
> > > When talking to JeffG about this in Whistler we agreed that having some kind
> > > of white list of sites where this is exposed is the easiest way forward with
> > > this. That gets us youtube help without the problems that were brought up in
> > > the thread. It also allows us to police the usage to get feel for how things
> > > are done. The current patch does not have the white list.
> > 
> > The whitelist is (to me) non-trivial, and so it would be useful to let this
> > land on Nightly for now, general access.
> 
> Would that be hidden behind #ifndef RELEASE_BUILD?  Otherwise, there is no
> difference between landing it on Nightly and shipping it.

Yes.
That sounds good to me.
Agree with Jeff.  Land this without a whitelist.  IF we have to keep it in nightly/aurora/beta, that's fine I suppose.
Flags: needinfo?(dougt)
Attached patch unmasked-renderer.diff (obsolete) — Splinter Review
It's not a huge diff, so let's just review it in its "final form".
Attachment #8617628 - Attachment is obsolete: true
Attachment #8634969 - Flags: review?(dglastonbury)
Comment on attachment 8634969 [details] [diff] [review]
unmasked-renderer.diff

Review of attachment 8634969 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/WebGLContextState.cpp
@@ +61,5 @@
>      gl->fEnable(cap);
>  }
>  
> +static JS::Value
> +StringValue(JSContext* cx, const nsAString& str, ErrorResult& rv)

Where did this come from? In a patch I moved it to be a shared utility function. Maybe that patch didn't land?
Attachment #8634969 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #27)
> Comment on attachment 8634969 [details] [diff] [review]
> unmasked-renderer.diff
> 
> Review of attachment 8634969 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/canvas/WebGLContextState.cpp
> @@ +61,5 @@
> >      gl->fEnable(cap);
> >  }
> >  
> > +static JS::Value
> > +StringValue(JSContext* cx, const nsAString& str, ErrorResult& rv)
> 
> Where did this come from? In a patch I moved it to be a shared utility
> function. Maybe that patch didn't land?

It's actually a new (though cribbed) function, as it deals with nsAString not nsACString.
Comment on attachment 8635515 [details] [diff] [review]
0006-Fix-test-to-allow-different-behavior-for-RELEASE_BUI.patch

Review of attachment 8635515 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/test/chrome/nonchrome_webgl_debug_renderer_info.html
@@ +35,5 @@
>    const UNMASKED_RENDERER_WEBGL = 0x9246;
>  
> +  var shouldHaveRendererInfo = false;
> +  if (!AppConstants.RELEASE_BUILD)
> +    shouldHaveRendererInfo = true;

var shouldHaveRendererInfo = !AppConstants.RELEASE_BUILD; ?!?!?!
Attachment #8635515 - Flags: review?(dglastonbury) → review+
Jeff, do you mind uplifting your WEBGL_debug_renderer_info patch to Aurora 41? YouTube already collects WEBGL_debug_renderer_info with users' problem reports for Chrome and IE. The sooner we get more Firefox users exposing WEBGL_debug_renderer_info, the sooner YouTube will be able to share with us their correlations between Firefox users and GPU bug data.
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/fa5d3f420ee6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
r=kamidphish, sr=jst

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: Secondary impact of worse insight into driver issues for important sites like YouTube.
[Describe test coverage new/current, TreeHerder]: In tree, included in patch.
[Risks and why]: none
[String/UUID change made/needed]: none
Flags: needinfo?(jgilbert)
Attachment #8639440 - Flags: superreview+
Attachment #8639440 - Flags: review+
Attachment #8639440 - Flags: approval-mozilla-aurora?
Attachment #8634969 - Attachment is obsolete: true
Comment on attachment 8639440 [details] [diff] [review]
unmasked-renderer.diff

Patch has been in m-c for a week and includes automated test. Let's uplift to Aurora.
Attachment #8639440 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracked. 

Chris, I noticed the dev-doc-needed keyword and wondering whether this needs to be added to FF42 release notes when it goes into Aurora channel. It is a bit peculiar as this setting is only available in Aurora and Nightly builds and not in Beta and Release builds.
Flags: needinfo?(cpeterson)
Ritu, that's a good question. We should update our dev docs to describe the WEBGL_debug_renderer_info API (and that it is limited to Nightly and Aurora for now). We don't really want to encourage websites to use WEBGL_debug_renderer_info (beyond debugging GPU problems), so I don't think we should highlight it in our release notes.
Flags: needinfo?(cpeterson)
Depends on: 1192989
(In reply to Chris Peterson [:cpeterson] from comment #39)
> Ritu, that's a good question. We should update our dev docs to describe the
> WEBGL_debug_renderer_info API (and that it is limited to Nightly and Aurora
> for now). We don't really want to encourage websites to use
> WEBGL_debug_renderer_info (beyond debugging GPU problems), so I don't think
> we should highlight it in our release notes.

This is not in release notes (which generally don't mention Nightly/Aurora only things).
I created a new reference page explaining the situation, though:
https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_debug_renderer_info
(In reply to Florian Scholz [:fscholz] from comment #41)
> (In reply to Chris Peterson [:cpeterson] from comment #39)
> > Ritu, that's a good question. We should update our dev docs to describe the
> > WEBGL_debug_renderer_info API (and that it is limited to Nightly and Aurora
> > for now). We don't really want to encourage websites to use
> > WEBGL_debug_renderer_info (beyond debugging GPU problems), so I don't think
> > we should highlight it in our release notes.
> 
> This is not in release notes (which generally don't mention Nightly/Aurora
> only things).
> I created a new reference page explaining the situation, though:
> https://developer.mozilla.org/en-US/docs/Web/API/WEBGL_debug_renderer_info

Great, thanks!
Blocks: 1336645
Blocks: 1337157
You need to log in before you can comment on or make changes to this bug.