881681 - WebGL context throws "NS_ERROR_FAILURE: Failure" on Canvas resize

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Conor Boyle]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20130611 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130611031140

Steps to reproduce:

Using Xubuntu 12.04 with the following graphics card and driver setup:

Graphics
--------

Adapter Description: Intel Open Source Technology Center -- Mesa DRI Intel(R) 965GM x86/MMX/SSE2
Device ID: Mesa DRI Intel(R) 965GM x86/MMX/SSE2
Driver Version: 2.1 Mesa 9.0.3
GPU Accelerated Windows: 0/1 Basic
Vendor ID: Intel Open Source Technology Center
WebGL Renderer: Intel Open Source Technology Center -- Mesa DRI Intel(R) 965GM x86/MMX/SSE2
windowLayerManagerRemote: false
AzureCanvasBackend: cairo
AzureContentBackend: none
AzureFallbackCanvasBackend: none

Open a WebGL program such as http://alteredqualia.com/three/examples/webgl_animation_skinning_tf2.html or http://glsl.heroku.com/e


Actual results:

"NS_ERROR_FAILURE" is output to the web console, usually indicating a line where canvas.width is changed.

The following is printed in the terminal:
"ATTENTION: default value of option force_s3tc_enable overridden by environment.", once on the first time, followed by "ATTENTION: option value of option force_s3tc_enable ignored." whenever another NS_ERROR_FAILURE is thrown. (the value of force_s3tc_enable is true).

The program does not run.


Expected results:

The program should have run.

This seems to be a regression, as I can run the examples (http://alteredqualia.com/three/examples/webgl_animation_skinning_tf2.html and http://glsl.heroku.com/e) fine on Firefox 21 and Chromium.

Comment 1

[Conor Boyle]

On the WebGL Conformance Tests at (https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html), The following Canvas tests fail:

conformance/canvas/canvas-test.html(*timeout*)

conformance/canvas/canvas-zero-size.html (0 of 1 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

conformance/canvas/drawingbuffer-static-canvas-test.html (7 of 8 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

conformance/canvas/drawingbuffer-hd-dpi-test.html (9 of 10 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

conformance/canvas/drawingbuffer-test.html (12 of 13 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

conformance/canvas/framebuffer-bindings-unaffected-on-resize.html(*timeout*)

conformance/canvas/texture-bindings-unaffected-on-resize.html(*timeout*)

conformance/canvas/to-data-url-test.html(*timeout*)

conformance/canvas/viewport-unchanged-upon-resize.html (2 of 3 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

In extensions:

conformance/extensions/oes-standard-derivatives.html (16 of 17 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

conformance/extensions/oes-texture-float.html (26 of 31 passed)

    failed: should be red
    failed: should be red
    failed: should be red
    failed: should be red
    failed: should be red

conformance/extensions/oes-texture-float-linear.html (31 of 51 passed)

    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255
    failed: should be 0,0,0,255

conformance/extensions/oes-element-index-uint.html (3 of 4 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

conformance/extensions/webgl-compressed-texture-s3tc.html (9 of 10 passed)

    failed: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).

All with the expected "NS_ERROR_Failure: Failure" and "ATTENTION: option value of option force_s3tc_enable ignored."

Bizzarely, conformance/textures/compressed-tex-image.html seems to pass:

PASS context exists
PASS gl.compressedTexImage2D(gl.TEXTURE_2D, 0, COMPRESSED_RGB_S3TC_DXT1_EXT, 4, 4, 0, new Uint8Array(8)) generated expected GL error: INVALID_ENUM.
PASS gl.compressedTexImage2D(gl.TEXTURE_2D, 0, COMPRESSED_RGBA_S3TC_DXT1_EXT, 4, 4, 0, new Uint8Array(8)) generated expected GL error: INVALID_ENUM.
PASS gl.compressedTexImage2D(gl.TEXTURE_2D, 0, COMPRESSED_RGBA_S3TC_DXT5_EXT, 4, 4, 0, new Uint8Array(16)) generated expected GL error: INVALID_ENUM.
PASS gl.compressedTexImage2D(gl.TEXTURE_2D, 0, ETC1_RGB8_OES, 4, 4, 0, new Uint8Array(8)) generated expected GL error: INVALID_ENUM.
PASS gl.compressedTexImage2D(gl.TEXTURE_2D, 0, COMPRESSED_RGB_PVRTC_4BPPV1_IMG, 8, 8, 0, new Uint8Array(8)) generated expected GL error: INVALID_ENUM.
PASS gl.compressedTexImage2D(gl.TEXTURE_2D, 0, COMPRESSED_RGBA_PVRTC_4BPPV1_IMG, 8, 8, 0, new Uint8Array(8)) generated expected GL error: INVALID_ENUM.
PASS formats = gl.getParameter(gl.COMPRESSED_TEXTURE_FORMATS) generated expected GL error: NO_ERROR.
PASS formats is non-null.
PASS formats.length is 0
PASS successfullyParsed is true

TEST COMPLETE

Comment 2

[Conor Boyle]

Using older Nightly build 2013-06-03, the bug is unreproducible.

Comment 3

[Conor Boyle]

Bug confirmed also affecting Windows Vista x86. Drivers would not seem to be the cause.

Comment 4

[Conor Boyle]

Bug does not affect build 2013-06-09. The changes in bug 879802 are likely the cause.

Comment 5

[Loic]

Regression range:
m-c
good=2013-06-09
bad=2013-06-10
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=efbe547a7972&tochange=252b1ac4d537

Suspected: bug 870232

(In reply to Conor Boyle from comment #4)
> Bug does not affect build 2013-06-09. The changes in bug 879802 are likely
> the cause.

Nope, bug 879802 is not in the regression range.

Comment 6

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/29363d0fd7ff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130607 Firefox/24.0 ID:20130607113648
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/313de3a7c9e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130607 Firefox/24.0 ID:20130607114246
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=29363d0fd7ff&tochange=313de3a7c9e2

Regressed by
	52e83bf32c77	David Zbarsky — Bug 798438 - Use a dictionary to set context options instead of an nsIPropertyBag r=bz

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 761165 [details] [diff] [review]
Patch

So this used to work because of the null-check of aOptions at the top of SetContextOptions?

Please use aOptions.isNullOrUndefined() instead, then.

r=me

Comment 10

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57b06e8e2222

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/57b06e8e2222

Comment 14

[Kelsey Gilbert [:jgilbert]]

I'm going to reopen this to get tests for it. This is the biggest dupe-fest I've seen in a while, and there's no excuse for something this important being missed by tests.

Comment 15

[David Zbarsky (:dzbarsky)]

Can we pull in the conformance tests?

Comment 16

[Kelsey Gilbert [:jgilbert]]

My theory is that the conformance tests exceptioned out, instead of issuing a failure like they should have.

The best bet here is to add a reftest that resizes a canvas.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Add test that assures that resize actually succeeds.

This test correctly fails without the patch in this bug.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> My theory is that the conformance tests exceptioned out,

That should be fatal, just like it is in mochitests normally (window.onerror triggering should be an automatic fail).  If the conformance tests _don't_ do that, they should.

Comment 19

[Kelsey Gilbert [:jgilbert]]

(In reply to Boris Zbarsky (:bz) from comment #18)
> > My theory is that the conformance tests exceptioned out,
> 
> That should be fatal, just like it is in mochitests normally (window.onerror
> triggering should be an automatic fail).  If the conformance tests _don't_
> do that, they should.

Indeed, though testing the conformance test manually fails, running `mochitest-plain content/canvas/test/webgl` appears to work fine.

Comment 20

[Kelsey Gilbert [:jgilbert]]

The test files are the same, but some files they include differ. The difference must be somewhere in there.

Comment 21

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

Comment on attachment 762399 [details] [diff] [review]
patch: Add test that assures that resize actually succeeds.

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

The below are questions, not criticism, but you told me that r- was more convenient for you in that case.

Also watch out for trailing \w.

::: content/canvas/test/reftest/webgl-resize-test.html
@@ +37,5 @@
> +}
> +
> +function runTest() {
> +  // If it takes much time, it's never going to happen.
> +  setTimeout(testComplete, 1000);

If you are only doing this in case an exception might prevent the next rAF callback to be registered, then this is better achieved by a try { ... } catch. Or are you specifically concerned that the callback may not be called because, say, the test browser could be switched to the background?

@@ +45,5 @@
> +
> +  if (gl)
> +    renderGL(gl);
> +  else
> +    renderBackup(canvas);

Why would it be OK for WebGL context creation to fail? Why would we want to silently fall back to something else in that case?

Comment 22

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #21)
> Comment on attachment 762399 [details] [diff] [review]
> patch: Add test that assures that resize actually succeeds.
> 
> Review of attachment 762399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The below are questions, not criticism, but you told me that r- was more
> convenient for you in that case.
Indeed! Thanks. :)
> 
> Also watch out for trailing \w.
> 
> ::: content/canvas/test/reftest/webgl-resize-test.html
> @@ +37,5 @@
> > +}
> > +
> > +function runTest() {
> > +  // If it takes much time, it's never going to happen.
> > +  setTimeout(testComplete, 1000);
> 
> If you are only doing this in case an exception might prevent the next rAF
> callback to be registered, then this is better achieved by a try { ... }
> catch. Or are you specifically concerned that the callback may not be called
> because, say, the test browser could be switched to the background?
I felt like having a killswitch would be safer. 30 seconds of thought doesn't bring up a case where js would stop running that wouldn't be caught by a try/catch. We can do that, instead.
> 
> @@ +45,5 @@
> > +
> > +  if (gl)
> > +    renderGL(gl);
> > +  else
> > +    renderBackup(canvas);
> 
> Why would it be OK for WebGL context creation to fail? Why would we want to
> silently fall back to something else in that case?
WebGL creation fails when we ask it to with a query string of '&nogl'. Part of the idea is to have three separate comparisons, so we're really sure we're comparing the right things. (more true for the color+alpha tests than the clear tests, but still) Also, one minor idea was that webgl failing wouldn't result in a hundred failures, but that's not really a good reason. In theory, this will also tell us if WebGL's busted or actually all canvas compositing, which could actually be useful to learn at-a-glance. (Though in this case, we wouldn't need to do this for each test)

I'm leaning towards pulling this out of all the tests, and replacing it with one-offs to differentiate webgl vs canvas compositing failures.

Comment 23

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

(In reply to Jeff Gilbert [:jgilbert] from comment #22)
> (In reply to Benoit Jacob [:bjacob] from comment #21)
> > Comment on attachment 762399 [details] [diff] [review]
> > patch: Add test that assures that resize actually succeeds.
> > 
> > Review of attachment 762399 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The below are questions, not criticism, but you told me that r- was more
> > convenient for you in that case.
> Indeed! Thanks. :)
> > 
> > Also watch out for trailing \w.
> > 
> > ::: content/canvas/test/reftest/webgl-resize-test.html
> > @@ +37,5 @@
> > > +}
> > > +
> > > +function runTest() {
> > > +  // If it takes much time, it's never going to happen.
> > > +  setTimeout(testComplete, 1000);
> > 
> > If you are only doing this in case an exception might prevent the next rAF
> > callback to be registered, then this is better achieved by a try { ... }
> > catch. Or are you specifically concerned that the callback may not be called
> > because, say, the test browser could be switched to the background?
> I felt like having a killswitch would be safer. 30 seconds of thought
> doesn't bring up a case where js would stop running that wouldn't be caught
> by a try/catch. We can do that, instead.

I would stick to try{...}catch unless one knows a specific reason not to, because if you look at it from the point of view of JS semantics, assuming that we can rely in reftests on rAF callbacks actually being called, really the only reason that could explain timing out should be a JS exception, and I'd rather have us notice if things started going wrong in another unexpected way.

> > 
> > @@ +45,5 @@
> > > +
> > > +  if (gl)
> > > +    renderGL(gl);
> > > +  else
> > > +    renderBackup(canvas);
> > 
> > Why would it be OK for WebGL context creation to fail? Why would we want to
> > silently fall back to something else in that case?
> WebGL creation fails when we ask it to with a query string of '&nogl'. Part
> of the idea is to have three separate comparisons, so we're really sure
> we're comparing the right things. (more true for the color+alpha tests than
> the clear tests, but still) Also, one minor idea was that webgl failing
> wouldn't result in a hundred failures, but that's not really a good reason.
> In theory, this will also tell us if WebGL's busted or actually all canvas
> compositing, which could actually be useful to learn at-a-glance. (Though in
> this case, we wouldn't need to do this for each test)
> 
> I'm leaning towards pulling this out of all the tests, and replacing it with
> one-offs to differentiate webgl vs canvas compositing failures.

What scares me with the current code is that the canvas 2D fallback path seems to render exactly like the WebGL path, so it's not clear to me how we would be able to tell, from looking at the results, which path was taken.

Comment 24

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #23)
> (In reply to Jeff Gilbert [:jgilbert] from comment #22)
> > (In reply to Benoit Jacob [:bjacob] from comment #21)
> > > Comment on attachment 762399 [details] [diff] [review]
> > > patch: Add test that assures that resize actually succeeds.
> > > 
> > > Review of attachment 762399 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > The below are questions, not criticism, but you told me that r- was more
> > > convenient for you in that case.
> > Indeed! Thanks. :)
> > > 
> > > Also watch out for trailing \w.
> > > 
> > > ::: content/canvas/test/reftest/webgl-resize-test.html
> > > @@ +37,5 @@
> > > > +}
> > > > +
> > > > +function runTest() {
> > > > +  // If it takes much time, it's never going to happen.
> > > > +  setTimeout(testComplete, 1000);
> > > 
> > > If you are only doing this in case an exception might prevent the next rAF
> > > callback to be registered, then this is better achieved by a try { ... }
> > > catch. Or are you specifically concerned that the callback may not be called
> > > because, say, the test browser could be switched to the background?
> > I felt like having a killswitch would be safer. 30 seconds of thought
> > doesn't bring up a case where js would stop running that wouldn't be caught
> > by a try/catch. We can do that, instead.
> 
> I would stick to try{...}catch unless one knows a specific reason not to,
> because if you look at it from the point of view of JS semantics, assuming
> that we can rely in reftests on rAF callbacks actually being called, really
> the only reason that could explain timing out should be a JS exception, and
> I'd rather have us notice if things started going wrong in another
> unexpected way.
> 
> > > 
> > > @@ +45,5 @@
> > > > +
> > > > +  if (gl)
> > > > +    renderGL(gl);
> > > > +  else
> > > > +    renderBackup(canvas);
> > > 
> > > Why would it be OK for WebGL context creation to fail? Why would we want to
> > > silently fall back to something else in that case?
> > WebGL creation fails when we ask it to with a query string of '&nogl'. Part
> > of the idea is to have three separate comparisons, so we're really sure
> > we're comparing the right things. (more true for the color+alpha tests than
> > the clear tests, but still) Also, one minor idea was that webgl failing
> > wouldn't result in a hundred failures, but that's not really a good reason.
> > In theory, this will also tell us if WebGL's busted or actually all canvas
> > compositing, which could actually be useful to learn at-a-glance. (Though in
> > this case, we wouldn't need to do this for each test)
> > 
> > I'm leaning towards pulling this out of all the tests, and replacing it with
> > one-offs to differentiate webgl vs canvas compositing failures.
> 
> What scares me with the current code is that the canvas 2D fallback path
> seems to render exactly like the WebGL path, so it's not clear to me how we
> would be able to tell, from looking at the results, which path was taken.

At least one of the tests is webgl-only, but I can see the concern. I'll pull out the fallback here, and we can address the rest in bug 883364.

Comment 25

[Conor Boyle]

This bug would appear to be resolved now. Thanks guys.

Comment 26

[Jason Smith [:jsmith]]

(In reply to Conor Boyle from comment #25)
> This bug would appear to be resolved now. Thanks guys.

See comment 14. This was reopened to land tests with the patch.

Comment 27

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Reftest for resize

Comment 28

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

Comment on attachment 765130 [details] [diff] [review]
patch: Reftest for resize

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

::: content/canvas/test/reftest/reftest.list
@@ +29,5 @@
>  pref(webgl.force-layers-readback,true) fuzzy-if(B2G,256,83)  == webgl-clear-test.html?readback&__&alpha&preserve  wrapper.html?green.png
>  pref(webgl.force-layers-readback,true) fuzzy-if(B2G,256,83)  == webgl-clear-test.html?readback&aa&alpha&preserve  wrapper.html?green.png
>  
> +# Check that resize works:
> +== webgl-resize-test.html?nogl  wrapper.html?green.png

you said on IRC you wanted to remove the ?nogl entry. What does ?nogl do, by the way?

::: content/canvas/test/reftest/webgl-resize-test.html
@@ +22,5 @@
> +  gl.canvas.width = 256;
> +  gl.canvas.height = 256;
> +  gl.clearColor(0.0, 1.0, 0.0, 1.0);
> +  gl.clear(gl.COLOR_BUFFER_BIT);
> +  

trailing \w

@@ +27,5 @@
> +  gl.finish();
> +}
> +
> +function runTest() {
> +  // If it takes much time, it's never going to happen.

This comment looks like it's orphaned?

Comment 29

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Reftest for resize

With nits addressed.

Comment 30

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Reftest for resize

Updated with fixes from bug 883364.

Comment 31

[David Zbarsky (:dzbarsky)]

Comment on attachment 774893 [details] [diff] [review]
patch: Reftest for resize

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

::: content/canvas/test/reftest/webgl-resize-test.html
@@ +34,5 @@
> +  if (gl)
> +    render(gl);
> +  else
> +    document.write("Failed to get WebGL context.");
> +

You don't need to fillText here like in the other bug because you force another rAF call, right?

Comment 32

[Kelsey Gilbert [:jgilbert]]

(In reply to David Zbarsky (:dzbarsky) from comment #31)
> Comment on attachment 774893 [details] [diff] [review]
> patch: Reftest for resize
> 
> Review of attachment 774893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/test/reftest/webgl-resize-test.html
> @@ +34,5 @@
> > +  if (gl)
> > +    render(gl);
> > +  else
> > +    document.write("Failed to get WebGL context.");
> > +
> 
> You don't need to fillText here like in the other bug because you force
> another rAF call, right?

No, this definitely needs fillText, too. Thanks for calling this out.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: patch for landing: Reftest for resize

r=dzbarsky

Comment 34

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 776867 [details] [diff] [review]
patch for landing: Reftest for resize

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

::: content/canvas/test/reftest/wrapper.html
@@ +11,5 @@
> +  function onImageLoad() {
> +    setTimeout(markComplete, 0);
> +  };
> +
> +  function markComplete() {

I actually don't think we need this. Let's remove it unless we find otherwise.

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: patch: Reftest for resize

How about without changing the wrapper file?

Comment 36

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1c068244a2

Comment 37

[Ed Morley [:emorley]]

Push backed out for android 2.2 armv6 reftest failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25952497&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25951856&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7bbbb1350256
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b338d7c3660
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/91b9edd45d8f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5623c4b04a1e

Comment 38

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aadb205e7f05

Comment 39

[Phil Ringnalda (:philor)]

Push backed out for android 2.2 armv6 reftest failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26464803&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26466250&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a1d63e503ee9

Comment 40

[Kelsey Gilbert [:jgilbert]]

Attachment: reftest-resize

Marked android 2.2 as random, as per the previous bug. R+ carried over from dzbarsky.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6d3868ec1d2b

Comment 41

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6d3868ec1d2b