[Site Isolation] Handle more cases of provisional navigations cancell…

…ing other provisional navigations

https://bugs.webkit.org/show_bug.cgi?id=274179
rdar://128083411

Reviewed by Sihui Liu.

In order to get more tests to work correctly, a few changes were needed:

1. The accounting for whether a frame identifier is a root frame identifier needed to be more robust
for the assertion to be correct near the time when a root frame was transitioning to or from being local.
I introduced the debug-only class FrameLifetimeVerifier for this.  Instead of keeping track of just the
most recently created Frame with a given identifier, keep track of both the most recently created LocalFrame
and the most recently created RemoteFrame.  Then we can strengthen our assertions to make sure that
Frame existence is exactly as we expect: there should only be one except during the time of transitioning
from one type to the other.

2. If Frame::detachFromPage is ever called before destruction of a Frame, then we would be unable to
inform the Page of the destruction of a root frame.  To make this work, move that informing to
Frame::detachFromPage and have the root frame collections contain all the root frames that are attached
to a Page instead of all the root frames that haven't been destroyed.

3. WebFrameProxy::prepareForProvisionalLoadInProcess was missing a case when there is a ProvisionalFrameProxy
but it's from a provisional navigation to a different process that we are now cancelling.  In that case,
we do need to make a new ProvisionalFrameProxy representing the navigation to the new process.

4. WebFrame::loadDidCommitInAnotherProcess was missing a call to FrameLoader::detachFromParent which is needed
to drop all strong references to the LocalFrame so it can be destroyed so we don't have multiple LocalFrames
in the same process with the same identifier.

I also updated checkFrameTreesInProcesses to copy the expected results so they print out correctly if
they don't match the actual results in a place in the frame tree that is not the beginning.

* Source/WebCore/page/Frame.cpp:
(WebCore::FrameLifetimeVerifier::singleton):
(WebCore::FrameLifetimeVerifier::frameCreated):
(WebCore::FrameLifetimeVerifier::frameDestroyed):
(WebCore::FrameLifetimeVerifier::isRootFrameIdentifier):
(WebCore::Frame::Frame):
(WebCore::Frame::~Frame):
(WebCore::Frame::detachFromPage):
(WebCore::Frame::isRootFrameIdentifier):
(WebCore::allFrames): Deleted.
* Source/WebCore/page/Frame.h:
* Source/WebCore/page/LocalFrame.cpp:
(WebCore::LocalFrame::~LocalFrame):
* Source/WebKit/UIProcess/WebFrameProxy.cpp:
(WebKit::WebFrameProxy::prepareForProvisionalLoadInProcess):
* Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:
(WebKit::RemoteLayerTreeDrawingArea::addRootFrame):
* Source/WebKit/WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::loadDidCommitInAnotherProcess):
* Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm:
(TestWebKitAPI::checkFrameTreesInProcesses):
(TestWebKitAPI::TEST(SiteIsolation, CancelProvisionalLoad)):

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

Source/WebCore/page/Frame.cpp

@@ -39,11 +39,64 @@
 namespace WebCore {
 
 #if ASSERT_ENABLED
-static HashMap<FrameIdentifier, WeakRef<Frame>>& allFrames()
-{
-    static NeverDestroyed<HashMap<FrameIdentifier, WeakRef<Frame>>> map;
-    return map;
-}
+class FrameLifetimeVerifier {
+public:
+    static FrameLifetimeVerifier& singleton()
+    {
+        static NeverDestroyed<FrameLifetimeVerifier> instance;
+        return instance.get();
+    }
+
+    void frameCreated(Frame& frame)
+    {
+        auto& pair = m_map.ensure(frame.frameID(), [] {
+            return std::pair<WeakPtr<LocalFrame>, WeakPtr<RemoteFrame>> { };
+        }).iterator->value;
+
+        switch (frame.frameType()) {
+        case Frame::FrameType::Local:
+            ASSERT_WITH_MESSAGE(!pair.first, "There should never be two LocalFrames with the same ID in the same process");
+            pair.first = downcast<LocalFrame>(frame);
+            break;
+        case Frame::FrameType::Remote:
+            ASSERT_WITH_MESSAGE(!pair.second, "There should never be two RemoteFrames with the same ID in the same process");
+            pair.second = downcast<RemoteFrame>(frame);
+            break;
+        }
+    }
+
+    void frameDestroyed(Frame& frame)
+    {
+        auto it = m_map.find(frame.frameID());
+        ASSERT(it != m_map.end());
+        auto& pair = it->value;
+        switch (frame.frameType()) {
+        case Frame::FrameType::Local:
+            ASSERT(pair.first == &frame);
+            if (pair.second)
+                pair.first = nullptr;
+            else
+                m_map.remove(it);
+            break;
+        case Frame::FrameType::Remote:
+            ASSERT(pair.second == &frame);
+            if (pair.first)
+                pair.second = nullptr;
+            else
+                m_map.remove(it);
+        }
+    }
+
+    bool isRootFrameIdentifier(FrameIdentifier identifier)
+    {
+        auto it = m_map.find(identifier);
+        if (it == m_map.end())
+            return false;
+        return it->value.first && it->value.first->isRootFrame();
+    }
+private:
+    HashMap<FrameIdentifier, std::pair<WeakPtr<LocalFrame>, WeakPtr<RemoteFrame>>> m_map;
+};
 #endif
 
 Frame::Frame(Page& page, FrameIdentifier frameID, FrameType frameType, HTMLFrameOwnerElement* ownerElement, Frame* parent, Frame* opener)
@@ -66,10 +119,7 @@ Frame::Frame(Page& page, FrameIdentifier frameID, FrameType frameType, HTMLFrame
         ownerElement->setContentFrame(*this);
 
 #if ASSERT_ENABLED
-    WeakPtr maybeOldFrame = allFrames().take(frameID);
-    auto addResult = allFrames().add(frameID, *this);
-    ASSERT(addResult.isNewEntry);
-    ASSERT_WITH_MESSAGE(!maybeOldFrame || is<LocalFrame>(maybeOldFrame) != is<LocalFrame>(*this), "The only time there should be two frames with the same ID in the same process is when swapping between local and remote frames.");
+    FrameLifetimeVerifier::singleton().frameCreated(*this);
 #endif
 }
 
@@ -79,13 +129,7 @@ Frame::~Frame()
     m_navigationScheduler->cancel();
 
 #if ASSERT_ENABLED
-    auto it = allFrames().find(m_frameID);
-    if (it == allFrames().end())
-        return;
-    if (it->value.ptr() == this)
-        allFrames().remove(it);
-    else
-        ASSERT_WITH_MESSAGE(is<LocalFrame>(it->value) != is<LocalFrame>(this), "The only time there should be two frames with the same ID in the same process is when swapping between local and remote frames.");
+    FrameLifetimeVerifier::singleton().frameDestroyed(*this);
 #endif
 }
 
@@ -103,6 +147,13 @@ void Frame::resetWindowProxy()
 
 void Frame::detachFromPage()
 {
+    if (isRootFrame()) {
+        if (m_page) {
+            m_page->removeRootFrame(downcast<LocalFrame>(*this));
+            if (auto* scrollingCoordinator = m_page->scrollingCoordinator())
+                scrollingCoordinator->rootFrameWasRemoved(frameID());
+        }
+    }
     m_page = nullptr;
 }
 
@@ -156,8 +207,7 @@ RefPtr<FrameView> Frame::protectedVirtualView() const
 #if ASSERT_ENABLED
 bool Frame::isRootFrameIdentifier(FrameIdentifier identifier)
 {
-    RefPtr localFrame = dynamicDowncast<LocalFrame>(allFrames().get(identifier));
-    return localFrame && localFrame->isRootFrame();
+    return FrameLifetimeVerifier::singleton().isRootFrameIdentifier(identifier);
 }
 #endif
 

Source/WebCore/page/Frame.h

@@ -78,7 +78,7 @@ class Frame : public ThreadSafeRefCounted<Frame, WTF::DestructionThread::Main>,
     WEBCORE_EXPORT void detachFromAllOpenedFrames();
     virtual bool isRootFrame() const = 0;
 #if ASSERT_ENABLED
-    static bool isRootFrameIdentifier(FrameIdentifier);
+    WEBCORE_EXPORT static bool isRootFrameIdentifier(FrameIdentifier);
 #endif
 
     WEBCORE_EXPORT void detachFromPage();

Source/WebCore/page/LocalFrame.cpp

@@ -232,13 +232,7 @@ LocalFrame::~LocalFrame()
     if (!isMainFrame() && localMainFrame)
         localMainFrame->selfOnlyDeref();
 
-    if (isRootFrame()) {
-        if (RefPtr page = this->page()) {
-            page->removeRootFrame(*this);
-            if (auto* scrollingCoordinator = page->scrollingCoordinator())
-                scrollingCoordinator->rootFrameWasRemoved(frameID());
-        }
-    }
+    detachFromPage();
 }
 
 void LocalFrame::addDestructionObserver(FrameDestructionObserver& observer)

Source/WebCore/page/Page.cpp

@@ -452,7 +452,6 @@ Page::~Page()
     m_validationMessageClient = nullptr;
     m_diagnosticLoggingClient = nullptr;
     m_performanceLoggingClient = nullptr;
-    m_rootFrames.clear();
     if (RefPtr localMainFrame = dynamicDowncast<LocalFrame>(m_mainFrame))
         localMainFrame->setView(nullptr);
     setGroupName(String());
@@ -468,6 +467,7 @@ Page::~Page()
         frame.willDetachPage();
         frame.detachFromPage();
     });
+    ASSERT(m_rootFrames.isEmpty());
 
     if (RefPtr scrollingCoordinator = m_scrollingCoordinator)
         scrollingCoordinator->pageDestroyed();

Source/WebKit/UIProcess/WebFrameProxy.cpp

@@ -439,7 +439,7 @@ void WebFrameProxy::prepareForProvisionalLoadInProcess(WebProcessProxy& process,
     RegistrableDomain navigationDomain(navigation.currentRequest().url());
     // addAllowedFirstPartyForCookies can be sync, but we need completionHander to be invoked after this function.
     auto aggregator = CallbackAggregator::create(WTFMove(completionHandler));
-    if (!m_provisionalFrame || navigation.currentRequestIsCrossSiteRedirect()) {
+    if (!m_provisionalFrame || m_provisionalFrame->process().coreProcessIdentifier() != process.coreProcessIdentifier() || navigation.currentRequestIsCrossSiteRedirect()) {
         RefPtr page = m_page.get();
         // FIXME: Main resource (of main or subframe) request redirects should go straight from the network to UI process so we don't need to make the processes for each domain in a redirect chain. <rdar://116202119>
         RegistrableDomain mainFrameDomain(page->mainFrame()->url());

Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm

@@ -134,6 +134,7 @@
 
 void RemoteLayerTreeDrawingArea::addRootFrame(WebCore::FrameIdentifier frameID)
 {
+    ASSERT(Frame::isRootFrameIdentifier(frameID));
     auto layer = GraphicsLayer::create(graphicsLayerFactory(), *this);
     layer->setName(makeString("drawing area root "_s, frameID));
     m_rootLayers.append(RootLayerInfo {

Source/WebKit/WebProcess/WebPage/WebFrame.cpp

@@ -395,6 +395,8 @@ void WebFrame::loadDidCommitInAnotherProcess(std::optional<WebCore::LayerHosting
     if (corePage->focusController().focusedFrame() == localFrame.get())
         corePage->focusController().setFocusedFrame(newFrame.ptr(), FocusController::BroadcastFocusedFrame::No);
 
+    localFrame->loader().detachFromParent();
+
     if (ownerElement)
         ownerElement->scheduleInvalidateStyleAndLayerComposition();
 }

Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm

@@ -155,7 +155,7 @@ static void printTree(const ExpectedFrameTree& n, size_t indent = 0)
 
 static void checkFrameTreesInProcesses(NSSet<_WKFrameTreeNode *> *actualTrees, Vector<ExpectedFrameTree>&& expectedFrameTrees)
 {
-    bool result = frameTreesMatch(actualTrees, WTFMove(expectedFrameTrees));
+    bool result = frameTreesMatch(actualTrees, Vector<ExpectedFrameTree> { expectedFrameTrees });
     if (!result) {
         WTFLogAlways("ACTUAL");
         for (_WKFrameTreeNode *n in actualTrees)
@@ -2616,24 +2616,50 @@ HTTPServer server({
         },
     });
 
-    [webView evaluateJavaScript:
-        @"let i = document.getElementById('testiframe');"
-        "i.addEventListener('load', () => { alert('iframe loaded') });"
-        "i.src = 'https://webkit.org/never_respond';"
-        "setTimeout(()=>{ i.src = 'https://example.com/respond_quickly' }, Math.random() * 500)"
-        completionHandler:nil];
-    EXPECT_WK_STREQ([webView _test_waitForAlert], "iframe loaded");
-    checkFrameTreesInProcesses(webView.get(), {
+    auto checkStateAfterSequentialFrameLoads = [webView = RetainPtr { webView }, navigationDelegate = RetainPtr { navigationDelegate }] (NSString *first, NSString *second, Vector<ExpectedFrameTree>&& expectedTrees) {
+        [webView evaluateJavaScript:[NSString stringWithFormat:@"i = document.getElementById('testiframe'); i.addEventListener('load', () => { alert('iframe loaded') }); i.src = '%@'; setTimeout(()=>{ i.src = '%@' }, Math.random() * 100)", first, second] completionHandler:nil];
+        EXPECT_WK_STREQ([webView _test_waitForAlert], "iframe loaded");
+        checkFrameTreesInProcesses(webView.get(), WTFMove(expectedTrees));
+    };
+
+    checkStateAfterSequentialFrameLoads(@"https://webkit.org/never_respond", @"https://example.com/respond_quickly", {
         { "https://webkit.org"_s,
             { { RemoteFrame }, { RemoteFrame } }
         }, { RemoteFrame,
             { { "https://example.com"_s }, { "https://example.com"_s } }
         },
     });
 
-    // FIXME: Add tests for other combinations of provisional loads starting in the process the frame is already in,
-    // a new process we've never seen before, and the main frame process then starting another provisional load in
-    // those processes. Also, test these cases for provisional loads that respond after a short delay to give a chance
+    checkStateAfterSequentialFrameLoads(@"https://example.com/never_respond", @"https://webkit.org/respond_quickly", {
+        { "https://webkit.org"_s,
+            { { RemoteFrame }, { "https://webkit.org"_s } }
+        }, { RemoteFrame,
+            { { "https://example.com"_s }, { RemoteFrame } }
+        },
+    });
+
+    checkStateAfterSequentialFrameLoads(@"https://apple.com/never_respond", @"https://webkit.org/respond_quickly", {
+        { "https://webkit.org"_s,
+            { { RemoteFrame }, { "https://webkit.org"_s } }
+        }, { RemoteFrame,
+            { { "https://example.com"_s }, { RemoteFrame } }
+        },
+    });
+
+    checkStateAfterSequentialFrameLoads(@"https://apple.com/never_respond", @"https://example.com/respond_quickly", {
+        { "https://webkit.org"_s,
+            { { RemoteFrame }, { RemoteFrame } }
+        }, { RemoteFrame,
+            { { "https://example.com"_s }, { "https://example.com"_s } }
+        },
+    });
+
+    // FIXME: Test loading https://apple.com/never_respond then https://apple.com/respond_quickly
+    // FrameLoader::continueLoadAfterNavigationPolicy returns early because the page is disconnected by something.
+    // We may need to destroy a provisional frame then make a new one if there's already a provisional frame in a process.
+    // WebProcessProxy::shutDown is also being called in the middle of loading, which isn't great.
+
+    // FIXME: Test cases for provisional loads that respond after a short delay to give a chance
     // that the load commits during the time another provisional navigation starts.
 }