Bug 176810

<rdar://problem/34435582>

Created attachment 351389 [details] Fix for bug

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

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

(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.

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

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

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

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

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

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

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.

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

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

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

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

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

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.

(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

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.

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Ping :)

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

Caitlin, can you make a bugzilla per patch?

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?

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?

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

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

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.

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.

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

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

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

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

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

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?

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?

(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?

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.

(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?

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.

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

(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

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>

All reviewed patches have been landed. Closing bug.

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

Reverted r243943 for reason: Caused test262 failures. Committed r244020: <https://trac.webkit.org/changeset/244020>

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

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.

(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.

(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.

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

Created attachment 367326 [details] Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() fix the exception check failure

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

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

Comment on attachment 367326 [details] Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() r=me. LGTM too

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

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>

All reviewed patches have been landed. Closing bug.