Bug 176810 - for..in on a Proxy loops over non enumerable properties
Summary: for..in on a Proxy loops over non enumerable properties
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-12 14:55 PDT by JF Paradis
Modified: 2019-04-16 08:59 PDT (History)
18 users (show)

See Also:


Attachments
Fix for bug (3.88 KB, patch)
2018-10-02 08:44 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.33 MB, application/zip)
2018-10-02 09:49 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.85 MB, application/zip)
2018-10-02 09:59 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.73 MB, application/zip)
2018-10-02 10:24 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.65 MB, application/zip)
2018-10-02 19:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.43 MB, application/zip)
2018-10-02 20:44 PDT, EWS Watchlist
no flags Details
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys (1.75 KB, patch)
2019-01-23 11:23 PST, Caitlin Potter (:caitp)
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (6.73 KB, patch)
2019-01-23 11:27 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord (2.81 KB, patch)
2019-01-23 11:28 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.79 MB, application/zip)
2019-01-23 12:23 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-01-23 12:57 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.23 MB, application/zip)
2019-01-23 13:11 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (9.49 MB, application/zip)
2019-01-23 13:26 PST, EWS Watchlist
no flags Details
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys (3.78 KB, patch)
2019-01-23 14:27 PST, Caitlin Potter (:caitp)
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord (1.99 KB, patch)
2019-01-23 15:23 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (6.70 KB, patch)
2019-01-23 15:25 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (2.43 MB, application/zip)
2019-01-23 15:28 PST, EWS Watchlist
no flags Details
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys (3.78 KB, patch)
2019-01-23 15:34 PST, Caitlin Potter (:caitp)
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord (2.54 KB, patch)
2019-01-23 15:44 PST, Caitlin Potter (:caitp)
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-highsierra (2.56 MB, application/zip)
2019-01-23 17:02 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.15 MB, application/zip)
2019-01-23 18:00 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.94 MB, application/zip)
2019-01-23 18:25 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.72 MB, application/zip)
2019-01-23 19:49 PST, EWS Watchlist
no flags Details
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys (13.02 KB, patch)
2019-01-24 04:31 PST, Caitlin Potter (:caitp)
mark.lam: review+
Details | Formatted Diff | Diff
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord (4.45 KB, patch)
2019-01-24 13:46 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (10.98 KB, patch)
2019-02-22 10:44 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (11.00 KB, patch)
2019-02-22 14:00 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.54 MB, application/zip)
2019-02-22 19:01 PST, EWS Watchlist
no flags Details
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (11.04 KB, patch)
2019-02-25 05:46 PST, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (15.58 KB, patch)
2019-04-08 11:35 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames() (15.75 KB, patch)
2019-04-12 09:32 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Paradis 2017-09-12 14:55:11 PDT
* 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
Comment 1 Radar WebKit Bug Importer 2017-09-14 10:13:01 PDT
<rdar://problem/34435582>
Comment 2 Xan Lopez 2018-10-02 08:44:19 PDT
Created attachment 351389 [details]
Fix for bug
Comment 3 Caio Lima 2018-10-02 09:01:18 PDT
Do you mind to add the other test case forcing the enumerable to false as well?
Comment 4 Saam Barati 2018-10-02 09:01:56 PDT
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
Comment 5 Xan Lopez 2018-10-02 09:48:23 PDT
(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 6 EWS Watchlist 2018-10-02 09:49:01 PDT
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
Comment 7 EWS Watchlist 2018-10-02 09:49:03 PDT
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 8 EWS Watchlist 2018-10-02 09:59:03 PDT
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
Comment 9 EWS Watchlist 2018-10-02 09:59:05 PDT
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 10 EWS Watchlist 2018-10-02 10:24:26 PDT
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
Comment 11 EWS Watchlist 2018-10-02 10:24:28 PDT
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 12 Caio Lima 2018-10-02 10:31:15 PDT
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 13 EWS Watchlist 2018-10-02 19:32:36 PDT
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
Comment 14 EWS Watchlist 2018-10-02 19:32:38 PDT
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 15 EWS Watchlist 2018-10-02 20:44:15 PDT
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
Comment 16 EWS Watchlist 2018-10-02 20:44:17 PDT
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
Comment 17 Saam Barati 2019-01-20 14:32:24 PST
Is this still an issue? r- because EWS is unhappy.
Comment 18 Caitlin Potter (:caitp) 2019-01-22 09:00:33 PST
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.
Comment 19 Caitlin Potter (:caitp) 2019-01-22 12:21:22 PST
(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
Comment 20 Saam Barati 2019-01-22 14:15:23 PST
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.
Comment 21 Caitlin Potter (:caitp) 2019-01-23 11:23:34 PST
Created attachment 359910 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys
Comment 22 Caitlin Potter (:caitp) 2019-01-23 11:27:43 PST
Created attachment 359911 [details]
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
Comment 23 Caitlin Potter (:caitp) 2019-01-23 11:28:50 PST
Created attachment 359912 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord
Comment 24 Caitlin Potter (:caitp) 2019-01-23 11:32:40 PST
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 25 EWS Watchlist 2019-01-23 12:23:38 PST
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
Comment 26 EWS Watchlist 2019-01-23 12:23:40 PST
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 27 EWS Watchlist 2019-01-23 12:57:26 PST
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
Comment 28 EWS Watchlist 2019-01-23 12:57:28 PST
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 29 EWS Watchlist 2019-01-23 13:11:18 PST
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
Comment 30 EWS Watchlist 2019-01-23 13:11:20 PST
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 31 EWS Watchlist 2019-01-23 13:13:39 PST
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 32 EWS Watchlist 2019-01-23 13:26:37 PST
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
Comment 33 EWS Watchlist 2019-01-23 13:26:40 PST
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
Comment 34 Caitlin Potter (:caitp) 2019-01-23 14:27:13 PST
Created attachment 359941 [details]
(1) [JSC] throw if ownKeys Proxy trap result contains duplicate keys
Comment 35 Caitlin Potter (:caitp) 2019-01-23 15:23:13 PST
Created attachment 359952 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord
Comment 36 Caitlin Potter (:caitp) 2019-01-23 15:25:33 PST
Created attachment 359953 [details]
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
Comment 37 EWS Watchlist 2019-01-23 15:28:04 PST
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
Comment 38 EWS Watchlist 2019-01-23 15:28:07 PST
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
Comment 39 Caitlin Potter (:caitp) 2019-01-23 15:34:09 PST
Created attachment 359959 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
Comment 40 Caitlin Potter (:caitp) 2019-01-23 15:39:48 PST
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.
Comment 41 Caitlin Potter (:caitp) 2019-01-23 15:44:18 PST
Created attachment 359961 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord
Comment 42 EWS Watchlist 2019-01-23 17:02:20 PST
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
Comment 43 EWS Watchlist 2019-01-23 17:02:23 PST
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 44 EWS Watchlist 2019-01-23 18:00:09 PST
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
Comment 45 EWS Watchlist 2019-01-23 18:00:11 PST
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 46 EWS Watchlist 2019-01-23 18:25:54 PST
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
Comment 47 EWS Watchlist 2019-01-23 18:25:56 PST
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 48 EWS Watchlist 2019-01-23 18:33:08 PST
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 49 EWS Watchlist 2019-01-23 19:49:10 PST
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
Comment 50 EWS Watchlist 2019-01-23 19:49:13 PST
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
Comment 51 Caitlin Potter (:caitp) 2019-01-24 04:31:38 PST
Created attachment 360004 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys
Comment 52 Caitlin Potter (:caitp) 2019-01-24 09:36:19 PST
With parts 1 & 2 passing EWS, a review of those would be good. I will have part 3 passing tomorrow
Comment 53 Caitlin Potter (:caitp) 2019-01-24 13:46:41 PST
Created attachment 360032 [details]
(3) Fix [[OwnPropertyKeys]] algorithm in JSDOMConvertRecord
Comment 54 Caitlin Potter (:caitp) 2019-01-28 03:27:30 PST
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
Comment 55 Caitlin Potter (:caitp) 2019-01-29 09:45:05 PST
Ping :)
Comment 56 Mark Lam 2019-02-14 23:30:46 PST
Comment on attachment 360004 [details]
(1) [JSC] throw if 'ownKeys' Proxy trap result contains duplicate keys

The ownKeys Proxy trap patch LGTM.
Comment 57 Saam Barati 2019-02-22 01:12:03 PST
Caitlin, can you make a bugzilla per patch?
Comment 58 Saam Barati 2019-02-22 01:15:55 PST
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 59 Saam Barati 2019-02-22 01:19:17 PST
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?
Comment 60 Caitlin Potter (:caitp) 2019-02-22 10:44:35 PST
Created attachment 362731 [details]
(2) [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
Comment 61 Caitlin Potter (:caitp) 2019-02-22 10:47:12 PST
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 62 Caitlin Potter (:caitp) 2019-02-22 11:01:36 PST
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 63 Caitlin Potter (:caitp) 2019-02-22 11:20:46 PST
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 64 EWS Watchlist 2019-02-22 13:27:34 PST
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
Comment 65 Caitlin Potter (:caitp) 2019-02-22 14:00:36 PST
Created attachment 362762 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
Comment 66 EWS Watchlist 2019-02-22 19:01:53 PST
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
Comment 67 EWS Watchlist 2019-02-22 19:01:56 PST
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
Comment 68 Caitlin Potter (:caitp) 2019-02-25 05:46:56 PST
Created attachment 362896 [details]
[JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()
Comment 69 Saam Barati 2019-02-25 12:06:46 PST
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 70 Caitlin Potter (:caitp) 2019-02-25 12:14:23 PST
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?
Comment 71 Saam Barati 2019-02-25 15:13:13 PST
(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 72 Caitlin Potter (:caitp) 2019-02-25 15:27:08 PST
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.
Comment 73 Saam Barati 2019-02-25 15:30:45 PST
(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 74 Caitlin Potter (:caitp) 2019-02-25 15:33:22 PST
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.
Comment 75 Caitlin Potter (:caitp) 2019-04-05 08:10:09 PDT
Is this still good to go? I've tested locally and it merges clean and passes `run-javascriptcore-tests`.
Comment 76 Saam Barati 2019-04-05 14:01:07 PDT
(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 77 WebKit Commit Bot 2019-04-05 14:28:16 PDT
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>
Comment 78 WebKit Commit Bot 2019-04-05 14:28:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 79 Ryan Haddad 2019-04-05 15:51:45 PDT
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
Comment 80 Ryan Haddad 2019-04-08 08:51:26 PDT
Reverted r243943 for reason:

Caused test262 failures.

Committed r244020: <https://trac.webkit.org/changeset/244020>
Comment 81 Ryan Haddad 2019-04-08 08:54:31 PDT
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
Comment 82 Caitlin Potter (:caitp) 2019-04-08 11:35:03 PDT
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.
Comment 83 Caitlin Potter (:caitp) 2019-04-08 14:58:09 PDT
(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.
Comment 84 Caitlin Potter (:caitp) 2019-04-08 15:13:33 PDT
(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.
Comment 85 Caitlin Potter (:caitp) 2019-04-12 08:42:03 PDT
Ok, I've managed to reproduce it, should have this fixed in the next patch version.
Comment 86 Caitlin Potter (:caitp) 2019-04-12 09:32:12 PDT
Created attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

fix the exception check failure
Comment 87 Caio Lima 2019-04-12 10:00:15 PDT
Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

LGTM
Comment 88 Caitlin Potter (:caitp) 2019-04-15 09:53:23 PDT
Saam, can you re-sign off on this when you get the chance?
Comment 89 Saam Barati 2019-04-15 15:13:34 PDT
Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

r=me. LGTM too
Comment 90 Caitlin Potter (:caitp) 2019-04-16 08:31:34 PDT
Comment on attachment 367326 [details]
Reland [JSC] Filter DontEnum properties in ProxyObject::getOwnPropertyNames()

Thanks, here goes
Comment 91 WebKit Commit Bot 2019-04-16 08:59:05 PDT
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>
Comment 92 WebKit Commit Bot 2019-04-16 08:59:08 PDT
All reviewed patches have been landed.  Closing bug.