Closed
Bug 1259702
Opened 9 years ago
Closed 8 years ago
WebGL crash: [@mozilla::WebGLShader::FindActiveOutputMappedNameByUserName]
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: posidron, Assigned: jerry)
References
Details
(Keywords: crash, Whiteboard: [gfx-noted])
Crash Data
Attachments
(4 files, 3 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jgilbert
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jerry
:
review+
|
Details | Diff | Splinter Review |
Tested with https://hg.mozilla.org/integration/mozilla-inbound/rev/39fb883bcd1c
I do not have a testcase yet for this one.
Reporter | ||
Comment 1•9 years ago
|
||
Run: ./framboise.py -fuzzer 1:WebGL,1:WebGL2 -setup inbound64-debug -debug -max-commands 50 -with-events -with-set-timeout -with-set-interval
Reporter | ||
Comment 2•9 years ago
|
||
Jerry, maybe you can take a look at this one as well?
Assignee: nobody → hshih
Whiteboard: [gfx-noted]
Assignee | ||
Comment 4•9 years ago
|
||
Sure, will do.
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment hidden (obsolete) |
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8735253 -
Flags: review?(jgilbert)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
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].
Assignee | ||
Comment 10•9 years ago
|
||
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.
Assignee | ||
Comment 11•9 years ago
|
||
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.
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8735716 -
Flags: review?(jgilbert)
Comment 13•9 years ago
|
||
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)
Attachment #8735253 -
Flags: review?(jgilbert) → review-
Comment 14•9 years ago
|
||
(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.
Flags: needinfo?(jgilbert)
Comment 15•9 years ago
|
||
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?
Attachment #8735716 -
Flags: review?(jgilbert) → review-
Assignee | ||
Comment 16•9 years ago
|
||
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.
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8737047 -
Flags: review?(jgilbert)
Assignee | ||
Updated•9 years ago
|
Attachment #8735716 -
Attachment is obsolete: true
Comment 18•9 years ago
|
||
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.
Attachment #8737047 -
Flags: review?(jgilbert) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8737047 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
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.
Updated•8 years ago
|
Crash Signature: [@ mozilla::WebGLShader::FindActiveOutputMappedNameByUserName]
Assignee | ||
Updated•8 years ago
|
Attachment #8738335 -
Attachment is obsolete: true
Comment 22•8 years ago
|
||
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f298d01adec5
test case for webgl getFragDataLocation(). r=jgilbert
Comment 23•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 24•8 years ago
|
||
This is too late for 48 beta and there have been no crashes for a while. Mark 48 as won't fix.
You need to log in
before you can comment on or make changes to this bug.
Description
•