Attachment 360597 Details for Bug 194030 – Patch

[patch] Patch

bug-194030-20190130105938.patch (text/plain), 11.34 KB, created by Chris Dumez on 2019-01-30 10:59:38 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-30 10:59:38 PST

Size: 11.34 KB

patch

obsolete

>Subversion Revision: 240715
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 11d679ebbcab423c02e3f231904033eabe96c9a0..eb3615a82e909bc8badf009e858882f5b4cf39cf 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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  Simon Fraser  <simon.fraser@apple.com>
> 
>         Add some basic geometry information to the scrolling tree
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 5b6e8422e3905efaf2a7906ff1e83132c0180317..520fe2ce7823cff705f386a92bd33ec8bee50a61 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -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;
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 6c00fb4de684e91774dafc7ea88f3b943c9d1288..85ca6de1448295b32003045f629e1c04180e232b 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -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();
> }
> 
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.h b/Source/WebKit/UIProcess/WebProcessPool.h
>index c0690a6759ded9f1cc82ca864800ac18e3dfa875..3aecd424d8b27fea3e75eef1064d80a6447f2a23 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.h
>+++ b/Source/WebKit/UIProcess/WebProcessPool.h
>@@ -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();
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 9b48791eb108c0b3fb78af7d052d7b6618f23b98..fe055e128a55767721faaaf360b1cc5b1afc3b0b 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-01-30  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r240708.
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index a632011269edf385e6de7f84ee8e631f66655f3e..7ee9eebb6ec4289e07e539c5194c26eff35dcbf4 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 194030: 360597