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

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
HW: ----
NextAction: ----
OS: ----
Priority: 3
Type: Bug
ES5



Sign in to add a comment

Non-enumerable property fails to shadow inherited enumerable property from for-in

Reported by erights@gmail.com, May 12 2010

Issue description

At https://mail.mozilla.org/pipermail/es5-discuss/2010-May/003536.html
Allen clarifies that, according to the ES5 spec,

    function showProps(obj) {
      var result = [];
      for (var k in obj) {
        result.push(k, ': ', ''+obj[k], '\n');
      }
      return result.join('');
    }
    var base = {x:8};
    var derived = Object.create(base, {x: {value: 9, enumerable: false}});
    showProps(derived);

Should print the empty string. On Chrome 5.0.375.38 beta, it prints "x: 9".
Besides violating the spec, it is also the less useful behavior for the
common for-in loop behavior (shown above as "obj[k]") of using the key to
index into the object being iterated, in order to get the corresponding value.

The corresponding WebKit bug is at
https://bugs.webkit.org/show_bug.cgi?id=38970
 

Comment 1 by ricow@chromium.org, May 12 2010

Labels: Priority-Low ES5

Comment 2 by ricow@chromium.org, May 12 2010

The bug is related to the way we find objects to add in the for-in-loop. The actual 
value of enumerable (on derived['x]) is actually set to false, this can be confirmed 
by doing:

  var base = {x:10};
  var derived = Object.create(base,  {x: {value: 9, enumerable: false}});
  var desc = Object.getOwnPropertyDescriptor(derived, "x");
  for(var v in desc)
    print(v + ": " + desc[v]);

which gives:
  value: 9
  writable: false
  enumerable: false
  configurable: false

In for-in-loop code that finds the elements to add we currently simply discard a 
value if it is not enumerable. Because we do a bottom up search for properties to add 
(starting at the actual object and traversing up the prototype chain) we will add the 
property to the result when processing the prototype of derived(base) even though we 
did not add it when processing derived. 
Can someone point me in the right direction of which functions I should look into? I understand the problem as stated by ricow, but I'm new to the codebase and just want to see if I can figure this out. A good direction would be appreciated.

Comment 4 by sam.mi...@gmail.com, Aug 19 2014

It looks like the problem is right about here: https://github.com/v8/v8/blob/master/src/objects.cc#L5999

On each turn through the loop over prototypes, v8 is considering only enumerable keys -- note the calls to NumberOfEnumElements() and GetEnumElementKeys().

I think it would be necessary to maintain a map of all elements, whether enumerable or not, and use an algorithm like this (pseudocode):

seenKeys = []

for each object in prototype chain:

  visibleKeys = GetEnumElementKeys()
  allKeys = GetElementKeys()

  visibleKeys = Subtract(visibleKeys, seenKeys)
  content = Union(content, visibleKeys)
  seenKeys = Union(seenKeys, allKeys)

ES5 describes its desired result, but does not specify an algorithm: http://www.ecma-international.org/ecma-262/5.1/#sec-12.6.4

ES6 (draft) describes an algorithm in terms of generators: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ordinary-object-internal-methods-and-internal-slots-enumerate

Note in ES6 the use of a Set to enforce the rule that a name is only considered once, and that higher-level nonEnumerable keys shadow lower-level enumerable keys.
Labels: Type-Bug

Comment 6 by ricow@chromium.org, Feb 9 2015

Cc: danno@chromium.org
Owner: ----
Danno, could you reassign this?
Status: Accepted
Update done during clean-up of issue tracker.

Comment 8 by habl...@google.com, Apr 29 2015

Status: Available

Comment 9 by adamk@chromium.org, Aug 13 2015

Cc: -danno@chromium.org
Labels: Area-Language

Comment 10 by adamk@chromium.org, Aug 13 2015

Owner: adamk@chromium.org
Status: Assigned

Comment 12 by adamk@chromium.org, Nov 17 2015

Cc: verwa...@chromium.org rossberg@chromium.org adamk@chromium.org
 Issue chromium:464954  has been merged into this issue.
Owner: cbruni@chromium.org
Since I've been working on the KeyAccumulator I'm going to hijack this issue.
Project Member

Comment 14 by 76821325...@developer.gserviceaccount.com, Jan 20 2016

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

commit ed24dfe80d1da0827b8571839ee52c03ad09c9c7
Author: cbruni <cbruni@chromium.org>
Date: Wed Jan 20 12:36:49 2016

[runtime] Do not use the enum-cache for keys retrieval.

Currently we fail to properly handle shadowed properties. If the
receiver defines a non-enumerable property that reappears on the
prototype as enumerable it incorrectly shows up in [[Enumerate]].
By extending the KeyAccumulator to track non-enumerable properties
we can now properly filter them out when seeing them further up in
the prototype-chain.

BUG= v8:705 
LOG=y

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

Cr-Commit-Position: refs/heads/master@{#33405}

[modify] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/src/elements.cc
[modify] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/src/key-accumulator.cc
[modify] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/src/key-accumulator.h
[modify] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/src/messages.cc
[modify] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/src/objects.cc
[modify] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/src/objects.h
[add] http://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7/test/mjsunit/regress/regress-705-shadowed_properties.js

Project Member

Comment 15 by 76821325...@developer.gserviceaccount.com, Jan 21 2016

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

commit 6e0573c6fff1c3041bab106d1197ab1b64aa9a6a
Author: cbruni <cbruni@chromium.org>
Date: Thu Jan 21 17:47:53 2016

Revert of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #10 id:180001 of https://codereview.chromium.org/1608523002/ )

Reason for revert:
tanks for-in significantly

Original issue's description:
> [runtime] Do not use the enum-cache for keys retrieval.
>
> Currently we fail to properly handle shadowed properties. If the
> receiver defines a non-enumerable property that reappears on the
> prototype as enumerable it incorrectly shows up in [[Enumerate]].
> By extending the KeyAccumulator to track non-enumerable properties
> we can now properly filter them out when seeing them further up in
> the prototype-chain.
>
> BUG= v8:705 
> LOG=y
>
> Committed: https://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7
> Cr-Commit-Position: refs/heads/master@{#33405}

TBR=jkummerow@chromium.org,bmeurer@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= v8:705 
LOG=n

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

Cr-Commit-Position: refs/heads/master@{#33443}

[modify] http://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a/src/elements.cc
[modify] http://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a/src/key-accumulator.cc
[modify] http://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a/src/key-accumulator.h
[modify] http://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a/src/messages.cc
[modify] http://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a/src/objects.cc
[modify] http://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a/src/objects.h
[delete] http://crrev.com/3b6b8119ffc9fcd8f1946697140e9e333ceb61ad/test/mjsunit/regress/regress-705-shadowed_properties.js

Project Member

Comment 16 by 76821325...@developer.gserviceaccount.com, Jan 22 2016

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

commit 5569e270eda517b5ea74e3a7676b3230cbe2f7a9
Author: cbruni <cbruni@chromium.org>
Date: Fri Jan 22 09:06:25 2016

Reland of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #1 id:1 of https://codereview.chromium.org/1619803003/ )

Reason for revert:
the deopt issues have been taken care of by benedikt

Original issue's description:
> Revert of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #10 id:180001 of https://codereview.chromium.org/1608523002/ )
>
> Reason for revert:
> tanks for-in significantly
>
> Original issue's description:
> > [runtime] Do not use the enum-cache for keys retrieval.
> >
> > Currently we fail to properly handle shadowed properties. If the
> > receiver defines a non-enumerable property that reappears on the
> > prototype as enumerable it incorrectly shows up in [[Enumerate]].
> > By extending the KeyAccumulator to track non-enumerable properties
> > we can now properly filter them out when seeing them further up in
> > the prototype-chain.
> >
> > BUG= v8:705 
> > LOG=y
> >
> > Committed: https://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7
> > Cr-Commit-Position: refs/heads/master@{#33405}
>
> TBR=jkummerow@chromium.org,bmeurer@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= v8:705 
> LOG=n
>
> Committed: https://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a
> Cr-Commit-Position: refs/heads/master@{#33443}

TBR=jkummerow@chromium.org,bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= v8:705 

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

Cr-Commit-Position: refs/heads/master@{#33458}

[modify] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/src/elements.cc
[modify] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/src/key-accumulator.cc
[modify] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/src/key-accumulator.h
[modify] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/src/messages.cc
[modify] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/src/objects.cc
[modify] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/src/objects.h
[add] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/test/mjsunit/regress/regress-705-shadowed_properties.js

Project Member

Comment 17 by 76821325...@developer.gserviceaccount.com, Jan 22 2016

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

commit 1c523a444b0dea8f7891c6bf9bc1790bf598023f
Author: cbruni <cbruni@chromium.org>
Date: Fri Jan 22 09:11:11 2016

Revert of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #1 id:1 of https://codereview.chromium.org/1612413003/ )

Reason for revert:
let me quickly revert the revert, wut?
Goal: my CL should not be in the tree!

Original issue's description:
> Reland of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #1 id:1 of https://codereview.chromium.org/1619803003/ )
>
> Reason for revert:
> the deopt issues have been taken care of by benedikt
>
> Original issue's description:
> > Revert of [runtime] Do not use the enum-cache for non-prototype objects. (patchset #10 id:180001 of https://codereview.chromium.org/1608523002/ )
> >
> > Reason for revert:
> > tanks for-in significantly
> >
> > Original issue's description:
> > > [runtime] Do not use the enum-cache for keys retrieval.
> > >
> > > Currently we fail to properly handle shadowed properties. If the
> > > receiver defines a non-enumerable property that reappears on the
> > > prototype as enumerable it incorrectly shows up in [[Enumerate]].
> > > By extending the KeyAccumulator to track non-enumerable properties
> > > we can now properly filter them out when seeing them further up in
> > > the prototype-chain.
> > >
> > > BUG= v8:705 
> > > LOG=y
> > >
> > > Committed: https://crrev.com/ed24dfe80d1da0827b8571839ee52c03ad09c9c7
> > > Cr-Commit-Position: refs/heads/master@{#33405}
> >
> > TBR=jkummerow@chromium.org,bmeurer@chromium.org
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG= v8:705 
> > LOG=n
> >
> > Committed: https://crrev.com/6e0573c6fff1c3041bab106d1197ab1b64aa9a6a
> > Cr-Commit-Position: refs/heads/master@{#33443}
>
> TBR=jkummerow@chromium.org,bmeurer@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= v8:705 
>
> Committed: https://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9
> Cr-Commit-Position: refs/heads/master@{#33458}

TBR=jkummerow@chromium.org,bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= v8:705 

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

Cr-Commit-Position: refs/heads/master@{#33459}

[modify] http://crrev.com/1c523a444b0dea8f7891c6bf9bc1790bf598023f/src/elements.cc
[modify] http://crrev.com/1c523a444b0dea8f7891c6bf9bc1790bf598023f/src/key-accumulator.cc
[modify] http://crrev.com/1c523a444b0dea8f7891c6bf9bc1790bf598023f/src/key-accumulator.h
[modify] http://crrev.com/1c523a444b0dea8f7891c6bf9bc1790bf598023f/src/messages.cc
[modify] http://crrev.com/1c523a444b0dea8f7891c6bf9bc1790bf598023f/src/objects.cc
[modify] http://crrev.com/1c523a444b0dea8f7891c6bf9bc1790bf598023f/src/objects.h
[delete] http://crrev.com/5569e270eda517b5ea74e3a7676b3230cbe2f7a9/test/mjsunit/regress/regress-705-shadowed_properties.js

Labels: Test262Failures
Labels: -test262failures Hotlist-test262
Cc: cbruni@chromium.org
 Issue 4905  has been merged into this issue.
Project Member

Comment 21 by bugdroid1@chromium.org, May 3 2016

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

commit c736a452575f406c9a05a8c202b0708cb60d43e5
Author: cbruni <cbruni@chromium.org>
Date: Tue May 03 15:29:28 2016

[keys] Moving property/keys related methods to KeyAccumulator in keys.cc

The Great Keys Migration:
This is part of a bigger effort to centralize optimizations for key collections
in a central place. This necessary to avoid the penalty that would be introduced
by fixing shadowed property iteration.

BUG= v8:4758 ,  v8:705 
LOG=N

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

[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/isolate.h
[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/keys.cc
[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/keys.h
[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/messages.cc
[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/objects.cc
[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/objects.h
[modify] https://crrev.com/c736a452575f406c9a05a8c202b0708cb60d43e5/src/runtime/runtime-array.cc

Comment 22 by adamk@chromium.org, May 23 2016

What's the status on this bug? Is work still under way to fix it?
Status: Started (was: Assigned)
This is WIP as I have to first refactor the KeyAccumulator to not have too much negative impact.
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 7 2016

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

commit 991968029cc8452077a7f8e069a2eb7ce36fca49
Author: cbruni <cbruni@chromium.org>
Date: Tue Jun 07 08:50:04 2016

[keys] keep track of the last non-empty prototype

Without the boundary prototypes we have to keep track of all shadowing properties
throughout the complete prototype chain. This contradicts the finding that most
objects have a rather large number of non-enumerable properties on the prototype
chain.

BUG= v8:705 ,  v8:4905 ,  v8:4706 

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

[modify] https://crrev.com/991968029cc8452077a7f8e069a2eb7ce36fca49/src/keys.cc
[modify] https://crrev.com/991968029cc8452077a7f8e069a2eb7ce36fca49/src/keys.h

Labels: SpecViolation-Backlog
Status: Fixed (was: Started)
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 17 2016

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

commit f2a5c65b1f95358d6897c7fdb8d9feb57a3d6998
Author: adamk <adamk@chromium.org>
Date: Mon Oct 17 22:12:52 2016

Remove flaky test expectation for for-in test that now passes consistently

R=cbruni@chromium.org
BUG= v8:705 

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

[modify] https://crrev.com/f2a5c65b1f95358d6897c7fdb8d9feb57a3d6998/test/test262/test262.status

Labels: Priority-3

Sign in to add a comment