Closed Bug 795701 Opened 7 years ago Closed 7 years ago

WebGL Renderer : no information in about:support

Categories

(Core :: Canvas: WebGL, defect)

18 Branch
x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox18 + verified
firefox19 --- verified

People

(Reporter: alice0775, Assigned: bjacob)

References

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/85f561c755f6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120929191424

Steps to reproduce:
1. Open about:config
   ---- Observe WebGL Renderer in Graphic section
2. Disable HWA
   Open Options(Tools > Options, Advanced, General,
   uncheck "Use hardware acceleration when available"

3. Restart Browser
4. Open about:config
   ---- Observe WebGL Renderer in Graphic section

Actual results:
  WebGL Renderer : no information

Expected results:
  If no reason,
  WebGL Renderer : Google Inc. -- ANGLE (ATI Radeon HD 4300/4500 Series) -- OpenGL ES 2.0 (ANGLE 1.0.0.1267)


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/96fd99a249cd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927084255
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3d81919584ff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20120927090955
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=96fd99a249cd&tochange=3d81919584ff
I'm also seeing WebGL Renderer 'No information' even though the option to enable it is checked in the options panel.

win32 m-c build on win7 x64
err in comment#0
s/Open about:config/Open about:support/g
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #1)
> I'm also seeing WebGL Renderer 'No information' even though the option to
> enable it is checked in the options panel.
> 
> win32 m-c build on win7 x64

Yes, I can confirm with HWA.Regression range is same as comment#0.
Summary: WebGL Renderer : no information in about:support if I disabled HWA → WebGL Renderer : no information in about:support
No need to disable hardware acceleration. I have WebGL renderer "No information" and HWA enabled.
Yes, this is a regression from bug 791521. In GDB we can see that WebGLContext::SetDimension fails as GetCanvas returns null. This is a consequence of GfxInfoWebGL.cpp doing really wacky stuff here: it relies on the ability to create a WebGL context without having a canvas. crazy. That's what needs fixing here.
Blocks: 791521
In fact, the whole idea of creating these objects from c++ code is crazy. Even if we create a canvas and simulate in c++ a getContext() on it, this will still be crazy if only because with WebIDL bindings each object needs to implement a GetParentObject() method allowing to walk the up a tree and eventually find a Window object.

I guess as an interim fix we can still do that, but that is crazy.
...no, instead, the right approach here is to implement that in privileged JS rather than in c++. patch coming.
The right way to fix this is to implement the WEBGL_debug_renderer_info extension (bug 742781) and have the about:support code use that.
Depends on: 742781
Assignee: nobody → bjacob
As discussed on IRC, data should be collected in Troubleshoot.jsm, and not rely on (or even touch) aboutSupport.js - which means you don't have a document to play with. Should be able to create a document using nsIDOMParser.parseFromString(), and create a <canvas> element from that.
(In reply to Blair McBride (:Unfocused) from comment #9)
> As discussed on IRC, data should be collected in Troubleshoot.jsm, and not
> rely on (or even touch) aboutSupport.js - which means you don't have a
> document to play with. Should be able to create a document using
> nsIDOMParser.parseFromString(), and create a <canvas> element from that.

So here is what I tried:


      var doc =
        Components.classes["@mozilla.org/xmlextras/domparser;1"]
        .createInstance(Components.interfaces.nsIDOMParser)
        .parseFromString("<html/>", "application/xhtml+xml");

      var canvas = doc.createElement("canvas");
      var gl = canvas.getContext("experimental-webgl");
      var ext = gl.getExtension("WEBGL_debug_renderer_info");
      data.webglRenderer = gl.getParameter(ext.UNMASKED_VENDOR_WEBGL)
                           + " -- "
                           + gl.getParameter(ext.UNMASKED_RENDERER_WEBGL);


This gives me the same weird error as when I try to access |document| or |window| directly in this .jsm file. Specifically, the line that triggers those errors is the getContext line:

      var gl = canvas.getContext("experimental-webgl");


This is all very mysterious to me; meanwhile I have a much simpler patch that fixes about:support and just has Troubleshoot.jsm omit the WebGL Renderer info; I'm going to upload that and let you figure the details of how to make The Right Way work, if that's ok.
(In reply to Benoit Jacob [:bjacob] from comment #10)
> This gives me the same weird error as when I try to access |document| or
> |window| directly in this .jsm file. Specifically, the line that triggers
> those errors is the getContext line:
> 
>       var gl = canvas.getContext("experimental-webgl");

What is the error? I tried this locally and got:
TypeError on line 7: canvas.getContext is not a function
because the canvas returned from this call is an HTMLElement, but not an HTMLCanvasElement. Using parseFromString("", "text/html"); to create an HTML document instead of an XML document seemed to make things work, though.

> This is all very mysterious to me; meanwhile I have a much simpler patch
> that fixes about:support and just has Troubleshoot.jsm omit the WebGL
> Renderer info; I'm going to upload that and let you figure the details of
> how to make The Right Way work, if that's ok.

Yes, that seems fine. We can add canvas stuff to Troubleshoot.jsm in a followup.
Now properly handling the case where WebGL is not available. Hope I got the localization right. At least it works here in en_US, with both cases (WebGL available and WebGL not available)
Attachment #672836 - Attachment is obsolete: true
Attachment #672836 - Flags: review?(bmcbride)
Attachment #672842 - Flags: review?(bmcbride)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> (In reply to Benoit Jacob [:bjacob] from comment #10)
> > This gives me the same weird error as when I try to access |document| or
> > |window| directly in this .jsm file. Specifically, the line that triggers
> > those errors is the getContext line:
> > 
> >       var gl = canvas.getContext("experimental-webgl");
> 
> What is the error?

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'TypeError: invalid 'in' operand data' when calling method: [nsIRunnable::run]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]
************************************************************

This seems to refer to lines below saying | if("some string" in data) |

> I tried this locally and got:
> TypeError on line 7: canvas.getContext is not a function
> because the canvas returned from this call is an HTMLElement, but not an
> HTMLCanvasElement. Using parseFromString("", "text/html"); to create an HTML
> document instead of an XML document seemed to make things work, though.

Trying now.
(In reply to Benoit Jacob [:bjacob] from comment #14)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> > I tried this locally and got:
> > TypeError on line 7: canvas.getContext is not a function
> > because the canvas returned from this call is an HTMLElement, but not an
> > HTMLCanvasElement. Using parseFromString("", "text/html"); to create an HTML
> > document instead of an XML document seemed to make things work, though.
> 
> Trying now.

Nope, that doesn't make any difference here.
Attached patch the real fix! (obsolete) — Splinter Review
OK, in fact Gavin's tip worked and I was just making a mistake (application/html instead of text/html).

So this works, and seems to handle properly the case where WebGL is not available (tested with webgl.disabled=true).
Attachment #672842 - Attachment is obsolete: true
Attachment #672842 - Flags: review?(bmcbride)
Attachment #672854 - Flags: review?(bmcbride)
Also, you may notice that this patch removes the part where we were producing the explanatory message when WebGL is not available. The reason for that is a mix of:
 - I got yet more errors I didn't understand when I tried to stick that part back in my patch
 - That logic (using nsIGfxInfo.getFeatureStatus) was very fool-prone anyways (generally speaking, nsIGfxInfo is a terrible interface -- I can speak about it, I was one of its authors).
 - I've long, long exceeded the amount of time I could afford to spend on this bug, sorry
Status: NEW → ASSIGNED
Comment on attachment 672854 [details] [diff] [review]
the real fix!

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

(In reply to Benoit Jacob [:bjacob] from comment #17)
>  - I've long, long exceeded the amount of time I could afford to spend on
> this bug, sorry

If need be, I'm happy to fix up the patch to address the following review comments. Just let me know.

::: toolkit/content/Troubleshoot.jsm
@@ +289,5 @@
>        data.direct2DEnabledMessage =
>          statusMsgForFeature(Ci.nsIGfxInfo.FEATURE_DIRECT2D);
>  
> +    {
> +      var doc =

Remove the unneeded { and } around this entire block of code.

Also, you should generally use 'let' instead of 'var'.

@@ +290,5 @@
>          statusMsgForFeature(Ci.nsIGfxInfo.FEATURE_DIRECT2D);
>  
> +    {
> +      var doc =
> +        Components.classes["@mozilla.org/xmlextras/domparser;1"]

Use the short form - Cc instead of Components.classes

@@ +291,5 @@
>  
> +    {
> +      var doc =
> +        Components.classes["@mozilla.org/xmlextras/domparser;1"]
> +        .createInstance(Components.interfaces.nsIDOMParser)

Ci instead of Components.interfaces

::: toolkit/content/aboutSupport.js
@@ +118,5 @@
>        delete data.directWriteVersion;
>      }
>  
> +    out.webglRenderer = data.webglRenderer;
> +    delete data.webglRenderer;

As-is, you don't need these two lines - |out| inherits from |data|, so you only want to set properties on |out| and delete properties from |data| when you want to change the value.

However, there's value in explicitly stating that WebGL isn't supported. So I think this should include a null-check (make it so data always has a webglRenderer property), and if the property is null then add the not-supported string from previous patches. Something like:

  if (!data.webglRenderer) {
    out.webglRenderer = strings.GetStringFromName("webglNotAvailable");
    delete data.webglRenderer;
  }
Attachment #672854 - Flags: review?(bmcbride) → review-
Attached patch the real fix, fixed (obsolete) — Splinter Review
- fixed to follow review comments
 - re-added the old code to generate the webglRendererMessage when WebGL is not available -- the patch is now much smaller.

Also, regarding the handling of error cases, there are only two cases here:
 - if WebGL context creation succeeds, then we unconditionally obtain a non-empty data.webglRenderer string
 - if WebGL context creation fails, then we ask nsIGfxInfo for an error message to show instead (like we used to before -- this patch is no longer changing that).

Tested in 3 configurations:
 - default, with WebGL working
 - with driver info spoofed so that WebGL is blacklisted
 - with webgl.disabled=true --- then all we get is "no information", and we don't have a way around that, it's a limitation of nsIGfxInfo that it doesn't tell you when a feature is disabled by user preference.

(Side note --- in yesterday's graphics meeting we discussed the possibility that one of use would take a month this quarter to rewrite driver blacklisting and GfxInfo. Just in case you wondered how long we'd have to cope with that bad stuff).
Attachment #674315 - Flags: review?(bmcbride)
Oops, had attached the wrong file.
Attachment #674315 - Attachment is obsolete: true
Attachment #674315 - Flags: review?(bmcbride)
Attachment #674317 - Flags: review?(bmcbride)
Attachment #672854 - Attachment is obsolete: true
Attachment #674317 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/fx-team/rev/6665e97d51aa
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla19
Canvases default to size 300x150. When calling getContext, a graphics surface of that size is created. In the present case, we're not going to use it, so it's just waste. We can avoid that by setting canvas.width and height to 1 before calling getContext.
Attachment #675236 - Flags: review?(bmcbride)
https://hg.mozilla.org/mozilla-central/rev/6665e97d51aa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Attachment #675236 - Flags: review?(bmcbride) → review+
Is it expected/intended that there's now less Information (i.e. Driver/ANGLE Version) in about:support compared to before?

ANGLE enabled: WebGL RendererATI Technologies Inc. -- ATI Radeon HD 4800 Series
Native enabled: WebGL RendererATI Technologies Inc. -- ATI Radeon HD 4800 Series
(In reply to XtC4UaLL [:xtc4uall] from comment #26)
arrg, ANGLE enabled: WebGL RendererGoogle Inc. -- ANGLE (ATI Radeon HD 4800 Series)
It's expected, at least. The new implementation is based on WEBGL_debug_renderer_info,
http://www.khronos.org/registry/webgl/extensions/WEBGL_debug_renderer_info/
which does not expose the VERSION string.

It's not a big deal: the driver version is already shown in about:support Graphics, and the ANGLE revision depends only on the Gecko version which is already shown in about:support.
Will this be Aurora approved?
(In reply to JK from comment #29)
> Will this be Aurora approved?

Good question. This would require backporting bug 742781. But it's the only way to avoid shipping a one-version temporary regression to the stable channel.
Comment on attachment 674317 [details] [diff] [review]
the real fix, fixed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 779611, WebIDL port of remaining WebGL interfaces
User impact if declined: causing about:support to miss WebGL renderer info. WebGL will still work unaffected, only about:support will miss information.
Testing completed (on m-c, etc.): m-c for a few days
Risk to taking this patch (and alternatives if risky): low risk. small patch to about:support code. However, backporting this patch depends on backporting bug 742781, which is a bit more intrusive.
String or UUID changes made by this patch: none
Attachment #674317 - Flags: approval-mozilla-aurora?
Attachment #674317 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Firefox 18 beta 4.

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121212073002
QA Contact: manuela.muntean
Verified on Firefox 19 RC.

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130215130331
Status: RESOLVED → VERIFIED
Depends on: 1171228
You need to log in before you can comment on or make changes to this bug.