Closed Bug 1324765 Opened 3 years ago Closed 3 years ago

Crash in std::vector<T>::_Buy

Categories

(Core :: Canvas: WebGL, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- verified
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bogdan_maris, Assigned: cleu)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-b3234e66-0929-478b-a0a9-82b202161220.
=============================================================

STR:
1. Visit: https://www.khronos.org/registry/webgl/sdk/tests/webgl-conformance-tests.html?version=2.0.1
2. Uncheck all tests
3. Check 'all/conformance/glsl/preprocessor' test
4. Hit run

Expected results:
- Test is successfully ran

Actual results:
- Firefox crashes.

Regression:
- This is not a regression, it crashes on old Nightly 2016-04-25 where WebGL2 was enabled by default.

Additional information:
- Other crash reports:
bp-ad552df8-22cd-43f5-b7d2-58a4a2161220
bp-9435ca69-67c0-4ae3-b5d1-dde972161220
bp-d89a8e3c-fb23-4464-a474-7963e2161220
bp-360d03b2-9f0e-4c8c-9293-e10d92161220
bp-b3234e66-0929-478b-a0a9-82b202161220
bp-b527f42f-219b-4398-a957-1a38e2161220
bp-8bdd5579-2d88-435c-b3b8-7d5c42161220
- Running this test on Ubuntu, Firefox will completely exit without crashing.
also bughunter confirms this as crash on load in opt beta builds on https://www.khronos.org/registry/webgl/sdk/tests/conformance/glsl/preprocessor/macro-expansion-tricky.html?webglVersion=2&dumpShaders=undefined&quiet=0 and reproduced on own testing . 

Jeff could you take a look ?
Blocks: 532972
Flags: needinfo?(jgilbert)
Assignee: nobody → jgilbert
Flags: needinfo?(jgilbert)
Crash in ANGLE? Gross.
Smells like OOM.
Please assign someone to investigate.
I think we just need to be catching the bad_alloc exception this should generate, but I'm not sure how simple this will be.
Assignee: jgilbert → howareyou322
Michael, please take a look.
Flags: needinfo?(cleu)
I can reproduce it on windows, looks like the test prepare some tricky macro which stack-smashes our preprocessor and make us crash.

I will look into it.
I found that this crash also happens under Mac OSX 10.11.6.

What this test do is declaring a recursive macro in GLSL like the code below.
#define m(a)
#define a m((a)

And the glsl code invoke this macro immediately, resulting a infinite growing preprocessing stack until we crash.

I think we are lack of some mechanism to check this kind of recursive macro expansion.
Assignee: howareyou322 → cleu
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLShader.cpp?q=%2Bfunction%3Amozilla%3A%3AWebGLShader%3A%3ACompileShader%28%29&redirect_type=single#217

This is where we generate stack with infinite depth and crash, I'm surveying Chromium's code to figure why they can pass the test since both of us using ANGLE for shader processing.
Flags: needinfo?(cleu)
ANGLE unconditionally enables:
SH_LIMIT_EXPRESSION_COMPLEXITY | SH_LIMIT_CALL_STACK_DEPTH


and sets:

  resources.MaxExpressionComplexity = 256;
  resources.MaxCallStackDepth = 256;

We should try doing the same.
Specifically, I believe the sources in question are at:
chromium/src/gpu/command_buffer/service/shader_translator.cc:191
chromium/src/gpu/command_buffer/service/gles2_cmd_decoder.cc:3763
Flags: needinfo?(cleu)
I've tested SH_LIMIT_EXPRESSION_COMPLEXITY | SH_LIMIT_CALL_STACK_DEPTH yesterday, it will hit an assertion here.
https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/compiler/preprocessor/MacroExpander.cpp?q=PopMacro&redirect_type=direct#182

I will try setting the resources field.
Flags: needinfo?(cleu)
OK, it seems that we have already set resources.MaxExpressionComplexity and resources.MaxCallStackDepth prior to my modification, and it still hit the assertion in MacroExpander.cpp

I will further investigate the shader compiling options by debugging chromium.
I confirmed SH_LIMIT_EXPRESSION_COMPLEXITY | SH_LIMIT_CALL_STACK_DEPTH doesn't prevent us form stackoverflow.

Here is where the infinite growing stack start
https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/compiler/translator/Compiler.cpp#303

And here is where SH_LIMIT_EXPRESSION_COMPLEXITY | SH_LIMIT_CALL_STACK_DEPTH are being checked
https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/compiler/translator/Compiler.cpp#330

It seems that we have something wrong with the preprocessor setting.
I've dumped shader compiler flags from Firefox and Chrome when running this test, it seems that compiler flags are irrelevant to the crash since I have tried to set Firefox's flag equal to Chrome but the crash still happen.

Firefox            0000 0011 1110 1000 1100
Chromium           0001 1001 1110 0000 1100

SH_REMOVE_INVARIANT_AND_CENTROID_FOR_ESSL3 | C0    F1
SH_INIT_GL_POSITION                        | C0    F1
SH_INIT_OUTPUT_VARIABLES                   | C1    F0
SH_SCALARIZE_VEC_AND_MAT_CONSTRUCTOR_ARGS  | C1    F0
Maybe they have a more sophisticated pre-preprocessor. We should try diffing the strings passed to ShCompile.
OK, they do have a more sophisticated pre-processor.

In this commit in ANGLE added checking mechanism to prevent infinite recursive macro expansion.
https://github.com/google/angle/commit/78b0c91daf16146034982fc3f7dd8d1ba90b3a0a

So I think we can simply update ANGLE and this problem will be solved.
(In reply to Michael Leu[:lenzak800](UTC+8) from comment #17)
> OK, they do have a more sophisticated pre-processor.
> 
> In this commit in ANGLE added checking mechanism to prevent infinite
> recursive macro expansion.
> https://github.com/google/angle/commit/
> 78b0c91daf16146034982fc3f7dd8d1ba90b3a0a
> 
> So I think we can simply update ANGLE and this problem will be solved.

Can we cherry-pick this cset? We can't take an ANGLE update in 51.
We should consider an ANGLE update for 52 while it's in Aurora.
Flags: needinfo?(cleu)
Yes.

I have cherry-picked the changeset and it won't crash anymore.

Do I need to issue a PR or something to our github repository of ANGLE, too?
Flags: needinfo?(cleu) → needinfo?(jgilbert)
Attachment #8823559 - Attachment is obsolete: true
(In reply to Michael Leu[:lenzak800](UTC+8) from comment #19)
> Created attachment 8823559 [details] [diff] [review]
> WIP Prevent infinite macro expansion
> 
> Yes.
> 
> I have cherry-picked the changeset and it won't crash anymore.
> 
> Do I need to issue a PR or something to our github repository of ANGLE, too?

Yes please.
Flags: needinfo?(jgilbert)
Comment on attachment 8823561 [details] [diff] [review]
Fix infinite recursion in macro expansion

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

r+ for cherry-pick
Attachment #8823561 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d776802873
Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5d776802873
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8825328 [details] [diff] [review]
Bug1324765 - Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert

Approval Request Comment
[Feature/Bug causing the regression]: WebGL 2
[User impact if declined]: Universal content process crash. Will occur when running WebGL conformance test suite.
[Is this code covered by automated tests?]: We should grab the test, too.
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: Beta uplift optional, but encouraged. We should take this in Aurora.
[Is the change risky?]: Low risk. This is a change cherry-picked from upstream.
[Why is the change risky/not risky?]: Not zero risk, since we're cherry-picking. It looks like the scope of this change is limited, so this should be fine.
[String changes made/needed]:
Attachment #8825328 - Flags: approval-mozilla-beta?
Attachment #8825328 - Flags: approval-mozilla-aurora?
Comment on attachment 8825328 [details] [diff] [review]
Bug1324765 - Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert

webgl fix for aurora52
Attachment #8825328 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8825328 [details] [diff] [review]
Bug1324765 - Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert

Fix a WebGL 2 issue. Beta51+. Should be in 51 beta 14.
Attachment #8825328 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I verified using latest Fx 51 beta 14 across platforms that this crash is not reproducible anymore.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.