[Site isolation] popup opener should be able to navigate to popup domain

https://bugs.webkit.org/show_bug.cgi?id=267752
rdar://121241095

Reviewed by Charlie Wolfe.

This is the first piece needed to get http/tests/security/frameNavigation/cross-origin-opener.html
running with --site-isolation without crashing.  It turns out we had written a lot of code around
ProvisionalPageProxy::initializeWebPage that only happened if there was an opener, and it needs to
happen sometimes if there isn't an opener.  This is a step towards making that code more robust.
It's not quite there, but it's a step closer than it was and covered by a unit test.

No change in behavior when site isolation is off.

* Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::ProvisionalPageProxy):
(WebKit::ProvisionalPageProxy::initializeWebPage):
(WebKit::ProvisionalPageProxy::didStartProvisionalLoadForFrame):
* Source/WebKit/UIProcess/ProvisionalPageProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::takeOpenedRemotePageProxyIfDomainEquals):
* Source/WebKit/UIProcess/WebPageProxy.h:
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm:
(TestWebKitAPI::TEST):

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

Source/WebKit/UIProcess/ProvisionalPageProxy.cpp

@@ -111,8 +111,6 @@ ProvisionalPageProxy::ProvisionalPageProxy(WebPageProxy& page, Ref<WebProcessPro
     }
 
     initializeWebPage(websitePolicies);
-
-    m_page->inspectorController().didCreateProvisionalPage(*this);
 }
 
 ProvisionalPageProxy::~ProvisionalPageProxy()
@@ -198,36 +196,44 @@ void ProvisionalPageProxy::initializeWebPage(RefPtr<API::WebsitePolicies>&& webs
     m_drawingArea = m_page->pageClient().createDrawingAreaProxy(m_process.copyRef());
 
     bool sendPageCreationParameters { true };
+    bool registerWithInspectorController { true };
     auto parameters = m_page->creationParameters(m_process, *m_drawingArea, WTFMove(websitePolicies));
 
     if (page().preferences().processSwapOnCrossSiteWindowOpenEnabled() || page().preferences().siteIsolationEnabled()) {
+        // FIXME: <rdar://121241026> This is not elegant and not robust. It needs to be.
         RegistrableDomain navigationDomain(m_request.url());
         RefPtr openerFrame = m_page->openerFrame();
-        if (RefPtr openerPage = openerFrame ? openerFrame->page() : nullptr) {
+        RefPtr openerPage = openerFrame ? openerFrame->page() : nullptr;
+        if (openerFrame) {
             parameters.openerFrameIdentifier = openerFrame->frameID();
             parameters.mainFrameIdentifier = m_page->mainFrame()->frameID();
-
-            if (auto existingRemotePageProxy = m_page->takeRemotePageProxyInOpenerProcessIfDomainEquals(navigationDomain)) {
-                ASSERT(existingRemotePageProxy->process().processID() == m_process->processID());
-                m_webPageID = existingRemotePageProxy->pageID();
-                m_mainFrame = existingRemotePageProxy->page()->mainFrame();
-                m_messageReceiverRegistration.stopReceivingMessages();
-                m_messageReceiverRegistration.transferMessageReceivingFrom(existingRemotePageProxy->messageReceiverRegistration(), *this);
-                LocalFrameCreationParameters localFrameCreationParameters {
-                    std::nullopt
-                };
-                m_process->send(Messages::WebPage::TransitionFrameToLocal(localFrameCreationParameters, m_page->mainFrame()->frameID()), m_webPageID);
-                sendPageCreationParameters = false;
-            } else if (RefPtr existingRemotePageProxy = openerPage->remotePageProxyForRegistrableDomain(navigationDomain)) {
-                ASSERT(existingRemotePageProxy->process().processID() == m_process->processID());
-                openerPage->addOpenedRemotePageProxy(m_page->identifier(), existingRemotePageProxy.releaseNonNull());
-                m_needsCookieAccessAddedInNetworkProcess = true;
-            } else {
-                auto remotePageProxy = RemotePageProxy::create(*openerPage, m_process, navigationDomain);
-                remotePageProxy->injectPageIntoNewProcess();
-                openerPage->addOpenedRemotePageProxy(m_page->identifier(), WTFMove(remotePageProxy));
-            }
         }
+        auto existingRemotePageProxy = m_page->takeRemotePageProxyInOpenerProcessIfDomainEquals(navigationDomain);
+        if (!existingRemotePageProxy) {
+            if ((existingRemotePageProxy = m_page->takeOpenedRemotePageProxyIfDomainEquals(navigationDomain)))
+                registerWithInspectorController = false; // FIXME: <rdar://121240770> This is a hack. There seems to be a bug in our interaction with WebPageInspectorController.
+        }
+        if (existingRemotePageProxy) {
+            ASSERT(existingRemotePageProxy->process().processID() == m_process->processID());
+            m_webPageID = existingRemotePageProxy->pageID();
+            m_mainFrame = existingRemotePageProxy->page()->mainFrame();
+            m_messageReceiverRegistration.stopReceivingMessages();
+            m_messageReceiverRegistration.transferMessageReceivingFrom(existingRemotePageProxy->messageReceiverRegistration(), *this);
+            LocalFrameCreationParameters localFrameCreationParameters {
+                std::nullopt
+            };
+            m_process->send(Messages::WebPage::TransitionFrameToLocal(localFrameCreationParameters, m_page->mainFrame()->frameID()), m_webPageID);
+            sendPageCreationParameters = false;
+        } else if (RefPtr existingRemotePageProxy = openerPage ? openerPage->remotePageProxyForRegistrableDomain(navigationDomain) : nullptr) {
+            ASSERT(existingRemotePageProxy->process().processID() == m_process->processID());
+            openerPage->addOpenedRemotePageProxy(m_page->identifier(), existingRemotePageProxy.releaseNonNull());
+            m_needsCookieAccessAddedInNetworkProcess = true;
+        } else if (openerPage) {
+            auto remotePageProxy = RemotePageProxy::create(*openerPage, m_process, navigationDomain);
+            remotePageProxy->injectPageIntoNewProcess();
+            openerPage->addOpenedRemotePageProxy(m_page->identifier(), WTFMove(remotePageProxy));
+        }
+        m_needsDidStartProvisionalLoad = false;
     }
 
     parameters.isProcessSwap = true;
@@ -238,6 +244,9 @@ void ProvisionalPageProxy::initializeWebPage(RefPtr<API::WebsitePolicies>&& webs
 
     if (m_page->isLayerTreeFrozenDueToSwipeAnimation())
         send(Messages::WebPage::FreezeLayerTreeDueToSwipeAnimation());
+
+    if (registerWithInspectorController)
+        m_page->inspectorController().didCreateProvisionalPage(*this);
 }
 
 void ProvisionalPageProxy::loadData(API::Navigation& navigation, const IPC::DataReference& data, const String& mimeType, const String& encoding, const String& baseURL, API::Object* userData, WebCore::ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad, std::optional<NavigatingToAppBoundDomain> isNavigatingToAppBoundDomain, std::optional<WebsitePoliciesData>&& websitePolicies, SubstituteData::SessionHistoryVisibility sessionHistoryVisibility)
@@ -368,11 +377,8 @@ void ProvisionalPageProxy::didStartProvisionalLoadForFrame(FrameIdentifier frame
     if (m_isServerRedirect)
         return;
 
-    // When this is true, we are reusing the main WebFrameProxy and its frame state will be updated in didStartProvisionalLoadForFrameShared.
-    bool isCrossSiteWindowOpen = m_page->openerFrame() && (page().preferences().processSwapOnCrossSiteWindowOpenEnabled() || page().preferences().siteIsolationEnabled());
-
     // Clients expect the Page's main frame's expectedURL to be the provisional one when a provisional load is started.
-    if (auto* pageMainFrame = m_page->mainFrame(); pageMainFrame && !isCrossSiteWindowOpen)
+    if (auto* pageMainFrame = m_page->mainFrame(); pageMainFrame && m_needsDidStartProvisionalLoad)
         pageMainFrame->didStartProvisionalLoad(url);
 
     m_page->didStartProvisionalLoadForFrameShared(m_process.copyRef(), frameID, WTFMove(frameInfo), WTFMove(request), navigationID, WTFMove(url), WTFMove(unreachableURL), userData);

Source/WebKit/UIProcess/ProvisionalPageProxy.h

@@ -199,6 +199,7 @@ class ProvisionalPageProxy : public IPC::MessageReceiver, public IPC::MessageSen
     bool m_wasCommitted { false };
     bool m_isProcessSwappingOnNavigationResponse { false };
     bool m_needsCookieAccessAddedInNetworkProcess { false };
+    bool m_needsDidStartProvisionalLoad { true };
     URL m_provisionalLoadURL;
     WebPageProxyMessageReceiverRegistration m_messageReceiverRegistration;
 

Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -1233,6 +1233,9 @@ void WebPageProxy::swapToProvisionalPage(std::unique_ptr<ProvisionalPageProxy> p
     ASSERT(!m_isClosed);
     WEBPAGEPROXY_RELEASE_LOG(Loading, "swapToProvisionalPage: newWebPageID=%" PRIu64, provisionalPage->webPageID().toUInt64());
 
+    for (auto& openedPage : internals().m_openedPages)
+        openedPage.internals().remotePageProxyState.remotePageProxyInOpenerProcess = nullptr;
+
     m_process = provisionalPage->process();
     internals().webPageID = provisionalPage->webPageID();
     pageClient().didChangeWebPageID();
@@ -13135,6 +13138,20 @@ void WebPageProxy::removeOpenedRemotePageProxy(WebPageProxyIdentifier pageID)
     internals().remotePageProxyState.openedRemotePageProxies.remove(pageID);
 }
 
+RefPtr<RemotePageProxy> WebPageProxy::takeOpenedRemotePageProxyIfDomainEquals(const WebCore::RegistrableDomain& domain)
+{
+    auto& map = internals().remotePageProxyState.openedRemotePageProxies;
+    // FIXME: <rdar://121240859> Use better data structures so we don't need to iterate the whole map.
+    for (auto it = map.begin(); it != map.end(); ++it) {
+        Ref remotePageProxy = it->value;
+        if (remotePageProxy->domain() == domain) {
+            map.remove(it);
+            return { WTFMove(remotePageProxy) };
+        }
+    }
+    return nullptr;
+}
+
 void WebPageProxy::addOpenedRemotePageProxy(WebPageProxyIdentifier pageID, Ref<RemotePageProxy>&& page)
 {
     // FIXME: When <rdar://116203552> is fixed we should be able to assert that the add result is a

Source/WebKit/UIProcess/WebPageProxy.h

@@ -2199,6 +2199,7 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
     void removeRemotePageProxy(const WebCore::RegistrableDomain&);
     void setRemotePageProxyInOpenerProcess(Ref<RemotePageProxy>&&);
     RefPtr<RemotePageProxy> takeRemotePageProxyInOpenerProcessIfDomainEquals(const WebCore::RegistrableDomain&);
+    RefPtr<RemotePageProxy> takeOpenedRemotePageProxyIfDomainEquals(const WebCore::RegistrableDomain&);
     void addOpenedRemotePageProxy(WebPageProxyIdentifier, Ref<RemotePageProxy>&&);
     void removeOpenedRemotePageProxy(WebPageProxyIdentifier);
 

Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm

@@ -1790,4 +1790,22 @@ HTTPServer server({
 }
 #endif
 
+TEST(SiteIsolation, NavigateOpener)
+{
+    HTTPServer server({
+        { "/example"_s, { "<script>w = window.open('https://webkit.org/webkit')</script>"_s } },
+        { "/webkit"_s, { "hi"_s } },
+        { "/webkit2"_s, { "hi"_s } }
+    }, HTTPServer::Protocol::HttpsProxy);
+
+    auto [opener, opened] = openerAndOpenedViews(server);
+    [opened.webView evaluateJavaScript:@"opener.location = '/webkit2'" completionHandler:nil];
+    [opener.navigationDelegate waitForDidFinishNavigation];
+    EXPECT_EQ(opened.webView.get()._webProcessIdentifier, opener.webView.get()._webProcessIdentifier);
+    checkFrameTreesInProcesses(opener.webView.get(), { { "https://webkit.org"_s } });
+    checkFrameTreesInProcesses(opened.webView.get(), { { "https://webkit.org"_s } });
+}
+
+// FIXME: <rdar://121240941> Add tests covering provisional navigation failures in cases like SiteIsolation.NavigateOpener.
+
 }