890573 - about:support breaks when webgl.disable-extensions is true

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[David Cook [:dncook]]

Attachment: dont_disable_renderer_info.patch

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

1. Set webgl.disable-extensions to true in about:config
2. Go to about:support


Actual results:

The following error gets logged, and the Graphics, JavaScript, Accessibility, and Library Versions sections don't show up properly.

Timestamp: 7/5/2013 9:05:03 PM
Error: Troubleshoot data provider failed: graphics
TypeError: ext is null
Source File: resource://gre/modules/Troubleshoot.jsm
Line: 101


Expected results:

No errors, everything shows up

I have attached a proposed patch to fix this by making the chrome-only WebGL extension WEBGL_debug_renderer_info ignore the webgl.disable-extensions preference.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 771724 [details] [diff] [review]
dont_disable_renderer_info.patch

Over to Jeff Gilbert, as I am currently on vacation. This seems basically fine, but needs an explicit comment (we need to enable this chrome-only extension even with disable-extensions because about:support requires it).

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Ah, wait, here is another possible approach: when mDisableExtensions is initialized, only honor the webgl.disable-extensions preference if the context is *not* chrome, as determined by this

   xpc::AccessCheck::isChrome(js::GetContextCompartment(cx))

check. If the context is chrome, just let mDisableExtensions be false. Jeff, opinion? Anyway, see you around the 18th.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 771724 [details] [diff] [review]
dont_disable_renderer_info.patch

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

Good, but I think it'd be better if this were inverted.
At this point, it doesn't really matter though.

::: content/canvas/src/WebGLContext.cpp
@@ +965,5 @@
>  bool WebGLContext::IsExtensionSupported(JSContext *cx, WebGLExtensionID ext) const
>  {
> +    switch (ext) {
> +        case WEBGL_debug_renderer_info:
> +            return xpc::AccessCheck::isChrome(js::GetContextCompartment(cx));

Let's invert these:

if is chrome:
  switch (ext):
    case debug_foo:
    case debug_bar:
      return true

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] (On vacation, be back on july 18) from comment #2)
> Ah, wait, here is another possible approach: when mDisableExtensions is
> initialized, only honor the webgl.disable-extensions preference if the
> context is *not* chrome, as determined by this
> 
>    xpc::AccessCheck::isChrome(js::GetContextCompartment(cx))
> 
> check. If the context is chrome, just let mDisableExtensions be false. Jeff,
> opinion? Anyway, see you around the 18th.

I think it's best not to do this, since chrome webgl might want to test with extensions disabled. Also, keeping it as special-cased means that what we see in about:support will always be accurate to what actual apps will see.

Comment 5

[David Cook [:dncook]]

Attachment: dont_disable_renderer_info_v2.patch

Addressed review comments

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3302257b7549

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3302257b7549