Issue metadata
Sign in to add a comment
|
asm.js with continue in loop freezes page
Reported by
oriol-bu...@hotmail.com,
Nov 27 2017
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0 Steps to reproduce the problem: 1. Write a asm.js module with a method that has a loop with a continue statement. 2. Call the method. What is the expected behavior? It works. What went wrong? Chromium freezes, CPU goes to 30%, after some time it asks me if I want to crash the page. Did this work before? N/A Chrome version: 64.0.3279.0 Channel: n/a OS Version: 10.0 Flash Version: Shockwave Flash 27.0 r0
,
Nov 29 2017
"Able to reproduce the issue on reported version 64.0.3279.0 using Mac 10.12.6, Ubuntu 14.04 and Windows 10. Bisect Info: ================ Good build: 62.0.3167.0 Bad build: 62.0.3168.0 CHANGELOG URL: You are probably looking for a change made after 489587 (known good), but no later than 489588 (first known bad). https://chromium.googlesource.com/chromium/src/+log/25d94996a601cebc16d479bb06c09f4a28da6090..bdf7e5fed2604997621c12c1fa2f057e9b7f2556 Change log from v8 auto roll: https://chromium.googlesource.com/v8/v8/+/57031e82db6864b0c0caa67502b222a255ef9a96 suspect:https://chromium-review.googlesource.com/583655 Suspecting same from changelog. @mstarzinger: Please confirm the issue and help in re-assigning if it is not related to your change. Note: Adding stable blocker for M-64 and requesting to merge the fix( (if its safe merge) to M-63. Thanks!"
,
Nov 30 2017
,
Dec 6 2017
Still we are able to reproduce the issue on Windows 7, mac 10.12.6 & Ubuntu 14.04 using chrome latest Canary-65.0.3285.0 . mstarzinger@, Could you please take a look and update the thread. Thanks..!
,
Dec 11 2017
,
Dec 11 2017
Checked more, it indeed looks like a very similar issue. The 'continue' statement just jumps back to the header of the loop, without executing the increment code. Bug 5912 was about do-while loops, where we fixed this by having =============================================== block <-- break target | loop | block <-- continue target | <body> | <condition> | br_if 0 =============================================== We still do the same in the new validator. For for-loops, however, we generate =============================================== block <-- break target | loop <-- continue target | <condition> | br_if 1 | <body> | <increment> | br 0 =============================================== So if the body contains a continue, we skip the increment code. Instead, we should generate something like this: =============================================== block <-- break target | loop | block <-- continue target | <condition> | br_if 2 | <body> | <increment> | br 0 ===============================================
,
Dec 18 2017
Friendly ping to get an update on this issue as it is marked as stable blocker. Thanks..!
,
Dec 18 2017
Michi is on christmas vacation already, I will take over this one. +Brad, as this is an asm.js issue. Will add you as reviewer for the CL.
,
Dec 18 2017
CL which implements what I described in #6: https://crrev.com/c/832447
,
Dec 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/9a241228cfcd5e2b1bfed3b58254b2cfb4e1fd33 commit 9a241228cfcd5e2b1bfed3b58254b2cfb4e1fd33 Author: Clemens Hammacher <clemensh@chromium.org> Date: Mon Dec 18 16:29:27 2017 [asm.js] Fix continue target in for loops Make sure that a continue still executed the increment part of a for loop by adding another nested block for the body, which is the break target for a continue in the body. The increment code lives outside this block, in the original loop. R=bradnelson@chromium.org CC=mstarzinger@chromium.org Bug: chromium:788916 Change-Id: I178b874ffac16d9237a0f4da097d2742bd93335a Reviewed-on: https://chromium-review.googlesource.com/832447 Commit-Queue: Brad Nelson <bradnelson@chromium.org> Reviewed-by: Brad Nelson <bradnelson@chromium.org> Cr-Commit-Position: refs/heads/master@{#50169} [modify] https://crrev.com/9a241228cfcd5e2b1bfed3b58254b2cfb4e1fd33/src/asmjs/asm-parser.cc [modify] https://crrev.com/9a241228cfcd5e2b1bfed3b58254b2cfb4e1fd33/test/mjsunit/wasm/asm-wasm.js
,
Dec 18 2017
,
Dec 18 2017
[Auto-generated comment by a script] We noticed that this issue is targeted for M-64; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-64 label, otherwise remove Merge-TBD label. Thanks.
,
Dec 18 2017
This should be merged to M64, as it breaks correct asm.js code. Merging it further back seems not necessary, as this is broken since M60.
,
Dec 19 2017
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 19 2017
Re #13: I was wrong, this is only broken since M62, when '--validate-asm' was enabled by default. I still think this is not important enough to be backmerged to M63, since it's already broken in the stable version since two months.
,
Dec 19 2017
Thanks for the fix clemensh@ - is this well tested in Canary/Dev? Is it a safe merge overall? Seems like this may have just landed in canary today?
,
Dec 19 2017
Yes, it will only be contained in today's canary. Let's give this some time to bake first. I will be on vacation from Friday to 2nd of January. Do you think we can merge back on Thursday?
,
Dec 19 2017
Oh, and I would classify this as a safe merge. It's quite small and uses the same logic which is already in use for other asm.js code (do-while loops).
,
Dec 21 2017
The NextAction date has arrived: 2017-12-21
,
Dec 22 2017
How does the change look clemensh@?
,
Jan 2 2018
,
Jan 2 2018
Thanks, prepared backmerge: https://crrev.com/c/846751
,
Jan 4 2018
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/cc5e3d3041b520234193b33535c5e019d97613f2 commit cc5e3d3041b520234193b33535c5e019d97613f2 Author: Clemens Hammacher <clemensh@chromium.org> Date: Thu Jan 04 14:19:42 2018 Merged: [asm.js] Fix continue target in for loops Make sure that a continue still executed the increment part of a for loop by adding another nested block for the body, which is the break target for a continue in the body. The increment code lives outside this block, in the original loop. Originally reviewed on: https://chromium-review.googlesource.com/832447 Bug: chromium:788916 R=bradnelson@chromium.org NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Change-Id: I6fe420a6dadb35ad8499185b3767923b94669001 Reviewed-on: https://chromium-review.googlesource.com/846751 Reviewed-by: Michael Hablich <hablich@chromium.org> Commit-Queue: Clemens Hammacher <clemensh@chromium.org> Cr-Commit-Position: refs/branch-heads/6.4@{#35} Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1} Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724} [modify] https://crrev.com/cc5e3d3041b520234193b33535c5e019d97613f2/src/asmjs/asm-parser.cc [modify] https://crrev.com/cc5e3d3041b520234193b33535c5e019d97613f2/test/mjsunit/wasm/asm-wasm.js
,
Jan 4 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by krajshree@chromium.org
, Nov 28 2017