Regression(r240562) Audio sometimes keeps playing in previous process after a process...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 19:56:44 +0000 (19:56 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Apr 2019 19:56:44 +0000 (19:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196774
<rdar://problem/49460572>

Reviewed by Alex Christensen.

r240562 added logic to prevent flashing on navigation. When we receive the DidFailToSuspendAfterProcessSwap
IPC from the previous process, we would delay closing the WebPage in that process until EnterAcceleratedCompositingMode
IPC is received from the new process. The issue is that this was racy as we would receive the EnterAcceleratedCompositingMode
IPC from the new process *before* receiving the DidFailToSuspendAfterProcessSwap IPC from the previous process, which which
case we would fail to close the WebPage and audio could keep playing.

To address the issue, the WebPageProxy keeps track of its last suspended page and notifies it whenever it receives the
EnterAcceleratedCompositingMode IPC. If the suspended page already received the DidFailToSuspendAfterProcessSwap IPC, it
will close the page. Otherwise, it will set a boolean data member indicating that we should not delay page closing when
the DidFailToSuspendAfterProcessSwap is eventually received.

* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::SuspendedPageProxy):
(WebKit::SuspendedPageProxy::pageEnteredAcceleratedCompositingMode):
(WebKit::SuspendedPageProxy::didProcessRequestToSuspend):
* UIProcess/SuspendedPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::suspendCurrentPageIfPossible):
(WebKit::WebPageProxy::enterAcceleratedCompositingMode):
* UIProcess/WebPageProxy.h:
* UIProcess/WebProcessPool.cpp:
* UIProcess/WebProcessPool.h:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/SuspendedPageProxy.cpp
Source/WebKit/UIProcess/SuspendedPageProxy.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessPool.h

index 58de1a1..3920df8 100644 (file)
@@ -1,3 +1,34 @@
+2019-04-10  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r240562) Audio sometimes keeps playing in previous process after a process-swap
+        https://bugs.webkit.org/show_bug.cgi?id=196774
+        <rdar://problem/49460572>
+
+        Reviewed by Alex Christensen.
+
+        r240562 added logic to prevent flashing on navigation. When we receive the DidFailToSuspendAfterProcessSwap
+        IPC from the previous process, we would delay closing the WebPage in that process until EnterAcceleratedCompositingMode
+        IPC is received from the new process. The issue is that this was racy as we would receive the EnterAcceleratedCompositingMode
+        IPC from the new process *before* receiving the DidFailToSuspendAfterProcessSwap IPC from the previous process, which which
+        case we would fail to close the WebPage and audio could keep playing.
+
+        To address the issue, the WebPageProxy keeps track of its last suspended page and notifies it whenever it receives the
+        EnterAcceleratedCompositingMode IPC. If the suspended page already received the DidFailToSuspendAfterProcessSwap IPC, it
+        will close the page. Otherwise, it will set a boolean data member indicating that we should not delay page closing when
+        the DidFailToSuspendAfterProcessSwap is eventually received.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
+        (WebKit::SuspendedPageProxy::pageEnteredAcceleratedCompositingMode):
+        (WebKit::SuspendedPageProxy::didProcessRequestToSuspend):
+        * UIProcess/SuspendedPageProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
+        (WebKit::WebPageProxy::enterAcceleratedCompositingMode):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebProcessPool.cpp:
+        * UIProcess/WebProcessPool.h:
+
 2019-04-10  Timothy Hatcher  <timothy@apple.com>
 
         WKScrollView background color does not match WKWebView before content is loaded.
index c118923..8d49480 100644 (file)
@@ -82,6 +82,9 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&
     : m_page(page)
     , m_process(WTFMove(process))
     , m_mainFrameID(mainFrameID)
+#if PLATFORM(MAC)
+    , m_shouldDelayClosingOnFailure(m_page.drawingArea() && m_page.drawingArea()->type() == DrawingAreaTypeTiledCoreAnimation)
+#endif
     , m_suspensionTimeoutTimer(RunLoop::main(), this, &SuspendedPageProxy::suspensionTimedOut)
 #if PLATFORM(IOS_FAMILY)
     , m_suspensionToken(m_process->throttler().backgroundActivityToken())
@@ -160,6 +163,16 @@ void SuspendedPageProxy::close()
     m_process->send(Messages::WebPage::Close(), m_page.pageID());
 }
 
+void SuspendedPageProxy::pageEnteredAcceleratedCompositingMode()
+{
+    m_shouldDelayClosingOnFailure = false;
+
+    if (m_suspensionState == SuspensionState::FailedToSuspend) {
+        // We needed the failed suspended page to stay alive to avoid flashing. Now we can get rid of it.
+        m_process->processPool().removeSuspendedPage(*this); // Will destroy |this|.
+    }
+}
+
 void SuspendedPageProxy::didProcessRequestToSuspend(SuspensionState newSuspensionState)
 {
     LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
@@ -177,13 +190,7 @@ void SuspendedPageProxy::didProcessRequestToSuspend(SuspensionState newSuspensio
 
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
 
-    bool shouldDelayClosingOnFailure = 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.
-    shouldDelayClosingOnFailure = m_page.drawingArea() && m_page.drawingArea()->type() == DrawingAreaTypeTiledCoreAnimation;
-#endif
-    if (m_suspensionState == SuspensionState::FailedToSuspend && !shouldDelayClosingOnFailure)
+    if (m_suspensionState == SuspensionState::FailedToSuspend && !m_shouldDelayClosingOnFailure)
         close();
 
     if (m_readyToUnsuspendHandler)
index 927fa7d..8da2fa2 100644 (file)
@@ -53,6 +53,8 @@ public:
     void unsuspend();
     void close();
 
+    void pageEnteredAcceleratedCompositingMode();
+
 #if !LOG_DISABLED
     const char* loggingString() const;
 #endif
@@ -70,6 +72,7 @@ private:
     Ref<WebProcessProxy> m_process;
     uint64_t m_mainFrameID;
     bool m_isClosed { false };
+    bool m_shouldDelayClosingOnFailure { false };
 
     SuspensionState m_suspensionState { SuspensionState::Suspending };
     CompletionHandler<void(SuspendedPageProxy*)> m_readyToUnsuspendHandler;
index 530fd82..0f337b1 100644 (file)
@@ -738,6 +738,8 @@ void WebPageProxy::launchProcess(const RegistrableDomain& registrableDomain)
 
 bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Optional<uint64_t> mainFrameID, ProcessSwapRequestedByClient processSwapRequestedByClient)
 {
+    m_lastSuspendedPage = nullptr;
+
     if (!mainFrameID)
         return false;
 
@@ -780,6 +782,7 @@ bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, Opt
     if (fromItem && m_preferences->usesPageCache())
         fromItem->setSuspendedPage(suspendedPage.get());
 
+    m_lastSuspendedPage = makeWeakPtr(*suspendedPage);
     m_process->processPool().addSuspendedPage(WTFMove(suspendedPage));
     return true;
 }
@@ -6771,6 +6774,7 @@ void WebPageProxy::resetState(ResetStateReason resetStateReason)
     m_mainFrame = nullptr;
     m_focusedFrame = nullptr;
     m_frameSetLargestFrame = nullptr;
+    m_lastSuspendedPage = nullptr;
 
 #if PLATFORM(COCOA)
     m_scrollingPerformanceData = nullptr;
@@ -7087,8 +7091,9 @@ void WebPageProxy::enterAcceleratedCompositingMode(const LayerTreeContext& layer
     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);
+
+    if (m_lastSuspendedPage)
+        m_lastSuspendedPage->pageEnteredAcceleratedCompositingMode();
 }
 
 void WebPageProxy::exitAcceleratedCompositingMode()
index 41a4322..23e824f 100644 (file)
@@ -2468,6 +2468,7 @@ WEBPAGEPROXY_LOADOPTIMIZER_ADDITIONS_2
     bool m_mayHaveUniversalFileReadSandboxExtension { false };
 
     std::unique_ptr<ProvisionalPageProxy> m_provisionalPage;
+    WeakPtr<SuspendedPageProxy> m_lastSuspendedPage;
 
 #if HAVE(PENCILKIT)
     std::unique_ptr<EditableImageController> m_editableImageController;
index 29b4b5a..2a60469 100644 (file)
@@ -2402,14 +2402,6 @@ void WebProcessPool::removeAllSuspendedPagesForPage(WebPageProxy& page, WebProce
     });
 }
 
-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 47834e7..3bb1f75 100644 (file)
@@ -471,7 +471,6 @@ public:
     // SuspendedPageProxy management.
     void addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&&);
     void removeAllSuspendedPagesForPage(WebPageProxy&, WebProcessProxy* = nullptr);
-    void closeFailedSuspendedPagesForPage(WebPageProxy&);
     std::unique_ptr<SuspendedPageProxy> takeSuspendedPage(SuspendedPageProxy&);
     void removeSuspendedPage(SuspendedPageProxy&);
     bool hasSuspendedPageFor(WebProcessProxy&, WebPageProxy&) const;