-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Begin implementing cross-site window.open in a new process #15065
Begin implementing cross-site window.open in a new process #15065
Conversation
EWS run on previous version of this PR (hash 4af069d) |
4af069d
to
25cd8f2
Compare
EWS run on previous version of this PR (hash 25cd8f2) |
25cd8f2
to
928eba2
Compare
EWS run on previous version of this PR (hash 928eba2) |
928eba2
to
61f5b0d
Compare
EWS run on previous version of this PR (hash 61f5b0d) |
61f5b0d
to
cfd18a6
Compare
EWS run on previous version of this PR (hash cfd18a6) |
Source/WebCore/page/Frame.cpp
Outdated
@@ -104,4 +105,12 @@ void Frame::disconnectOwnerElement() | |||
document->frameWasDisconnectedFromOwner(); | |||
} | |||
|
|||
void Frame::takeWindowProxy(Frame& frame) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the naming slightly confusing. Maybe it should be takeWindowProxyFrom()
.
@@ -46,7 +46,7 @@ void FrameLoadState::removeObserver(Observer& observer) | |||
|
|||
void FrameLoadState::didStartProvisionalLoad(const URL& url) | |||
{ | |||
ASSERT(m_provisionalURL.isEmpty()); | |||
ASSERT(m_provisionalURL.isEmpty() || m_provisionalURL == url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we may be papering over a real issue here? We don't want didStartProvisionalLoad to get called several times for the same load. This would be observable by the client app too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look closer. You're probably right
@@ -215,6 +215,8 @@ struct WebPageProxy::Internals final : WebPopupMenuProxy::Client | |||
PageAllowedToRunInTheBackgroundCounter::Token pageAllowedToRunInTheBackgroundToken; | |||
|
|||
HashMap<WebCore::RegistrableDomain, WeakPtr<RemotePageProxy>> domainToRemotePageProxyMap; | |||
RefPtr<RemotePageProxy> remotePageProxyInOpenerProcess; | |||
HashSet<Ref<RemotePageProxy>> openedRemotePageProxies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like we're leaking these objects here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a FIXME comment by WebPageProxy::addOpenedRemotePageProxy
@@ -1390,7 +1390,8 @@ static void enableWindowOpenPSON(WKWebViewConfiguration *configuration) | |||
} | |||
} | |||
|
|||
TEST(ProcessSwap, CrossSiteWindowOpenWithOpener) | |||
// FIXME: Get this working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem OK to regress pre-existing PSON API tests with new feature.
Why is there even a behavior change here? Isn't the new behavior gated by a feature flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two tests are the two existing tests that enable the ProcessSwapOnCrossSiteWindowOpenEnabled feature. That's why they are affected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. No concern then.
@@ -7178,7 +7179,8 @@ static bool hasOverlay(CALayer *layer) | |||
|
|||
static const char* openedPage = "Hello World"; | |||
|
|||
TEST(ProcessSwap, SameSiteWindowWithOpenerNavigateToFile) | |||
// FIXME: Get this working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem OK to regress pre-existing PSON API tests with new feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM though it'd be nice to fix the duplicate calls to didStartProvisionalLoad.
cfd18a6
to
f39ee89
Compare
EWS run on current version of this PR (hash f39ee89) |
https://bugs.webkit.org/show_bug.cgi?id=258231 rdar://110931870 Reviewed by Chris Dumez. This takes some ideas from WebKit#10169 and some of Pascoe's more recent work, rebases on top of significant site isolation architecture changes since then, and cleans them up with a slightly expanded test. The back/forward navigations in the existing API test ProcessSwap.SameSiteWindowWithOpenerNavigateToFile cover some transitions that have not been implemented yet, so I temporarily disable the test until those transitions are implemented. This greatly simplified this PR and allowed me to break the development into more manageable pieces, each of which can be merged with test coverage showing progress, and each of which is covered by a currently off-by-default feature flag. Two important transitions need to happen when implementing window.open in a new process. The first is when the ProvisionalPageProxy is constructed. The main frame already exists in the opener process (and it is showing about:blank unless it has had a same-site URL loaded in it) but when we make a frame in the new process we need that main frame to have an opener frame as a RemoteFrame, so we need to have the opener inject a WebPage with a remote frame tree into the process first. We do this by creating a RemotePageProxy owned by the opener in the process we are provisionally navigating to. The second transition happens when we begin receiving HTTP body for the provisional navigation and the navigation commits. In ProvisionalPageProxy::didCommitLoadForFrame we need to tell the frames of the opened page in the opener process to transition to remote frames (which are still needed to be the targets of operations like postMessage) and we do this by sending the WebPage::DidCommitLoadInAnotherProcess message. We then create a RemotePageProxy for those remote frames to communicate with and set up message receivers. Because the process already has a WebPage and frame tree in it, we separate the new function RemotePageProxy::injectPageIntoNewProcess from what used to be done in each RemotePageProxy constructor. We need to prevent this WebPage from being closed or suspended, which we do in the changes in WebPageProxy.cpp. Since each Page can have 0 or 1 openers, we add a new member to WebPageProxy::Internals named remotePageProxyInOpenerProcess to retain ownership of the RemotePageProxy communicating with frames in that process. Each Page can have an unlimited number of Pages it has opened, so the symmetric other side of that member is a set of openedRemotePageProxies. A few other small changes are needed to hook up remote openers, and make it possible for a main frame to transition between local and remote, which requires changing the main frame of a WebCore::Page. * Source/WebCore/bindings/js/WindowProxy.cpp: (WebCore::WindowProxy::replaceFrame): * Source/WebCore/bindings/js/WindowProxy.h: * Source/WebCore/page/Frame.cpp: (WebCore::Frame::resetWindowProxy): (WebCore::Frame::takeWindowProxy): * Source/WebCore/page/Frame.h: * Source/WebCore/page/Page.cpp: (WebCore::Page::setMainFrame): * Source/WebCore/page/Page.h: * Source/WebCore/page/RemoteFrame.cpp: (WebCore::RemoteFrame::createSubframeWithContentsInAnotherProcess): * Source/WebCore/page/RemoteFrame.h: * Source/WebKit/Shared/FrameInfoData.h: * Source/WebKit/Shared/FrameInfoData.serialization.in: * Source/WebKit/Shared/WebPageCreationParameters.cpp: (WebKit::WebPageCreationParameters::encode const): (WebKit::WebPageCreationParameters::decode): * Source/WebKit/Shared/WebPageCreationParameters.h: * Source/WebKit/UIProcess/API/APIFrameInfo.h: * Source/WebKit/UIProcess/API/APIFrameTreeNode.h: * Source/WebKit/UIProcess/API/Cocoa/WKFrameInfo.mm: (-[WKFrameInfo _processIdentifier]): * Source/WebKit/UIProcess/API/Cocoa/_WKFrameTreeNode.mm: (-[_WKFrameTreeNode _processIdentifier]): * Source/WebKit/UIProcess/FrameLoadState.cpp: (WebKit::FrameLoadState::didStartProvisionalLoad): * Source/WebKit/UIProcess/ProvisionalFrameProxy.cpp: (WebKit::ProvisionalFrameProxy::ProvisionalFrameProxy): * Source/WebKit/UIProcess/ProvisionalPageProxy.cpp: (WebKit::ProvisionalPageProxy::initializeWebPage): (WebKit::ProvisionalPageProxy::didCreateMainFrame): (WebKit::ProvisionalPageProxy::didCommitLoadForFrame): * Source/WebKit/UIProcess/RemotePageProxy.cpp: (WebKit::RemotePageProxy::RemotePageProxy): (WebKit::RemotePageProxy::injectPageIntoNewProcess): (WebKit::RemotePageProxy::~RemotePageProxy): * Source/WebKit/UIProcess/RemotePageProxy.h: (WebKit::RemotePageProxy::pageID const): (WebKit::RemotePageProxy::domain const): * Source/WebKit/UIProcess/WebFrameProxy.h: (WebKit::WebFrameProxy::setProcess): * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::suspendCurrentPageIfPossible): (WebKit::WebPageProxy::commitProvisionalPage): (WebKit::WebPageProxy::shouldClosePreviousPage): (WebKit::WebPageProxy::continueNavigationInNewProcess): (WebKit::WebPageProxy::getAllFrameTrees): (WebKit::WebPageProxy::createNewPage): (WebKit::WebPageProxy::addRemotePageProxy): (WebKit::WebPageProxy::setRemotePageProxyInOpenerProcess): (WebKit::WebPageProxy::addOpenedRemotePageProxy): * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/UIProcess/WebPageProxyInternals.h: * Source/WebKit/UIProcess/WebProcessPool.cpp: (WebKit::WebProcessPool::processForNavigation): * Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.cpp: (WebKit::WebRemoteFrameClient::changeLocation): * Source/WebKit/WebProcess/WebPage/WebFrame.cpp: (WebKit::WebFrame::info const): (WebKit::WebFrame::didCommitLoadInAnotherProcess): (WebKit::WebFrame::transitionToLocal): * Source/WebKit/WebProcess/WebPage/WebFrame.h: * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::WebPage): (WebKit::m_appHighlightsVisible): (WebKit::WebPage::didCommitLoadInAnotherProcess): (WebKit::WebPage::transitionFrameToLocal): (WebKit::WebPage::transitionFrameToLocalAndLoadRequest): Deleted. * Source/WebKit/WebProcess/WebPage/WebPage.h: * Source/WebKit/WebProcess/WebPage/WebPage.messages.in: Canonical link: https://commits.webkit.org/265321@main
f39ee89
to
9bcdeaa
Compare
Committed 265321@main (9bcdeaa): https://commits.webkit.org/265321@main Reviewed commits have been landed. Closing PR #15065 and removing active labels. |
9bcdeaa
f39ee89