Expose WEBGL_debug_renderer_info extension to Web content

RESOLVED FIXED in Firefox 41

Status

()

Core
Graphics
--
enhancement
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: jet, Assigned: jgilbert)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla42
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39 wontfix, firefox40 wontfix, firefox41+ fixed, firefox42 fixed)

Details

(Whiteboard: webgl-extension, [gfx-noted])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

3 years ago
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.
Blocks: 1083588
status-firefox38: --- → wontfix
status-firefox38.0.5: --- → wontfix
status-firefox39: --- → affected
status-firefox40: --- → affected

Comment 5

3 years ago
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)

Comment 7

3 years ago
Okay we'll take a look at it
Flags: needinfo?(kfung)
Flags: needinfo?(acomminos)

Updated

3 years ago
Assignee: nobody → kfung
(Assignee)

Comment 8

3 years ago
Created attachment 8617080 [details] [diff] [review]
0001-Allow-unprivileged-WEBGL_debug_renderer_info.patch

Here's the full version. We will be having a more concrete discussion about this soon.
(Assignee)

Comment 9

3 years ago
Created attachment 8617628 [details] [diff] [review]
0002-Add-test-for-WEBGL_debug_renderer_info.patch

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)
(Assignee)

Comment 12

3 years ago
I have posted an "Intent to implement and ship" email to dev-platform.
(Assignee)

Updated

3 years ago
Attachment #8617080 - Flags: review?(dglastonbury)
(Assignee)

Updated

3 years ago
Attachment #8617080 - Flags: superreview?(jst)
(Assignee)

Updated

3 years ago
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
Keywords: dev-doc-needed
Comment hidden (off-topic)
Comment hidden (off-topic)
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.
status-firefox39: affected → wontfix
status-firefox42: --- → affected
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.
Doug, comment 19.
Flags: needinfo?(dougt)
(Assignee)

Comment 21

3 years ago
(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)

Comment 22

3 years ago
(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.
(Assignee)

Comment 23

3 years ago
(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.

Comment 24

3 years ago
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)
status-firefox40: affected → wontfix
(Assignee)

Comment 26

2 years ago
Created attachment 8634969 [details] [diff] [review]
unmasked-renderer.diff

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+
(Assignee)

Comment 28

2 years ago
(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 29

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f266de469f27

Comment 30

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad967c23a4ea
(Assignee)

Comment 31

2 years ago
Created attachment 8635515 [details] [diff] [review]
0006-Fix-test-to-allow-different-behavior-for-RELEASE_BUI.patch
Attachment #8635515 - Flags: review?(dglastonbury)
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+

Comment 33

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5d3f420ee6
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.
status-firefox42: affected → fixed
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/fa5d3f420ee6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Comment 36

2 years ago
Created attachment 8639440 [details] [diff] [review]
unmasked-renderer.diff

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?
(Assignee)

Updated

2 years ago
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.
tracking-firefox41: --- → +
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)
https://hg.mozilla.org/releases/mozilla-aurora/rev/8cf5636886f0
status-firefox41: affected → fixed
Flags: in-testsuite+
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
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 42

2 years ago
(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!
(Assignee)

Updated

10 months ago
Blocks: 1336645
(Assignee)

Updated

10 months ago
Duplicate of this bug: 742798
(Assignee)

Updated

10 months ago
Blocks: 1337157
You need to log in before you can comment on or make changes to this bug.