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

window.open should be able to open a popup with the same domain as an existing site-isolated iframe #20088

Conversation

achristensen07
Copy link
Contributor

@achristensen07 achristensen07 commented Nov 7, 2023

8a0ed5b

window.open should be able to open a popup with the same domain as an existing site-isolated iframe
https://bugs.webkit.org/show_bug.cgi?id=264323
rdar://118015550

Reviewed by Pascoe.

If we already have a RemotePageProxy communicating with a WebPage in the right process, use it instead
of making a new one and getting confused about which one to use.  If we do this, we need to grant that
process cookie access because it may have a new first party.

In addition, WebFrame::documentLoaderDetached was sending WebPageProxy::DidDestroyNavigation which was
destroying the navigation while it was continuing in another process, which caused assertions.  To fix
this, don't send this message if the load is continuing in another process.

* LayoutTests/http/tests/site-isolation/iframe-and-window-open-expected.txt: Added.
* LayoutTests/http/tests/site-isolation/iframe-and-window-open.html: Added.
* LayoutTests/http/tests/site-isolation/resources/post-message-to-opener.html: Added.
* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::initializeWebPage):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
(WebKit::ProvisionalPageProxy::needsCookieAccessAddedInNetworkProcess const):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::continueNavigationInNewProcess):

Canonical link: https://commits.webkit.org/270368@main

786bc4f

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  watch ⏳ πŸ›  jsc-mips
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim ⏳ πŸ§ͺ jsc-mips-tests

@achristensen07 achristensen07 self-assigned this Nov 7, 2023
@achristensen07 achristensen07 added the WebKit Process Model Bugs related to WebKit's multi-process architecture label Nov 7, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 7, 2023
@achristensen07 achristensen07 force-pushed the eng/window-open-should-be-able-to-open-a-popup-with-the-same-domain-as-an-existing-site-isolated-iframe branch from 60f9034 to 31a5f52 Compare November 7, 2023 15:32
@achristensen07 achristensen07 removed the merging-blocked Applied to prevent a change from being merged label Nov 7, 2023
@achristensen07 achristensen07 force-pushed the eng/window-open-should-be-able-to-open-a-popup-with-the-same-domain-as-an-existing-site-isolated-iframe branch from 31a5f52 to 786bc4f Compare November 7, 2023 23:14
@achristensen07 achristensen07 added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 8, 2023
… existing site-isolated iframe

https://bugs.webkit.org/show_bug.cgi?id=264323
rdar://118015550

Reviewed by Pascoe.

If we already have a RemotePageProxy communicating with a WebPage in the right process, use it instead
of making a new one and getting confused about which one to use.  If we do this, we need to grant that
process cookie access because it may have a new first party.

In addition, WebFrame::documentLoaderDetached was sending WebPageProxy::DidDestroyNavigation which was
destroying the navigation while it was continuing in another process, which caused assertions.  To fix
this, don't send this message if the load is continuing in another process.

* LayoutTests/http/tests/site-isolation/iframe-and-window-open-expected.txt: Added.
* LayoutTests/http/tests/site-isolation/iframe-and-window-open.html: Added.
* LayoutTests/http/tests/site-isolation/resources/post-message-to-opener.html: Added.
* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::initializeWebPage):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
(WebKit::ProvisionalPageProxy::needsCookieAccessAddedInNetworkProcess const):
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::continueNavigationInNewProcess):

Canonical link: https://commits.webkit.org/270368@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/window-open-should-be-able-to-open-a-popup-with-the-same-domain-as-an-existing-site-isolated-iframe branch from 786bc4f to 8a0ed5b Compare November 8, 2023 05:04
@webkit-commit-queue
Copy link
Collaborator

Committed 270368@main (8a0ed5b): https://commits.webkit.org/270368@main

Reviewed commits have been landed. Closing PR #20088 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 8a0ed5b into WebKit:main Nov 8, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Process Model Bugs related to WebKit's multi-process architecture
Projects
None yet
5 participants