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).
cinfuzz
<rdar://problem/47417498>
Moving my patch for this from https://bugs.webkit.org/show_bug.cgi?id=176810
Created attachment 362734 [details] [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
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
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
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) {
Created attachment 362910 [details] [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
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
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?
Created attachment 362914 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys
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.
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.
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.
Created attachment 364289 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys replace the important accidentally deleted lines to fix the build!
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
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
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
Created attachment 364304 [details] [JSC] throw if ownKeys Proxy trap result contains duplicate keys also replace the part that throws the exception
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
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
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
The win-future failures don't look related to this change, to me.
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>
All reviewed patches have been landed. Closing bug.
*** Bug 195443 has been marked as a duplicate of this bug. ***