WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
203818
Proxy's [[OwnPropertyKeys]] is incorrect in DontEnumPropertiesMode::Exclude
https://bugs.webkit.org/show_bug.cgi?id=203818
Summary
Proxy's [[OwnPropertyKeys]] is incorrect in DontEnumPropertiesMode::Exclude
Alexey Shvayka
Reported
2019-11-04 11:03:01 PST
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
Attachments
Patch
(10.26 KB, patch)
2019-11-04 12:48 PST
,
Alexey Shvayka
no flags
Details
Formatted Diff
Diff
Patch
(11.10 KB, patch)
2019-11-04 14:15 PST
,
Alexey Shvayka
no flags
Details
Formatted Diff
Diff
Patch
(11.04 KB, patch)
2019-11-06 14:18 PST
,
Alexey Shvayka
no flags
Details
Formatted Diff
Diff
Patch
(11.36 KB, patch)
2020-01-06 10:28 PST
,
Alexey Shvayka
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Alexey Shvayka
Comment 1
2019-11-04 12:48:57 PST
Created
attachment 382761
[details]
Patch
Alexey Shvayka
Comment 2
2019-11-04 14:15:48 PST
Created
attachment 382773
[details]
Patch Adjust test.
Yusuke Suzuki
Comment 3
2019-11-04 14:40:21 PST
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?
Alexey Shvayka
Comment 4
2019-11-05 15:26:13 PST
(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.
Alexey Shvayka
Comment 5
2019-11-06 14:18:46 PST
Created
attachment 382961
[details]
Patch Remove ASSERT.
Keith Miller
Comment 6
2019-11-20 10:32:53 PST
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?
Alexey Shvayka
Comment 7
2019-11-20 12:06:42 PST
(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).
Alexey Shvayka
Comment 8
2020-01-06 10:28:05 PST
Created
attachment 386860
[details]
Patch Set reviewer and mark two more tests as passing.
WebKit Commit Bot
Comment 9
2020-01-06 11:30:20 PST
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.
WebKit Commit Bot
Comment 10
2020-01-06 11:31:08 PST
Comment on
attachment 386860
[details]
Patch Clearing flags on attachment: 386860 Committed
r254070
: <
https://trac.webkit.org/changeset/254070
>
WebKit Commit Bot
Comment 11
2020-01-06 11:31:10 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 12
2020-01-06 11:32:24 PST
<
rdar://problem/58348548
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug