Bug 190094 - Make crossOriginObject.then undefined for promises
Summary: Make crossOriginObject.then undefined for promises
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://github.com/whatwg/html/pull/3242
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-28 15:14 PDT by Chris Dumez
Modified: 2018-10-01 09:11 PDT (History)
10 users (show)

See Also:


Attachments
Patch (12.89 KB, patch)
2018-09-28 15:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (19.65 KB, patch)
2018-09-28 17:08 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.45 MB, application/zip)
2018-09-28 18:25 PDT, EWS Watchlist
no flags Details
Patch (20.97 KB, patch)
2018-09-28 18:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.57 MB, application/zip)
2018-09-28 19:29 PDT, EWS Watchlist
no flags Details
Patch (21.49 KB, patch)
2018-09-28 19:36 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-09-28 15:14:32 PDT
Make crossOriginObject.then undefined for promises. This allows promises to work better with cross-origin WindowProxy and Location objects.

Specification:
- https://github.com/whatwg/html/pull/3242
- https://github.com/whatwg/dom/issues/536
Comment 1 Chris Dumez 2018-09-28 15:36:52 PDT
Created attachment 351126 [details]
Patch
Comment 2 Darin Adler 2018-09-28 15:46:23 PDT
Comment on attachment 351126 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:177
> +    if (propertyName == vm.propertyNames->builtinNames().thenPublicName() || propertyName == vm.propertyNames->toStringTagSymbol || propertyName == vm.propertyNames->hasInstanceSymbol || propertyName == vm.propertyNames->isConcatSpreadableSymbol) {

I would like it if we had a named function for this check. It would have two benefits: 1) Could put *vm.propertyNames into a local variable to make the code more readable, although we could also do that here. 2) The name of the function could help make it clear what these names have in common.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:408
> +    propertyNames.add(vm.propertyNames->builtinNames().thenPublicName());
>      propertyNames.add(vm.propertyNames->toStringTagSymbol);
>      propertyNames.add(vm.propertyNames->hasInstanceSymbol);
>      propertyNames.add(vm.propertyNames->isConcatSpreadableSymbol);

Would be even better if somehow the list of property names existed only once, and could be used both for the if statement above and this adding here.

> Source/WebCore/bindings/js/JSLocationCustom.cpp:58
> +    if (propertyName == vm.propertyNames->builtinNames().thenPublicName() || propertyName == vm.propertyNames->toStringTagSymbol || propertyName == vm.propertyNames->hasInstanceSymbol || propertyName == vm.propertyNames->isConcatSpreadableSymbol) {

Annoying that this repeats the same list but does not share code with DOMWindow. Would be nice to fix that.

> Source/WebCore/bindings/js/JSLocationCustom.cpp:185
> +    propertyNames.add(vm.propertyNames->builtinNames().thenPublicName());
>      propertyNames.add(vm.propertyNames->toStringTagSymbol);
>      propertyNames.add(vm.propertyNames->hasInstanceSymbol);
>      propertyNames.add(vm.propertyNames->isConcatSpreadableSymbol);

Ditto.
Comment 3 Chris Dumez 2018-09-28 17:08:21 PDT
Created attachment 351145 [details]
Patch
Comment 4 EWS Watchlist 2018-09-28 18:25:37 PDT
Comment on attachment 351145 [details]
Patch

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

New failing tests:
http/tests/security/cross-frame-access-enumeration.html
Comment 5 EWS Watchlist 2018-09-28 18:25:39 PDT
Created attachment 351155 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 Chris Dumez 2018-09-28 18:43:35 PDT
Created attachment 351156 [details]
Patch
Comment 7 EWS Watchlist 2018-09-28 19:29:46 PDT
Comment on attachment 351156 [details]
Patch

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

New failing tests:
http/tests/security/cross-frame-access-enumeration.html
Comment 8 EWS Watchlist 2018-09-28 19:29:48 PDT
Created attachment 351161 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Chris Dumez 2018-09-28 19:36:11 PDT
Created attachment 351163 [details]
Patch
Comment 10 WebKit Commit Bot 2018-10-01 09:11:00 PDT
Comment on attachment 351163 [details]
Patch

Clearing flags on attachment: 351163

Committed r236661: <https://trac.webkit.org/changeset/236661>
Comment 11 WebKit Commit Bot 2018-10-01 09:11:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2018-10-01 09:11:31 PDT
<rdar://problem/44910057>