[PSON] Avoid tearing down the drawing area when suspending a WebPage due to process...

https://bugs.webkit.org/show_bug.cgi?id=190879

Reviewed by Antti Koivisto.

Avoid tearing down the drawing area when suspending a WebPage due to process-swap. We really only need to reset
the drawing area upon resuming the WebPage. There is no strict need to destroy the drawing area on suspension
and this has caused various crashes because code usually assumes we always have a drawing area.

This patch also drops various drawing area null checks that were added to address PSON crashes.

* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::tearDownDrawingAreaInWebProcess): Deleted.
* UIProcess/SuspendedPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::suspendCurrentPageIfPossible):
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
(WebKit::WebPageProxy::didCompletePageTransition):
(WebKit::WebPageProxy::enterAcceleratedCompositingMode):
* UIProcess/WebPageProxy.h:
* WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::invalidateContentsAndRootView):
(WebKit::WebChromeClient::invalidateContentsForSlowScroll):
(WebKit::WebChromeClient::contentsSizeChanged const):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage):
* WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
(WebKit::RemoteScrollingCoordinator::scheduleTreeStateCommit):
* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::reinitializeWebPage):
(WebKit::WebPage::exitAcceleratedCompositingMode):
(WebKit::WebPage::setIsSuspended):
(WebKit::WebPage::tearDownDrawingAreaForSuspend): Deleted.
* WebProcess/WebPage/WebPage.h:
* WebProcess/WebPage/WebPage.messages.in:

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

diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index bcd8222..a496bd1 100644 (file)
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,41 @@
+2018-10-26  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Avoid tearing down the drawing area when suspending a WebPage due to process-swap
+        https://bugs.webkit.org/show_bug.cgi?id=190879
+
+        Reviewed by Antti Koivisto.
+
+        Avoid tearing down the drawing area when suspending a WebPage due to process-swap. We really only need to reset
+        the drawing area upon resuming the WebPage. There is no strict need to destroy the drawing area on suspension
+        and this has caused various crashes because code usually assumes we always have a drawing area.
+
+        This patch also drops various drawing area null checks that were added to address PSON crashes.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::tearDownDrawingAreaInWebProcess): Deleted.
+        * UIProcess/SuspendedPageProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
+        (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+        (WebKit::WebPageProxy::didCompletePageTransition):
+        (WebKit::WebPageProxy::enterAcceleratedCompositingMode):
+        * UIProcess/WebPageProxy.h:
+        * WebProcess/WebCoreSupport/WebChromeClient.cpp:
+        (WebKit::WebChromeClient::invalidateContentsAndRootView):
+        (WebKit::WebChromeClient::invalidateContentsForSlowScroll):
+        (WebKit::WebChromeClient::contentsSizeChanged const):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage):
+        * WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm:
+        (WebKit::RemoteScrollingCoordinator::scheduleTreeStateCommit):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::reinitializeWebPage):
+        (WebKit::WebPage::exitAcceleratedCompositingMode):
+        (WebKit::WebPage::setIsSuspended):
+        (WebKit::WebPage::tearDownDrawingAreaForSuspend): Deleted.
+        * WebProcess/WebPage/WebPage.h:
+        * WebProcess/WebPage/WebPage.messages.in:
+
 2018-10-26  Antti Koivisto  <antti@apple.com>
 
         Use random() instead of begin() to limit cache sizes

diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
index 14c5327..afedf9f 100644 (file)
--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
@@ -103,11 +103,6 @@ SuspendedPageProxy::~SuspendedPageProxy()
     });
 }
 
-void SuspendedPageProxy::tearDownDrawingAreaInWebProcess()
-{
-    m_process->send(Messages::WebPage::TearDownDrawingAreaForSuspend(), m_page.pageID());
-}
-
 void SuspendedPageProxy::unsuspend()
 {
     ASSERT(m_isSuspended);

diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
index 0d5585e..7fa0c3e 100644 (file)
--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
@@ -46,8 +46,6 @@ public:
     uint64_t mainFrameID() const { return m_mainFrameID; }
     const WebCore::SecurityOriginData& origin() const { return m_origin; }
 
-    void tearDownDrawingAreaInWebProcess();
-
     void unsuspend();
 
 #if !LOG_DISABLED

diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
index 9dea798..3aec625 100644 (file)
--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
@@ -724,10 +724,14 @@ void WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, std
     }
 
     auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *currentItem, *mainFrameID);
+
+#if PLATFORM(MAC)
     m_pageSuspendedDueToCurrentNavigation = makeWeakPtr(suspendedPage.get());
-    m_process->processPool().addSuspendedPageProxy(WTFMove(suspendedPage));
+#endif
+
+    LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), suspendedPage->loggingString(), m_process->processIdentifier(), currentItem->itemID().logString());
 
-    LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), m_pageSuspendedDueToCurrentNavigation->loggingString(), m_process->processIdentifier(), currentItem->itemID().logString());
+    m_process->processPool().addSuspendedPageProxy(WTFMove(suspendedPage));
 }
 
 void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess)
@@ -2491,9 +2495,11 @@ void WebPageProxy::receivedNavigationPolicyDecision(PolicyAction policyAction, A
     }
     
     if (policyAction == PolicyAction::Use && frame.isMainFrame()) {
+#if PLATFORM(MAC)
         // We're about to navigate so we need to reset m_pageSuspendedDueToCurrentNavigation. If we decide to process swap,
         // continueNavigationInNewProcess() will take care of updating m_pageSuspendedDueToCurrentNavigation as needed.
         m_pageSuspendedDueToCurrentNavigation = nullptr;
+#endif
 
         String reason;
         auto proposedProcess = process().processPool().processForNavigation(*this, *navigation, processSwapRequestedByClient, policyAction, reason);
@@ -3516,19 +3522,18 @@ void WebPageProxy::didFinishProgress()
 
 void WebPageProxy::didCompletePageTransition(bool isInitialEmptyDocument)
 {
+#if PLATFORM(MAC)
     // Attach drawing area for the initial empty document only if this is not a process swap.
     if (isInitialEmptyDocument && m_pageSuspendedDueToCurrentNavigation)
         return;
 
-#if PLATFORM(MAC)
     if (!m_drawingArea)
         return;
 
     // Drawing area for the suspended page will be torn down when the attach completes.
     m_drawingArea->attachInWebProcess();
 #else
-    if (m_pageSuspendedDueToCurrentNavigation)
-        m_pageSuspendedDueToCurrentNavigation->tearDownDrawingAreaInWebProcess();
+    UNUSED_PARAM(isInitialEmptyDocument);
 #endif
 }
 
@@ -6425,10 +6430,6 @@ WebPageCreationParameters WebPageProxy::creationParameters()
 void WebPageProxy::enterAcceleratedCompositingMode(const LayerTreeContext& layerTreeContext)
 {
     pageClient().enterAcceleratedCompositingMode(layerTreeContext);
-
-    // We have completed the page transition and can tear down the layers in the suspended process.
-    if (m_pageSuspendedDueToCurrentNavigation)
-        m_pageSuspendedDueToCurrentNavigation->tearDownDrawingAreaInWebProcess();
 }
 
 void WebPageProxy::exitAcceleratedCompositingMode()

diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
index 9ce1e8f..656d5b5 100644 (file)
--- a/Source/WebKit/UIProcess/WebPageProxy.h
+++ b/Source/WebKit/UIProcess/WebPageProxy.h
@@ -2259,8 +2259,10 @@ private:
 
     std::optional<MonotonicTime> m_pageLoadStart;
 
+#if PLATFORM(MAC)
     // FIXME: We should try and get rid of this data member.
     WeakPtr<SuspendedPageProxy> m_pageSuspendedDueToCurrentNavigation;
+#endif
 
     RunLoop::Timer<WebPageProxy> m_resetRecentCrashCountTimer;
     unsigned m_recentCrashCount { 0 };

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
index 9c431f1..879b10a 100644 (file)
--- a/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp
@@ -523,8 +523,7 @@ void WebChromeClient::invalidateContentsAndRootView(const IntRect& rect)
             return;
     }
 
-    if (auto* drawingArea = m_page.drawingArea())
-        drawingArea->setNeedsDisplayInRect(rect);
+    m_page.drawingArea()->setNeedsDisplayInRect(rect);
 }
 
 void WebChromeClient::invalidateContentsForSlowScroll(const IntRect& rect)
@@ -542,8 +541,7 @@ void WebChromeClient::invalidateContentsForSlowScroll(const IntRect& rect)
         return;
     }
 #endif
-    if (auto* drawingArea = m_page.drawingArea())
-        drawingArea->setNeedsDisplayInRect(rect);
+    m_page.drawingArea()->setNeedsDisplayInRect(rect);
 }
 
 void WebChromeClient::scroll(const IntSize& scrollDelta, const IntRect& scrollRect, const IntRect& clipRect)
@@ -597,8 +595,7 @@ void WebChromeClient::contentsSizeChanged(Frame& frame, const IntSize& size) con
 
     m_page.send(Messages::WebPageProxy::DidChangeContentSize(size));
 
-    if (auto* drawingArea = m_page.drawingArea())
-        drawingArea->mainFrameContentSizeChanged(size);
+    m_page.drawingArea()->mainFrameContentSizeChanged(size);
 
     if (frameView && !frameView->delegatesScrolling())  {
         bool hasHorizontalScrollbar = frameView->horizontalScrollbar();

diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
index eb25673..c18fb09 100644 (file)
--- a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
@@ -1435,8 +1435,7 @@ void WebFrameLoaderClient::transitionToCommittedForNewPage()
     m_frame->coreFrame()->view()->setProhibitsScrolling(shouldDisableScrolling);
     m_frame->coreFrame()->view()->setVisualUpdatesAllowedByClient(!webPage->shouldExtendIncrementalRenderingSuppression());
 #if PLATFORM(COCOA)
-    if (webPage->drawingArea())
-        m_frame->coreFrame()->view()->setViewExposedRect(webPage->drawingArea()->viewExposedRect());
+    m_frame->coreFrame()->view()->setViewExposedRect(webPage->drawingArea()->viewExposedRect());
 #endif
 #if PLATFORM(IOS_FAMILY)
     m_frame->coreFrame()->view()->setDelegatesScrolling(true);

diff --git a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm
index 33d8dfd..18ef164 100644 (file)
--- a/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm
+++ b/Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteScrollingCoordinator.mm
@@ -61,8 +61,7 @@ RemoteScrollingCoordinator::~RemoteScrollingCoordinator()
 
 void RemoteScrollingCoordinator::scheduleTreeStateCommit()
 {
-    if (auto* drawingArea = m_webPage->drawingArea())
-        drawingArea->scheduleCompositingLayerFlush();
+    m_webPage->drawingArea()->scheduleCompositingLayerFlush();
 }
 
 bool RemoteScrollingCoordinator::coordinatesScrollingForFrameView(const FrameView& frameView) const

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
index 9e26dc1..05ffc85 100644 (file)
--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
@@ -653,7 +653,14 @@ void WebPage::enableEnumeratingAllNetworkInterfaces()
 
 void WebPage::reinitializeWebPage(WebPageCreationParameters&& parameters)
 {
-    if (!m_drawingArea) {
+    ASSERT(m_drawingArea);
+
+    if (m_shouldResetDrawingArea) {
+        // Make sure we destroy the previous drawing area before constructing the new one as DrawingArea registers / unregisters
+        // itself as an IPC::MesssageReceiver in its constructor / destructor.
+        m_drawingArea = nullptr;
+        m_shouldResetDrawingArea = false;
+
         m_drawingArea = DrawingArea::create(*this, parameters);
         m_drawingArea->setPaintingEnabled(false);
         m_drawingArea->setShouldScaleViewToFitDocument(parameters.shouldScaleViewToFitDocument);
@@ -1155,8 +1162,7 @@ void WebPage::enterAcceleratedCompositingMode(GraphicsLayer* layer)
 
 void WebPage::exitAcceleratedCompositingMode()
 {
-    if (m_drawingArea)
-        m_drawingArea->setRootCompositingLayer(0);
+    m_drawingArea->setRootCompositingLayer(0);
 }
 
 void WebPage::close()
@@ -6128,14 +6134,10 @@ void WebPage::setIsSuspended(bool suspended)
 
     m_isSuspended = suspended;
 
-    setLayerTreeStateIsFrozen(true);
-}
-
-void WebPage::tearDownDrawingAreaForSuspend()
-{
     if (!m_isSuspended)
-        return;
-    m_drawingArea = nullptr;
+        m_shouldResetDrawingArea = true;
+
+    setLayerTreeStateIsFrozen(true);
 }
 
 void WebPage::frameBecameRemote(uint64_t frameID, GlobalFrameIdentifier&& remoteFrameIdentifier, GlobalWindowIdentifier&& remoteWindowIdentifier)

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
index b2551e7..420ebdc 100644 (file)
--- a/Source/WebKit/WebProcess/WebPage/WebPage.h
+++ b/Source/WebKit/WebProcess/WebPage/WebPage.h
@@ -1424,7 +1424,6 @@ private:
     void urlSchemeTaskDidComplete(uint64_t handlerIdentifier, uint64_t taskIdentifier, const WebCore::ResourceError&);
 
     void setIsSuspended(bool);
-    void tearDownDrawingAreaForSuspend();
 
     RefPtr<WebImage> snapshotAtSize(const WebCore::IntRect&, const WebCore::IntSize& bitmapSize, SnapshotOptions);
     RefPtr<WebImage> snapshotNode(WebCore::Node&, SnapshotOptions, unsigned maximumPixelCount = std::numeric_limits<unsigned>::max());
@@ -1449,6 +1448,7 @@ private:
 
     WebCore::IntSize m_viewSize;
     std::unique_ptr<DrawingArea> m_drawingArea;
+    bool m_shouldResetDrawingArea { false };
 
     HashSet<PluginView*> m_pluginViews;
     bool m_hasSeenPlugin { false };

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.messages.in b/Source/WebKit/WebProcess/WebPage/WebPage.messages.in
index 6c87104..29f56af 100644 (file)
--- a/Source/WebKit/WebProcess/WebPage/WebPage.messages.in
+++ b/Source/WebKit/WebProcess/WebPage/WebPage.messages.in
@@ -512,7 +512,6 @@ messages -> WebPage LegacyReceiver {
     URLSchemeTaskDidComplete(uint64_t handlerIdentifier, uint64_t taskIdentifier, WebCore::ResourceError error)
 
     SetIsSuspended(bool suspended)
-    TearDownDrawingAreaForSuspend();
 
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
     StorageAccessResponse(bool wasGranted, uint64_t contextId)