Changeset 244075 in webkit

Timestamp:

Apr 9, 2019, 7:20:12 AM (7 years ago)

Author:

Chris Dumez

Message:

Loads using loadHTMLString() cause flashing when process-swapping
​https://bugs.webkit.org/show_bug.cgi?id=196714
<rdar://problem/49637354>

Reviewed by Antti Koivisto.

Source/WebKit:

Our logic to decide if we should construct a SuspendedPageProxy on process-swap was assuming
a SuspendedPageProxy is only useful for PageCache and would therefore not create one if PageCache
is disabled or if there is no associated WebBackForwardListItem. However, constructing a
SuspendedPageProxy is also useful to prevent flashing when process-swapping as we need to keep
displaying the layer of the previous process until there is something meaningful to show in the
new process.

This patch makes it so that we now construct a SuspendedPageProxy on process-swap, even if
PageCache is disabled or if there is no associated WebBackForwardListItem. The process in
question will not be useful for PageCache but it will avoid flashing. The SuspendedPageProxy's
process may also get used for future navigations to the same site (as demonstrated by the
API test) which is beneficial for performance.

• UIProcess/SuspendedPageProxy.cpp:

(WebKit::SuspendedPageProxy::SuspendedPageProxy):

• UIProcess/SuspendedPageProxy.h:
• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::suspendCurrentPageIfPossible):

• UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::findReusableSuspendedPageProcess):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/SuspendedPageProxy.cpp (modified) (2 diffs)
• Source/WebKit/UIProcess/SuspendedPageProxy.h (modified) (2 diffs)
• Source/WebKit/UIProcess/WebPageProxy.cpp (modified) (3 diffs)
• Source/WebKit/UIProcess/WebProcessPool.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-04-09  Chris Dumez  <cdumez@apple.com>
+
+        Loads using loadHTMLString() cause flashing when process-swapping
+        https://bugs.webkit.org/show_bug.cgi?id=196714
+        <rdar://problem/49637354>
+
+        Reviewed by Antti Koivisto.
+
+        Our logic to decide if we should construct a SuspendedPageProxy on process-swap was assuming
+        a SuspendedPageProxy is only useful for PageCache and would therefore not create one if PageCache
+        is disabled or if there is no associated WebBackForwardListItem. However, constructing a
+        SuspendedPageProxy is also useful to prevent flashing when process-swapping as we need to keep
+        displaying the layer of the previous process until there is something meaningful to show in the
+        new process.
+
+        This patch makes it so that we now construct a SuspendedPageProxy on process-swap, even if
+        PageCache is disabled or if there is no associated WebBackForwardListItem. The process in
+        question will not be useful for PageCache but it will avoid flashing. The SuspendedPageProxy's
+        process may also get used for future navigations to the same site (as demonstrated by the
+        API test) which is beneficial for performance.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
+        * UIProcess/SuspendedPageProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::findReusableSuspendedPageProcess):
+
 2019-04-08  Don Olmstead  <don.olmstead@sony.com>
 

trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp

@@ -79 +79 @@
 #endif
 
-SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, WebBackForwardListItem& item, uint64_t mainFrameID)
+SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&& process, uint64_t mainFrameID)
     : m_page(page)
     , m_process(WTFMove(process))
     , m_mainFrameID(mainFrameID)
-    , m_registrableDomain(URL(URL(), item.url()))
     , m_suspensionTimeoutTimer(RunLoop::main(), this, &SuspendedPageProxy::suspensionTimedOut)
 #if PLATFORM(IOS_FAMILY)
@@ -89 +88 @@
 #endif
 {
-    item.setSuspendedPage(this);
     m_process->incrementSuspendedPageCount();
     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID(), *this);

trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h

@@ -42 +42 @@
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    SuspendedPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, WebBackForwardListItem&, uint64_t mainFrameID);
+    SuspendedPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, uint64_t mainFrameID);
     ~SuspendedPageProxy();
 
@@ -48 +48 @@
     WebProcessProxy& process() { return m_process.get(); }
     uint64_t mainFrameID() const { return m_mainFrameID; }
-    const WebCore::RegistrableDomain& registrableDomain() const { return m_registrableDomain; }
 
     bool failedToSuspend() const { return m_suspensionState == SuspensionState::FailedToSuspend; }

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -742 +742 @@
         return false;
 
-    if (!m_preferences->usesPageCache()) {
-        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because page cache is disabled", m_process->processIdentifier());
-        return false;
-    }
-
     // If the client forced a swap then it may not be Web-compatible to suspend the previous page because other windows may have an opener link to the page.
     if (processSwapRequestedByClient == ProcessSwapRequestedByClient::Yes) {
@@ -764 +759 @@
 
     auto* fromItem = navigation.fromItem();
-    if (!fromItem) {
-        RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because the navigation does not have a fromItem", m_process->processIdentifier());
-        return false;
-    }
 
     // If the source and the destination back / forward list items are the same, then this is a client-side redirect. In this case,
     // there is no need to suspend the previous page as there will be no way to get back to it.
-    if (fromItem == m_backForwardList->currentItem()) {
+    if (fromItem && fromItem == m_backForwardList->currentItem()) {
         RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because this is a client-side redirect", m_process->processIdentifier());
         return false;
     }
 
-    if (fromItem->url() != pageLoadState().url()) {
+    if (fromItem && fromItem->url() != pageLoadState().url()) {
         RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Not suspending current page for process pid %i because fromItem's URL does not match the page URL.", m_process->processIdentifier());
         ASSERT_NOT_REACHED();
@@ -783 +774 @@
 
     RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "suspendCurrentPageIfPossible: Suspending current page for process pid %i", m_process->processIdentifier());
-    auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *fromItem, *mainFrameID);
-
-    LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), suspendedPage->loggingString(), m_process->processIdentifier(), fromItem->itemID().logString());
+    auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *mainFrameID);
+
+    LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), suspendedPage->loggingString(), m_process->processIdentifier(), fromItem ? fromItem->itemID().logString() : 0);
+
+    if (fromItem && m_preferences->usesPageCache())
+        fromItem->setSuspendedPage(suspendedPage.get());
 
     m_process->processPool().addSuspendedPage(WTFMove(suspendedPage));

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -2357 +2357 @@
 {
     auto it = m_suspendedPages.findIf([&](auto& suspendedPage) {
-        return suspendedPage->registrableDomain() == registrableDomain && &suspendedPage->process().websiteDataStore() == &dataStore;
+        return suspendedPage->process().registrableDomain() == registrableDomain && &suspendedPage->process().websiteDataStore() == &dataStore;
     });
     if (it == m_suspendedPages.end())

trunk/Tools/ChangeLog

@@ +1 @@
+2019-04-09  Chris Dumez  <cdumez@apple.com>
+
+        Loads using loadHTMLString() cause flashing when process-swapping
+        https://bugs.webkit.org/show_bug.cgi?id=196714
+        <rdar://problem/49637354>
+
+        Reviewed by Antti Koivisto.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2019-04-09  Carlos Garcia Campos  <cgarcia@igalia.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

@@ -2347 +2347 @@
 {
     auto processPoolConfiguration = psonProcessPoolConfiguration();
+    processPoolConfiguration.get().usesWebProcessCache = NO;
     auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
 
@@ -2387 +2388 @@
     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, ReuseSuspendedProcessLoadHTMLString)
+{
+    auto processPoolConfiguration = psonProcessPoolConfiguration();
+    processPoolConfiguration.get().usesWebProcessCache = NO;
+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    [webViewConfiguration setProcessPool:processPool.get()];
+    auto handler = adoptNS([[PSONScheme alloc] init]);
+    [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()];
+
+    NSString *htmlString = @"<html><body>TEST</body></html>";
+    [webView loadHTMLString:htmlString baseURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto webkitPID = [webView _webProcessIdentifier];
+
+    [webView loadHTMLString:htmlString baseURL:[NSURL URLWithString:@"pson://www.apple.com/main1.html"]];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto applePID = [webView _webProcessIdentifier];
+
+    EXPECT_NE(webkitPID, applePID);
+
+    [webView loadHTMLString:htmlString baseURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+
+    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]);
+
+    [webView loadHTMLString:htmlString baseURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
 
     TestWebKitAPI::Util::run(&done);