[Site Isolation] Frame removal notifications can complete before the …

…frame is removed https://bugs.webkit.org/show_bug.cgi?id=271666 rdar://125367065 Reviewed by Alex Christensen. When we broadcast the removal of a frame, it's possible to receive a `DidDestroyFrame` message from a process we are notifying about the frame's removal before receiving it from the process that actually initiated the removal. To avoid this race, we should not broadcast frame removal until the frame has actually been destroyed. * LayoutTests/platform/mac-site-isolation/TestExpectations: * Source/WebCore/html/HTMLFrameOwnerElement.cpp: (WebCore::HTMLFrameOwnerElement::disconnectContentFrame): * Source/WebCore/loader/EmptyClients.cpp: (WebCore::EmptyFrameLoaderClient::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebCore/loader/EmptyFrameLoaderClient.h: * Source/WebCore/loader/LocalFrameLoaderClient.h: * Source/WebCore/page/Frame.h: * Source/WebCore/page/LocalFrame.cpp: (WebCore::LocalFrame::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebCore/page/LocalFrame.h: * Source/WebCore/page/RemoteFrame.cpp: (WebCore::RemoteFrame::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebCore/page/RemoteFrame.h: * Source/WebCore/page/RemoteFrameClient.h: * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::didDestroyFrame): (WebKit::WebPageProxy::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebKit/UIProcess/WebPageProxy.h: * Source/WebKit/UIProcess/WebPageProxy.messages.in: * Source/WebKit/UIProcess/WebProcessProxy.cpp: (WebKit::WebProcessProxy::didDestroyFrame): * Source/WebKit/UIProcess/WebProcessProxy.h: * Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp: (WebKit::WebFrameLoaderClient::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h: * Source/WebKit/WebProcess/WebCoreSupport/WebLocalFrameLoaderClient.cpp: (WebKit::WebLocalFrameLoaderClient::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebKit/WebProcess/WebCoreSupport/WebLocalFrameLoaderClient.h: * Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.cpp: (WebKit::WebRemoteFrameClient::broadcastFrameRemovalToOtherProcesses): Deleted. * Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.h: * Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h: Canonical link: https://commits.webkit.org/276665@main
WebKit · Mar 26, 2024 · ef59cf8 · ef59cf8
1 parent e7c3172
commit ef59cf8
Show file tree

Hide file tree

Showing 23 changed files with 11 additions and 61 deletions.
diff --git a/LayoutTests/platform/mac-site-isolation/TestExpectations b/LayoutTests/platform/mac-site-isolation/TestExpectations
@@ -210,7 +210,6 @@ http/tests/security/cross-origin-indexeddb-allowed.html [ Skip ]
 http/tests/security/cross-origin-reified-window-location-setting.html [ Skip ]
 http/tests/security/cross-origin-session-storage-third-party-blocked.html [ Skip ]
 http/tests/security/cross-origin-worker-indexeddb-allowed.html [ Skip ]
-http/tests/security/data-url-host.html [ Skip ]
 http/tests/security/detached-sandboxed-frame-access.html [ Skip ]
 http/tests/security/location-cross-origin.html [ Skip ]
 http/tests/security/service-worker-network-load.html [ Skip ]

diff --git a/Source/WebCore/html/HTMLFrameOwnerElement.cpp b/Source/WebCore/html/HTMLFrameOwnerElement.cpp
@@ -78,8 +78,6 @@ void HTMLFrameOwnerElement::clearContentFrame()
 void HTMLFrameOwnerElement::disconnectContentFrame()
 {
     if (RefPtr frame = m_contentFrame.get()) {
-        if (frame->settings().siteIsolationEnabled())
-            frame->broadcastFrameRemovalToOtherProcesses();
         frame->frameDetached();
         frame->disconnectOwnerElement();
     }

diff --git a/Source/WebCore/loader/EmptyClients.cpp b/Source/WebCore/loader/EmptyClients.cpp
@@ -606,10 +606,6 @@ void EmptyFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navig
 {
 }
 
-void EmptyFrameLoaderClient::broadcastFrameRemovalToOtherProcesses()
-{
-}
-
 void EmptyFrameLoaderClient::broadcastMainFrameURLChangeToOtherProcesses(const URL&)
 {
 }

diff --git a/Source/WebCore/loader/EmptyFrameLoaderClient.h b/Source/WebCore/loader/EmptyFrameLoaderClient.h
@@ -96,7 +96,6 @@ class WEBCORE_EXPORT EmptyFrameLoaderClient : public LocalFrameLoaderClient {
     void dispatchDecidePolicyForResponse(const ResourceResponse&, const ResourceRequest&, const String&, FramePolicyFunction&&) final;
     void dispatchDecidePolicyForNewWindowAction(const NavigationAction&, const ResourceRequest&, FormState*, const String&, std::optional<HitTestResult>&&, FramePolicyFunction&&) final;
     void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, const ResourceResponse& redirectResponse, FormState*, const String&, uint64_t, std::optional<HitTestResult>&&, bool, SandboxFlags, PolicyDecisionMode, FramePolicyFunction&&) final;
-    void broadcastFrameRemovalToOtherProcesses() final;
     void broadcastMainFrameURLChangeToOtherProcesses(const URL&) final;
     void cancelPolicyCheck() final;
 

diff --git a/Source/WebCore/loader/LocalFrameLoaderClient.h b/Source/WebCore/loader/LocalFrameLoaderClient.h
@@ -372,7 +372,6 @@ class WEBCORE_EXPORT LocalFrameLoaderClient : public FrameLoaderClient {
     virtual void modelInlinePreviewUUIDs(CompletionHandler<void(Vector<String>)>&&) const { }
 #endif
 
-    virtual void broadcastFrameRemovalToOtherProcesses() = 0;
     virtual void broadcastMainFrameURLChangeToOtherProcesses(const URL&) = 0;
 
     virtual void dispatchLoadEventToOwnerElementInAnotherProcess() = 0;

diff --git a/Source/WebCore/page/Frame.h b/Source/WebCore/page/Frame.h
@@ -83,7 +83,6 @@ class Frame : public ThreadSafeRefCounted<Frame, WTF::DestructionThread::Main>,
     virtual void frameDetached() = 0;
     virtual bool preventsParentFromBeingComplete() const = 0;
     virtual void changeLocation(FrameLoadRequest&&) = 0;
-    virtual void broadcastFrameRemovalToOtherProcesses() = 0;
     virtual void didFinishLoadInAnotherProcess() = 0;
 
     virtual FrameView* virtualView() const = 0;

diff --git a/Source/WebCore/page/LocalFrame.cpp b/Source/WebCore/page/LocalFrame.cpp
@@ -364,11 +364,6 @@ void LocalFrame::changeLocation(FrameLoadRequest&& request)
     checkedLoader()->changeLocation(WTFMove(request));
 }
 
-void LocalFrame::broadcastFrameRemovalToOtherProcesses()
-{
-    checkedLoader()->client().broadcastFrameRemovalToOtherProcesses();
-}
-
 void LocalFrame::didFinishLoadInAnotherProcess()
 {
     checkedLoader()->provisionalLoadFailedInAnotherProcess();

diff --git a/Source/WebCore/page/LocalFrame.h b/Source/WebCore/page/LocalFrame.h
@@ -327,7 +327,6 @@ class LocalFrame final : public Frame {
     void frameDetached() final;
     bool preventsParentFromBeingComplete() const final;
     void changeLocation(FrameLoadRequest&&) final;
-    void broadcastFrameRemovalToOtherProcesses() final;
     void didFinishLoadInAnotherProcess() final;
 
     FrameView* virtualView() const final;

diff --git a/Source/WebCore/page/RemoteFrame.cpp b/Source/WebCore/page/RemoteFrame.cpp
@@ -95,11 +95,6 @@ void RemoteFrame::changeLocation(FrameLoadRequest&& request)
     m_client->changeLocation(WTFMove(request));
 }
 
-void RemoteFrame::broadcastFrameRemovalToOtherProcesses()
-{
-    m_client->broadcastFrameRemovalToOtherProcesses();
-}
-
 void RemoteFrame::updateRemoteFrameAccessibilityOffset(IntPoint offset)
 {
     m_client->updateRemoteFrameAccessibilityOffset(frameID(), offset);

diff --git a/Source/WebCore/page/RemoteFrame.h b/Source/WebCore/page/RemoteFrame.h
@@ -78,7 +78,6 @@ class RemoteFrame final : public Frame {
     void frameDetached() final;
     bool preventsParentFromBeingComplete() const final;
     void changeLocation(FrameLoadRequest&&) final;
-    void broadcastFrameRemovalToOtherProcesses() final;
     void didFinishLoadInAnotherProcess() final;
     bool isRootFrame() const final { return false; }
 

diff --git a/Source/WebCore/page/RemoteFrameClient.h b/Source/WebCore/page/RemoteFrameClient.h
@@ -47,7 +47,6 @@ class RemoteFrameClient : public FrameLoaderClient {
     virtual void postMessageToRemote(FrameIdentifier source, const String& sourceOrigin, FrameIdentifier target, std::optional<SecurityOriginData> targetOrigin, const MessageWithMessagePorts&) = 0;
     virtual void changeLocation(FrameLoadRequest&&) = 0;
     virtual String renderTreeAsText(size_t baseIndent, OptionSet<RenderAsTextFlag>) = 0;
-    virtual void broadcastFrameRemovalToOtherProcesses() = 0;
     virtual void closePage() = 0;
     virtual void bindRemoteAccessibilityFrames(int processIdentifier, FrameIdentifier target, const std::span<const uint8_t> dataToken, CompletionHandler<void(const std::span<const uint8_t>, int)>&&) = 0;
     virtual void updateRemoteFrameAccessibilityOffset(FrameIdentifier target, IntPoint) = 0;

diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
@@ -5760,7 +5760,7 @@ void WebPageProxy::didCreateSubframe(WebCore::FrameIdentifier parentID, WebCore:
     parent->didCreateSubframe(newFrameID, frameName);
 }
 
-void WebPageProxy::didDestroyFrame(FrameIdentifier frameID)
+void WebPageProxy::didDestroyFrame(IPC::Connection& connection, FrameIdentifier frameID)
 {
 #if ENABLE(WEB_AUTHN)
     protectedWebsiteDataStore()->authenticatorManager().cancelRequest(webPageID(), frameID);
@@ -5769,6 +5769,12 @@ void WebPageProxy::didDestroyFrame(FrameIdentifier frameID)
         automationSession->didDestroyFrame(frameID);
     if (RefPtr frame = WebFrameProxy::webFrame(frameID))
         frame->disconnect();
+
+    forEachWebContentProcess([&](auto& webProcess, auto pageID) {
+        if (!webProcess.hasConnection() || webProcess.connection() == &connection)
+            return;
+        webProcess.send(Messages::WebPage::FrameWasRemovedInAnotherProcess(frameID), pageID);
+    });
 }
 
 void WebPageProxy::disconnectFramesFromPage()
@@ -6383,15 +6389,6 @@ void WebPageProxy::createRemoteSubframesInOtherProcesses(WebFrameProxy& newFrame
     });
 }
 
-void WebPageProxy::broadcastFrameRemovalToOtherProcesses(IPC::Connection& connection, WebCore::FrameIdentifier frameID)
-{
-    forEachWebContentProcess([&](auto& webProcess, auto pageID) {
-        if (!webProcess.hasConnection() || webProcess.connection() == &connection)
-            return;
-        webProcess.send(Messages::WebPage::FrameWasRemovedInAnotherProcess(frameID), pageID);
-    });
-}
-
 void WebPageProxy::broadcastMainFrameURLChangeToOtherProcesses(IPC::Connection& connection, const URL& newURL)
 {
     forEachWebContentProcess([&](auto& webProcess, auto pageID) {

diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
@@ -2263,7 +2263,6 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
     WebProcessProxy* processForRegistrableDomain(const WebCore::RegistrableDomain&);
 
     void createRemoteSubframesInOtherProcesses(WebFrameProxy&, const String& frameName);
-    void broadcastFrameRemovalToOtherProcesses(IPC::Connection&, WebCore::FrameIdentifier);
     void broadcastMainFrameURLChangeToOtherProcesses(IPC::Connection&, const URL&);
 
     void addOpenedPage(WebPageProxy&);
@@ -2321,7 +2320,7 @@ class WebPageProxy final : public API::ObjectImpl<API::Object::Type::Page>, publ
     void generateTestReport(const String& message, const String& group);
 
     void frameCreated(WebCore::FrameIdentifier, WebFrameProxy&);
-    void didDestroyFrame(WebCore::FrameIdentifier);
+    void didDestroyFrame(IPC::Connection&, WebCore::FrameIdentifier);
     void disconnectFramesFromPage();
 
     void didCommitLoadForFrame(WebCore::FrameIdentifier, FrameInfoData&&, WebCore::ResourceRequest&&, uint64_t navigationID, const String& mimeType, bool frameHasCustomContentProvider, WebCore::FrameLoadType, const WebCore::CertificateInfo&, bool usedLegacyTLS, bool wasPrivateRelayed, bool containsPluginDocument, WebCore::HasInsecureContent, WebCore::MouseEventPolicy, const UserData&);

diff --git a/Source/WebKit/UIProcess/WebPageProxy.messages.in b/Source/WebKit/UIProcess/WebPageProxy.messages.in
@@ -623,7 +623,6 @@ messages -> WebPageProxy {
 #endif
 
     DidApplyLinkDecorationFiltering(URL originalURL, URL adjustedURL)
-    BroadcastFrameRemovalToOtherProcesses(WebCore::FrameIdentifier frameID)
     BroadcastMainFrameURLChangeToOtherProcesses(URL newURL)
 
     DispatchLoadEventToFrameOwnerElement(WebCore::FrameIdentifier frameID)

diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
@@ -1351,10 +1351,10 @@ void WebProcessProxy::didFinishLaunching(ProcessLauncher* launcher, IPC::Connect
     beginResponsivenessChecks();
 }
 
-void WebProcessProxy::didDestroyFrame(WebCore::FrameIdentifier frameID, WebPageProxyIdentifier pageID)
+void WebProcessProxy::didDestroyFrame(IPC::Connection& connection, FrameIdentifier frameID, WebPageProxyIdentifier pageID)
 {
     if (RefPtr page = m_pageMap.get(pageID))
-        page->didDestroyFrame(frameID);
+        page->didDestroyFrame(connection, frameID);
 }
 
 auto WebProcessProxy::visiblePageToken() const -> VisibleWebPageToken

diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
@@ -554,7 +554,7 @@ class WebProcessProxy : public AuxiliaryProcessProxy {
 
     // IPC message handlers.
     void updateBackForwardItem(const BackForwardListItemState&);
-    void didDestroyFrame(WebCore::FrameIdentifier, WebPageProxyIdentifier);
+    void didDestroyFrame(IPC::Connection&, WebCore::FrameIdentifier, WebPageProxyIdentifier);
     void didDestroyUserGestureToken(WebCore::PageIdentifier, uint64_t);
 
     bool canBeAddedToWebProcessCache() const;

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
@@ -200,12 +200,4 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
     });
 }
 
-void WebFrameLoaderClient::broadcastFrameRemovalToOtherProcesses()
-{
-    auto* webPage = m_frame->page();
-    if (!webPage)
-        return;
-    webPage->send(Messages::WebPageProxy::BroadcastFrameRemovalToOtherProcesses(m_frame->frameID()));
-}
-
 }
diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
@@ -60,7 +60,6 @@ class WebFrameLoaderClient {
     WebFrameLoaderClient(Ref<WebFrame>&&);
 
     void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, const WebCore::ResourceResponse& redirectResponse, WebCore::FormState*, const String&, uint64_t, std::optional<WebCore::HitTestResult>&&, bool, WebCore::SandboxFlags, WebCore::PolicyDecisionMode, WebCore::FramePolicyFunction&&);
-    void broadcastFrameRemovalToOtherProcesses();
 
     Ref<WebFrame> m_frame;
 };

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebLocalFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebLocalFrameLoaderClient.cpp
@@ -1941,11 +1941,6 @@ void WebLocalFrameLoaderClient::getLoadDecisionForIcons(const Vector<std::pair<W
         webPage->send(Messages::WebPageProxy::GetLoadDecisionForIcon(icon.first, CallbackID::fromInteger(icon.second)));
 }
 
-void WebLocalFrameLoaderClient::broadcastFrameRemovalToOtherProcesses()
-{
-    WebFrameLoaderClient::broadcastFrameRemovalToOtherProcesses();
-}
-
 void WebLocalFrameLoaderClient::broadcastMainFrameURLChangeToOtherProcesses(const URL& url)
 {
     RefPtr webPage = m_frame->page();

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebLocalFrameLoaderClient.h b/Source/WebKit/WebProcess/WebCoreSupport/WebLocalFrameLoaderClient.h
@@ -276,7 +276,6 @@ class WebLocalFrameLoaderClient final : public WebCore::LocalFrameLoaderClient,
 
     inline bool hasPlugInView() const;
 
-    void broadcastFrameRemovalToOtherProcesses() final;
     void broadcastMainFrameURLChangeToOtherProcesses(const URL&) final;
 
     void documentLoaderDetached(uint64_t navigationID, WebCore::LoadWillContinueInAnotherProcess) final;

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.cpp
@@ -134,11 +134,6 @@ void WebRemoteFrameClient::bindRemoteAccessibilityFrames(int processIdentifier,
     completionHandler(resultToken, processIdentifierResult);
 }
 
-void WebRemoteFrameClient::broadcastFrameRemovalToOtherProcesses()
-{
-    WebFrameLoaderClient::broadcastFrameRemovalToOtherProcesses();
-}
-
 void WebRemoteFrameClient::closePage()
 {
     if (auto* page = m_frame->page())

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.h b/Source/WebKit/WebProcess/WebCoreSupport/WebRemoteFrameClient.h
@@ -53,7 +53,6 @@ class WebRemoteFrameClient final : public WebCore::RemoteFrameClient, public Web
     void unbindRemoteAccessibilityFrames(int) final;
     void updateRemoteFrameAccessibilityOffset(WebCore::FrameIdentifier, WebCore::IntPoint) final;
 
-    void broadcastFrameRemovalToOtherProcesses() final;
     void closePage() final;
     void focus() final;
     void unfocus() final;

diff --git a/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h b/Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
@@ -130,7 +130,6 @@ class WebFrameLoaderClient : public WebCore::LocalFrameLoaderClient, public CanM
     void dispatchDecidePolicyForResponse(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, const String&, WebCore::FramePolicyFunction&&) final;
     void dispatchDecidePolicyForNewWindowAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, WebCore::FormState*, const WTF::String& frameName, std::optional<WebCore::HitTestResult>&&, WebCore::FramePolicyFunction&&) final;
     void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, const WebCore::ResourceResponse& redirectResponse, WebCore::FormState*, const String&, uint64_t, std::optional<WebCore::HitTestResult>&&, bool, WebCore::SandboxFlags, WebCore::PolicyDecisionMode, WebCore::FramePolicyFunction&&) final;
-    void broadcastFrameRemovalToOtherProcesses() final { }
     void broadcastMainFrameURLChangeToOtherProcesses(const URL&) final { }
     void cancelPolicyCheck() final;