1. If Object.keys is called on Proxy w/o "ownKeys" trap, filtering DontEnum properties should be observed by "getOwnPropertyDescriptor" trap. ECMA262: https://tc39.es/ecma262/#sec-enumerableownpropertynames (step 4) Test262: https://test262.report/browse/built-ins/Object/keys/property-traps-order-with-proxied-array.js 2. If Object.keys is called on Proxy with "ownKeys" trap, non-enumerable properties of Proxy's target are ignored during invariants validation. ECMA262: https://tc39.es/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys (step 11) Test262: https://test262.report/browse/built-ins/Object/keys/proxy-non-enumerable-prop-invariant-1.js https://test262.report/browse/built-ins/Object/keys/proxy-non-enumerable-prop-invariant-2.js
Created attachment 382761 [details] Patch
Created attachment 382773 [details] Patch Adjust test.
Comment on attachment 382773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382773&action=review Nice! I put r- because I found a bug. > Source/JavaScriptCore/runtime/ProxyObject.cpp:1081 > + ASSERT(enumerationMode.includeJSObjectProperties()); "getOwnPropertyNames " is a part of public interface of JSObject. So, we allow clients to call this "getOwnPropertyNames" function directly. Like, object->methodTable(vm)->getOwnPropertyNames(object, ...); So, this ASSERT is not correct. Can you fix it?
(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 382773 [details] > Patch > > So, this ASSERT is not correct. Can you fix it? Thank you for review! Allow me to expand on this ASSERT: An object getOwnPropertyNames() is called on with JSObjectPropertiesMode::Exclude should be checked beforehand as this mode is special. The only usage of JSObjectPropertiesMode::Exclude in JSC codebase is in JSObject::getGenericPropertyNames, which is used only in JSPropertyNameEnumerator on objects passing Structure::canAccessPropertiesQuicklyForEnumeration check (ProxyObjects don't). I've added this ASSERT because it doesn't quite make sense to call ProxyObject::getOwnPropertyNames with JSObjectPropertiesMode::Exclude and it is hard to do so by accident. ProxyObject::getGenericPropertyNames and friends are also public API, yet they have RELEASE_ASSERT_NOT_REACHED.
Created attachment 382961 [details] Patch Remove ASSERT.
Comment on attachment 382961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382961&action=review r=me with nit. > Source/JavaScriptCore/runtime/ProxyObject.cpp:944 > + EnumerationMode enumerationMode(DontEnumPropertiesMode::Include); Nit: It seems like we only use this in one place below. Can you just pass DontEnumPropertiesMode::Include to getOwnPropertyNames directly?
(In reply to Keith Miller from comment #6) > Comment on attachment 382961 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382961&action=review > > r=me with nit. > > > Source/JavaScriptCore/runtime/ProxyObject.cpp:944 > > + EnumerationMode enumerationMode(DontEnumPropertiesMode::Include); > > Nit: It seems like we only use this in one place below. Can you just pass > DontEnumPropertiesMode::Include to getOwnPropertyNames directly? 1. We also use `enumerationMode` in ProxyObject.cpp:1011 (invariants validation). 2. We also invoke `performGetOwnPropertyNames` in ProxyObject.cpp:1063 (we can't pass EnumerationMode there).
Created attachment 386860 [details] Patch Set reviewer and mark two more tests as passing.
The commit-queue encountered the following flaky tests while processing attachment 386860 [details]: storage/indexeddb/modern/index-cursor-3.html bug 205818 (author: beidson@apple.com) transitions/default-timing-function.html bug 138901 (authors: dino@apple.com and graouts@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 386860 [details] Patch Clearing flags on attachment: 386860 Committed r254070: <https://trac.webkit.org/changeset/254070>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58348548>