Bug 176810 – for..in on a Proxy loops over non enumerable properties

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Paradis]

* Steps to reproduce:

var foo = new Proxy(['a', 'b'],{ 
    ownKeys: function(target) { return Reflect.ownKeys(target); }
});
for (var p in foo) console.log(p);

* Actual results (nightly):

// 0
// 1
// length

* Expected results:

// 0
// 1

This works as expected in Chrome and FF.

* Discussion:

The getOwnPropertyDescriptor trap is called, but it seems like the results are ignored (we loop even if enumerable is forced to false):

var foo = new Proxy(["a", "b"],{ 
    getOwnPropertyDescriptor: function(target, prop) { var desc = Reflect.getOwnPropertyDescriptor(target, prop); desc.enumerable=false; return desc; }, 
    ownKeys: function(target) { return Reflect.ownKeys(target); }
})

This is correct:

Object.keys(foo)
// []

This is incorrect:

for (var p in foo) console.log(p)
// 0
// 1
// length

[Radar WebKit Bug Importer]

<rdar://problem/34435582>

[Xan Lopez]

Created attachment 351389 [details]
Fix for bug

[Caio Lima]

Do you mind to add the other test case forcing the enumerable to false as well?

[Saam Barati]

Comment on attachment 351389 [details]
Fix for bug

View in context: https://bugs.webkit.org/attachment.cgi?id=351389&action=review

> Source/JavaScriptCore/runtime/ProxyObject.cpp:952
> +            bool isPropertyDefined = target->getOwnPropertyDescriptor(exec, ident.impl(), descriptor);

Is this actually what the spec says to do? This is observable behavior

[Xan Lopez]

(In reply to Saam Barati from comment #4)
> Comment on attachment 351389 [details]
> Fix for bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351389&action=review
> 
> > Source/JavaScriptCore/runtime/ProxyObject.cpp:952
> > +            bool isPropertyDefined = target->getOwnPropertyDescriptor(exec, ident.impl(), descriptor);
> 
> Is this actually what the spec says to do? This is observable behavior

So the issue is this being called more than once basically? Because it's already called a bit further below in that method. If that's the thing then we need a way to get this information without it being user observable (or a completely different approach), yeah.

[EWS Watchlist]

Comment on attachment 351389 [details]
Fix for bug

Attachment 351389 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9427839

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 351402 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351389 [details]
Fix for bug

Attachment 351389 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9427866

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 351405 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351389 [details]
Fix for bug

Attachment 351389 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9427781

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 351412 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Caio Lima]

Comment on attachment 351389 [details]
Fix for bug

View in context: https://bugs.webkit.org/attachment.cgi?id=351389&action=review

>>> Source/JavaScriptCore/runtime/ProxyObject.cpp:952
>>> +            bool isPropertyDefined = target->getOwnPropertyDescriptor(exec, ident.impl(), descriptor);
>> 
>> Is this actually what the spec says to do? This is observable behavior
> 
> So the issue is this being called more than once basically? Because it's already called a bit further below in that method. If that's the thing then we need a way to get this information without it being user observable (or a completely different approach), yeah.

Maybe one way to check if the property is enumerable without observable behaviour is:

```
PropertySlot slot(target, PropertySlot::InternalMethodType::VMInquiry);
target->methodTable(vm)->getOwnPropertySlot(this, exec, ident, slot);
DefinePropertyAttributes property = DefinePropertyAttributes(slot.attributes())

std::optional<bool> maybeEnumerable  = attr.enumerable();
```

I'm not sure if that is the correct way to perform this check.

[EWS Watchlist]

Comment on attachment 351389 [details]
Fix for bug

Attachment 351389 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9434013

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 351468 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 351389 [details]
Fix for bug

Attachment 351389 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9435067

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 351472 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Saam Barati]

Is this still an issue? r- because EWS is unhappy.

[Caitlin Potter (:caitp)]

So there are a few issues at the moment.

The spec compliant fix for this particular issue, would be to filter the PropertyNameArray after getOwnPropertyNames() is completed. This ensures that it doesn't interfere with the invariant validation at the end.

There are a few other compliance issues, which are maybe less of an issue:

Duplicate property names. Currently, PropertyNameArray does not account for duplicates. Unfortunately, in the case of ProxyObject, duplicates are observable. However, there would be a performance penalty if PropertyNameArray were changed to support duplicates for all JSCell types --- So probably the thing to do is special case `getOwnPropertyNames` for ProxyObjects where needed.

There's a third failure in the WPT regression, which I believe is related to JSDOMConvertRecord not handling Symbol-named properties, which the test seems to expect. This looks like it should be simple enough to fix with

```diff
-        JSC::PropertyNameArray keys(&vm, JSC::PropertyNameMode::Strings, JSC::PrivateSymbolMode::Exclude);
+        JSC::PropertyNameArray keys(&vm, JSC::PropertyNameMode::StringsAndSymbols, JSC::PrivateSymbolMode::Exclude);
         object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode());
```

---

So, I'm happy to provide fixes for 0-3 of these problems, but it's important to keep in mind that each one very likely will negatively impact performance in some way, for the sake of improved spec compliance in less common cases.

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #18)
> So there are a few issues at the moment.
> 
> The spec compliant fix for this particular issue, would be to filter the
> PropertyNameArray after getOwnPropertyNames() is completed. This ensures
> that it doesn't interfere with the invariant validation at the end.
> 
> There are a few other compliance issues, which are maybe less of an issue:
> 
> Duplicate property names. Currently, PropertyNameArray does not account for
> duplicates. Unfortunately, in the case of ProxyObject, duplicates are
> observable. However, there would be a performance penalty if
> PropertyNameArray were changed to support duplicates for all JSCell types
> --- So probably the thing to do is special case `getOwnPropertyNames` for
> ProxyObjects where needed.
> 
> There's a third failure in the WPT regression, which I believe is related to
> JSDOMConvertRecord not handling Symbol-named properties, which the test
> seems to expect. This looks like it should be simple enough to fix with
> 
> ```diff
> -        JSC::PropertyNameArray keys(&vm, JSC::PropertyNameMode::Strings,
> JSC::PrivateSymbolMode::Exclude);
> +        JSC::PropertyNameArray keys(&vm,
> JSC::PropertyNameMode::StringsAndSymbols, JSC::PrivateSymbolMode::Exclude);
>          object->methodTable(vm)->getOwnPropertyNames(object, &state, keys,
> JSC::EnumerationMode());
> ```
> 
> ---
> 
> So, I'm happy to provide fixes for 0-3 of these problems, but it's important
> to keep in mind that each one very likely will negatively impact performance
> in some way, for the sake of improved spec compliance in less common cases.

It looks like https://github.com/tc39/ecma262/pull/833 will incur a bit more work here, too

[Saam Barati]

Thanks for volunteering to look into this Caitlin. We should find a way to do this that does not slow doesn't for..in for things that aren't a Proxy. It'd be great if we could also not slow down code operating on Proxy.

[Caitlin Potter (:caitp)]

Created attachment 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Created attachment 359912 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

[Caitlin Potter (:caitp)]

Ok --- So 3 patches uploaded, #2 is the one relevant to this bug, and the others fix other issues that show up in the WPT regression from Xan's patch.

Please take a look --- I haven't done a full test run yet, so I'm sure this will require updating test expectations here and there.

[EWS Watchlist]

Comment on attachment 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

Attachment 359910 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10861624

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359922 [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 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

Attachment 359910 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10861860

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359930 [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 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

Attachment 359910 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10861793

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359935 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

Attachment 359910 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10861669

New failing tests:
stress/proxy-own-keys.js.no-cjit-collect-continuously
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-eager
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-eager-no-cjit
stress/proxy-own-keys.js.bytecode-cache
stress/proxy-own-keys.js.no-ftl
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-no-put-stack-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-small-pool
stress/proxy-own-keys.js.ftl-eager-no-cjit-b3o1
stress/proxy-own-keys.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-own-keys.js.no-llint
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-llint
stress/proxy-own-keys.js.ftl-no-cjit-no-inline-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-b3o1
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-cjit-collect-continuously
stress/proxy-own-keys.js.dfg-eager
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.dfg-maximal-flush-validate-no-cjit
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.default
stress/proxy-own-keys.js.no-cjit-validate-phases
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-ftl
stress/proxy-own-keys.js.ftl-no-cjit-small-pool
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.dfg-eager-no-cjit-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-eager-no-cjit-b3o1
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.bytecode-cache
stress/proxy-own-keys.js.ftl-eager
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-own-keys.js.dfg-maximal-flush-validate-no-cjit
es6.yaml/es6/Proxy_ownKeys_duplicates.js.default
stress/proxy-own-keys.js.ftl-no-cjit-b3o1
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-cjit-validate-phases
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.dfg-eager
stress/proxy-own-keys.js.default
stress/proxy-own-keys.js.ftl-no-cjit-no-put-stack-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-no-inline-validate
stress/proxy-own-keys.js.dfg-eager-no-cjit-validate
stress/proxy-own-keys.js.ftl-eager-no-cjit
apiTests

[EWS Watchlist]

Comment on attachment 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

Attachment 359910 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10861941

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359937 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Caitlin Potter (:caitp)]

Created attachment 359941 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

[Caitlin Potter (:caitp)]

Created attachment 359952 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

[Caitlin Potter (:caitp)]

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

[EWS Watchlist]

Comment on attachment 359941 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys

Attachment 359941 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10863905

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359955 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Caitlin Potter (:caitp)]

Created attachment 359959 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

[Caitlin Potter (:caitp)]

Comment on attachment 359952 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

View in context: https://bugs.webkit.org/attachment.cgi?id=359952&action=review

> Source/WebCore/bindings/js/JSDOMConvertRecord.h:107
> +                    // ToString(key) should fail in this case.

This branch is never actually taken in this patchset by itself --- It depends on the change on line 88 to use JSC::PropertyNameMode::StringsAndSymbols. I'll fix that up.

[Caitlin Potter (:caitp)]

Created attachment 359961 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

[EWS Watchlist]

Comment on attachment 359961 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

Attachment 359961 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10865613

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359975 [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 359961 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

Attachment 359961 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10866169

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359979 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 359961 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

Attachment 359961 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10866315

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359980 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 359959 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

Attachment 359959 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10866449

New failing tests:
stress/proxy-own-keys.js.no-cjit-collect-continuously
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-eager
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-eager-no-cjit
stress/proxy-own-keys.js.bytecode-cache
stress/proxy-own-keys.js.no-ftl
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-no-put-stack-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-small-pool
stress/proxy-own-keys.js.ftl-eager-no-cjit-b3o1
stress/proxy-own-keys.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-own-keys.js.no-llint
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-llint
stress/proxy-own-keys.js.ftl-no-cjit-no-inline-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-b3o1
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-cjit-collect-continuously
stress/proxy-own-keys.js.dfg-eager
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.dfg-maximal-flush-validate-no-cjit
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.default
stress/proxy-own-keys.js.no-cjit-validate-phases
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-ftl
stress/proxy-own-keys.js.ftl-no-cjit-small-pool
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.dfg-eager-no-cjit-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-eager-no-cjit-b3o1
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.bytecode-cache
stress/proxy-own-keys.js.ftl-eager
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-own-keys.js.dfg-maximal-flush-validate-no-cjit
es6.yaml/es6/Proxy_ownKeys_duplicates.js.default
stress/proxy-own-keys.js.ftl-no-cjit-b3o1
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.no-cjit-validate-phases
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.dfg-eager
stress/proxy-own-keys.js.default
stress/proxy-own-keys.js.ftl-no-cjit-no-put-stack-validate
stress/Object_static_methods_Object.getOwnPropertyDescriptors-proxy.js.ftl-no-cjit-no-inline-validate
stress/proxy-own-keys.js.dfg-eager-no-cjit-validate
stress/proxy-own-keys.js.ftl-eager-no-cjit
apiTests

[EWS Watchlist]

Comment on attachment 359961 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

Attachment 359961 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10867540

New failing tests:
imported/w3c/web-platform-tests/fetch/api/headers/headers-record.html

[EWS Watchlist]

Created attachment 359988 [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

[Caitlin Potter (:caitp)]

Created attachment 360004 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

[Caitlin Potter (:caitp)]

With parts 1 & 2 passing EWS, a review of those would be good. I will have part 3 passing tomorrow

[Caitlin Potter (:caitp)]

Created attachment 360032 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

[Caitlin Potter (:caitp)]

In the case of patch (1), v8 is also in the process of implementing the spec change https://chromium-review.googlesource.com/c/v8/v8/+/1419779

[Caitlin Potter (:caitp)]

Ping :)

[Mark Lam]

Comment on attachment 360004 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

The ownKeys Proxy trap patch LGTM.

[Saam Barati]

Caitlin, can you make a bugzilla per patch?

[Saam Barati]

Comment on attachment 359953 [details]
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

View in context: https://bugs.webkit.org/attachment.cgi?id=359953&action=review

LGTM but this should have its own bugzilla

> Source/JavaScriptCore/runtime/ProxyObject.cpp:999
> +                throwVMTypeError(exec, scope, makeString("Proxy object's non-extensible 'target' has configurable property '", String(impl), "' that was not in the result from the 'ownKeys' trap"));

Can you add tests for this?

> Source/JavaScriptCore/runtime/ProxyObject.cpp:1005
> +            throwVMTypeError(exec, scope, "Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target"_s);

Can you add tests for this?

> Source/JavaScriptCore/runtime/ProxyObject.cpp:1011
> +        // Filtering DontEnum properties is observable in proxies and must occur following the invariant checks above.

Can you add tests for this?

[Saam Barati]

Comment on attachment 360032 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

View in context: https://bugs.webkit.org/attachment.cgi?id=360032&action=review

> Source/WebCore/bindings/js/JSDOMConvertRecord.h:107
> +                    // ToString(key) should fail in this case.

Is key always supposed to be converted to a string here?

[Caitlin Potter (:caitp)]

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

[Caitlin Potter (:caitp)]

Comment on attachment 359953 [details]
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

View in context: https://bugs.webkit.org/attachment.cgi?id=359953&action=review

This bug is specifically about patch (2), so there's no need to file a new bug for that one. I'll file separate bugs for (1) and (3)

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:999
>> +                throwVMTypeError(exec, scope, makeString("Proxy object's non-extensible 'target' has configurable property '", String(impl), "' that was not in the result from the 'ownKeys' trap"));
> 
> Can you add tests for this?

Done (though I haven't run it yet, will see if EWS is happy with it)

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:1005
>> +            throwVMTypeError(exec, scope, "Proxy handler's 'ownKeys' method returned a key that was not present in its non-extensible target"_s);
> 
> Can you add tests for this?

Ditto

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:1011
>> +        // Filtering DontEnum properties is observable in proxies and must occur following the invariant checks above.
> 
> Can you add tests for this?

Done

[Caitlin Potter (:caitp)]

Comment on attachment 360004 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

mlam: can we carry the R+ over to https://bugs.webkit.org/show_bug.cgi?id=185211 ? :) Sorry it took so long to do anything with this, been busy.

[Caitlin Potter (:caitp)]

Comment on attachment 360032 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord

View in context: https://bugs.webkit.org/attachment.cgi?id=360032&action=review

>> Source/WebCore/bindings/js/JSDOMConvertRecord.h:107
>> +                    // ToString(key) should fail in this case.
> 
> Is key always supposed to be converted to a string here?

For all current uses of record<> in the tree (and there aren't that many), the IDLType is a String type.

Unfortunately, this isn't enforced in the template signature, so it isn't guaranteed, but as far as the current codebase is concerned it's a guarantee.

[EWS Watchlist]

Comment on attachment 362731 [details]
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

Attachment 362731 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/11249180

New failing tests:
stress/proxy-own-keys.js.no-cjit-collect-continuously
stress/proxy-own-keys.js.no-ftl
stress/proxy-own-keys.js.ftl-no-cjit-b3o0
stress/proxy-own-keys.js.bytecode-cache
stress/proxy-own-keys.js.ftl-eager-no-cjit
stress/proxy-own-keys.js.ftl-no-cjit-small-pool
stress/proxy-own-keys.js.default
stress/proxy-own-keys.js.dfg-eager
stress/proxy-own-keys.js.ftl-eager-no-cjit-b3o1
stress/proxy-own-keys.js.ftl-no-cjit-validate-sampling-profiler
stress/proxy-own-keys.js.ftl-eager
stress/proxy-own-keys.js.ftl-no-cjit-no-put-stack-validate
stress/proxy-own-keys.js.no-llint
stress/proxy-own-keys.js.ftl-no-cjit-no-inline-validate
stress/proxy-own-keys.js.dfg-eager-no-cjit-validate
stress/proxy-own-keys.js.no-cjit-validate-phases
stress/proxy-own-keys.js.dfg-maximal-flush-validate-no-cjit

[Caitlin Potter (:caitp)]

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

[EWS Watchlist]

Comment on attachment 362762 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

Attachment 362762 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11254939

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 362812 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Caitlin Potter (:caitp)]

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

[Saam Barati]

Comment on attachment 362896 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

View in context: https://bugs.webkit.org/attachment.cgi?id=362896&action=review

r=me

> Source/WebCore/ChangeLog:15
> +        This patch adds DontEnum filtering for ProxyObjects, however we continue
> +        to explicitly filter them in JSDOMConvertRecord, which needs to use the
> +        property descriptor after filtering. This change prevents observably
> +        fetching the property descriptor twice per property.

Can you add tests for this?

[Caitlin Potter (:caitp)]

Comment on attachment 362896 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

View in context: https://bugs.webkit.org/attachment.cgi?id=362896&action=review

>> Source/WebCore/ChangeLog:15
>> +        fetching the property descriptor twice per property.
> 
> Can you add tests for this?

This is covered by some existing web-platform-tests cases --- do we need extra coverage outside of WPT?

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #70)
> Comment on attachment 362896 [details]
> [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362896&action=review
> 
> >> Source/WebCore/ChangeLog:15
> >> +        fetching the property descriptor twice per property.
> > 
> > Can you add tests for this?
> 
> This is covered by some existing web-platform-tests cases --- do we need
> extra coverage outside of WPT?

Does this LOC:
"object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));"
make that test pass?

[Caitlin Potter (:caitp)]

Comment on attachment 362896 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

View in context: https://bugs.webkit.org/attachment.cgi?id=362896&action=review

>>>> Source/WebCore/ChangeLog:15
>>>> +        fetching the property descriptor twice per property.
>>> 
>>> Can you add tests for this?
>> 
>> This is covered by some existing web-platform-tests cases --- do we need extra coverage outside of WPT?
> 
> Does this LOC:
> "object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));"
> make that test pass?

Yes and no --- The change to ProxyObject.cpp, in conjunction with the unchanged JSDOMConvertRecord.h breaks some tests (because it causes some observable extra getOwnPropertyDescriptor checks on Proxies), which are fixed by the change you mentioned.

The reason is pretty straight forward: JSDOMConvertRecord was previously doing its own DontEnum filtering (with a FIXME asking if it was necessary --- which it was, in the case of Proxies). Now, Proxy getOwnPropertyNames can do its own DontEnum filtering. To avoid filtering twice (which results in extra observable gOPD calls), we can turn off the filtering for one of those cases. Because JSDOMConvertRecord does other things with the property descriptor than just filtering DontEnums, I've opted to turn off the filtering from ProxyObject::getOwnPropertyNames() for that particular use case.

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #72)
> Comment on attachment 362896 [details]
> [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362896&action=review
> 
> >>>> Source/WebCore/ChangeLog:15
> >>>> +        fetching the property descriptor twice per property.
> >>> 
> >>> Can you add tests for this?
> >> 
> >> This is covered by some existing web-platform-tests cases --- do we need extra coverage outside of WPT?
> > 
> > Does this LOC:
> > "object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));"
> > make that test pass?
> 
> Yes and no --- The change to ProxyObject.cpp, in conjunction with the
> unchanged JSDOMConvertRecord.h breaks some tests (because it causes some
> observable extra getOwnPropertyDescriptor checks on Proxies), which are
> fixed by the change you mentioned.
> 
> The reason is pretty straight forward: JSDOMConvertRecord was previously
> doing its own DontEnum filtering (with a FIXME asking if it was necessary
> --- which it was, in the case of Proxies). Now, Proxy getOwnPropertyNames
> can do its own DontEnum filtering. To avoid filtering twice (which results
> in extra observable gOPD calls), we can turn off the filtering for one of
> those cases. Because JSDOMConvertRecord does other things with the property
> descriptor than just filtering DontEnums, I've opted to turn off the
> filtering from ProxyObject::getOwnPropertyNames() for that particular use
> case.

It sounds like the there could be a test that would have done an extra GOPD call that this fixes?

[Caitlin Potter (:caitp)]

Comment on attachment 362896 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

View in context: https://bugs.webkit.org/attachment.cgi?id=362896&action=review

>>>>>> Source/WebCore/ChangeLog:15
>>>>>> +        fetching the property descriptor twice per property.
>>>>> 
>>>>> Can you add tests for this?
>>>> 
>>>> This is covered by some existing web-platform-tests cases --- do we need extra coverage outside of WPT?
>>> 
>>> Does this LOC:
>>> "object->methodTable(vm)->getOwnPropertyNames(object, &state, keys, JSC::EnumerationMode(JSC::DontEnumPropertiesMode::Include));"
>>> make that test pass?
>> 
>> Yes and no --- The change to ProxyObject.cpp, in conjunction with the unchanged JSDOMConvertRecord.h breaks some tests (because it causes some observable extra getOwnPropertyDescriptor checks on Proxies), which are fixed by the change you mentioned.
>> 
>> The reason is pretty straight forward: JSDOMConvertRecord was previously doing its own DontEnum filtering (with a FIXME asking if it was necessary --- which it was, in the case of Proxies). Now, Proxy getOwnPropertyNames can do its own DontEnum filtering. To avoid filtering twice (which results in extra observable gOPD calls), we can turn off the filtering for one of those cases. Because JSDOMConvertRecord does other things with the property descriptor than just filtering DontEnums, I've opted to turn off the filtering from ProxyObject::getOwnPropertyNames() for that particular use case.
> 
> It sounds like the there could be a test that would have done an extra GOPD call that this fixes?

There are :) https://github.com/web-platform-tests/wpt/blob/master/fetch/api/headers/headers-record.html looks at the ordering of observable trap calls and was failing without the fix due to GOPD firing twice instead of once.

[Caitlin Potter (:caitp)]

Is this still good to go? I've tested locally and it merges clean and passes `run-javascriptcore-tests`.

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #75)
> Is this still good to go? I've tested locally and it merges clean and passes
> `run-javascriptcore-tests`.

LGTM

[WebKit Commit Bot]

Comment on attachment 362896 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

Clearing flags on attachment: 362896

Committed r243943: <https://trac.webkit.org/changeset/243943>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change has causes two test262 failures:


---------------NEW FAILING TESTS SUMMARY---------------

FAIL test/built-ins/Object/keys/proxy-keys.js (default)
Full Output:
Exception: Test262Error: Expected SameValue(«12», «10») to be true

FAIL test/built-ins/Object/keys/proxy-keys.js (strict mode)
Full Output:
Exception: Test262Error: Expected SameValue(«12», «10») to be true

https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/2262

[Ryan Haddad]

Reverted r243943 for reason:

Caused test262 failures.

Committed r244020: <https://trac.webkit.org/changeset/244020>

[Ryan Haddad]

stress/proxy-own-keys.js was also failing an assertion on the debug JSC queue due to an unchecked exception: 

ERROR: Unchecked JS exception:
    This scope can throw a JS exception: getOwnPropertySlotCommon @ ./runtime/ProxyObject.cpp:376
        (ExceptionScope::m_recursionDepth was 7)
    But the exception was unchecked as of this scope: getOwnPropertySlotCommon @ ./runtime/ProxyObject.cpp:376
        (ExceptionScope::m_recursionDepth was 7)

Unchecked exception detected at:
    1   0x1080bbc53 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
    2   0x10808f9ea JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
    3   0x10808fa33 JSC::ThrowScope::ThrowScope(JSC::VM&, JSC::ExceptionEventLocation)
    4   0x108011e47 JSC::ProxyObject::getOwnPropertySlotCommon(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
    5   0x108017280 JSC::ProxyObject::performGetOwnPropertyNames(JSC::ExecState*, JSC::PropertyNameArray&, JSC::EnumerationMode)
    6   0x10800f2c2 JSC::ProxyObject::getOwnPropertyNames(JSC::JSObject*, JSC::ExecState*, JSC::PropertyNameArray&, JSC::EnumerationMode)
    7   0x107fd7442 JSC::ownPropertyKeys(JSC::ExecState*, JSC::JSObject*, JSC::PropertyNameMode, JSC::DontEnumPropertiesMode)
    8   0x107fd8870 JSC::objectConstructorKeys(JSC::ExecState*)
    9   0x2967d48c16b
    10  0x2967d4b9f37
    11  0x106e53369 vmEntryToJavaScript
    12  0x107acda1e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
    13  0x107accfa0 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
    14  0x107dfc8e5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
    15  0x10570a4c7 runWithOptions(GlobalObject*, CommandLine&, bool&)
    16  0x1056e00ca jscmain(int, char**)::$_6::operator()(JSC::VM&, GlobalObject*, bool&) const
    17  0x1056c19f4 int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, bool, jscmain(int, char**)::$_6 const&)
    18  0x1056c0121 jscmain(int, char**)
    19  0x1056bff8e main
    20  0x7fff57f7f015 start

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2517

[Caitlin Potter (:caitp)]

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

There was some redundant DontEnum filtering code in ObjectConstructor.cpp's ownPropertyKeys() function, which I've removed (and verified that it compiles and passes test262). It may still be slightly broken, I'll see shortly.

[Caitlin Potter (:caitp)]

(In reply to Ryan Haddad from comment #81)
> stress/proxy-own-keys.js was also failing an assertion on the debug JSC
> queue due to an unchecked exception: 
> 
> ERROR: Unchecked JS exception:
>     This scope can throw a JS exception: getOwnPropertySlotCommon @
> ./runtime/ProxyObject.cpp:376
>         (ExceptionScope::m_recursionDepth was 7)
>     But the exception was unchecked as of this scope:
> getOwnPropertySlotCommon @ ./runtime/ProxyObject.cpp:376
>         (ExceptionScope::m_recursionDepth was 7)
> 
> Unchecked exception detected at:
>     1   0x1080bbc53 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned
> int, JSC::ExceptionEventLocation&)
>     2   0x10808f9ea JSC::ThrowScope::ThrowScope(JSC::VM&,
> JSC::ExceptionEventLocation)
>     3   0x10808fa33 JSC::ThrowScope::ThrowScope(JSC::VM&,
> JSC::ExceptionEventLocation)
>     4   0x108011e47
> JSC::ProxyObject::getOwnPropertySlotCommon(JSC::ExecState*,
> JSC::PropertyName, JSC::PropertySlot&)
>     5   0x108017280
> JSC::ProxyObject::performGetOwnPropertyNames(JSC::ExecState*,
> JSC::PropertyNameArray&, JSC::EnumerationMode)
>     6   0x10800f2c2 JSC::ProxyObject::getOwnPropertyNames(JSC::JSObject*,
> JSC::ExecState*, JSC::PropertyNameArray&, JSC::EnumerationMode)
>     7   0x107fd7442 JSC::ownPropertyKeys(JSC::ExecState*, JSC::JSObject*,
> JSC::PropertyNameMode, JSC::DontEnumPropertiesMode)
>     8   0x107fd8870 JSC::objectConstructorKeys(JSC::ExecState*)
>     9   0x2967d48c16b
>     10  0x2967d4b9f37
>     11  0x106e53369 vmEntryToJavaScript
>     12  0x107acda1e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
>     13  0x107accfa0 JSC::Interpreter::executeProgram(JSC::SourceCode const&,
> JSC::ExecState*, JSC::JSObject*)
>     14  0x107dfc8e5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&,
> JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
>     15  0x10570a4c7 runWithOptions(GlobalObject*, CommandLine&, bool&)
>     16  0x1056e00ca jscmain(int, char**)::$_6::operator()(JSC::VM&,
> GlobalObject*, bool&) const
>     17  0x1056c19f4 int runJSC<jscmain(int, char**)::$_6>(CommandLine
> const&, bool, jscmain(int, char**)::$_6 const&)
>     18  0x1056c0121 jscmain(int, char**)
>     19  0x1056bff8e main
>     20  0x7fff57f7f015 start
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2517

I wasn't able to reproduce this with the reland patch, with ENABLE_EXCEPTION_SCOPE_VERIFICATION defined to 1 in cmakeconfig.h --- I can see if I can reproduce it without the changes, but otherwise I could use some guidance on building with all the checks enabled, if you (or anyone else) can spare the time to provide that.

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #83)
> (In reply to Ryan Haddad from comment #81)
> > stress/proxy-own-keys.js was also failing an assertion on the debug JSC
> > queue due to an unchecked exception: 
> > 
> > ERROR: Unchecked JS exception:
> >     This scope can throw a JS exception: getOwnPropertySlotCommon @
> > ./runtime/ProxyObject.cpp:376
> >         (ExceptionScope::m_recursionDepth was 7)
> >     But the exception was unchecked as of this scope:
> > getOwnPropertySlotCommon @ ./runtime/ProxyObject.cpp:376
> >         (ExceptionScope::m_recursionDepth was 7)
> > 
> > Unchecked exception detected at:
> >     1   0x1080bbc53 JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned
> > int, JSC::ExceptionEventLocation&)
> >     2   0x10808f9ea JSC::ThrowScope::ThrowScope(JSC::VM&,
> > JSC::ExceptionEventLocation)
> >     3   0x10808fa33 JSC::ThrowScope::ThrowScope(JSC::VM&,
> > JSC::ExceptionEventLocation)
> >     4   0x108011e47
> > JSC::ProxyObject::getOwnPropertySlotCommon(JSC::ExecState*,
> > JSC::PropertyName, JSC::PropertySlot&)
> >     5   0x108017280
> > JSC::ProxyObject::performGetOwnPropertyNames(JSC::ExecState*,
> > JSC::PropertyNameArray&, JSC::EnumerationMode)
> >     6   0x10800f2c2 JSC::ProxyObject::getOwnPropertyNames(JSC::JSObject*,
> > JSC::ExecState*, JSC::PropertyNameArray&, JSC::EnumerationMode)
> >     7   0x107fd7442 JSC::ownPropertyKeys(JSC::ExecState*, JSC::JSObject*,
> > JSC::PropertyNameMode, JSC::DontEnumPropertiesMode)
> >     8   0x107fd8870 JSC::objectConstructorKeys(JSC::ExecState*)
> >     9   0x2967d48c16b
> >     10  0x2967d4b9f37
> >     11  0x106e53369 vmEntryToJavaScript
> >     12  0x107acda1e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
> >     13  0x107accfa0 JSC::Interpreter::executeProgram(JSC::SourceCode const&,
> > JSC::ExecState*, JSC::JSObject*)
> >     14  0x107dfc8e5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&,
> > JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> >     15  0x10570a4c7 runWithOptions(GlobalObject*, CommandLine&, bool&)
> >     16  0x1056e00ca jscmain(int, char**)::$_6::operator()(JSC::VM&,
> > GlobalObject*, bool&) const
> >     17  0x1056c19f4 int runJSC<jscmain(int, char**)::$_6>(CommandLine
> > const&, bool, jscmain(int, char**)::$_6 const&)
> >     18  0x1056c0121 jscmain(int, char**)
> >     19  0x1056bff8e main
> >     20  0x7fff57f7f015 start
> > 
> > https://build.webkit.org/builders/
> > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2517
> 
> I wasn't able to reproduce this with the reland patch, with
> ENABLE_EXCEPTION_SCOPE_VERIFICATION defined to 1 in cmakeconfig.h --- I can
> see if I can reproduce it without the changes, but otherwise I could use
> some guidance on building with all the checks enabled, if you (or anyone
> else) can spare the time to provide that.

Yeah, I am unable to reproduce the crash with the fixed version or original version.

[Caitlin Potter (:caitp)]

Ok, I've managed to reproduce it, should have this fixed in the next patch version.

[Caitlin Potter (:caitp)]

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

fix the exception check failure

[Caio Lima]

Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

LGTM

[Caitlin Potter (:caitp)]

Saam, can you re-sign off on this when you get the chance?

[Saam Barati]

Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

r=me. LGTM too

[Caitlin Potter (:caitp)]

Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

Thanks, here goes

[WebKit Commit Bot]

Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

Clearing flags on attachment: 367326

Committed r244330: <https://trac.webkit.org/changeset/244330>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.