Attachment 357479 Details for Bug 192772 – Patch

[patch] Patch

bug-192772-20181217144322.patch (text/plain), 14.57 KB, created by Chris Dumez on 2018-12-17 14:43:23 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-12-17 14:43:23 PST

Size: 14.57 KB

patch

obsolete

>Subversion Revision: 239282
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index cc9c4ab4fa4c7dca456962bff2591f4c082d317b..b7138215b8a81bdc50666af52a9661167f8cde95 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-12-17  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
>+        https://bugs.webkit.org/show_bug.cgi?id=192772
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        With r239182, if the page in the previous process would fail to enter PageCache, we would destroy
>+        the corresponding SuspendedPageProxy, which would potentially terminate the process. This would
>+        regress performance when trying to navigate back in history to that page. This would also regress
>+        performance when link-navigating to the same domain as we would have previously reused the suspended
>+        page's process for such navigation.
>+
>+        Address the issue by keeping the SuspendedPageProxy alive even if the WebPage fails to suspend.
>+        When trying to reuse a SuspendedPageProxy, if the page failed to suspend, reuse its process but
>+        not the suspended page itself.
>+
>+        * UIProcess/SuspendedPageProxy.cpp:
>+        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
>+        (WebKit::SuspendedPageProxy::waitUntilReadyToUnsuspend):
>+        (WebKit::SuspendedPageProxy::unsuspend):
>+        (WebKit::SuspendedPageProxy::didSuspend):
>+        (WebKit::SuspendedPageProxy::didFailToSuspend):
>+        (WebKit::SuspendedPageProxy::loggingString const):
>+        * UIProcess/SuspendedPageProxy.h:
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::swapToWebProcess):
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::processForNavigationInternal):
>+
> 2018-12-17  Zan Dobersek  <zdobersek@igalia.com>
> 
>         Unreviewed WPE build fix after r239277.
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>index 7ed22380405003c06f2b637eab0fd1c4f0c23863..61a0af9ac4c4cd07949b9ca9b0d8af6b20cc2c8a 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>@@ -94,7 +94,7 @@ SuspendedPageProxy::~SuspendedPageProxy()
>     if (m_readyToUnsuspendHandler)
>         m_readyToUnsuspendHandler(nullptr);
> 
>-    if (!m_isSuspended)
>+    if (m_suspensionState == SuspensionState::Resumed)
>         return;
> 
>     // If the suspended page was not consumed before getting destroyed, then close the corresponding page
>@@ -114,18 +114,26 @@ void SuspendedPageProxy::waitUntilReadyToUnsuspend(CompletionHandler<void(Suspen
>     if (m_readyToUnsuspendHandler)
>         m_readyToUnsuspendHandler(nullptr);
> 
>-    if (!m_finishedSuspending)
>+    switch (m_suspensionState) {
>+    case SuspensionState::Suspending:
>         m_readyToUnsuspendHandler = WTFMove(completionHandler);
>-    else
>+        break;
>+    case SuspensionState::FailedToSuspend:
>+    case SuspensionState::Suspended:
>         completionHandler(this);
>+        break;
>+    case SuspensionState::Resumed:
>+        ASSERT_NOT_REACHED();
>+        completionHandler(nullptr);
>+        break;
>+    }
> }
> 
> void SuspendedPageProxy::unsuspend()
> {
>-    ASSERT(m_isSuspended);
>-    ASSERT(m_finishedSuspending);
>+    ASSERT(m_suspensionState == SuspensionState::Suspended);
> 
>-    m_isSuspended = false;
>+    m_suspensionState = SuspensionState::Resumed;
>     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
>     m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
> }
>@@ -134,7 +142,8 @@ void SuspendedPageProxy::didSuspend()
> {
>     LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
> 
>-    m_finishedSuspending = true;
>+    ASSERT(m_suspensionState == SuspensionState::Suspending);
>+    m_suspensionState = SuspensionState::Suspended;
> 
> #if PLATFORM(IOS_FAMILY)
>     m_suspensionToken = nullptr;
>@@ -146,11 +155,11 @@ void SuspendedPageProxy::didSuspend()
> 
> void SuspendedPageProxy::didFailToSuspend()
> {
>-    // We are unusable due to failure to suspend so remove ourselves from the WebProcessPool.
>-    auto protectedThis = m_process->processPool().takeSuspendedPage(*this);
>+    ASSERT(m_suspensionState == SuspensionState::Suspending);
>+    m_suspensionState = SuspensionState::FailedToSuspend;
> 
>     if (m_readyToUnsuspendHandler)
>-        m_readyToUnsuspendHandler(nullptr);
>+        m_readyToUnsuspendHandler(this);
> }
> 
> void SuspendedPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder& decoder)
>@@ -180,7 +189,7 @@ void SuspendedPageProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&,
> #if !LOG_DISABLED
> const char* SuspendedPageProxy::loggingString() const
> {
>-    return debugString("(", String::format("%p", this), " page ID ", String::number(m_page.pageID()), ", m_finishedSuspending ", String::number(m_finishedSuspending), ")");
>+    return debugString("(", String::format("%p", this), " page ID ", String::number(m_page.pageID()), ", m_suspensionState ", String::number(static_cast<unsigned>(m_suspensionState)), ")");
> }
> #endif
> 
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>index b661f52a86c5236cd7f182adba2576fbf86a0153..1b56d06af6681da2b636b994f875bd19e24b05b9 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>@@ -47,6 +47,8 @@ public:
>     uint64_t mainFrameID() const { return m_mainFrameID; }
>     const String& registrableDomain() const { return m_registrableDomain; }
> 
>+    bool failedToSuspend() const { return m_suspensionState == SuspensionState::FailedToSuspend; }
>+
>     void waitUntilReadyToUnsuspend(CompletionHandler<void(SuspendedPageProxy*)>&&);
>     void unsuspend();
> 
>@@ -67,9 +69,8 @@ private:
>     uint64_t m_mainFrameID;
>     String m_registrableDomain;
> 
>-    bool m_isSuspended { true };
>-
>-    bool m_finishedSuspending { false };
>+    enum class SuspensionState : uint8_t { Suspending, FailedToSuspend, Suspended, Resumed };
>+    SuspensionState m_suspensionState { SuspensionState::Suspending };
>     CompletionHandler<void(SuspendedPageProxy*)> m_readyToUnsuspendHandler;
> #if PLATFORM(IOS_FAMILY)
>     ProcessThrottler::BackgroundActivityToken m_suspensionToken;
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 50224ed1d166e5b9878df95bac909cac4fc2b27f..fcb4c8373f323cb3ee364d77773aa236dde3befc 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -780,11 +780,16 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, std::unique_
>     // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
>     // already exists and already has a main frame.
>     if (destinationSuspendedPage) {
>-        ASSERT(!m_mainFrame);
>-        ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
>-        destinationSuspendedPage->unsuspend();
>-        m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
>-        m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
>+        if (!destinationSuspendedPage->failedToSuspend()) {
>+            ASSERT(!m_mainFrame);
>+            ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
>+            destinationSuspendedPage->unsuspend();
>+            m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
>+            m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
>+        } else {
>+            // We failed to suspend the page so destroy it now and merely reuse its WebContent process.
>+            destinationSuspendedPage = nullptr;
>+        }
>     }
> 
>     m_process->addExistingWebPage(*this, m_pageID, WebProcessProxy::BeginsUsingDataStore::No);
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index fa43a3d1e783092bb7c0218147fd164f827b03c0..82830a53f61494b12020d48152078a5c93e2a5ca 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -2177,7 +2177,7 @@ void WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API:
>         if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
>             return suspendedPage->waitUntilReadyToUnsuspend([createNewProcess = WTFMove(createNewProcess), completionHandler = WTFMove(completionHandler)](SuspendedPageProxy* suspendedPage) mutable {
>                 if (!suspendedPage)
>-                    return completionHandler(createNewProcess(), nullptr, "Using new process because target back/forward item's process failed to suspend"_s);
>+                    return completionHandler(createNewProcess(), nullptr, "Using new process because target back/forward item's suspended page is not reusable"_s);
>                 Ref<WebProcessProxy> process = suspendedPage->process();
>                 completionHandler(WTFMove(process), suspendedPage, "Using target back/forward item's process"_s);
>             });
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index e7c0324a8cc02f13896c2d323169ed189ed6796a..ff857b1ef58bdf9bfe946369c5786b567c706053 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-12-17  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
>+        https://bugs.webkit.org/show_bug.cgi?id=192772
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2018-12-17  Daniel Bates  <dabates@apple.com>
> 
>         Support concatenating StringView with other string types
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 0701dc4be64909b96c2db8f5316221002c236619..2434bcec8c1e11b543bdb78f8d5f2fe5be97f7a3 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -1792,6 +1792,110 @@ TEST(ProcessSwap, ReuseSuspendedProcess)
>     EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
> }
> 
>+static const char* failsToEnterPageCacheTestBytes = R"PSONRESOURCE(
>+<body>
>+<script>
>+// Pages with dedicated workers do not go into page cache.
>+var myWorker = new Worker('worker.js');
>+</script>
>+</body>
>+)PSONRESOURCE";
>+
>+TEST(ProcessSwap, ReuseSuspendedProcessEvenIfPageCacheFails)
>+{
>+    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:failsToEnterPageCacheTestBytes];
>+    [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.apple.com/main1.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    auto applePID = [webView _webProcessIdentifier];
>+
>+    EXPECT_NE(webkitPID, applePID);
>+
>+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
>+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
>+
>+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    // We should have gone back to the apple.com process for this load since we reuse SuspendedPages' process when possible.
>+    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
>+}
>+
>+TEST(ProcessSwap, ReuseSuspendedProcessOnBackEvenIfPageCacheFails)
>+{
>+    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:failsToEnterPageCacheTestBytes];
>+    [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.apple.com/main1.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    auto applePID = [webView _webProcessIdentifier];
>+
>+    EXPECT_NE(webkitPID, applePID);
>+
>+    [webView goBack];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
>+}
>+
> static const char* mainFramesOnlyMainFrame = R"PSONRESOURCE(
> <script>
> function loaded() {

Actions: View | Formatted Diff | Diff

Attachments on bug 192772: 357479