Skip to content

Commit

Permalink
Use connection instead of mainFrameOrOpenerDomain when deciding what …
Browse files Browse the repository at this point in the history
…process to use for site isolated window.open

https://bugs.webkit.org/show_bug.cgi?id=271280
rdar://125044857

Reviewed by Charlie Wolfe.

This is the first of at least 2 steps to get http/tests/site-isolation/iframe-and-window-open.html to not assert
when run with --site-isolation.  The other is to make WebProcessPool::createWebPage not assume that the opened page's
main frame needs to start in the same process as the opener's main frame process because window.open can be called
from iframes.

* Source/WebKit/UIProcess/BrowsingContextGroup.cpp:
(WebKit::BrowsingContextGroup::processForConnection):
(WebKit::BrowsingContextGroup::addPage):
* Source/WebKit/UIProcess/BrowsingContextGroup.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::didCreateMainFrame):
(WebKit::WebPageProxy::decidePolicyForNavigationActionSync):
(WebKit::WebPageProxy::creationParameters):
(WebKit::WebPageProxy::mainFrameOrOpenerDomain const): Deleted.
* Source/WebKit/UIProcess/WebPageProxy.h:
* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::createWebPage):

Canonical link: https://commits.webkit.org/276384@main
  • Loading branch information
achristensen07 committed Mar 20, 2024
1 parent ac7e2c9 commit cc094b8
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 19 deletions.
15 changes: 14 additions & 1 deletion Source/WebKit/UIProcess/BrowsingContextGroup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,19 @@ Ref<FrameProcess> BrowsingContextGroup::ensureProcessForDomain(const WebCore::Re
return FrameProcess::create(process, *this, domain, preferences);
}

Ref<FrameProcess> BrowsingContextGroup::ensureProcessForConnection(IPC::Connection& connection, WebPageProxy& page, const WebPreferences& preferences)
{
if (preferences.siteIsolationEnabled() || preferences.processSwapOnCrossSiteWindowOpenEnabled()) {
for (auto& process : m_processMap.values()) {
if (!process)
continue;
if (process->process().connection() == &connection)
return *process;
}
}
return FrameProcess::create(page.process(), *this, WebCore::RegistrableDomain(URL(page.currentURL())), preferences);
}

FrameProcess* BrowsingContextGroup::processForDomain(const WebCore::RegistrableDomain& domain)
{
auto process = m_processMap.get(domain);
Expand Down Expand Up @@ -112,7 +125,7 @@ void BrowsingContextGroup::addPage(WebPageProxy& page)
return true;
}

if (domain == page.mainFrameOrOpenerDomain())
if (process->process().coreProcessIdentifier() == page.process().coreProcessIdentifier())
return false;
auto newRemotePage = makeUnique<RemotePageProxy>(page, process->process(), domain);
newRemotePage->injectPageIntoNewProcess();
Expand Down
5 changes: 5 additions & 0 deletions Source/WebKit/UIProcess/BrowsingContextGroup.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
#include <wtf/WeakHashMap.h>
#include <wtf/WeakListHashSet.h>

namespace IPC {
class Connection;
}

namespace WebKit {

class FrameProcess;
Expand All @@ -44,6 +48,7 @@ class BrowsingContextGroup : public RefCounted<BrowsingContextGroup>, public Can
~BrowsingContextGroup();

Ref<FrameProcess> ensureProcessForDomain(const WebCore::RegistrableDomain&, WebProcessProxy&, const WebPreferences&);
Ref<FrameProcess> ensureProcessForConnection(IPC::Connection&, WebPageProxy&, const WebPreferences&);
FrameProcess* processForDomain(const WebCore::RegistrableDomain&);
void addFrameProcess(FrameProcess&);
void removeFrameProcess(FrameProcess&);
Expand Down
20 changes: 5 additions & 15 deletions Source/WebKit/UIProcess/WebPageProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5725,7 +5725,7 @@ void WebPageProxy::preferencesDidChange()
send(Messages::WebPage::PreferencesDidChange(preferencesStore()));
}

void WebPageProxy::didCreateMainFrame(FrameIdentifier frameID)
void WebPageProxy::didCreateMainFrame(IPC::Connection& connection, FrameIdentifier frameID)
{
// The DecidePolicyForNavigationActionSync IPC is synchronous and may therefore get processed before the DidCreateMainFrame one.
// When this happens, decidePolicyForNavigationActionSync() calls didCreateMainFrame() and we need to ignore the DidCreateMainFrame
Expand All @@ -5738,7 +5738,7 @@ void WebPageProxy::didCreateMainFrame(FrameIdentifier frameID)
MESSAGE_CHECK(m_process, !m_mainFrame);
MESSAGE_CHECK(m_process, WebFrameProxy::canCreateFrame(frameID));

Ref mainFrame = WebFrameProxy::create(*this, m_browsingContextGroup->ensureProcessForDomain(mainFrameOrOpenerDomain(), protectedProcess(), preferences()), frameID);
Ref mainFrame = WebFrameProxy::create(*this, m_browsingContextGroup->ensureProcessForConnection(connection, *this, preferences()), frameID);
m_mainFrame = mainFrame.copyRef();
if (m_preferences->siteIsolationEnabled() || m_preferences->processSwapOnCrossSiteWindowOpenEnabled())
m_browsingContextGroup->addPage(*this);
Expand Down Expand Up @@ -7096,14 +7096,14 @@ void WebPageProxy::logFrameNavigation(const WebFrameProxy& frame, const URL& pag
websiteDataStore().protectedNetworkProcess()->send(Messages::NetworkProcess::LogFrameNavigation(m_websiteDataStore->sessionID(), RegistrableDomain { targetURL }, RegistrableDomain { pageURL }, RegistrableDomain { sourceURL }, isRedirect, frame.isMainFrame(), MonotonicTime::now() - internals().didFinishDocumentLoadForMainFrameTimestamp, wasPotentiallyInitiatedByUser), 0);
}

void WebPageProxy::decidePolicyForNavigationActionSync(NavigationActionData&& data, CompletionHandler<void(PolicyDecision&&)>&& reply)
void WebPageProxy::decidePolicyForNavigationActionSync(IPC::Connection& connection, NavigationActionData&& data, CompletionHandler<void(PolicyDecision&&)>&& reply)
{
auto& frameInfo = data.frameInfo;
RefPtr frame = WebFrameProxy::webFrame(frameInfo.frameID);
if (!frame) {
// This synchronous IPC message was processed before the asynchronous DidCreateMainFrame / DidCreateSubframe one so we do not know about this frameID yet.
if (frameInfo.isMainFrame)
didCreateMainFrame(frameInfo.frameID);
didCreateMainFrame(connection, frameInfo.frameID);
else {
MESSAGE_CHECK(m_process, frameInfo.parentFrameID);
RefPtr parentFrame = WebFrameProxy::webFrame(*frameInfo.parentFrameID);
Expand Down Expand Up @@ -9923,6 +9923,7 @@ WebPageCreationParameters WebPageProxy::creationParameters(WebProcessProxy& proc

parameters.processDisplayName = configuration().processDisplayName();

// FIXME: This should be std::optional<SubframeProcessPageParameters>&& instead of SubframeProcessPageParameters*
if (subframeProcessPageParameters)
parameters.subframeProcessPageParameters = *subframeProcessPageParameters;
parameters.openerFrameIdentifier = m_openerFrame ? std::optional(m_openerFrame->frameID()) : std::nullopt;
Expand Down Expand Up @@ -13393,17 +13394,6 @@ WebProcessProxy* WebPageProxy::processForRegistrableDomain(const WebCore::Regist
return process ? &process->process() : nullptr;
}

RegistrableDomain WebPageProxy::mainFrameOrOpenerDomain() const
{
URL currentURL { this->currentURL() };
// After window.open but before a load is started in another process,
// currentURL is about:blank but we can use the opener to tell what
// domain's process the frame was in.
if (currentURL.isEmpty() && m_openerFrame)
currentURL = m_openerFrame->url();
return RegistrableDomain { currentURL };
}

WebPageProxy* WebPageProxy::openerPage() const
{
return m_openerFrame ? m_openerFrame->page() : nullptr;
Expand Down
5 changes: 2 additions & 3 deletions Source/WebKit/UIProcess/WebPageProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -2255,7 +2255,6 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
#endif

WebProcessProxy* processForRegistrableDomain(const WebCore::RegistrableDomain&);
WebCore::RegistrableDomain mainFrameOrOpenerDomain() const;

void createRemoteSubframesInOtherProcesses(WebFrameProxy&, const String& frameName);
void broadcastFrameRemovalToOtherProcesses(IPC::Connection&, WebCore::FrameIdentifier);
Expand Down Expand Up @@ -2455,7 +2454,7 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
void requestPointerLock();
#endif

void didCreateMainFrame(WebCore::FrameIdentifier);
void didCreateMainFrame(IPC::Connection&, WebCore::FrameIdentifier);
void didCreateSubframe(WebCore::FrameIdentifier parent, WebCore::FrameIdentifier newFrameID, const String& frameName);

void didStartProvisionalLoadForFrame(WebCore::FrameIdentifier, FrameInfoData&&, WebCore::ResourceRequest&&, uint64_t navigationID, URL&&, URL&& unreachableURL, const UserData&);
Expand Down Expand Up @@ -2489,7 +2488,7 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ

void decidePolicyForNavigationAction(Ref<WebProcessProxy>&&, WebFrameProxy&, NavigationActionData&&, CompletionHandler<void(PolicyDecision&&)>&&);
void decidePolicyForNavigationActionAsync(NavigationActionData&&, CompletionHandler<void(PolicyDecision&&)>&&);
void decidePolicyForNavigationActionSync(NavigationActionData&&, CompletionHandler<void(PolicyDecision&&)>&&);
void decidePolicyForNavigationActionSync(IPC::Connection&, NavigationActionData&&, CompletionHandler<void(PolicyDecision&&)>&&);
void decidePolicyForNewWindowAction(NavigationActionData&&, const String& frameName, CompletionHandler<void(PolicyDecision&&)>&&);
void decidePolicyForResponse(IPC::Connection&, FrameInfoData&&, uint64_t navigationID, const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, bool canShowMIMEType, const String& downloadAttribute, bool isShowingInitialAboutBlank, WebCore::CrossOriginOpenerPolicyValue activeDocumentCOOPValue, CompletionHandler<void(PolicyDecision&&)>&&);
void beginSafeBrowsingCheck(const URL&, bool, WebFramePolicyListenerProxy&);
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/UIProcess/WebProcessPool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1235,6 +1235,7 @@ Ref<WebPageProxy> WebProcessPool::createWebPage(PageClient& pageClient, Ref<API:
RefPtr<WebProcessProxy> process;
auto lockdownMode = pageConfiguration->lockdownModeEnabled() ? WebProcessProxy::LockdownMode::Enabled : WebProcessProxy::LockdownMode::Disabled;
RefPtr relatedPage = pageConfiguration->relatedPage();
// FIXME: If site isolation is enabled, this needs to get the correct process instead of assuming we always want the process of the related page.
if (relatedPage && !relatedPage->isClosed() && relatedPage->hasSameGPUAndNetworkProcessPreferencesAs(pageConfiguration)) {
// Sharing processes, e.g. when creating the page via window.open().
process = &relatedPage->ensureRunningProcess();
Expand Down

0 comments on commit cc094b8

Please sign in to comment.