Array.push sometimes returns the wrong thing
Reported by
da...@toumetis.com,
Oct 14 2016
|
||||||||||||||||||||||
Issue description
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2891.0 Safari/537.36
Steps to reproduce the problem:
1. Part of a Promises implementation:
var undef,
nextTick = (function() {
var fns = [],
enqueueFn = function(fn) {
var wat = fns.push(fn);
var ret = wat === 1;
console.log('enqueued, length = ' + fns.length + ', push returned ' + wat + ', ret = ' + ret);
return ret;
},
callFns = function() {
var fnsToCall = fns, i = 0, len = fns.length;
fns = [];
while(i < len) {
fnsToCall[i++]();
}
};
return function(fn) { // old browsers
enqueueFn(fn) && setTimeout(callFns, 0);
};
})(),
2. Call this function repeatedly, passing a function to be run on the next pass through the event loop.
The misbehaviour only appears to trigger in the full app, I cannot generate it simply by looping. It also only shows up in the minimised form of the app where the entire javascript is in a single inline script in index.html.
What is the expected behavior?
console should always be of the form
enqueued, length = <n>, push returned <n>, ret = <n === 1>
What went wrong?
fns.push(fn) occasionally returns the passed item not the new length of the array
enqueued, length = 1, push returned 1, ret = true
enqueued, length = 2, push returned 2, ret = false
enqueued, length = 3, push returned 3, ret = false
enqueued, length = 4, push returned 4, ret = false
enqueued, length = 1, push returned function (){var i=0,cb,defer,fn;
while(i<len){cb=callbacks[i++];
defer=cb.defer;
fn=cb.fn;
if(fn){var ctx=cb.ctx,res;
try{res=ctx?fn.call(ctx,arg):fn(arg)
}catch(e){defer.reject(e);
continue
}isResolved?defer.resolve(res):defer.notify(res)
}else{isResolved?isFulfilled?defer.resolve(arg):defer.reject(arg):defer.notify(arg)
}}}, ret = false
enqueued, length = 2, push returned 2, ret = false
enqueued, length = 3, push returned 3, ret = false
Did this work before? Yes 53.0.2785
Chrome version: 56.0.2891.0 Channel: n/a
OS Version: OS X 10.11.6
Flash Version:
,
Oct 14 2016
Sorry for extra post. I wanted to point out I can reproduce this issue on 54.0.2840.59 (64-bit) on both OSX and Windows.
,
Oct 14 2016
I can confirm this on OSX Version 54.0.2840.59 (64-bit) as well
,
Oct 14 2016
,
Oct 14 2016
,
Oct 14 2016
paul.martin, would you be able to provide a workable example on jsbin maybe? This would significantly speed up understanding and confirming the issue.
,
Oct 15 2016
Unfortunately I have not been able to reproduce this in a small sample, despite great effort. At least not something that can be hosted on jsbin. However I have put up a sample that triggers the issue immediately at this url: https://chromebug.workspace.com/cr656037/index.html?#/defect/table In particular I added an extra if statement that throws when the return value of Array.push is != to the new array length. It will throw immediately on jsviews.js:1676 if (tmplBindingKey != tmplBindings.length) throw 'tmplBindings.push returned incorrect value = ' + tmplBindingKey + ' should be ' + tmplBindings.length; I will continue to attempt to reproduce this is a smaller sample, but in the mean time if I can provide you with additional information, please let me know.
,
Oct 15 2016
I wanted to point out another thing about this issue. If you have a breakpoint set in the debugger it does not reproduce. The only way I can get it to reproduce is to just have the debugger open with no breakpoints set. BTW for me it triggers everytime. I've been told by other developers for them it's intermittent - about 50% of the time. If you get to a login dialog, that means it did not trigger the issue. Simple refresh a few times and odds are good it will hit.
,
Oct 16 2016
I did some investigation using the test page provided by paul.martin. The intermittent aspect of this bug, and the fact that it doesn't reproduce when there is an active debugger breakpoint, seem to suggest that it is due to some kind of race condition in Chrome. It seems quite a bad bug to me, so I hope a fix can be found and published as rapidly as possible.
,
Oct 16 2016
See https://github.com/BorisMoore/jsviews/issues/350#issuecomment-254014181 for screenshots of call stack and (rather strange) debugger state after the error has occurred.
,
Oct 17 2016
,
Oct 17 2016
,
Oct 17 2016
IIUC, when the repro in #7 shows the login box, that means everything worked. When it doesn't ask for login credentials but just shows an empty page, an exception can be found printed to the DevTools console, meaning the bug happened. Based on this, I've bisected this issue between M53 (known good) and M54 (known bad): You are probably looking for a change made after 411602 (known good), but no later than 411611 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/f882cef562cffee9766803b759963db682ef9ef3..dd6ed6ab19569f383dece7ccdfb5c68a4d4539f9 That range contains a V8 roll: https://chromium.googlesource.com/v8/v8/+log/b460adce..8d221b19 Suspecting 50f223e [turbofan] Add inlined Array.prototype.push support. by bmeurer I can't repro with M55 any more, but somewhere in the M54 -> M55 range this repro becomes flaky, so I haven't been able to bisect where it got fixed. Possibly related: issue 644689 and its fix 4ed27fc836acfc3218a5e4ce6d878a513e9df788 (which hasn't been backmerged to M54).
,
Oct 17 2016
,
Oct 18 2016
The Array.prototype.push inlined version always returns the value, so this was always wrong (in TurboFan). I'll fix that and back-merge to M54.
,
Oct 18 2016
,
Oct 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/85844420a2cab6e8f4582e462b28f7f86db2d651 commit 85844420a2cab6e8f4582e462b28f7f86db2d651 Author: bmeurer <bmeurer@chromium.org> Date: Tue Oct 18 08:01:47 2016 [turbofan] Fix return value of Array.prototype.push. The inlined version of Array.prototype.push returned the value that was pushed instead of the new "length" property value. R=jarin@chromium.org BUG= chromium:656037 Review-Url: https://codereview.chromium.org/2425903002 Cr-Commit-Position: refs/heads/master@{#40384} [modify] https://crrev.com/85844420a2cab6e8f4582e462b28f7f86db2d651/src/compiler/js-builtin-reducer.cc [modify] https://crrev.com/85844420a2cab6e8f4582e462b28f7f86db2d651/src/compiler/typer.cc [add] https://crrev.com/85844420a2cab6e8f4582e462b28f7f86db2d651/test/mjsunit/regress/regress-crbug-656037.js
,
Oct 18 2016
,
Oct 18 2016
This too late for M54 as we're already in Stable. Feel free to push back if there's a critical reason to get it in this milestone.
,
Oct 19 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Oct 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/fa372a9a86597f71b6b7785250ccee473adaa83a commit fa372a9a86597f71b6b7785250ccee473adaa83a Author: Benedikt Meurer <bmeurer@google.com> Date: Wed Oct 19 07:23:14 2016 Merged: [turbofan] Fix return value of Array.prototype.push. Revision: 85844420a2cab6e8f4582e462b28f7f86db2d651 BUG= chromium:656037 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true TBR=jarin@chromium.org Review URL: https://codereview.chromium.org/2433863002 . Cr-Commit-Position: refs/branch-heads/5.5@{#16} Cr-Branched-From: 3cbd5838bd8376103daa45d69dade929ee4e0092-refs/heads/5.5.372@{#1} Cr-Branched-From: b3c8b0ce2c9af0528837d8309625118d4096553b-refs/heads/master@{#40015} [modify] https://crrev.com/fa372a9a86597f71b6b7785250ccee473adaa83a/src/compiler/js-builtin-reducer.cc [modify] https://crrev.com/fa372a9a86597f71b6b7785250ccee473adaa83a/src/compiler/typer.cc [add] https://crrev.com/fa372a9a86597f71b6b7785250ccee473adaa83a/test/mjsunit/regress/regress-crbug-656037.js
,
Oct 19 2016
#19: Not sure what you consider "critical"; the situation is: - it's a regression in M54 - it breaks websites - users are complaining about it - the fix is safe Are those valid reasons to fix a bug on Stable these days, or not?
,
Oct 19 2016
bmeurer@, Could you please provide the TEST steps to manually verify the fix.
,
Oct 19 2016
#23: Since an automated test is included in the CL, manual testing isn't really necessary... but you can run the following in the DevTools console:
function f(a) { try {} catch(e) {}; return a.push(1); }; var a = []; for (var i = 1; i < 20000; i++) { if (f(a) !== i) throw "You've hit the bug in iteration " + i; }; console.log("Fix verified!")
If it throws an error, you've hit the bug. If it prints "Fix verified", you've verified the fix.
,
Oct 19 2016
#19: I was expecting/hoping to see the fix merged back to M54; for us it's critical - It breaks previously working (and correct) code that's deployed by our customers - It's in a fundamental part of the language so we need to check and issue work rounds for all of the libraries used as well as our own code.
,
Oct 19 2016
Please merge to 5.4. Post-mortem will be provided shortly.
,
Oct 19 2016
,
Oct 19 2016
#19: This is critical for us as well. I'd also point out that libraries like jQuery (via sizzle) are directly impacted by this bug. This bug is quite nefarious and has a very broad reach.
,
Oct 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0112cbb3a62f30d855a7d2d54237f239c9ce0642 commit 0112cbb3a62f30d855a7d2d54237f239c9ce0642 Author: Benedikt Meurer <bmeurer@google.com> Date: Wed Oct 19 11:44:00 2016 Merged: [turbofan] Fix return value of Array.prototype.push. Revision: 85844420a2cab6e8f4582e462b28f7f86db2d651 BUG= chromium:656037 LOG=N NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true TBR=jarin@chromium.org Review URL: https://codereview.chromium.org/2434753002 . Cr-Commit-Position: refs/branch-heads/5.4@{#65} Cr-Branched-From: 5ce282769772d94937eb2cb88eb419a6890c8b2d-refs/heads/5.4.500@{#2} Cr-Branched-From: ad07b49d7b47b40a2d6f74d04d1b76ceae2a0253-refs/heads/master@{#38841} [modify] https://crrev.com/0112cbb3a62f30d855a7d2d54237f239c9ce0642/src/compiler/js-builtin-reducer.cc [modify] https://crrev.com/0112cbb3a62f30d855a7d2d54237f239c9ce0642/src/compiler/typer.cc [add] https://crrev.com/0112cbb3a62f30d855a7d2d54237f239c9ce0642/test/mjsunit/regress/regress-crbug-656037.js
,
Oct 19 2016
,
Oct 19 2016
,
Oct 19 2016
,
Dec 8 2016
|
||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||
Comment 1 by paul.mar...@workspace.com
, Oct 14 2016