Closed Bug 896254 Opened 8 years ago Closed 8 years ago

WebGL2RenderingContext WebIDL interface should be disabled on release channels

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- unaffected
firefox25 + verified

People

(Reporter: Ms2ger, Assigned: guillaume.abadie)

References

Details

(Keywords: verifyme)

Attachments

(3 files)

Per our prefixing policy, we should not expose window.WebGL2RenderingContext on the release channels.
Assignee: nobody → gabadie
Attached patch patch revision 1Splinter Review
Attachment #779273 - Flags: review?(bjacob)
Attachment #779273 - Flags: review?(bjacob) → review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9bfbd308d1
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/fc9bfbd308d1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
The patch has been done by the wrong way.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 892978
So the correct way to do this is to include the .webidl files in all builds, and hide all of WebGL2 interfaces behind a pref, like this:

[Pref="foo.bar.webgl2]
interface WebGL2FooBar {
  // ...
};

Then, these interfaces will only be exposed if this pref is set, which won't be the case for our users anywhere except where they explicitly opt in to test this feature.  You can see an example of this in Gamepad.webidl, among others.
Here is the new way to solve this bug.
Attachment #782891 - Flags: review?(jgilbert)
Attachment #782891 - Flags: review?(ehsan)
Comment on attachment 782891 [details] [diff] [review]
new way to solve the problem's patch

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

This is apparently the correct way to do this, it seems.
Attachment #782891 - Flags: review?(jgilbert) → review+
To be sure, let's wait the Ehsan's R+.
This patch is green like an apple :
https://tbpl.mozilla.org/?tree=Try&rev=37eeac151ef8
Keep waiting for Ehsan's R+.
Attachment #782891 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/2fcf08bde979
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Guillaume, what should QA look for to verify this is fixed?
Flags: needinfo?(guillaume.abadie)
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #13)
> Guillaume, what should QA look for to verify this is fixed?

http://dxr.mozilla.org/mozilla-central/source/dom/webidl/WebGL2RenderingContext.webidl#l9
http://dxr.mozilla.org/mozilla-central/source/dom/webidl/WebGL2RenderingContext.webidl#l13

So the behavior is HTMLCanvasElement::getContext('experimental-webgl2') should never ever succeed with webgl.enable-prototype-webgl2 to False or not set thanks by:

http://dxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGL2Context.cpp#l37
http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLCanvasElement.cpp#l722
Flags: needinfo?(guillaume.abadie)
This is even stronger than that. The expected behavior is that the WebGL2RenderingContext interface should not even exist.

The test is the following: go to the Browser Console (Ctrl+Shift+J) or any place where you can evaluate a JS expression, and enter this:

  0 instanceof WebGL2RenderingContext

This should say:

  ReferenceError: WebGL2RenderingContext is not defined

meaning that WebGL2RenderingContext is not even known as an interface. That's the expected behavior on Stable/Beta channels. If instead this just says

  false

then that means that WebGL2RenderingContext is known as an interface, which is the expected behavior on Nightly and Aurora.
Note that the test in comment 15 is independent of the machine you test this on (doesn't depend on drivers, GPU, etc) while the test in comment 14 is very machine-dependent.
Keywords: verifyme
This should have an automated test.
Flags: in-testsuite?
Flags: needinfo?(guillaume.abadie)
Verified as fixed with Firefox 25 beta 2 (build ID: 20130923194050), using the STR from comment 15, on: Ubuntu 12.10 32-bit, Win 7 64-bit and Mac OS X 10.8.4. 

On the other hand, with latest Aurora and Nightly, I also get the " ReferenceError: WebGL2RenderingContext is not defined " message, on all 3 platforms. Is this expected?
QA Contact: manuela.muntean
Flags: needinfo?(Ms2ger)
Doesn't look like the pref is enabled by default anywhere, so that seems expected.
Flags: needinfo?(Ms2ger)
Guillaume, can you please write that automated test?
(In reply to Manuela Muntean [:Manuela] [QA] from comment #18)
> Verified as fixed with Firefox 25 beta 2 (build ID: 20130923194050), using
> the STR from comment 15, on: Ubuntu 12.10 32-bit, Win 7 64-bit and Mac OS X
> 10.8.4. 
> 
> On the other hand, with latest Aurora and Nightly, I also get the "
> ReferenceError: WebGL2RenderingContext is not defined " message, on all 3
> platforms. Is this expected?

Yes.

(In reply to :Ms2ger from comment #20)
> Guillaume, can you please write that automated test?

I can do this real quick.
Flags: needinfo?(guillaume.abadie)
Attached patch no-webgl2-exposeSplinter Review
Attachment #8348326 - Flags: review?(bjacob)
Comment on attachment 8348326 [details] [diff] [review]
no-webgl2-expose

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

That is fine to land on beta/release...  but can't you make a single test that would automatically detect the channel and require WebGL2RenderingContext to be exposed or not exposed accordingly? I mean, the present test looks like it will have to be landed over and over again on each beta train.
Attachment #8348326 - Flags: review?(bjacob) → review+
Oh no, i see --- you can land this everywhere since even on Nightly, webgl2 is not exposed by default. ok, fine then :-)
You need to log in before you can comment on or make changes to this bug.