Bug 181416 - We should not return undefined for most properties of a detached Window
Summary: We should not return undefined for most properties of a detached Window
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-08 16:48 PST by Chris Dumez
Modified: 2018-01-09 19:31 PST (History)
8 users (show)

See Also:


Attachments
Patch (18.06 KB, patch)
2018-01-08 16:55 PST, 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-01-08 16:48:05 PST
We should not return undefined for most properties on a detached Window. WebKit currently only exposes "closed" and "close" properties on detached / frameless windows. However, this does not match the HTML specification [1] or the behavior of Firefox and Chrome.
Comment 1 Chris Dumez 2018-01-08 16:48:20 PST
<rdar://problem/36162489>
Comment 2 Chris Dumez 2018-01-08 16:55:35 PST
Created attachment 330756 [details]
Patch
Comment 3 Chris Dumez 2018-01-09 10:16:33 PST
Comment on attachment 330756 [details]
Patch

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

> LayoutTests/fast/frames/detached-frame-property.html:37
> +    // Both Chrome and Firefox disagree with us and return a valid object.

I'll look into this in a follow-up.
Comment 4 Ryosuke Niwa 2018-01-09 10:37:11 PST
Comment on attachment 330756 [details]
Patch

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

> LayoutTests/fast/frames/detached-frame-property-expected.txt:16
> +PASS !detachedWindow.navigator is true

How about detachedWindow.location?
Is it null? If it's not null, what happens when you try to navigate?

How about other methods like alert, confirm, etc...?
Comment 5 Chris Dumez 2018-01-09 10:43:54 PST
Comment on attachment 330756 [details]
Patch

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

>> LayoutTests/fast/frames/detached-frame-property-expected.txt:16
>> +PASS !detachedWindow.navigator is true
> 
> How about detachedWindow.location?
> Is it null? If it's not null, what happens when you try to navigate?
> 
> How about other methods like alert, confirm, etc...?

Location is tested below and is null. This is not as per spec and does not match other browsers (which keep returning the location object). I will look into returning a valid object for location in a follow-up as I already commented below.

Note that we do not allow anything new here, you could already store frame.contentWindow or frame.contentWindow.location or (or other window properties) in a local variable, then detach the frame, then use that local variable. Therefore, we already deal with these objects being used without a frame.

In the case of window.alert / confirm, they return early when m_frame is null. As I said, we already handle this fine because JS could already call these operations on a frameless window by simply storing the operation in a local variable, then detached the frame and call the operation on the frameless window.
Comment 6 Chris Dumez 2018-01-09 10:53:56 PST
Comment on attachment 330756 [details]
Patch

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

>>> LayoutTests/fast/frames/detached-frame-property-expected.txt:16
>>> +PASS !detachedWindow.navigator is true
>> 
>> How about detachedWindow.location?
>> Is it null? If it's not null, what happens when you try to navigate?
>> 
>> How about other methods like alert, confirm, etc...?
> 
> Location is tested below and is null. This is not as per spec and does not match other browsers (which keep returning the location object). I will look into returning a valid object for location in a follow-up as I already commented below.
> 
> Note that we do not allow anything new here, you could already store frame.contentWindow or frame.contentWindow.location or (or other window properties) in a local variable, then detach the frame, then use that local variable. Therefore, we already deal with these objects being used without a frame.
> 
> In the case of window.alert / confirm, they return early when m_frame is null. As I said, we already handle this fine because JS could already call these operations on a frameless window by simply storing the operation in a local variable, then detached the frame and call the operation on the frameless window.

E.g. window.alert on a detached window was already easy to achieve via:
window.alert.call(aDetachedWindow, "test")
Comment 7 Ryosuke Niwa 2018-01-09 18:55:52 PST
Comment on attachment 330756 [details]
Patch

Sounds good to me.
Comment 8 WebKit Commit Bot 2018-01-09 19:31:33 PST
Comment on attachment 330756 [details]
Patch

Clearing flags on attachment: 330756

Committed r226676: <https://trac.webkit.org/changeset/226676>
Comment 9 WebKit Commit Bot 2018-01-09 19:31:35 PST
All reviewed patches have been landed.  Closing bug.