1259702 - WebGL crash: [@mozilla::WebGLShader::FindActiveOutputMappedNameByUserName]

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

Tested with https://hg.mozilla.org/integration/mozilla-inbound/rev/39fb883bcd1c

I do not have a testcase yet for this one.

Comment 1

[Christoph Diehl [:posidron]]

Run: ./framboise.py -fuzzer 1:WebGL,1:WebGL2 -setup inbound64-debug -debug -max-commands 50 -with-events -with-set-timeout -with-set-interval

Comment 2

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Jerry, maybe you can take a look at this one as well?

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Sure, will do.

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If we have a webgl code like:

  var program = gl.createProgram();
  var vertexShaderSrc = "xxxxxxx";
  var fragmentShaderSrc = "xxxxx";
  var vertexShader = gl.createShader(gl.VERTEX_SHADER);
  var fragmentShader = gl.createShader(gl.FRAGMENT_SHADER);

  gl.shaderSource(vertexShader, vertexShaderSrc);
  gl.compileShader(vertexShader);
  gl.attachShader(program, vertexShader);

  gl.shaderSource(fragmentShader, fragmentShaderSrc);
  gl.compileShader(fragmentShader);
  gl.attachShader(program, vertexShader);

  gl.linkProgram(program);
  gl.useProgram(program);
  gl.getFragDataLocation(program, "foobar"); // If we have some problem in fragmentShader(the fragment 
                                             // shader is not valid), we will dereference a nullptr in 
                                             // gecko.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: check the shader status in program before query. v1

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8735253 [details] [diff] [review]
check the shader status in program before query. v1

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

::: dom/canvas/WebGLProgram.cpp
@@ +1053,5 @@
>  bool
>  WebGLProgram::FindActiveOutputMappedNameByUserName(const nsACString& userName,
>                                                     nsCString* const out_mappedName) const
>  {
> +    if (mFragShader && mFragShader->FindActiveOutputMappedNameByUserName(userName, out_mappedName)) {

Please reference https://hg.mozilla.org/mozilla-central/annotate/63be002b4a803df1122823841ef7633b7561d873/dom/canvas/WebGLProgram.cpp#l571

It's fine to just return false if the fragment shader is not ready.

@@ +1064,5 @@
>  bool
>  WebGLProgram::FindAttribUserNameByMappedName(const nsACString& mappedName,
>                                               nsDependentCString* const out_userName) const
>  {
> +    if (mVertShader && mVertShader->FindAttribUserNameByMappedName(mappedName, out_userName))

The same reason as the previous one.

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf8aa7d48e6

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The simplified test case in comment 5 is not correct. It doesn't hit the crash. I'm trying to figure out the crash code sequence. The patch is still prevent the crash with the original test case attachment 8734692 [details].

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Update the test case:
  var program = gl.createProgram();
  var vertexShaderSrc = "xxxxxxx";
  var fragmentShaderSrc = "xxxxx";
  var vertexShader = gl.createShader(gl.VERTEX_SHADER);
  var fragmentShader = gl.createShader(gl.FRAGMENT_SHADER);

  gl.shaderSource(vertexShader, vertexShaderSrc);
  gl.compileShader(vertexShader);
  gl.attachShader(program, vertexShader);

  gl.shaderSource(fragmentShader, fragmentShaderSrc);
  gl.compileShader(fragmentShader);
  gl.attachShader(program, fragmentShader);

  gl.linkProgram(program);
  gl.useProgram(program);
  gl.detachShader(program, fragmentShader);  // Detatch fragment shader.
  gl.getFragDataLocation(program, "foobar"); // If we detach Shader the fragment shader and then try to 
                                             // query some information from it, we will hit the crash.

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please reference comment 10.

If we already do the linkProgram() and useProgram() and call detachShader() after them, what will we get if we really send some draw call(e.g. draw array)?
Will we just get a whole black result(or the color we use glClear() before)?

If we want to use another shader, why don't we just switch to the new program which contain that shader?
I still can't find a scenario for detachShader() usage.

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Test case: test case for webgl getFragDataLocation(). v1

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8735253 [details] [diff] [review]
check the shader status in program before query. v1

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

This does not fix the root cause here. (and should not be necessary)

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #10)
> Update the test case:
>   var program = gl.createProgram();
>   var vertexShaderSrc = "xxxxxxx";
>   var fragmentShaderSrc = "xxxxx";
>   var vertexShader = gl.createShader(gl.VERTEX_SHADER);
>   var fragmentShader = gl.createShader(gl.FRAGMENT_SHADER);
> 
>   gl.shaderSource(vertexShader, vertexShaderSrc);
>   gl.compileShader(vertexShader);
>   gl.attachShader(program, vertexShader);
> 
>   gl.shaderSource(fragmentShader, fragmentShaderSrc);
>   gl.compileShader(fragmentShader);
>   gl.attachShader(program, fragmentShader);
> 
>   gl.linkProgram(program);
>   gl.useProgram(program);
>   gl.detachShader(program, fragmentShader);  // Detatch fragment shader.
>   gl.getFragDataLocation(program, "foobar"); // If we detach Shader the
> fragment shader and then try to 
>                                              // query some information from
> it, we will hit the crash.

This is actually a different root-cause than the crash in the testcase. We need to fix both, but the fix is different.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8735716 [details] [diff] [review]
Test case: test case for webgl getFragDataLocation(). v1

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

Let's clean this up a bit more.

::: dom/canvas/test/webgl-mochitest.ini
@@ +89,5 @@
>  [webgl-mochitest/test_webgl2_invalidate_framebuffer.html]
>  skip-if = toolkit == 'android' #bug 865443- seperate suite - the non_conf* tests pass except for one on armv6 tests
>  [webgl-mochitest/test_webgl2_alpha_luminance.html]
>  skip-if = toolkit == 'android' #bug 865443- seperate suite - the non_conf* tests pass except for one on armv6 tests
> +[webgl-mochitest/test_webgl_fuzzy_pattern.html]

Call it test_fuzzing_bugs.html.

::: dom/canvas/test/webgl-mochitest/test_webgl_fuzzy_pattern.html
@@ +12,5 @@
> +  var canvas = document.createElement('canvas');
> +  canvas.id = 'webglCanvas';
> +  canvas.height = 512;
> +  canvas.width = 512;
> +  document.body.appendChild(canvas);

Remove everything but the `var canvas` line.

@@ +24,5 @@
> +
> +  var program = gl.createProgram();
> +
> +  var vertexShaderSrc =
> +    "varying lowp vec4 vColor; \

vColor isn't used, so just remove it.

@@ +45,5 @@
> +  gl.shaderSource(fragmentShader, fragmentShaderSrc);
> +  gl.compileShader(fragmentShader);
> +  gl.attachShader(program, fragmentShader);
> +
> +  gl.linkProgram(program);

Assert that the link was successful after this, and mark a failure and return early if it failed.

@@ +67,5 @@
> +
> +try {
> +  var prefArrArr = [
> +    ['webgl.force-enabled', true],
> +    ['webgl.disable-angle', true],

Why are we disabling ANGLE here?

@@ +68,5 @@
> +try {
> +  var prefArrArr = [
> +    ['webgl.force-enabled', true],
> +    ['webgl.disable-angle', true],
> +    ['webgl.bypass-shader-validation', true],

Why bypass shader validation?

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8735716 [details] [diff] [review]
Test case: test case for webgl getFragDataLocation(). v1

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

::: dom/canvas/test/webgl-mochitest/test_webgl_fuzzy_pattern.html
@@ +68,5 @@
> +try {
> +  var prefArrArr = [
> +    ['webgl.force-enabled', true],
> +    ['webgl.disable-angle', true],
> +    ['webgl.bypass-shader-validation', true],

This prefs comes from the fuzzy testing setting.
I will clean up them.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Test case: test case for webgl getFragDataLocation(). v2

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8737047 [details] [diff] [review]
Test case: test case for webgl getFragDataLocation(). v2

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

::: dom/canvas/test/webgl-mochitest/test_fuzzing_bugs.html
@@ +75,5 @@
> +  warning('No SpecialPowers, but trying WebGL2 anyway...');
> +  run();
> +}
> +
> +SimpleTest.waitForExplicitFinish();

Put this before the try {} block.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: test case for webgl getFragDataLocation(). v3. r=jgilbert

update for comment 18

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I'm still waiting for bug 1241042. After that, I will check in the webgl test case.

With bug 1241042, there will be no crash with the usage in this bug.

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: test case for webgl getFragDataLocation(). v4. r=jgilbert

rebase

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f298d01adec5
test case for webgl getFragDataLocation(). r=jgilbert

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f298d01adec5

Comment 24

[Gerry Chang [:gchang]]

This is too late for 48 beta and there have been no crashes for a while. Mark 48 as won't fix.