Regression(PSON) Load hang can occur on history navigation
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Jan 2019 20:27:37 +0000 (20:27 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Jan 2019 20:27:37 +0000 (20:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194030
<rdar://problem/47656939>

Reviewed by Antti Koivisto.

Source/WebKit:

We do not support having more than one WebPage in a WebProcess with the same page ID. As a result,
if we decide to reuse an existing process on process-swap, we need to make sure that we either use
its suspended page (when possible, meaning that it is for the right HistoryItem / page) or we need
make sure we drop the existing suspended page for this process / pagePID combination, so that the
WebPage on WebProcess side gets closed before we attempt to do the new load.

We were doing this correctly in 2 places in WebProcessPool::processForNavigationInternal() but failed
to do so in a third place, when doing back to a HistoryItem which does not have a SuspendedPage but
whose process is still alive (presumably because it is kept alive by another suspended page). This
patch fixes this third place to remove any suspended page in the process for the current page before
reusing the process. An assertion was also added to the call site in
WebPageProxy::receivedNavigationPolicyDecision() to make sure we catch this more easily in the
future.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):
(WebKit::WebProcessPool::removeAllSuspendedPagesForPage):
(WebKit::WebProcessPool::hasSuspendedPageFor const):
* UIProcess/WebProcessPool.h:

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240725 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessPool.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index f7369e5..2acc2f6 100644 (file)
@@ -1,3 +1,33 @@
+2019-01-30  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON) Load hang can occur on history navigation
+        https://bugs.webkit.org/show_bug.cgi?id=194030
+        <rdar://problem/47656939>
+
+        Reviewed by Antti Koivisto.
+
+        We do not support having more than one WebPage in a WebProcess with the same page ID. As a result,
+        if we decide to reuse an existing process on process-swap, we need to make sure that we either use
+        its suspended page (when possible, meaning that it is for the right HistoryItem / page) or we need
+        make sure we drop the existing suspended page for this process / pagePID combination, so that the
+        WebPage on WebProcess side gets closed before we attempt to do the new load.
+
+        We were doing this correctly in 2 places in WebProcessPool::processForNavigationInternal() but failed
+        to do so in a third place, when doing back to a HistoryItem which does not have a SuspendedPage but
+        whose process is still alive (presumably because it is kept alive by another suspended page). This
+        patch fixes this third place to remove any suspended page in the process for the current page before
+        reusing the process. An assertion was also added to the call site in
+        WebPageProxy::receivedNavigationPolicyDecision() to make sure we catch this more easily in the
+        future.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::processForNavigationInternal):
+        (WebKit::WebProcessPool::removeAllSuspendedPagesForPage):
+        (WebKit::WebProcessPool::hasSuspendedPageFor const):
+        * UIProcess/WebProcessPool.h:
+
 2019-01-30  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [GTK][Wayland] REGRESSION(r240712): Clear the GL context if it's the current one on dispose
index 5b6e842..520fe2c 100644 (file)
@@ -2722,6 +2722,11 @@ void WebPageProxy::receivedNavigationPolicyDecision(PolicyAction policyAction, A
         if (!shouldProcessSwap)
             return;
 
+        // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
+        // In the case where the destination WebProcess has a SuspendedPageProxy for this WebPage, we should have thrown
+        // it away to support WebProcess re-use.
+        ASSERT(destinationSuspendedPage || !process().processPool().hasSuspendedPageFor(processForNavigation, this));
+
         auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
         if (suspendedPage && suspendedPage->failedToSuspend())
             suspendedPage = nullptr;
index 6c00fb4..85ca6de 100644 (file)
@@ -2190,8 +2190,14 @@ void WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API:
             });
         }
 
-        if (auto* process = WebProcessProxy::processForIdentifier(targetItem->itemID().processIdentifier))
-            return completionHandler(*process, nullptr, "Using target back/forward item's process"_s);
+        if (RefPtr<WebProcessProxy> process = WebProcessProxy::processForIdentifier(targetItem->itemID().processIdentifier)) {
+            // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
+            // In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
+            // WebProcess re-use.
+            removeAllSuspendedPagesForPage(page, process.get());
+
+            return completionHandler(process.releaseNonNull(), nullptr, "Using target back/forward item's process"_s);
+        }
     }
 
     if (navigation.treatAsSameOriginNavigation())
@@ -2225,9 +2231,7 @@ void WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API:
                 // In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
                 // WebProcess re-use.
                 // In the future it would be great to refactor-out this limitation (https://bugs.webkit.org/show_bug.cgi?id=191166).
-                m_suspendedPages.removeAllMatching([&](auto& suspendedPage) {
-                    return &suspendedPage->page() == &page && &suspendedPage->process() == process;
-                });
+                removeAllSuspendedPagesForPage(page, process);
 
                 return completionHandler(makeRef(*process), nullptr, reason);
             }
@@ -2263,10 +2267,10 @@ void WebProcessPool::addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&& susp
     m_suspendedPages.append(WTFMove(suspendedPage));
 }
 
-void WebProcessPool::removeAllSuspendedPagesForPage(WebPageProxy& page)
+void WebProcessPool::removeAllSuspendedPagesForPage(WebPageProxy& page, WebProcessProxy* process)
 {
-    m_suspendedPages.removeAllMatching([&page](auto& suspendedPage) {
-        return &suspendedPage->page() == &page;
+    m_suspendedPages.removeAllMatching([&page, process](auto& suspendedPage) {
+        return &suspendedPage->page() == &page && (!process || &suspendedPage->process() == process);
     });
 }
 
@@ -2294,10 +2298,10 @@ void WebProcessPool::removeSuspendedPage(SuspendedPageProxy& suspendedPage)
         m_suspendedPages.remove(it);
 }
 
-bool WebProcessPool::hasSuspendedPageFor(WebProcessProxy& process) const
+bool WebProcessPool::hasSuspendedPageFor(WebProcessProxy& process, WebPageProxy* page) const
 {
-    return m_suspendedPages.findIf([&process](auto& suspendedPage) {
-        return &suspendedPage->process() == &process;
+    return m_suspendedPages.findIf([&process, page](auto& suspendedPage) {
+        return &suspendedPage->process() == &process && (!page || &suspendedPage->page() == page);
     }) != m_suspendedPages.end();
 }
 
index c0690a6..3aecd42 100644 (file)
@@ -446,11 +446,11 @@ public:
 
     // SuspendedPageProxy management.
     void addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&&);
-    void removeAllSuspendedPagesForPage(WebPageProxy&);
+    void removeAllSuspendedPagesForPage(WebPageProxy&, WebProcessProxy* = nullptr);
     void closeFailedSuspendedPagesForPage(WebPageProxy&);
     std::unique_ptr<SuspendedPageProxy> takeSuspendedPage(SuspendedPageProxy&);
     void removeSuspendedPage(SuspendedPageProxy&);
-    bool hasSuspendedPageFor(WebProcessProxy&) const;
+    bool hasSuspendedPageFor(WebProcessProxy&, WebPageProxy* = nullptr) const;
     unsigned maxSuspendedPageCount() const { return m_maxSuspendedPageCount; }
 
     void didReachGoodTimeToPrewarm();
index 4eaf961..20a423d 100644 (file)
@@ -1,3 +1,15 @@
+2019-01-30  Chris Dumez  <cdumez@apple.com>
+
+        Regression(PSON) Load hang can occur on history navigation
+        https://bugs.webkit.org/show_bug.cgi?id=194030
+        <rdar://problem/47656939>
+
+        Reviewed by Antti Koivisto.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-01-30  Zalan Bujtas  <zalan@apple.com>
 
         [LFC] Expand tests coverage.
index a632011..7ee9eeb 100644 (file)
@@ -2311,6 +2311,64 @@ TEST(ProcessSwap, HistoryItemIDConfusion)
     EXPECT_WK_STREQ(@"pson://www.google.com/main.html", [backForwardList.forwardList[1].URL absoluteString]);
 }
 
+TEST(ProcessSwap, GoToSecondItemInBackHistory)
+{
+    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main1.html" toData:withSubframesTestBytes];
+    [handler addMappingFromURLString:@"pson://www.webkit.org/main2.html" toData:withSubframesTestBytes];
+    [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:withSubframesTestBytes];
+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+
+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto applePID = [webView _webProcessIdentifier];
+    EXPECT_NE(webkitPID, applePID);
+
+    [webView goToBackForwardListItem:webView.get().backForwardList.backList[0]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+    EXPECT_WK_STREQ(@"pson://www.webkit.org/main1.html", [[webView URL] absoluteString]);
+
+    [webView goToBackForwardListItem:webView.get().backForwardList.forwardList[1]];
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
+    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
+}
+
 static const char* keepNavigatingFrameBytes = R"PSONRESOURCE(
 <body>
 <iframe id="testFrame1" src="about:blank"></iframe>