RESOLVED FIXED 185211
JavaScriptCore should throw TypeError if [[OwnPropertyKeys]] returns duplicate entries 
https://bugs.webkit.org/show_bug.cgi?id=185211
Summary JavaScriptCore should throw TypeError if [[OwnPropertyKeys]] returns duplicat...
acsv2
Reported 2018-05-02 13:17:03 PDT
Hello, according ES6 specification (https://github.com/tc39/ecma262/pull/833), the engine should throw a TypeError if ownKeys have duplicate entries. OS = Ubuntu 17.10 x64 JavaScriptCore: 606.1.9.4 Steps to reproduce: var proxy = new Proxy({}, { ownKeys: function (t) { return ["A", "A"]; } }); var keys = Object.keys(proxy); Actual results: Pass without failures Expected results: TypeError: proxy [[OwnPropertyKeys]] can't report property '"A"' more than once Actually, only SpiderMonkey supports this specification. V8 have an open issue (https://bugs.chromium.org/p/v8/issues/detail?id=6776).
Attachments
[JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys (13.01 KB, patch)
2019-02-22 10:59 PST, Caitlin Potter (:caitp) 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.70 MB, application/zip)
2019-02-22 17:03 PST, EWS Watchlist 		no flags
Details
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (13.02 KB, patch)
2019-02-25 10:45 PST, Caitlin Potter (:caitp) 		no flags
DetailsFormatted DiffDiff
[JSC] throw if ownKeys Proxy trap result contains duplicate keys (13.02 KB, patch)
2019-02-25 11:52 PST, Caitlin Potter (:caitp) 		no flags
DetailsFormatted DiffDiff
[JSC] throw if ownKeys Proxy trap result contains duplicate keys (13.02 KB, patch)
2019-03-11 14:34 PDT, Caitlin Potter (:caitp) 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews100 for mac-highsierra (2.45 MB, application/zip)
2019-03-11 15:37 PDT, EWS Watchlist 		no flags
Details
[JSC] throw if ownKeys Proxy trap result contains duplicate keys (13.17 KB, patch)
2019-03-11 16:12 PDT, Caitlin Potter (:caitp) 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.62 MB, application/zip)
2019-03-11 16:12 PDT, EWS Watchlist 		no flags
Details
Archive of layout-test-results from ews202 for win-future (12.93 MB, application/zip)
2019-03-11 23:37 PDT, EWS Watchlist 		no flags
Details
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
isol2
Comment 1 2018-08-21 09:22:40 PDT
cinfuzz
Radar WebKit Bug Importer
Comment 2 2019-01-20 14:37:25 PST
<rdar://problem/47417498>
Caitlin Potter (:caitp)
Comment 3 2019-02-22 10:53:42 PST
Moving my patch for this from https://bugs.webkit.org/show_bug.cgi?id=176810
Caitlin Potter (:caitp)
Comment 4 2019-02-22 10:59:37 PST
Created attachment 362734 [details] [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
EWS Watchlist
Comment 5 2019-02-22 17:03:02 PST
Comment on attachment 362734 [details] [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys Attachment 362734 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11253019 New failing tests: fast/viewport/ios/device-width-viewport-after-changing-view-scale.html
EWS Watchlist
Comment 6 2019-02-22 17:03:04 PST
Created attachment 362796 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Darin Adler
Comment 7 2019-02-25 09:56:00 PST
Comment on attachment 362734 [details] [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys View in context: https://bugs.webkit.org/attachment.cgi?id=362734&action=review > Source/JavaScriptCore/runtime/ProxyObject.cpp:960 > + if (seenKeys.contains(ident.impl())) { > + throwTypeError(exec, scope, "Proxy handler's 'ownKeys' trap result must not contain any duplicate names"_s); > + return doExitEarly; > + } > + seenKeys.add(ident.impl()); This does twice as many hash lookups as necessary. Instead, this should take advantage of the return value from add. if (!seenKeys.add(ident.impl()).isNewEntry) {
Caitlin Potter (:caitp)
Comment 8 2019-02-25 10:45:22 PST
Created attachment 362910 [details] [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
Caitlin Potter (:caitp)
Comment 9 2019-02-25 10:45:50 PST
Comment on attachment 362734 [details] [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys View in context: https://bugs.webkit.org/attachment.cgi?id=362734&action=review >> Source/JavaScriptCore/runtime/ProxyObject.cpp:960 >> + seenKeys.add(ident.impl()); > > This does twice as many hash lookups as necessary. Instead, this should take advantage of the return value from add. > > if (!seenKeys.add(ident.impl()).isNewEntry) { Done, thanks for the tip
Saam Barati
Comment 10 2019-02-25 11:32:57 PST
Comment on attachment 362910 [details] [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() View in context: https://bugs.webkit.org/attachment.cgi?id=362910&action=review r=me > Source/JavaScriptCore/runtime/ProxyObject.cpp:955 > + // keys filtered by type). nit: I'd link to the spec here since you're referencing it. > Source/JavaScriptCore/runtime/ProxyObject.cpp:962 > + if (!(type & resultFilter)) > + return dontExitEarly; Can a test be added for the observability of calling toPropertyKey before this branch?
Caitlin Potter (:caitp)
Comment 11 2019-02-25 11:52:09 PST
Created attachment 362914 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys
Caitlin Potter (:caitp)
Comment 12 2019-02-25 11:52:44 PST
Comment on attachment 362910 [details] [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() View in context: https://bugs.webkit.org/attachment.cgi?id=362910&action=review >> Source/JavaScriptCore/runtime/ProxyObject.cpp:955 >> + // keys filtered by type). > > nit: I'd link to the spec here since you're referencing it. Done >> Source/JavaScriptCore/runtime/ProxyObject.cpp:962 >> + return dontExitEarly; > > Can a test be added for the observability of calling toPropertyKey before this branch? It isn't actually observable, because this closure is only invoked on values which are strings or symbols. It's only done to convert the JSValue into an Identifier. There are already tests which verify that it throws in the case of non-String/Symbols in stress/proxy-own-keys.js.
Saam Barati
Comment 13 2019-03-11 11:10:51 PDT
Comment on attachment 362914 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys View in context: https://bugs.webkit.org/attachment.cgi?id=362914&action=review > Source/JavaScriptCore/runtime/ProxyObject.cpp:959 > + } I believe this is the cause of your build issues.
Saam Barati
Comment 14 2019-03-11 11:11:20 PDT
Comment on attachment 362914 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys View in context: https://bugs.webkit.org/attachment.cgi?id=362914&action=review > Source/JavaScriptCore/runtime/ProxyObject.cpp:942 > + HashSet<UniquedStringImpl*> seenKeys; This isn't used.
Caitlin Potter (:caitp)
Comment 15 2019-03-11 14:34:11 PDT
Created attachment 364289 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys replace the important accidentally deleted lines to fix the build!
EWS Watchlist
Comment 16 2019-03-11 15:37:47 PDT
Comment on attachment 364289 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys Attachment 364289 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11461299 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html
EWS Watchlist
Comment 17 2019-03-11 15:37:49 PDT
Created attachment 364303 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 18 2019-03-11 16:12:00 PDT
Comment on attachment 364289 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys Attachment 364289 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11461750 New failing tests: imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html
Caitlin Potter (:caitp)
Comment 19 2019-03-11 16:12:02 PDT
Created attachment 364304 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys also replace the part that throws the exception
EWS Watchlist
Comment 20 2019-03-11 16:12:03 PDT
Created attachment 364305 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 21 2019-03-11 23:37:01 PDT
Comment on attachment 364304 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys Attachment 364304 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11468561 New failing tests: js/dom/custom-constructors.html
EWS Watchlist
Comment 22 2019-03-11 23:37:37 PDT
Created attachment 364358 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Caitlin Potter (:caitp)
Comment 23 2019-03-12 06:24:00 PDT
The win-future failures don't look related to this change, to me.
WebKit Commit Bot
Comment 24 2019-04-05 06:45:15 PDT
Comment on attachment 364304 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys Clearing flags on attachment: 364304 Committed r243933: <https://trac.webkit.org/changeset/243933>
WebKit Commit Bot
Comment 25 2019-04-05 06:45:18 PDT
All reviewed patches have been landed. Closing bug.
Alexey Shvayka
Comment 26 2020-05-04 16:38:48 PDT
*** Bug 195443 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.