Attachment 360002 Details for Bug 193716 – patch

[patch] patch

suspended-page-back-flash-5.patch (text/plain), 9.39 KB, created by Antti Koivisto on 2019-01-24 02:11:31 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Antti Koivisto

Created: 2019-01-24 02:11:31 PST

Size: 9.39 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 240427)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,38 @@
>+2019-01-24  Antti Koivisto  <antti@apple.com>
>+
>+        [PSON] Flash on back navigation on Mac
>+        https://bugs.webkit.org/show_bug.cgi?id=193716
>+        <rdar://problem/47148458>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We close the page immediately if we fail to suspend. Layers disappear and we get a flash.
>+
>+        * UIProcess/SuspendedPageProxy.cpp:
>+        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
>+        (WebKit::SuspendedPageProxy::close):
>+
>+        Track closed state so we don't send the message twice, causing unhandled message errors in web process.
>+
>+        (WebKit::SuspendedPageProxy::didProcessRequestToSuspend):
>+
>+        Close the suspended page if the suspension fails.
>+        Skip this if we are using web process side compositing on Mac.
>+
>+        * UIProcess/SuspendedPageProxy.h:
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::enterAcceleratedCompositingMode):
>+
>+        On Mac, close the failed SuspendedPageProxy when entering compositing mode. At this point we don't need it to keep layers alive.
>+
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::closeFailedSuspendedPagesForPage):
>+        * UIProcess/WebProcessPool.h:
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::suspendForProcessSwap):
>+
>+        Don't close the page on suspension failure. This is now managed by the UI process.
>+
> 2019-01-23  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         [Curl] Unreviewed build fix for r240292 and friends.
>Index: Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>===================================================================
>--- Source/WebKit/UIProcess/SuspendedPageProxy.cpp	(revision 240407)
>+++ Source/WebKit/UIProcess/SuspendedPageProxy.cpp	(working copy)
>@@ -26,6 +26,7 @@
> #include "config.h"
> #include "SuspendedPageProxy.h"
> 
>+#include "DrawingAreaProxy.h"
> #include "Logging.h"
> #include "WebPageMessages.h"
> #include "WebPageProxy.h"
>@@ -99,7 +100,7 @@ SuspendedPageProxy::~SuspendedPageProxy(
> 
>     // If the suspended page was not consumed before getting destroyed, then close the corresponding page
>     // on the WebProcess side.
>-    m_process->send(Messages::WebPage::Close(), m_page.pageID());
>+    close();
> 
>     if (m_suspensionState == SuspensionState::Suspending)
>         m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
>@@ -139,6 +140,17 @@ void SuspendedPageProxy::unsuspend()
>     m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
> }
> 
>+void SuspendedPageProxy::close()
>+{
>+    ASSERT(m_suspensionState != SuspensionState::Resumed);
>+
>+    if (m_isClosed)
>+        return;
>+
>+    m_isClosed = true;
>+    m_process->send(Messages::WebPage::Close(), m_page.pageID());
>+}
>+
> void SuspendedPageProxy::didProcessRequestToSuspend(SuspensionState newSuspensionState)
> {
>     LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
>@@ -154,6 +166,15 @@ void SuspendedPageProxy::didProcessReque
> 
>     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
> 
>+    bool shouldKeepOnFailure = false;
>+#if PLATFORM(MAC)
>+    // With web process side tiles, we need to keep the suspended page around on failure to avoid flashing.
>+    // It is removed by WebPageProxy::enterAcceleratedCompositingMode when the target page is ready.
>+    shouldKeepOnFailure = m_page.drawingArea() && m_page.drawingArea()->type() == DrawingAreaTypeTiledCoreAnimation;
>+#endif
>+    if (m_suspensionState == SuspensionState::FailedToSuspend && !shouldKeepOnFailure)
>+        close();
>+
>     if (m_readyToUnsuspendHandler)
>         m_readyToUnsuspendHandler(this);
> }
>Index: Source/WebKit/UIProcess/SuspendedPageProxy.h
>===================================================================
>--- Source/WebKit/UIProcess/SuspendedPageProxy.h	(revision 240407)
>+++ Source/WebKit/UIProcess/SuspendedPageProxy.h	(working copy)
>@@ -52,6 +52,7 @@ public:
> 
>     void waitUntilReadyToUnsuspend(CompletionHandler<void(SuspendedPageProxy*)>&&);
>     void unsuspend();
>+    void close();
> 
> #if !LOG_DISABLED
>     const char* loggingString() const;
>@@ -69,6 +70,7 @@ private:
>     Ref<WebProcessProxy> m_process;
>     uint64_t m_mainFrameID;
>     String m_registrableDomain;
>+    bool m_isClosed { false };
> 
>     SuspensionState m_suspensionState { SuspensionState::Suspending };
>     CompletionHandler<void(SuspendedPageProxy*)> m_readyToUnsuspendHandler;
>Index: Source/WebKit/UIProcess/WebPageProxy.cpp
>===================================================================
>--- Source/WebKit/UIProcess/WebPageProxy.cpp	(revision 240407)
>+++ Source/WebKit/UIProcess/WebPageProxy.cpp	(working copy)
>@@ -6847,7 +6847,12 @@ void WebPageProxy::isJITEnabled(Completi
> 
> void WebPageProxy::enterAcceleratedCompositingMode(const LayerTreeContext& layerTreeContext)
> {
>+#if PLATFORM(MAC)
>+    ASSERT(m_drawingArea->type() == DrawingAreaTypeTiledCoreAnimation);
>+#endif
>     pageClient().enterAcceleratedCompositingMode(layerTreeContext);
>+    // We needed the failed suspended page to stay alive to avoid flashing. Now we can get rid of it.
>+    m_process->processPool().closeFailedSuspendedPagesForPage(*this);
> }
> 
> void WebPageProxy::exitAcceleratedCompositingMode()
>Index: Source/WebKit/UIProcess/WebProcessPool.cpp
>===================================================================
>--- Source/WebKit/UIProcess/WebProcessPool.cpp	(revision 240407)
>+++ Source/WebKit/UIProcess/WebProcessPool.cpp	(working copy)
>@@ -2262,6 +2262,14 @@ void WebProcessPool::removeAllSuspendedP
>     });
> }
> 
>+void WebProcessPool::closeFailedSuspendedPagesForPage(WebPageProxy& page)
>+{
>+    for (auto& suspendedPage : m_suspendedPages) {
>+        if (&suspendedPage->page() == &page && suspendedPage->failedToSuspend())
>+            suspendedPage->close();
>+    }
>+}
>+
> std::unique_ptr<SuspendedPageProxy> WebProcessPool::takeSuspendedPage(SuspendedPageProxy& suspendedPage)
> {
>     return m_suspendedPages.takeFirst([&suspendedPage](auto& item) {
>Index: Source/WebKit/UIProcess/WebProcessPool.h
>===================================================================
>--- Source/WebKit/UIProcess/WebProcessPool.h	(revision 240407)
>+++ Source/WebKit/UIProcess/WebProcessPool.h	(working copy)
>@@ -447,6 +447,7 @@ public:
>     // SuspendedPageProxy management.
>     void addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&&);
>     void removeAllSuspendedPagesForPage(WebPageProxy&);
>+    void closeFailedSuspendedPagesForPage(WebPageProxy&);
>     std::unique_ptr<SuspendedPageProxy> takeSuspendedPage(SuspendedPageProxy&);
>     void removeSuspendedPage(SuspendedPageProxy&);
>     bool hasSuspendedPageFor(WebProcessProxy&) const;
>Index: Source/WebKit/WebProcess/WebPage/WebPage.cpp
>===================================================================
>--- Source/WebKit/WebProcess/WebPage/WebPage.cpp	(revision 240407)
>+++ Source/WebKit/WebProcess/WebPage/WebPage.cpp	(working copy)
>@@ -1340,7 +1340,6 @@ void WebPage::sendClose()
> void WebPage::suspendForProcessSwap()
> {
>     auto failedToSuspend = [this, protectedThis = makeRef(*this)] {
>-        close();
>         send(Messages::WebPageProxy::DidFailToSuspendAfterProcessSwap());
>     };
> 
>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 240427)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-01-24  Antti Koivisto  <antti@apple.com>
>+
>+        [PSON] Flash on back navigation on Mac
>+        https://bugs.webkit.org/show_bug.cgi?id=193716
>+        <rdar://problem/47148458>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
>+        Closing of the previous page is delayed so waiting for didFinishNavigation is
>+        not sufficient to gurantee we have received all the messages. Wait for them.
>+
> 2019-01-23  David Kilzer  <ddkilzer@apple.com>
> 
>         check-webkit-style should warn when using soft-linking macros in a header
>Index: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>===================================================================
>--- Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm	(revision 240407)
>+++ Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm	(working copy)
>@@ -2340,6 +2340,11 @@ TEST(ProcessSwap, PageShowHide)
>     TestWebKitAPI::Util::run(&done);
>     done = false;
> 
>+    while ([receivedMessages count] < 7) {
>+        TestWebKitAPI::Util::run(&receivedMessage);
>+        receivedMessage = false;
>+    }
>+
>     EXPECT_EQ(7u, [receivedMessages count]);
>     EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html - pageshow NOT persisted", receivedMessages.get()[0]);
>     if ([receivedMessages.get()[1] hasPrefix:@"pson://www.webkit.org/main.html"]) {
>@@ -2430,6 +2435,11 @@ TEST(ProcessSwap, LoadUnload)
>     TestWebKitAPI::Util::run(&done);
>     done = false;
> 
>+    while ([receivedMessages count] < 7) {
>+        TestWebKitAPI::Util::run(&receivedMessage);
>+        receivedMessage = false;
>+    }
>+
>     EXPECT_EQ(7u, [receivedMessages count]);
>     EXPECT_WK_STREQ(@"pson://www.webkit.org/main.html - load", receivedMessages.get()[0]);
>     if ([receivedMessages.get()[1] hasPrefix:@"pson://www.webkit.org/main.html"]) {

Flags:

cdumez: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 193716: 359883 | 359894 | 360002 | 360003 | 360019