Some further information on this:

1) Within the for loop that iterates the object properties, 'key' is always correct
2) Once the for loop is exited, the key is intermittently undefined instead of it containing the value of the last key name

I could not find a way to do it but the title of this issue should likely be renamed to more accurately describe the problem based on these additional findings.

See https://jsfiddle.net/Lqxhy4ev/4/ for more details.

This bug is triggered by code inside of the isPlainObject function of jQuery 1.x. So it could be quite widespread.

hablich@ can you investigate this?

That sounds like it is related to TF.

Also reproduces fine in D8 btw

Deleted: test.js 1.5 KB

test.js 1.5 KB View Download

Reduced repro:

---
// Flags: --allow-natives-syntax

function isPlainObject(obj) {
  var key;
  for (key in obj) { }
  return key === undefined;
};

var Widget = function() {};

Widget.prototype = { foo: function() {} };

var item = new Widget();

%OptimizeFunctionOnNextCall(isPlainObject);
assertFalse(isPlainObject(item));
---

Seems to be a bug in the AstGraphBuilder; the initial graph already has undefined for the key use outside the for-in loop.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/4dab7b5a1d6722002d47d0be2481cb65602a2451

commit 4dab7b5a1d6722002d47d0be2481cb65602a2451
Author: mstarzinger <mstarzinger@chromium.org>
Date: Tue Sep 20 12:36:51 2016

[turbofan] Fix loop assignment analysis on ForInStatements.

The implicit assignment to the induction variable in a ForInStatement
has been ignored by the AST loop assignment analysis. This was hidden
for cases where the parser introduced a ".for" temporary, but triggers
when the variable is declared outside the loop.

R=bmeurer@chromium.org
TEST=mjsunit/regress/regress-crbug-647887
BUG= chromium:647887 

Review-Url: https://codereview.chromium.org/2356733002
Cr-Commit-Position: refs/heads/master@{#39551}

[modify] https://crrev.com/4dab7b5a1d6722002d47d0be2481cb65602a2451/src/compiler/ast-loop-assignment-analyzer.cc
[add] https://crrev.com/4dab7b5a1d6722002d47d0be2481cb65602a2451/test/mjsunit/regress/regress-crbug-647887.js

The above fix should address the issue. We probably want to merge that back after sufficient backing time in Canary.

Agreed, this should be on the next Stable push.

Adding milestone tracking labels...

All -

Thanks for the quick attention and fix for this, greatly appreciated!

In reviewing the fix (assigning key when variable is proxied), I'm curious why this issue presents intermittently and only with try..catch as opposed to every time.  Just trying to better understand.  Appreciate any insight you can provide.  

Thanks again!

Re #13: The underlying reason is that V8 contains multiple compilers. This bug is specific to the TurboFan compiler. So it only triggers when the function is determined to be hot and gets optimized (hence only intermittently) and only for functions that the compiler pipeline decides to optimize with TurboFan and not with Crankshaft (the latter doesn't support exception handling, the former does, hence only with try-catch).

[Automated comment] Request affecting a post-stable build (M53), manual review required.

Your change meets the bar and is auto-approved for M54 (branch: 2840)

@mstarzinger - Thanks for the additional info, greatly appreciated!

Please confirm whether this change is baked/verified in Canary and safe to merge?If yes, merge your change to M54 (branch: 2840) ASAP so that we could take this for next Beta Release.

Please reply to comment #18 (Please confirm whether this change is baked/verified in Canary and safe to merge?).

Tested the same on win10, mac 10.11.6 and Linux 14.04 chrome version 55.0.2868.0 - isNotPlain is 100,000 as expected

Please find the screenshot

Adding the TE-Verified labels

Deleted: 647887.png 498 KB

	647887.png 498 KB View Download

Re #19/#18: Yes this had baking time in Canary by now. I will do the merge to M54 (V8 5.4 branch) now.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e6ff184457db4d7726639c61d69dd80f44a8ad5e

commit e6ff184457db4d7726639c61d69dd80f44a8ad5e
Author: Michael Starzinger <mstarzinger@google.com>
Date: Fri Sep 23 08:31:50 2016

Merged: [turbofan] Fix loop assignment analysis on ForInStatements.

Revision: 4dab7b5a1d6722002d47d0be2481cb65602a2451

BUG= chromium:647887 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=bmeurer@chromium.org, hablich@chromium.org

Review URL: https://codereview.chromium.org/2364843002 .

Cr-Commit-Position: refs/branch-heads/5.4@{#55}
Cr-Branched-From: 5ce282769772d94937eb2cb88eb419a6890c8b2d-refs/heads/5.4.500@{#2}
Cr-Branched-From: ad07b49d7b47b40a2d6f74d04d1b76ceae2a0253-refs/heads/master@{#38841}

[modify] https://crrev.com/e6ff184457db4d7726639c61d69dd80f44a8ad5e/src/compiler/ast-loop-assignment-analyzer.cc
[add] https://crrev.com/e6ff184457db4d7726639c61d69dd80f44a8ad5e/test/mjsunit/regress/regress-crbug-647887.js

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/ebc2b13efeec46fa4e47884d0dc41bd118d3d033

commit ebc2b13efeec46fa4e47884d0dc41bd118d3d033
Author: Michael Starzinger <mstarzinger@google.com>
Date: Fri Sep 23 08:48:17 2016

Merged: [turbofan] Fix loop assignment analysis on ForInStatements.

Revision: 4dab7b5a1d6722002d47d0be2481cb65602a2451

BUG= chromium:647887 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=hablich@chromium.org, bmeurer@chromium.org

Review URL: https://codereview.chromium.org/2354333006 .

Cr-Commit-Position: refs/branch-heads/5.3@{#73}
Cr-Branched-From: 820a23aade5e74a92d794e05a0c2b3597f0da4b5-refs/heads/5.3.332@{#2}
Cr-Branched-From: 37538cb2c1b4d75c41af386cb4fedbe5566f5608-refs/heads/master@{#37308}

[modify] https://crrev.com/ebc2b13efeec46fa4e47884d0dc41bd118d3d033/src/compiler/ast-loop-assignment-analyzer.cc
[add] https://crrev.com/ebc2b13efeec46fa4e47884d0dc41bd118d3d033/test/mjsunit/regress/regress-crbug-647887.js

Per comment #25, this is already merged to M53, so removing "Merge-Approved-53" label.

Verified the fix on Windows(7, 10), Mac OSX 10.11.6 and Linux(ubuntu) with Chrome version 53.0.2785.143. Steps followed as mentioned in bug report.

No further backport needed for Node.js.