Last Comment Bug 748112 - WebGL Water demo broken on Windows by long identifier mapping, because of double underscores
: WebGL Water demo broken on Windows by long identifier mapping, because of dou...
Status: VERIFIED FIXED
webgl-conformance[qa!]
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks: 676071
  Show dependency treegraph
 
Reported: 2012-04-23 14:02 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-07-19 06:58 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
verified
verified


Attachments
fix double underscore bug in long id mapping (866 bytes, patch)
2012-04-23 14:19 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
better: also handle the case of underscore at position 8 (1.33 KB, patch)
2012-04-24 07:09 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-04-23 14:02:40 PDT
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) 2012-04-23 14:19:37 PDT
Created attachment 617644 [details] [diff] [review]
fix double underscore bug in long id mapping
Comment 2 Boris Zbarsky [:bz] 2012-04-23 18:24:29 PDT
Does this need to land on any branches?
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-04-23 20:04:49 PDT
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) 2012-04-24 07:09:06 PDT
Created attachment 617855 [details] [diff] [review]
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) 2012-04-24 08:10:07 PDT
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) 2012-04-24 08:13:00 PDT
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 2012-04-24 11:22:46 PDT
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) 2012-04-24 13:05:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8977089ce890
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-04-24 13:07:21 PDT
Interestingly, i can only reproduce the problem on Windows. I guess that only the D3DCompiler rejects double underscores.
Comment 11 Gregg Tavares 2012-05-10 00:47:49 PDT
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) 2012-05-10 05:02:23 PDT
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) 2012-05-10 05:06:33 PDT
(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] 2012-05-11 16:45:35 PDT
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) 2012-05-13 05:58:16 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/742565069606
Comment 16 Paul Silaghi, QA [:pauly] 2012-06-19 04:42:23 PDT
(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] 2012-07-19 06:58:17 PDT
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0 beta 1

Note You need to log in before you can comment on or make changes to this bug.