Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should COOP-related browsing context switch result in nullification of a popup's WindowProxy's window? #10457

Open
yoavweiss opened this issue Jul 3, 2024 · 8 comments

Comments

@yoavweiss
Copy link
Collaborator

yoavweiss commented Jul 3, 2024

What is the issue with the HTML Standard?

When a document opens a popup (using something like const popup = window.open(url)), and that popup requires a browsing context group switch due to COOP enforcement, it's not immediately obvious (to me) what the value of popup.window should be.

When adding an assertion on that front to popup-test.js#89, Chromium and WebKit seem to disagree on what that value should be. The value is asynchronously being set to null in Chromium (once the response is received), while remaining a global object in WebKit. This might be a bug in one of them, but it'd be good to clarify the behavior, so that we'd know which needs to be aligned.

Update: clarified that the value is being set async.

/cc @domfarolino @ArthurSonzogni @cdumez

@annevk
Copy link
Member

annevk commented Jul 3, 2024

This should follow from https://html.spec.whatwg.org/multipage/nav-history-apis.html#window-open-steps. If the document associated with the window.open() call is enforcing policy it will typically be null. If it's the document associated with url it can't be null, since we don't know that synchronously.

@domfarolino
Copy link
Member

If it's the document associated with url it can't be null, since we don't know that synchronously.

Right, but I think the question is: is there any mechanism that should set popup.window to null asynchronously (as Chromium does at least) after the browsing context group switch is performed, to ensure that the opener doesn't continue to hold a handle to the new Window loaded in the popup, which now lives in another BCG.

@yoavweiss
Copy link
Collaborator Author

Exactly, the value in Chromium is asynchronously being set to null, once the response is received. I'll clarify that in the OP.

@ArthurSonzogni
Copy link
Member

Exactly, the value in Chromium is asynchronously being set to null, once the response is received. I'll clarify that in the OP.

The opener (original window), the popup's old document (initial content), and the popup's new document (updated content) can run in 3 separate processes. As a result, updating the WindowProxy in the opener must be asynchronous, relative to the two other, no matter the implementation.

An interesting observation made while writing tests is that it's possible for the opener to detect (through external channels) an event triggered by running JS in popup's new document before receiving the .closed event, which signals the removal of the old document. This might seem counterintuitive at first, but it's logical when you consider that the new document starts executing before the opener explicitly acknowledges that the WindowProxy has been updated.

So either WebKit as a different chain of causality (e.g. Waiting for WindowProxy update before creating the new document) or this a difference of degree (WebKit IPCs winning the race ften)

@annevk
Copy link
Member

annevk commented Jul 4, 2024

Wait what? popup.window should always be the same as just popup (assuming popup is a WindowProxy).

And once something is a WindowProxy, you can't just nullify it.

Or maybe you are talking about popup.opener and whether that is equal to window? I guess once url is loaded popup would be closed and that might severe that relationship, although I'm not entirely sure.

@ArthurSonzogni
Copy link
Member

Wait what? popup.window should always be the same as just popup (assuming popup is a WindowProxy).

Chrome implementation for the 3 self referencial attributes (window, self, frames) appears to check the WindowProxy's window still exist before returning itself.
code search

WebKit seems to do the same as Chrome fo self(). (I did not found ::window() and ::frame())
code search
code search 2

Firefox: I did not find information.

@annevk
Copy link
Member

annevk commented Jul 8, 2024

That goes against the specification though. And it's not immediately clear to me what the advantage of that is given that the code already has a reference anyway.

@ArthurSonzogni
Copy link
Member

That goes against the specification though. And it's not immediately clear to me what the advantage of that is given that the code already has a reference anyway.

I briefly reviewed the situation. The current WebKit behavior (which Chromium inherited) seems to be a long-standing issue. Yuki attempted to address it in late 2016, but the changes were reverted in early 2017.

Adding wpt tests would likely help ensure the behavior aligns with the specification going forward.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 9, 2024
Given the fact that the null check in COOP noopener's WPTs is
non-standard [1] and flaky [2], it's better to remove it for now.

[1] whatwg/html#10457
[2] #46979 (comment)

Bug: 344963946
Change-Id: I0bc0e7d153cc400938079a9d12209f57ea8310fc
aarongable pushed a commit to chromium/chromium that referenced this issue Jul 9, 2024
Given the fact that the null check in COOP noopener's WPTs is
non-standard [1] and flaky [2], it's better to remove it for now.

[1] whatwg/html#10457
[2] web-platform-tests/wpt#46979 (comment)

Bug: 344963946
Change-Id: I0bc0e7d153cc400938079a9d12209f57ea8310fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5687771
Commit-Queue: Yoav Weiss (@Shopify) <yoavweiss@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1324852}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 9, 2024
Given the fact that the null check in COOP noopener's WPTs is
non-standard [1] and flaky [2], it's better to remove it for now.

[1] whatwg/html#10457
[2] #46979 (comment)

Bug: 344963946
Change-Id: I0bc0e7d153cc400938079a9d12209f57ea8310fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5687771
Commit-Queue: Yoav Weiss (@Shopify) <yoavweiss@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1324852}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants