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

Status: RESOLVED FIXED

(Core :: Canvas: WebGL, defect)

Description

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

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.

Comment 1

[Carsten Book [:Tomcat]]

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 ?

Comment 2

[Jeff Gilbert [:jgilbert]]

Crash in ANGLE? Gross.

Comment 3

[Jeff Gilbert [:jgilbert]]

Smells like OOM.

Comment 4

[Jeff Gilbert [:jgilbert]]

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.

Comment 5

[Peter Chang[:pchang]]

Michael, please take a look.

Comment 6

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

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.

Comment 7

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

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.

Comment 8

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

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.

Comment 9

[Jeff Gilbert [:jgilbert]]

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.

Comment 10

[Jeff Gilbert [:jgilbert]]

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

Comment 11

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

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.

Comment 12

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

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.

Comment 13

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

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.

Comment 14

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

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

Comment 15

[Jeff Gilbert [:jgilbert]]

Maybe they have a more sophisticated pre-preprocessor. We should try diffing the strings passed to ShCompile.

Comment 16

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

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

Comment 17

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

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.

Comment 18

[Jeff Gilbert [:jgilbert]]

(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.

Comment 19

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

Attachment: 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?

Comment 20

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

Attachment: Fix infinite recursion in macro expansion

Comment 21

[Jeff Gilbert [: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 22

[Jeff Gilbert [: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

Comment 23

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

OK, I have sent a PR.

https://github.com/mozilla/angle/pull/10

Comment 24

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

Attachment: Bug1324765 - Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert

This patch is for Gecko repository.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5d776802873
Prevent infinite macro expansion by cherry-picking changeset from newer ANGLE version. r=jgilbert

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 27

[Jeff Gilbert [: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 28

[Julien Cristau [:jcristau]]

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

Comment 29

[Gerry Chang [:gchang]]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ce6b5197e49f

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/74b16b5e1e8f

Comment 32

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

I verified using latest Fx 51 beta 14 across platforms that this crash is not reproducible anymore.