Bug 185211 – JavaScriptCore should throw TypeError if [[OwnPropertyKeys]] returns duplicate entries

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[acsv2]

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

[Radar WebKit Bug Importer]

<rdar://problem/47417498>

[Caitlin Potter (:caitp)]

Moving my patch for this from https://bugs.webkit.org/show_bug.cgi?id=176810

[Caitlin Potter (:caitp)]

Created attachment 362734 [details]
[JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

[EWS Watchlist]

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]

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 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)]

Created attachment 362910 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

[Caitlin Potter (:caitp)]

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 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)]

Created attachment 362914 [details]
[JSC] throw if ownKeys Proxy trap result contains duplicate keys

[Caitlin Potter (:caitp)]

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 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 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)]

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 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]

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 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)]

Created attachment 364304 [details]
[JSC] throw if ownKeys Proxy trap result contains duplicate keys

also replace the part that throws the exception

[EWS Watchlist]

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 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]

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)]

The win-future failures don't look related to this change, to me.

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Alexey Shvayka]

*** Bug 195443 has been marked as a duplicate of this bug. ***