Closed Bug 881681 Opened 6 years ago Closed 6 years ago

WebGL context throws "NS_ERROR_FAILURE: Failure" on Canvas resize

Categories

(Core :: Canvas: WebGL, defect)

24 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox23 --- unaffected
firefox24 + fixed

People

(Reporter: conorlpboyle, Assigned: jgilbert)

References

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

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.
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
Component: Untriaged → Web Apps
Summary: WebGL context throws "NS_ERROR_FAILURE: Failure". → WebGL context throws "NS_ERROR_FAILURE: Failure" on Canvas resize
Component: Web Apps → Canvas: WebGL
Product: Firefox → Core
Using older Nightly build 2013-06-03, the bug is unreproducible.
Bug confirmed also affecting Windows Vista x86. Drivers would not seem to be the cause.
Severity: normal → critical
Bug does not affect build 2013-06-09. The changes in bug 879802 are likely the cause.
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.
Severity: critical → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Linux → All
Hardware: x86 → All
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
Blocks: 798438
Attached patch PatchSplinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #761165 - Flags: review?(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
Attachment #761165 - Flags: review?(bzbarsky) → review+
Blocks: 881264
Blocks: 881520
No longer blocks: 881520
Duplicate of this bug: 881520
https://hg.mozilla.org/mozilla-central/rev/57b06e8e2222
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 882626
Blocks: 882924
Duplicate of this bug: 882924
Blocks: 881805
Duplicate of this bug: 881805
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can we pull in the conformance tests?
Flags: needinfo?(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.
Flags: needinfo?(jgilbert)
This test correctly fails without the patch in this bug.
Attachment #762399 - Flags: review?(bjacob)
> 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.
(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.
The test files are the same, but some files they include differ. The difference must be somewhere in there.
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?
Attachment #762399 - Flags: review?(bjacob) → review-
(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.
(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.
(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.
This bug would appear to be resolved now. Thanks guys.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WORKSFORME
(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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attached patch patch: Reftest for resize (obsolete) — Splinter Review
Assignee: dzbarsky → jgilbert
Attachment #762399 - Attachment is obsolete: true
Attachment #765130 - Flags: review?(bjacob)
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?
Attachment #765130 - Flags: review?(bjacob) → review+
Attached patch patch: Reftest for resize (obsolete) — Splinter Review
With nits addressed.
Attachment #765130 - Attachment is obsolete: true
Attachment #765142 - Flags: review+
Attached patch patch: Reftest for resize (obsolete) — Splinter Review
Updated with fixes from bug 883364.
Attachment #765142 - Attachment is obsolete: true
Attachment #774893 - Flags: review?(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?
Attachment #774893 - Flags: review?(dzbarsky) → review+
(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.
r=dzbarsky
Attachment #776867 - Flags: review+
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.
Attachment #776867 - Flags: review+ → review-
Attached patch patch: Reftest for resize (obsolete) — Splinter Review
How about without changing the wrapper file?
Attachment #774893 - Attachment is obsolete: true
Attachment #776867 - Attachment is obsolete: true
Attachment #776876 - Flags: review?(dzbarsky)
Attachment #776876 - Flags: review?(dzbarsky) → review+
Depends on: 883364
Attached patch reftest-resizeSplinter Review
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
Attachment #776876 - Attachment is obsolete: true
Attachment #789905 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6d3868ec1d2b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.