Closed Bug 1324765 Opened 3 years ago Closed 3 years ago
Crash in std::vector<T>::_Buy
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 ?
Assignee: nobody → 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.
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.
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.
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
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.
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.
I think the difference in parsing behavior is in glslang_parse(context) here. https://dxr.mozilla.org/mozilla-central/source/gfx/angle/src/compiler/translator/ParseContext.cpp?q=%2Bfunction%3A%22sh%3A%3APaParseStrings%28size_t%2C+const+char+%2Aconst+%2A%2C+const+int+%2A%2C+sh%3A%3ATParseContext+%2A%29%22&redirect_type=single#4485
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.
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)
(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.
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+
OK, I have sent a PR. https://github.com/mozilla/angle/pull/10
This patch is for Gecko repository.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d776802873 Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert
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]:
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+
I verified using latest Fx 51 beta 14 across platforms that this crash is not reproducible anymore.
You need to log in before you can comment on or make changes to this bug.