asm.js parsing regression related to unnecessary parens
Reported by
scott.no...@gmail.com,
Sep 13 2017
|
||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.79 Safari/537.36 Steps to reproduce the problem: 1. Add Signal Desktop development version in chrome://extensions https://github.com/WhisperSystems/Signal-Desktop/blob/master/CONTRIBUTING.md 2. Launch extension 3. Right-click and select 'Inspect Background Page' What is the expected behavior? No warning. No previous version of Chrome since I started on the project in May has shown this warning. What went wrong? Warning in the console: "Invalid asm.js: Unexpected token" The two problematic lines are: HEAP32[((ptr)>>2)]=value4; and HEAP32[((dest)>>2)]=((HEAP32[((src)>>2)])|0); They can each be fixed by removing all of the extra parens. But again, this is something that worked up until very recently. Did this work before? Yes 60.x Chrome version: 61.0.3163.79 Channel: stable OS Version: OS X 10.11.6 Flash Version:
,
Sep 14 2017
Able to reproduce the issue on Windows 10, mac 10.12.6 and Ubuntu 14.04 using chrome reported version #61.0.3163.79 and latest canary #63.0.3214.0. Bisect Information: ===================== Good build: 61.0.3163.53 Revision(359969) Bad Build : 61.0.3163.54 Revision(360248) Change Log URL: https://chromium.googlesource.com/chromium/src/+log/61.0.3163.53..61.0.3163.54?pretty=fuller&n=10000 From the above change log suspecting below change Change-Id: I798cc67255d07bbd8404254ec0b6332e77ec0452 Reviewed-on: https://chromium-review.googlesource.com/619293 bradnelson@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. Also could you please confirm if it can be marked as a stable blocker as the issue is broken in M61 and can it be considered for next stable refresh. Thanks...!!
,
Sep 17 2017
,
Sep 18 2017
The new asm.js validator doesn't materialize a full ast so there some places where it's less generous in terms of the grammar. Though as this is just a matter of extra parens, this is probably supposed to be allowed by the spec. Getting this right might require infinite rewind, or something clever to avoid relying on local pattern matching. It's less that it was working before the change, than that the old pipeline wasn't deciding which asm.js is valid. For 61, this just means it won't hit the new pipeline, though in later versions this will also mean the code won't be optimized at all. We should probably fix, but not sure the priority. Did this asm.js come from emscripten or some other source? Trying if a simple local fix can work.
,
Sep 18 2017
,
Sep 18 2017
I'm not sure how it was generated, but it was generated quite a while ago. Since it's sensitive encryption code, it's not something we change very often.
,
Sep 18 2017
Okay, just got confirmation that it was emscripten
,
Oct 31 2017
Have the same problem with unity game converted to js with emscripten + sfe. Archive with game attached. Just run testing-unity.html in chrome and you will see warning: Invalid asm.js: Unexpected token In FF this code compiles without error.
,
Oct 31 2017
Ok, I found that in my case there is no extra parens. To find where problem I use sed:
sed -i "s/0;/0;\n/ig" bin.asm.code.unityweb
After doing this I have this error:
bin.asm.code.unityweb:419968 Invalid asm.js: Unexpected token
Problem line:
c[$<<2>>2]=a;return}function $$F(a,$){a=a|0;
After digging I found that problem is in:
$<<2>>2
If I replace it with ($<<2)>>2 then it pass validation on this line.
After that I replace all similar places with sed:
sed -i "s/\([A-Za-z$]\)<<2>>2/(\\1<<2)>>2/ig" bin.asm.code.unityweb
and this solve the problem.
,
Oct 31 2017
Should I fill a new bug report?
,
Nov 2 2017
I guess filing a new bug report makes sense (cc bradnelson, mstarzinger)
,
Nov 3 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by ligim...@chromium.org
, Sep 13 2017