1405600 - WebGL2.0 gpu instancing crashes on Windows Nightly

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P3)

Description

[marcot]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171003220138

Steps to reproduce:

- built a unity webgl test with gpu instancing enabled
- when accessing https://files.unity3d.com/marcot/instancing/ Nightly 57 and 58 crash



Actual results:

WebGL 2.0 instancing demo is broken on Nightly


Expected results:

not crash. see on Chrome

Comment 1

[Jukka Jylänki]

Trying out with mozregression, I see that Firefox build from 2016-01-01, https://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-01-03-03-30-mozilla-central/firefox-46.0a1.en-US.win64.zip also crashes, so this one does not look like a regression, but the crash has been present probably since instancing support(?) was added.

Comment 2

[YF (Yang)]

(In reply to Jukka Jylänki from comment #1)
> Trying out with mozregression, I see that Firefox build from 2016-01-01,
> https://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-01-03-03-30-
> mozilla-central/firefox-46.0a1.en-US.win64.zip also crashes, so this one
> does not look like a regression, but the crash has been present probably
> since instancing support(?) was added.

I think it occur since bug 894492.

Comment 3

[Michael Leu[:Lenzak](UTC+8)]

I tested this website with ANGLE3229 but the crash still happens.

I also found that we can render this website with no problem if we have angle disabled.

The crash spot is inside DrawElements, but the it seems that the cause is inconsistent shader state.

I suspect it is actually an ANGLE bug, but I have no idea why chrome can run it without problem for now.

Comment 4

[Daosheng Mu[:daoshengmu]]

Crash call stack:

libGLESv2.dll!rx::ProgramD3D::ensureUniformBlocksInitialized() Line 1681	C++	Symbols loaded.
libGLESv2.dll!rx::ProgramD3D::updateUniformBufferCache(const gl::Caps & caps, unsigned int reservedVertex, unsigned int reservedFragment) Line 1781	C++	Symbols loaded.
libGLESv2.dll!rx::StateManager11::syncUniformBuffers(const gl::Context * context, rx::ProgramD3D * programD3D) Line 2838	C++	Symbols loaded.
libGLESv2.dll!rx::StateManager11::updateState(const gl::Context * context, unsigned int drawMode) Line 1952	C++	Symbols loaded.
libGLESv2.dll!rx::Context11::prepareForDrawCall(const gl::Context * context, unsigned int drawMode) Line 389	C++	Symbols loaded.
libGLESv2.dll!rx::Context11::drawElementsInstanced(const gl::Context * context, unsigned int mode, int count, unsigned int type, const void * indices, int instances) Line 193	C++	Symbols loaded.
libGLESv2.dll!gl::Context::drawElementsInstanced(unsigned int mode, int count, unsigned int type, const void * indices, int instances) Line 1824	C++	Symbols loaded.
libGLESv2.dll!gl::DrawElementsInstanced(unsigned int mode, int count, unsigned int type, const void * indices, int instancecount) Line 1439	C++	Symbols loaded.
libGLESv2.dll!glDrawElementsInstanced(unsigned int mode, int count, unsigned int type, const void * indices, int instanceCount) Line 1320	C++	Symbols loaded.
xul.dll!mozilla::gl::GLContext::raw_fDrawElementsInstanced(unsigned int mode, int count, unsigned int type, const void * indices, int primcount) Line 2405	C++	Symbols loaded.

The crash results from vertexShaderD3D is a nullptr. If I mark the lines at Program::detachShader(), it could resolve this problem.

Comment 5

[Daosheng Mu[:daoshengmu]]

It looks like ANLGE does not allow shaders be detached when someone has DIRTY_BIT_PROGRAM_UNIFORM_BUFFERS bit. In this example, it calls ensureUniformBlocksInitialized() to check if any uniform changes from the vertex shader before drawElementsInstanced(). However, the vertex shader has been detached.

Comment 6

[Daosheng Mu[:daoshengmu]]

Attachment: uniformblock.zip

Giving an example for reproducing this crash. It is happened when calling detachShader() at libs/maximus.js #ln445. It works well in Chromium and when disable ANGLE in Firefox.

Probably, we need to avoid the real detach shaders internally. Only detaching  them when deleting shader programs.

Comment 7

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Review commit: https://reviewboard.mozilla.org/r/194510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194510/

Comment 8

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194510/diff/1-2/

Comment 9

[Daosheng Mu[:daoshengmu]]

This is because we detach shaders before initializing uniform blocks from shaders. The original design for ProgramD3D::ensureUniformBlocksInitialized() is lazy init, it supposes to be updated when emitting draw calls. However, if we detach shaders before calling draw calls, we have no shaders for getting the uniform blocks information. Therefore, I think we need a fix for forcing initialize uniform blocks when detaching shaders.

Comment 10

[Daosheng Mu[:daoshengmu]]

I open an issue at Chromium/ANGLE as well, https://bugs.chromium.org/p/angleproject/issues/detail?id=2208.

Comment 11

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194510/diff/2-3/

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

https://reviewboard.mozilla.org/r/194510/#review200322

::: gfx/angle/src/libANGLE/Program.cpp:523
(Diff revision 3)
>  {
> +    // Because we do lazy init in ensureUniformBlocksInitialized,
> +    // we must initialize them before detaching shaders,
> +    // otherwise, we will have no shaders for getting uniform blocks
> +    // information from shaders when doing draw calls.
> +    mProgram->ensureUniformBlocksInitialized();

Why not do this at LinkProgram time?

Comment 13

[Daosheng Mu[:daoshengmu]]

(In reply to Jeff Gilbert [:jgilbert] from comment #12)
> Comment on attachment 8923311 [details]
> Bug 1405600 - Initialize uniform blocks before detaching shaders;
> 
> https://reviewboard.mozilla.org/r/194510/#review200322
> 
> ::: gfx/angle/src/libANGLE/Program.cpp:523
> (Diff revision 3)
> >  {
> > +    // Because we do lazy init in ensureUniformBlocksInitialized,
> > +    // we must initialize them before detaching shaders,
> > +    // otherwise, we will have no shaders for getting uniform blocks
> > +    // information from shaders when doing draw calls.
> > +    mProgram->ensureUniformBlocksInitialized();
> 
> Why not do this at LinkProgram time?

I think the original purpose that ANGLE did is lazy init for avoiding taking redundant time when doing linkProgram(). Only calling ensureUniformBlocksInitialized() when we need this Program for emitting draw calls. Therefore, I think this fix is available to put it at the detachShader(). An alternative way is using refPtr for vertex/fragment shader in ProgramImp, but it could bring some extra efforts.

Comment 14

[Jamie Madill]

Looking at fixing this in http://anglebug.com/2208. The original reason was a problem with initialization order for uniform blocks. In the GL back-end, we optimized out unused (and non-std140) uniform blocks, so we deferred the uniform block linking step until after the ProgramImpl::link call. However the D3D back-end needed access the the linked Uniform Block info before initializing the D3D-specific uniform block info. Hence, it deferred this until the uniform blocks were used, but this was a bad thing. Should be fixed when https://chromium-review.googlesource.com/c/angle/angle/+/753521 lands. Thanks for investigating.

Comment 15

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1405600 - Part 2: Add WebGL2 uniform block crash test case;

Review commit: https://reviewboard.mozilla.org/r/198024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/198024/

Comment 16

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194510/diff/3-4/

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

https://reviewboard.mozilla.org/r/194510/#review200322

> Why not do this at LinkProgram time?

It sounds a lot like this should just happen at LinkProgram time. I don't see it being an optimization to do it anywhere else, and LinkProgram is called infrequently enough that even if it were an optimization, we should treat a redundent check in LinkProgram as no-cost.

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8926780 [details]
Bug 1405600 - Part 2: Add WebGL2 uniform block crash test case;

https://reviewboard.mozilla.org/r/198024/#review204682

::: dom/canvas/test/webgl-mochitest/test_webgl2_uniform_block.html:8
(Diff revision 1)
> +<title>WebGL2 test: using uniform blocks</title>
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<link rel="stylesheet" href="/tests/SimpleTest/test.css">
> +<script src="webgl-util.js"></script>
> +<script id="vs" type="x-shader/x-vertex"
> +>#version 300 es

Take a look at test_without_index_validation.html for how to avoid having to avoid a starting newline like this.

::: dom/canvas/test/webgl-mochitest/test_webgl2_uniform_block.html:60
(Diff revision 1)
> +
> +      gl.clear(gl.COLOR_BUFFER_BIT);
> +      gl.useProgram(prog);
> +      gl.vertexAttribPointer(prog.POSITION, 2, gl.FLOAT, false, 0, 0);
> +      gl.enableVertexAttribArray(prog.POSITION);
> +      gl.drawArrays(gl.TRIANGLE_STRIP, 0, 3);

drawArrays(gl.POINTS, 0, 1), and use vertexAttrib4f for setting the position. This way you don't need to worry about ARRAY_BUFFER VBOs.

::: dom/canvas/test/webgl-mochitest/test_webgl2_uniform_block.html:64
(Diff revision 1)
> +      gl.enableVertexAttribArray(prog.POSITION);
> +      gl.drawArrays(gl.TRIANGLE_STRIP, 0, 3);
> +    }
> +
> +    var vs = WebGLUtil.createShaderById(gl, 'vs');
> +    var fs = WebGLUtil.createShaderById(gl, 'fs');

Just do the minimal raw webgl code needed for this, like I did in test_without_index_validation.html:

  const vs = gl.createShader(gl.VERTEX_SHADER);
  gl.shaderSource(vs, vertSource.innerHTML.trim());
  gl.compileShader(vs);
  const fs = gl.createShader(gl.FRAGMENT_SHADER);
  gl.shaderSource(fs, fragSource.innerHTML.trim());
  gl.compileShader(fs);
  const prog = gl.createProgram();
  gl.attachShader(prog, vs);
  gl.attachShader(prog, fs);
  gl.linkProgram(prog);
  gl.useProgram(prog);

Don't bother asserting or checking things that the test isn't trying to test, especially if we get decent JS errors if they failed. (linkProgram will fail if linking failed, but this should never happen in this test anyways, outside initial development) In generally, assume that things just work, except for the parts this test is designed to evaluate.

::: dom/canvas/test/webgl-mochitest/test_webgl2_uniform_block.html:94
(Diff revision 1)
> +    gl.bufferData(gl.ARRAY_BUFFER, new Float32Array([-0.5, -0.5,
> +                                                     0.5, -0.5,
> +                                                     0.0, 0.5]), gl.STATIC_DRAW);
> +
> +    gl.clearColor(0, 1, 0, 1);
> +    testUniformBlock(prog);

Why is this in a separate function if we only call it once?

Comment 19

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194510/diff/4-5/

Comment 20

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8926780 [details]
Bug 1405600 - Part 2: Add WebGL2 uniform block crash test case;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198024/diff/1-2/

Comment 21

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

https://reviewboard.mozilla.org/r/194510/#review200322

> It sounds a lot like this should just happen at LinkProgram time. I don't see it being an optimization to do it anywhere else, and LinkProgram is called infrequently enough that even if it were an optimization, we should treat a redundent check in LinkProgram as no-cost.

Sounds good. I have moved it to Program::Link()

Comment 22

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

https://reviewboard.mozilla.org/r/194510/#review205778

::: gfx/angle/src/libANGLE/Program.cpp:828
(Diff revision 5)
>  
> +    // Because we do lazy init in ensureUniformBlocksInitialized,
> +    // we must initialize them when linking shaders,
> +    // otherwise, we will have no shaders for getting uniform blocks
> +    // information from shaders when doing draw calls.
> +    mProgram->ensureUniformBlocksInitialized();

Technically this should go before the timing measurement.

Comment 23

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194510/diff/5-6/

Comment 24

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8926780 [details]
Bug 1405600 - Part 2: Add WebGL2 uniform block crash test case;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/198024/diff/2-3/

Comment 25

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #23)
> Comment on attachment 8923311 [details]
> Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/194510/diff/5-6/

According to Comment 22 to some adjustment.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/168bbdf67be5
Part 1: Initialize uniform blocks before detaching shaders; r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/8adf0199bc28
Part 2: Add WebGL2 uniform block crash test case; r=jgilbert

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/168bbdf67be5
https://hg.mozilla.org/mozilla-central/rev/8adf0199bc28

Comment 28

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

Approval Request Comment
[Feature/Bug causing the regression]: After Bug 1371190 updates ANGLE, it causes this crash.
[User impact if declined]: Users would have chance to crash when using WebGL uniform blocks.
[Is this code covered by automated tests?]: Yes. I give the test case at my second patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: nope
[List of other uplifts needed for the feature/fix]: nope.
[Is the change risky?]: nope.
[Why is the change risky/not risky?]: It solves the crash and has covered by the test case.
[String changes made/needed]: nope.

Comment 29

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8926780 [details]
Bug 1405600 - Part 2: Add WebGL2 uniform block crash test case;

Approval Request Comment
[Feature/Bug causing the regression]: After Bug 1371190 updates ANGLE, it causes this crash.
[User impact if declined]: This is the test case for the first patch.
[Is this code covered by automated tests?]: Yes, this is the test.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: nope.
[List of other uplifts needed for the feature/fix]: nope.
[Is the change risky?]: nope.
[Why is the change risky/not risky?]: this is just a test.
[String changes made/needed]: nope.

Comment 30

[Gerry Chang [:gchang]]

Comment on attachment 8923311 [details]
Bug 1405600 - Part 1: Initialize uniform blocks before detaching shaders;

There are no crashes recently. Let it ride the train. Beta58-.