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.
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.
<rdar://problem/51390792>
*** Bug 212449 has been marked as a duplicate of this bug. ***
Given the now undated dupe, do we want to raise the priority of fixing this?
(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.
(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
Created attachment 410062 [details] Patch
(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
(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?
(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
(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().
The EWS bot failures also appear to be real. Please investigate. Thanks.
Comment on attachment 410062 [details] Patch It seems that the crash is happening in some LayoutTests.
Created attachment 415055 [details] Patch Use different approach.
(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).
Created attachment 415264 [details] Patch Fix getOwnPropertySlot() overrides to report properties returned by getOwnPropertyNames() as enumerable.
Created attachment 415295 [details] Patch Fix testapi.
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.
Committed r270874: <https://trac.webkit.org/changeset/270874>
(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.
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.
*** Bug 221176 has been marked as a duplicate of this bug. ***