Last Comment Bug 890573 - about:support breaks when webgl.disable-extensions is true
: about:support breaks when webgl.disable-extensions is true
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla25
Assigned To: dncook
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-05 19:13 PDT by dncook
Modified: 2013-07-10 11:27 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
dont_disable_renderer_info.patch (1.62 KB, patch)
2013-07-05 19:13 PDT, dncook
jgilbert: review+
Details | Diff | Splinter Review
dont_disable_renderer_info_v2.patch (1.84 KB, patch)
2013-07-08 23:17 PDT, dncook
jgilbert: review+
Details | Diff | Splinter Review

Description dncook 2013-07-05 19:13:45 PDT
Created attachment 771724 [details] [diff] [review]
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) 2013-07-07 03:02:20 PDT
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) 2013-07-07 03:10:02 PDT
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 Jeff Gilbert [:jgilbert] 2013-07-08 13:54:35 PDT
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 Jeff Gilbert [:jgilbert] 2013-07-08 14:01:40 PDT
(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 dncook 2013-07-08 23:17:54 PDT
Created attachment 772514 [details] [diff] [review]
dont_disable_renderer_info_v2.patch

Addressed review comments
Comment 6 Ryan VanderMeulen [:RyanVM] 2013-07-09 06:13:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/3302257b7549
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-07-10 11:27:55 PDT
https://hg.mozilla.org/mozilla-central/rev/3302257b7549

Note You need to log in before you can comment on or make changes to this bug.