Bug 38970 – Non-enumerable property fails to shadow inherited enumerable property from for-in

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark S. Miller]

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 WebKit nightly as of this writing, 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.

[Jordan Harband]

Ping - adding some people to the CC list.

This is also an issue in v8 ( https://code.google.com/p/v8/issues/detail?id=705 ) although Firefox and IE are correct.

[Radar WebKit Bug Importer]

<rdar://problem/51390792>

[Mark Lam]

*** Bug 212449 has been marked as a duplicate of this bug. ***

[Maciej Stachowiak]

Given the now undated dupe, do we want to raise the priority of fixing this?

[Yusuke Suzuki]

(In reply to Maciej Stachowiak from comment #4)
> Given the now undated dupe, do we want to raise the priority of fixing this?

Recently, TC39 attempted to define for-in's semantics more precisely, and it is now merged into the spec at 2020/01/02 https://github.com/tc39/ecma262/pull/1791.
And after this refinement, a lot of semantics are clarified, and the new spec clearly requires this behavior should be fixed. Since a lot of for-in semantics are clarified / updated, I think it is good time to fix it / update it, and I think raising priority of this is good idea.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #5)
> (In reply to Maciej Stachowiak from comment #4)
> > Given the now undated dupe, do we want to raise the priority of fixing this?
> 
> Recently, TC39 attempted to define for-in's semantics more precisely, and it
> is now merged into the spec at 2020/01/02
> https://github.com/tc39/ecma262/pull/1791.
> And after this refinement, a lot of semantics are clarified, and the new
> spec clearly requires this behavior should be fixed. Since a lot of for-in
> semantics are clarified / updated, I think it is good time to fix it /
> update it, and I think raising priority of this is good idea.

Agreed

[Alexey Shvayka]

Created attachment 410062 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #7)
> Created attachment 410062 [details]
> Patch

                                                            r267623                    patch

in-by-val-inside-for-in-loop                            18.7430+-0.6262           18.1054+-0.5515          might be 1.0352x faster
has-own-property-for-in-loop-with-this                  34.1394+-1.0114           33.6293+-0.9859          might be 1.0152x faster
has-own-property-for-in-loop                            36.1299+-1.1519     ^     33.6704+-0.8729        ^ definitely 1.0730x faster
has-own-property-for-in-loop-with-heap-variable         35.0119+-1.0183           33.3996+-0.7344          might be 1.0483x faster
has-own-property-for-in-loop-reflect-name               35.3447+-1.2661           33.6938+-1.0116          might be 1.0490x faster
for-in-on-object-with-lazily-materialized-properties   233.4685+-3.2478     !    331.7142+-13.2340       ! definitely 1.4208x slower
let-for-in                                              10.5997+-0.3751           10.2912+-0.2934          might be 1.0300x faster
object-values                                           41.7760+-2.9022     ?     43.1566+-0.5000        ? might be 1.0330x slower
object-entries                                          95.5208+-2.0629           94.5192+-3.1518          might be 1.0106x faster
object-get-own-property-symbols-on-large-array          53.9934+-1.2623     ^     51.2972+-1.2180        ^ definitely 1.0526x faster
object-get-own-property-symbols                         67.7095+-2.4705     ?     71.7645+-2.2434        ? might be 1.0599x slower
object-keys-string-object                               60.4447+-1.2345     ^     56.8687+-1.1331        ^ definitely 1.0629x faster
object-keys-typed-array                                 67.5286+-1.3963     ^     60.4957+-1.5904        ^ definitely 1.1163x faster
object-keys-map-values                                  59.0130+-2.1141     ?     61.2572+-2.0413        ? might be 1.0380x slower
object-keys                                             26.7807+-0.5887     !     28.3109+-0.4814        ! definitely 1.0571x slower
reflect-own-keys-function                               90.4955+-1.1324     ^     58.9401+-1.0891        ^ definitely 1.5354x faster
reflect-own-keys                                        72.8491+-1.7695     ?     76.2500+-2.3467        ? might be 1.0467x slower

[Mark Lam]

(In reply to Alexey Shvayka from comment #8)
> object-keys                                             26.7807+-0.5887    
> !     28.3109+-0.4814        ! definitely 1.0571x slower

Can you investigate this one?

[Mark Lam]

(In reply to Mark Lam from comment #9)
> (In reply to Alexey Shvayka from comment #8)
> > object-keys                                             26.7807+-0.5887    
> > !     28.3109+-0.4814        ! definitely 1.0571x slower
> 
> Can you investigate this one?

And this one?
for-in-on-object-with-lazily-materialized-properties   233.4685+-3.2478     !    331.7142+-13.2340       ! definitely 1.4208x slower

[Alexey Shvayka]

(In reply to Mark Lam from comment #9)
> (In reply to Alexey Shvayka from comment #8)
> > object-keys                                             26.7807+-0.5887    
> > !     28.3109+-0.4814        ! definitely 1.0571x slower
> 
> Can you investigate this one?

We had a similar slowdown in https://bugs.webkit.org/show_bug.cgi?id=216227: I believe it's due to increased Structure::getPropertyNamesFromStructure() code size and can be regained by making a separate method for PropertyNameMode::StringsAndSymbols (a follow-up?)

This patch is careful to preserve Object.keys performance by early-returning for DontEnumPropertyMode::Exclude and utilizing addKnownUniqueWithDontEnum().

[Mark Lam]

The EWS bot failures also appear to be real.  Please investigate.  Thanks.

[Yusuke Suzuki]

Comment on attachment 410062 [details]
Patch

It seems that the crash is happening in some LayoutTests.

[Alexey Shvayka]

Created attachment 415055 [details]
Patch

Use different approach.

[Alexey Shvayka]

(In reply to Mark Lam from comment #10)
> > Can you investigate this one?
> 
> And this one?
> for-in-on-object-with-lazily-materialized-properties
>    233.4685+-3.2478     !    331.7142+-13.2340       ! definitely 1.4208x slower

At first, I thought this regression is ErrorInstance-related; I was so wrong.
This microbenchmark appears to be the only one which measures cold for/in runs (no JSPropertyNameEnumerator caching).
Objects like Date or userland ES6 classes, which have a lot of DontEnum methods on their prototypes, slow down more than 6x.

(In reply to Alexey Shvayka from comment #14)
> Created attachment 415055 [details]
> Patch
> 
> Use different approach.

In the new patch, instead of keeping track of DontEnum properties, we simply check enumerability the same time property existence is verified.
It is neutral on microbenchmarks and is way easier to review (not minding the renames).

[Alexey Shvayka]

Created attachment 415264 [details]
Patch

Fix getOwnPropertySlot() overrides to report properties returned by getOwnPropertyNames() as enumerable.

[Alexey Shvayka]

Created attachment 415295 [details]
Patch

Fix testapi.

[Keith Miller]

Comment on attachment 415295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415295&action=review

r=me with some nits.

> Source/JavaScriptCore/ChangeLog:31
> +        This patch introduces a new invariant: all properties getOwn*PropertyNames() returns
> +        in DontEnumPropertiesMode::Exclude should be reported as [[Enumerable]] by
> +        getOwnPropertySlot(). JSCallbackObject and RuntimeArray are fixed to follow it.

Is there a way we can assert this? I think we could at least for the VMInquiry flavor of getOwnPropertySlot()?

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:167
> +            // FIXME: Set ReadOnly conditionally, based on setProperty presence in class inheritance chain.

Can you open a bug for this and link to it here?

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:199
> +                    // FIXME: getStaticValue() performs the same loop & checks just to acquire `entry`.

Ditto on bugzilla.

[Alexey Shvayka]

Committed r270874: <https://trac.webkit.org/changeset/270874>

[Alexey Shvayka]

(In reply to Keith Miller from comment #18)
> Is there a way we can assert this? I think we could at least for the
> VMInquiry flavor of getOwnPropertySlot()?

That's a super useful assert to have: https://bugs.webkit.org/show_bug.cgi?id=141952 would be caught by it.
Since the invariant is required by for/in, and we have the most for/in tests for different objects, I believe JSObject::get[Generic]PropertyNames() is the place to have it.
However, there are quite a few get*PropertyNames() call sites there, so I've filed https://bugs.webkit.org/show_bug.cgi?id=219926 and suggest adding it after https://bugs.webkit.org/show_bug.cgi?id=212954 is resolved.

[Alexey Shvayka]

Comment on attachment 415295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415295&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        (with any data structure used), this patch simply adds [[Enumerable]] check to

How does checking [[Enumerable]] fixes shadowing issue?

Let's consider a for/in invoked on an object A with prototype chain: A <= B <= C <= null.

Given that:
  1) hasEnumerableProperty is invoked on the same object A as for/in, and they both go up the prototype chain;
  2) PropertyNameArray contains no duplicate identifiers;
  3) properties are enumerated in DontEnumPropertiesMode::Exclude.

Precondition: for/in body is pure.

If for/in reached object C, which enumerated property K, but [[Enumerable]] check failed, we can be sure that either A or B is shadowing K (= has DontEnum property attribute), and skip it.

> Source/JavaScriptCore/ChangeLog:26
> +        b) "shadowing" is broken if a DontEnum property of already visited object is

The downside of this approach, but IMO it's worth it since [[Enumerable]] check is free.

[Alexey Shvayka]

*** Bug 221176 has been marked as a duplicate of this bug. ***