New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 707270 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
OOO until 2019-02-10
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Unexpected javascript computation results

Reported by pavel.ka...@delightex.com, Mar 31 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce the problem:
1. Open this page: https://cospac.es/H8Hi
2. Click "Play". Do not rotate camera, just watch the scene.

What is the expected behavior?
Game works as programmed.

What went wrong?
After several seconds (~5) all of moving items disappear.

Did this work before? N/A 

Chrome version: 57.0.2987.133  Channel: stable
OS Version: OS X 10.12.3
Flash Version: 

Unfortunately we can not reproduce this in minimal tests. But here what we observe:

Affected javascript is in file code.js: 

1. Compute dt as result of tItems[index + 1] - tItems[index];
2. If dt is below zero, log dt, tItems[index + 1], tItems[index], and tItems[index + 1] - tItems[index];
3. Log output is in file log.txt. As you can see, dt is not equal to tItems[index + 1] - tItems[index].
 
code.js
476 bytes View Download
log.txt
114 bytes View Download
The issue does not always reproduce even in one version of chrome, so I'm not sure that my bisect results are correct. Anyway, here they are:

You are probably looking for a change made after 397237 (known good), but no later than 397246 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/560392b6d278bf932ba30f8130902a4292d88e7b..867134974146cb103320e398e7544b717bbbc0fe
in the current version of obfuscated production code issue is happening in file `delightex.CoSpaces-0.js` on line `b=f[a.c+1]-f[a.c];`. Insert `if (b < 0) { console.log(f[a.c+1]-f[a.c], b); }` right after this line, hit cmd+s, wait a little bit while change is applied and hit "Play" button: you will see `4.5 -8e-323` in console output.
Labels: Needs-Triage-M57
here is a footage of this bug being reproduced as described in #c2: https://youtu.be/WDDOM82NTLE
the issue does not reproduce in chrome when launched with --js-flags="--noopt"
Cc: rbasuvula@chromium.org
Labels: -Needs-Triage-M57 M-59 hasbisect OS-Linux OS-Windows
Owner: alex...@chromium.org
Status: Assigned (was: Unconfirmed)
Tested in chrome stable #57.0.2987.133 and canary #59.0.3061.0 on Mac 10.12.3,Ubuntu 14.04 & Win 10.0 able to reproduce the issue.
Below are the Bisect Details:

Bisect Info:
=============
Good Build: 53.0.2754.0(Revision - 397000)
Bad Build: 53.0.2756.0 (Revision - 397359)

Bisect URL:
=========== 
You are probably looking for a change made after 397237 (known good), but no later than 397250 (first known bad).
CHANGELOG URL:
https://chromium.googlesource.com/chromium/src/+log/560392b6d278bf932ba30f8130902a4292d88e7b..f848daf2b342548c8ec90b21396097339688819a

From the CL above, assigning the issue to the concern owner

@ alexmos : Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url:  https://codereview.chromium.org/2021513002
Note: Unrelated culprits are coming in per-revision bisect so,providing the normal bisect.
Sorry for intervention but I think this has nothing to do with webkit fullscreen changes mentioned in #c6 (and more narrow CL in #c1 does not include them as well) and is related to v8 rollout instead. 

--js-flags="--noopt" making the issue go away makes me almost sure.

Comment 8 by woxxom@gmail.com, Apr 4 2017

Yeah, js options being involved is rather indicative of V8 rollup r397238 in the posted bisect range.

Comment 9 by rtoy@chromium.org, Apr 4 2017

Components: -Blink Blink>JavaScript
Owner: hablich@chromium.org
Agreed, my r397249 only concerns plumbing for fullscreen with OOPIFs, and neither fullscreen nor OOPIFs are involved here.  Reassigning to hablich@ for further v8 triage.
Is there anything we can do to help you to resolve this issue?

Attaching IR flow and a screenshot from vegorov's irhydra with affected code...
Screen Shot 2017-04-20 at 21.57.07.png
343 KB View Download
code.asm.gz
199 KB Download
hydrogen.cfg.gz
8.1 MB Download
I have to point that this issue looks very similar to 690470: arrays involved as well, debugger and breakpoints make the issue go away...
Cc: bmeu...@chromium.org hablich@chromium.org
Owner: petermarshall@chromium.org
Peter, WDYT?
Status: Started (was: Assigned)
I can reproduce this on Linux on 58.0.3029.81.

When I split the f[a.c+1]-f[a.c] apart into separate loads, we can see that f[a.c+1] loads 3.01089895782657e-310, and f[a.c] loads 3.01089895782736e-310. So it appears that the property loads are incorrect, not the arithmetic.

The values loaded look a lot like an object field is being treated as a double field. It seems to only happen in optimized code; when the loads are done as part of a call to console.log() or a breakpoint is set, the correct values are loaded.
Labels: Merge-Request-58 Merge-Request-59
This is actually fixed on the current canary. I did a bisect which indicates the fix is in this range: https://chromium.googlesource.com/chromium/src/+log/049ab8b91cb873da45755e6c678d153b7fc3d856..9740714c369114929625061c17be977df4fac810

Bisecting manually into the V8 roll indicates the fix was in https://chromium.googlesource.com/v8/v8/+/2d856544e5e3cb8abf99a30749b4bfe39c29886a ([ic] Fix handling of elements kind transitions in polymorphic keyed ICs.).

This CL looks highly related to the problem, so I'd say this bug has been fixed since 60.0.3078.0. tools/release/mergeinfo.py tells us this was not back-merged, which it probably should be.
Project Member

Comment 16 by sheriffbot@chromium.org, May 4 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Request-58 Merge-Rejected-58
This issue was originally reported on M57 (not an M58 regression) and M58 is already in Stable since 04/19. We're only taking absolutely critical merges in for M58 stable respin (if any). Hence, rejecting merge to M58. Please let me know if there is any concern here. Thank you.
Project Member

Comment 18 by sheriffbot@chromium.org, May 8 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This change has been approved for M59. Can you please merge this in 3071 ASAP?
Project Member

Comment 20 by bugdroid1@chromium.org, May 10 2017

Labels: merge-merged-5.9
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/fb9d10cecfbf66d280075f052d925ce7f44984a9

commit fb9d10cecfbf66d280075f052d925ce7f44984a9
Author: Peter Marshall <p.s.marshall0@gmail.com>
Date: Wed May 10 09:13:41 2017

Merged: [ic] Fix handling of elements kind transitions in polymorphic keyed ICs.

Revision: 2d856544e5e3cb8abf99a30749b4bfe39c29886a

BUG= chromium:707270 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=bmeurer@chromium.org

Change-Id: I39126ccdc6c968c1047a85e53012be10a22fd90f
Reviewed-on: https://chromium-review.googlesource.com/501767
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/5.9@{#41}
Cr-Branched-From: fe9bb7e6e251159852770160cfb21dad3cf03523-refs/heads/5.9.211@{#1}
Cr-Branched-From: 70ad23791a21c0dd7ecef8d4d8dd30ff6fc291f6-refs/heads/master@{#44591}
[modify] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/src/compiler/access-info.cc
[modify] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/src/crankshaft/hydrogen.cc
[modify] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/src/ic/handler-compiler.cc
[modify] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/src/ic/handler-compiler.h
[modify] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/src/ic/ic.cc
[modify] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/src/ic/ic.h
[add] https://crrev.com/fb9d10cecfbf66d280075f052d925ce7f44984a9/test/mjsunit/regress/regress-crbug-700733.js

Labels: merge-merged-59
Status: Fixed (was: Started)
Project Member

Comment 22 by sheriffbot@chromium.org, May 12 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Hotlist-Merge-Approved -Merge-Approved-59

Sign in to add a comment