748112 - WebGL Water demo broken on Windows by long identifier mapping, because of double underscores

Status: VERIFIED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Test case: enable Web Console with JS warnings, and go to http://madebyevan.com/webgl-water/

Expected results: works

Actual: [15:13:12.398] uncaught exception: compile error: ERROR: 0:7: 'webgl__gl_Model_bbba1bfc05b59690' : identifiers containing two consecutive underscores (__) are reserved as possible future keywords 

The problem is that long identifier mapping prepends identifiers with "webgl_" so "_foo" becomes "webgl__foo".

I believed that this actually regressed with the latest ANGLE update, but the real bug here comes from the long identifier mapping, bug 676071.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: fix double underscore bug in long id mapping

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does this need to land on any branches?

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

This needs to land wherever the WebGL Water demo is broken. My guess is that it's only been broken since the last ANGLE update, which means it'll have to be backported to Aurora 14. But it could also be needed on Beta 13 if it's been regressed ever since long id mapping landed.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: better: also handle the case of underscore at position 8

The problem is that we take the 9 first chars of the identifier, and build a shortened identifier like

webgl_$(nine_first_chars)_$(hash_value)

so when $(nine_first_chars) ends with an underscore, we have the same problem.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 617855 [details] [diff] [review]
better: also handle the case of underscore at position 8

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any): None. This part of ANGLE is not even used at all yet on Android

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

man, I will hate toys^Wmobile even more if this fails to land in time due to m-c approval :-/

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 617855 [details] [diff] [review]
better: also handle the case of underscore at position 8

[Triage Comment]
Merge of 14 to Aurora has completed.  Please renominate for mozilla-aurora.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8977089ce890

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Interestingly, i can only reproduce the problem on Windows. I guess that only the D3DCompiler rejects double underscores.

Comment 10

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/8977089ce890

Comment 11

[Gregg Tavares]

I updated this test
https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/glsl/misc/shader-with-256-character-identifier.frag.html

I think that would show this bug if it still existed.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 617855 [details] [diff] [review]
better: also handle the case of underscore at position 8

[Approval Request Comment]
Regression caused by (bug #): 734657
User impact if declined: some webgl apps don't work
Testing completed (on m-c, etc.): On m-c for a while
Risk to taking this patch (and alternatives if risky): very low
String changes made by this patch: none

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Gregg Tavares from comment #11)
> I updated this test
> https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/
> conformance/glsl/misc/shader-with-256-character-identifier.frag.html
> 
> I think that would show this bug if it still existed.

Yup, definitely would. Thanks!

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 617855 [details] [diff] [review]
better: also handle the case of underscore at position 8

[Triage Comment]
Broken WebGL apps, and we're very early in the cycle. Approving for Aurora 14.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/742565069606

Comment 16

[Paul Silaghi, QA [:pauly]]

(In reply to Benoit Jacob [:bjacob] from comment #0)
> Test case: enable Web Console with JS warnings, and go to
> http://madebyevan.com/webgl-water/
> 
> Expected results: works
> 
> Actual: [15:13:12.398] uncaught exception: compile error: ERROR: 0:7:
> 'webgl__gl_Model_bbba1bfc05b59690' : identifiers containing two consecutive
> underscores (__) are reserved as possible future keywords

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0 beta 7

Comment 17

[Paul Silaghi, QA [:pauly]]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0 beta 1