Attachment 362462 Details for Bug 194845 – Patch

[patch] Patch

bug-194845-20190219174514.patch (text/plain), 19.37 KB, created by Tim Horton on 2019-02-19 17:45:15 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Tim Horton

Created: 2019-02-19 17:45:15 PST

Size: 19.37 KB

patch

obsolete

>Subversion Revision: 241573
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 7f9cfdf0846ec9ae81232afbac75f699ae915a8b..9c10f57b0739799443adf5af5170a6ce14ed128c 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,61 @@
>+2019-02-19  Tim Horton  <timothy_horton@apple.com>
>+
>+        Crash under RemoteLayerTreePropertyApplier::applyProperties when reattaching to old process
>+        https://bugs.webkit.org/show_bug.cgi?id=194845
>+        <rdar://problem/47944579>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        No new tests (OOPS!). Yet.
>+
>+        * page/Page.cpp:
>+        (WebCore::Page::installedPageOverlaysChanged): Deleted.
>+        * page/Page.h:
>+        (WebCore::Page::pageOverlayController):
>+        Move this to PageOverlayController, like r240940 intended.
>+
>+        * page/PageOverlay.cpp:
>+        (WebCore::PageOverlay::willReattachToPage):
>+        * page/PageOverlay.h:
>+        Add a PageOverlay::Client method to inform clients when the overlay's
>+        page is reattached. Importantly, this means "destroy any outstanding owned GraphicsLayers".
>+
>+        * page/PageOverlayController.cpp:
>+        (WebCore::PageOverlayController::attachViewOverlayLayers):
>+        (WebCore::PageOverlayController::detachViewOverlayLayers):
>+        These are now only called internally, and only manipulate the root layer
>+        for view-relative overlays, like the name says.
>+
>+        (WebCore::PageOverlayController::installedPageOverlaysChanged):
>+        Moved from Page. Stop checking isInWindow; because the root layers are
>+        installed by the DrawingArea and RenderLayerCompositor, we can just install
>+        unconditionally and don't need to worry about in-window state.
>+        Also, fold updateForceSynchronousScrollLayerPositionUpdates in here.
>+
>+        (WebCore::PageOverlayController::installLayerForOverlay):
>+        Factor overlay layer creation out into its own function.
>+
>+        (WebCore::PageOverlayController::installPageOverlay):
>+        Adopt installLayerForOverlay and the local installedPageOverlaysChanged.
>+
>+        (WebCore::PageOverlayController::uninstallPageOverlay):
>+        Adopt the local installedPageOverlaysChanged.
>+
>+        (WebCore::PageOverlayController::recreatePageOverlayLayers):
>+        Tear down all page overlay related GraphicsLayers, first by removing
>+        the directly-owned layers for each overlay, then by informing the overlay
>+        client that we're going to reattach (see above), then by destroying the root
>+        layers. Then, re-create all of the individual overlay layers and call
>+        installedPageOverlaysChanged() to re-create and re-attach the root layers.
>+
>+        (WebCore::PageOverlayController::destroyRootLayers):
>+        (WebCore::PageOverlayController::willDetachRootLayer): Deleted.
>+        * page/PageOverlayController.h:
>+        * page/mac/ServicesOverlayController.h:
>+        * page/mac/ServicesOverlayController.mm:
>+        (WebCore::ServicesOverlayController::willReattachToPage):
>+        Invalidate old highlights, which will unparent and toss their GraphicsLayers.
>+
> 2019-02-14  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Occasional crash under WebCore::CSSStyleSheet::item called from Inspector
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index c6a2ae0bc0e555bec3950f28a4c3f3d01c75c426..66e35eaed15a4450cc79d0bdbbafe99eb025cd0b 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-02-19  Tim Horton  <timothy_horton@apple.com>
>+
>+        Crash under RemoteLayerTreePropertyApplier::applyProperties when reattaching to old process
>+        https://bugs.webkit.org/show_bug.cgi?id=194845
>+        <rdar://problem/47944579>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When re-attaching to an old Web Content process, we re-create the DrawingArea.
>+        If that process kept PlatformCALayerRemote instances alive, we can end up
>+        with layers from an old RemoteLayerTreeContext in the new layer tree.
>+        For normal page layers, this does not happen because RenderLayerCompositor
>+        is also torn down during the swap, but Page Overlays create and maintain
>+        layers outside of the normal flow.
>+
>+        Also, since we accumulate layer births from the PlatformCALayerRemote
>+        constructor, if we keep layers around and then try to switch *back* to
>+        the process, the UI process will have deleted them, and will not get
>+        a new creation notification. This results in a crash when looking up
>+        the layer. Avoid the crash with a null check.
>+
>+        In order to actually make Page Overlays installed before the swap continue
>+        working, introduce a variety of mechanisms to make them re-create their
>+        GraphicsLayers when we swap DrawingArea (and RemoteLayerTreeContext).
>+        See the WebCore ChangeLog for details.
>+
>+        * WebProcess/WebCoreSupport/WebInspectorClient.cpp:
>+        (WebKit::WebInspectorClient::willReattachToPage):
>+        * WebProcess/WebCoreSupport/WebInspectorClient.h:
>+        Unparent and toss GraphicsLayers for old repaint highlights.
>+
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::reinitializeWebPage):
>+        Rebuild all page overlay GraphicsLayers when re-creating the DrawingArea.
>+
> 2019-02-14  Tim Horton  <timothy_horton@apple.com>
> 
>         Fix the build.
>diff --git a/Source/WebCore/page/Page.cpp b/Source/WebCore/page/Page.cpp
>index 690735d4f0cb20c702b2b7b47d48bb2a322e9276..2b7dd0dba3c3036dcaff027d8a4c41cc4c053a64 100644
>--- a/Source/WebCore/page/Page.cpp
>+++ b/Source/WebCore/page/Page.cpp
>@@ -2641,19 +2641,6 @@ void Page::appearanceDidChange()
>     }
> }
> 
>-void Page::installedPageOverlaysChanged()
>-{
>-    if (isInWindow()) {
>-        if (pageOverlayController().hasViewOverlays())
>-            chrome().client().attachViewOverlayGraphicsLayer(&pageOverlayController().layerWithViewOverlays());
>-        else
>-            chrome().client().attachViewOverlayGraphicsLayer(nullptr);
>-    }
>-
>-    if (auto* frameView = mainFrame().view())
>-        frameView->setNeedsCompositingConfigurationUpdate();
>-}
>-
> void Page::setUnobscuredSafeAreaInsets(const FloatBoxExtent& insets)
> {
>     if (m_unobscuredSafeAreaInsets == insets)
>diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
>index 9464a8ddaf07a543f0b868ab083649caff7204b7..29d28c8f2c0b3b38731f53951e02c6a8be3d1f22 100644
>--- a/Source/WebCore/page/Page.h
>+++ b/Source/WebCore/page/Page.h
>@@ -413,8 +413,6 @@ public:
> 
>     WheelEventDeltaFilter* wheelEventDeltaFilter() { return m_recentWheelEventDeltaFilter.get(); }
>     PageOverlayController& pageOverlayController() { return *m_pageOverlayController; }
>-    
>-    void installedPageOverlaysChanged();
> 
> #if PLATFORM(MAC)
> #if ENABLE(SERVICE_CONTROLS) || ENABLE(TELEPHONE_NUMBER_DETECTION)
>diff --git a/Source/WebCore/page/PageOverlay.cpp b/Source/WebCore/page/PageOverlay.cpp
>index e2f97cc65f25ad4eeb4a9e74ab86a68120c0faaa..e27b1882133acadec8306f2cb0cce77d93d7bbb8 100644
>--- a/Source/WebCore/page/PageOverlay.cpp
>+++ b/Source/WebCore/page/PageOverlay.cpp
>@@ -153,6 +153,12 @@ void PageOverlay::setPage(Page* page)
>     m_fadeAnimationTimer.stop();
> }
> 
>+void PageOverlay::willReattachToPage(Page* page)
>+{
>+    ASSERT(m_page == page);
>+    m_client.willReattachToPage(*this, page);
>+}
>+
> void PageOverlay::setNeedsDisplay(const IntRect& dirtyRect)
> {
>     if (auto pageOverlayController = controller()) {
>diff --git a/Source/WebCore/page/PageOverlay.h b/Source/WebCore/page/PageOverlay.h
>index 6633037b2eb25b9c350ef79df6c91e8267327358..0eb0d21fff56209477edd8d209f28e7d9c881536 100644
>--- a/Source/WebCore/page/PageOverlay.h
>+++ b/Source/WebCore/page/PageOverlay.h
>@@ -53,6 +53,7 @@ public:
>     public:
>         virtual void willMoveToPage(PageOverlay&, Page*) = 0;
>         virtual void didMoveToPage(PageOverlay&, Page*) = 0;
>+        virtual void willReattachToPage(PageOverlay&, Page*) { }
>         virtual void drawRect(PageOverlay&, GraphicsContext&, const IntRect& dirtyRect) = 0;
>         virtual bool mouseEvent(PageOverlay&, const PlatformMouseEvent&) = 0;
>         virtual void didScrollFrame(PageOverlay&, Frame&) { }
>@@ -76,6 +77,7 @@ public:
>     virtual PageOverlayID pageOverlayID() const { return m_pageOverlayID; }
> 
>     void setPage(Page*);
>+    void willReattachToPage(Page*);
>     Page* page() const { return m_page; }
>     WEBCORE_EXPORT void setNeedsDisplay(const IntRect& dirtyRect);
>     WEBCORE_EXPORT void setNeedsDisplay();
>diff --git a/Source/WebCore/page/PageOverlayController.cpp b/Source/WebCore/page/PageOverlayController.cpp
>index 86a68529581683223cf02cedf865930e7661f5c3..c865231a2e47a948885f38f58bc7b1ff8cacec01 100644
>--- a/Source/WebCore/page/PageOverlayController.cpp
>+++ b/Source/WebCore/page/PageOverlayController.cpp
>@@ -85,14 +85,26 @@ bool PageOverlayController::hasViewOverlays() const
> 
> void PageOverlayController::attachViewOverlayLayers()
> {
>-    if (hasViewOverlays())
>-        m_page.chrome().client().attachViewOverlayGraphicsLayer(&layerWithViewOverlays());
>+    ASSERT(hasViewOverlays());
>+    m_page.chrome().client().attachViewOverlayGraphicsLayer(&layerWithViewOverlays());
> }
> 
> void PageOverlayController::detachViewOverlayLayers()
> {
>     m_page.chrome().client().attachViewOverlayGraphicsLayer(nullptr);
>-    willDetachRootLayer();
>+}
>+
>+void PageOverlayController::installedPageOverlaysChanged()
>+{
>+    if (hasViewOverlays())
>+        attachViewOverlayLayers();
>+    else
>+        detachViewOverlayLayers();
>+
>+    if (auto* frameView = m_page.mainFrame().view())
>+        frameView->setNeedsCompositingConfigurationUpdate();
>+
>+    updateForceSynchronousScrollLayerPositionUpdates();
> }
> 
> GraphicsLayer* PageOverlayController::documentOverlayRootLayer() const
>@@ -164,15 +176,8 @@ GraphicsLayer& PageOverlayController::layerWithViewOverlays()
>     return *m_viewOverlayRootLayer;
> }
> 
>-void PageOverlayController::installPageOverlay(PageOverlay& overlay, PageOverlay::FadeMode fadeMode)
>+void PageOverlayController::installLayerForOverlay(PageOverlay& overlay)
> {
>-    createRootLayersIfNeeded();
>-
>-    if (m_pageOverlays.contains(&overlay))
>-        return;
>-
>-    m_pageOverlays.append(&overlay);
>-
>     auto layer = GraphicsLayer::create(m_page.chrome().client().graphicsLayerFactory(), *this);
>     layer->setAnchorPoint({ });
>     layer->setBackgroundColor(overlay.backgroundColor());
>@@ -192,19 +197,29 @@ void PageOverlayController::installPageOverlay(PageOverlay& overlay, PageOverlay
>     auto& rawLayer = layer.get();
>     m_overlayGraphicsLayers.set(&overlay, WTFMove(layer));
> 
>-    updateForceSynchronousScrollLayerPositionUpdates();
>-
>-    overlay.setPage(&m_page);
>-
>     if (FrameView* frameView = m_page.mainFrame().view())
>         frameView->enterCompositingMode();
> 
>     updateOverlayGeometry(overlay, rawLayer);
>+}
>+
>+void PageOverlayController::installPageOverlay(PageOverlay& overlay, PageOverlay::FadeMode fadeMode)
>+{
>+    createRootLayersIfNeeded();
>+
>+    if (m_pageOverlays.contains(&overlay))
>+        return;
>+
>+    m_pageOverlays.append(&overlay);
>+
>+    installLayerForOverlay(overlay);
>+
>+    overlay.setPage(&m_page);
> 
>     if (fadeMode == PageOverlay::FadeMode::Fade)
>         overlay.startFadeInAnimation();
> 
>-    m_page.installedPageOverlaysChanged();
>+    installedPageOverlaysChanged();
> }
> 
> void PageOverlayController::uninstallPageOverlay(PageOverlay& overlay, PageOverlay::FadeMode fadeMode)
>@@ -222,8 +237,26 @@ void PageOverlayController::uninstallPageOverlay(PageOverlay& overlay, PageOverl
>     bool removed = m_pageOverlays.removeFirst(&overlay);
>     ASSERT_UNUSED(removed, removed);
> 
>-    updateForceSynchronousScrollLayerPositionUpdates();
>-    m_page.installedPageOverlaysChanged();
>+    installedPageOverlaysChanged();
>+}
>+
>+void PageOverlayController::recreatePageOverlayLayers()
>+{
>+    Vector<RefPtr<PageOverlay>> allOverlays = m_pageOverlays;
>+
>+    for (auto& overlay : m_pageOverlays) {
>+        if (auto optionalLayer = m_overlayGraphicsLayers.take(overlay.get()))
>+            optionalLayer.value()->removeFromParent();
>+
>+        overlay->willReattachToPage(&m_page);
>+    }
>+
>+    destroyRootLayers();
>+
>+    for (auto& overlay : m_pageOverlays)
>+        installLayerForOverlay(*overlay);
>+
>+    installedPageOverlaysChanged();
> }
> 
> void PageOverlayController::updateForceSynchronousScrollLayerPositionUpdates()
>@@ -272,8 +305,10 @@ GraphicsLayer& PageOverlayController::layerForOverlay(PageOverlay& overlay) cons
>     return *m_overlayGraphicsLayers.get(&overlay);
> }
> 
>-void PageOverlayController::willDetachRootLayer()
>+void PageOverlayController::destroyRootLayers()
> {
>+    detachViewOverlayLayers();
>+
>     GraphicsLayer::unparentAndClear(m_documentOverlayRootLayer);
>     GraphicsLayer::unparentAndClear(m_viewOverlayRootLayer);
> 
>diff --git a/Source/WebCore/page/PageOverlayController.h b/Source/WebCore/page/PageOverlayController.h
>index e61482bbd8a9caaadb75cacc13c7b000fe7e3df3..959df61381633b4e676218448d364974601ba25b 100644
>--- a/Source/WebCore/page/PageOverlayController.h
>+++ b/Source/WebCore/page/PageOverlayController.h
>@@ -47,9 +47,6 @@ public:
>     bool hasDocumentOverlays() const;
>     bool hasViewOverlays() const;
> 
>-    void attachViewOverlayLayers();
>-    void detachViewOverlayLayers();
>-
>     GraphicsLayer& layerWithDocumentOverlays();
>     GraphicsLayer& layerWithViewOverlays();
> 
>@@ -81,14 +78,24 @@ public:
>     WEBCORE_EXPORT bool copyAccessibilityAttributeBoolValueForPoint(String attribute, FloatPoint, bool& value);
>     WEBCORE_EXPORT Vector<String> copyAccessibilityAttributesNames(bool parameterizedNames);
> 
>+    WEBCORE_EXPORT void recreatePageOverlayLayers();
>+
> private:
>     void createRootLayersIfNeeded();
> 
>     WEBCORE_EXPORT GraphicsLayer* documentOverlayRootLayer() const;
>     WEBCORE_EXPORT GraphicsLayer* viewOverlayRootLayer() const;
> 
>+    void attachViewOverlayLayers();
>+    void detachViewOverlayLayers();
>+
>+    void destroyRootLayers();
>+
>+    void installedPageOverlaysChanged();
>+
>     void willDetachRootLayer();
> 
>+    void installLayerForOverlay(PageOverlay&);
>     void updateSettingsForLayer(GraphicsLayer&);
>     void updateForceSynchronousScrollLayerPositionUpdates();
> 
>diff --git a/Source/WebCore/page/mac/ServicesOverlayController.h b/Source/WebCore/page/mac/ServicesOverlayController.h
>index 8f8a5fb443eb23e4a3c7f9a8abe7a2a0f45f1893..4cb005e336b82e468d592d39c12090fd396951db 100644
>--- a/Source/WebCore/page/mac/ServicesOverlayController.h
>+++ b/Source/WebCore/page/mac/ServicesOverlayController.h
>@@ -97,6 +97,7 @@ private:
>     // PageOverlay::Client
>     void willMoveToPage(PageOverlay&, Page*) override;
>     void didMoveToPage(PageOverlay&, Page*) override;
>+    void willReattachToPage(PageOverlay&, Page*) override;
>     void drawRect(PageOverlay&, GraphicsContext&, const IntRect& dirtyRect) override;
>     bool mouseEvent(PageOverlay&, const PlatformMouseEvent&) override;
>     void didScrollFrame(PageOverlay&, Frame&) override;
>diff --git a/Source/WebCore/page/mac/ServicesOverlayController.mm b/Source/WebCore/page/mac/ServicesOverlayController.mm
>index c96bed10869f675dbf5145f9f3e0f4123f02fbe7..d2d7c65e96078879b4d446adbb86dd466c5a1f10 100644
>--- a/Source/WebCore/page/mac/ServicesOverlayController.mm
>+++ b/Source/WebCore/page/mac/ServicesOverlayController.mm
>@@ -226,6 +226,13 @@ void ServicesOverlayController::didMoveToPage(PageOverlay&, Page*)
> {
> }
> 
>+void ServicesOverlayController::willReattachToPage(PageOverlay&, Page*)
>+{
>+    for (auto& highlight : m_highlights)
>+        highlight->invalidate();
>+    m_highlights.clear();
>+}
>+
> static const uint8_t AlignmentNone = 0;
> static const uint8_t AlignmentLeft = 1 << 0;
> static const uint8_t AlignmentRight = 1 << 1;
>diff --git a/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm b/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm
>index 0d7387151e3718fd8e5a39d17c916849bc6f9fa4..989295b4310341a2563ac08f2fa856b4975df7a7 100644
>--- a/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm
>+++ b/Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm
>@@ -101,6 +101,8 @@ bool RemoteLayerTreeHost::updateLayerTree(const RemoteLayerTreeTransaction& tran
> 
>         auto* node = nodeForID(layerID);
>         ASSERT(node);
>+        if (!node)
>+            continue;
> 
>         if (properties.changedProperties.contains(RemoteLayerTreeTransaction::ClonedContentsChanged) && properties.clonedLayerID)
>             clonesToUpdate.append({ layerID, properties.clonedLayerID });
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp
>index 2966c3d5e4f4e8a8256a8a3c1cda1110a3e6a383..d7567ea84f4819d9bee05b39e61787bf7f807791 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.cpp
>@@ -225,6 +225,14 @@ void WebInspectorClient::didMoveToPage(PageOverlay&, Page*)
> {
> }
> 
>+void WebInspectorClient::willReattachToPage(PageOverlay&, Page*)
>+{
>+    for (auto& layer : m_paintRectLayers)
>+        layer->removeFromParent();
>+
>+    m_paintRectLayers.clear();
>+}
>+
> void WebInspectorClient::drawRect(PageOverlay&, WebCore::GraphicsContext& context, const WebCore::IntRect& /*dirtyRect*/)
> {
>     m_page->corePage()->inspectorController().drawHighlight(context);
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.h b/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.h
>index d5dabf160e2ad98d989961deb0a62fe71a44d09a..15d4a3353ee52840bb9a1a7d617658a1758d15ff 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.h
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebInspectorClient.h
>@@ -74,6 +74,7 @@ private:
>     // PageOverlay::Client
>     void willMoveToPage(WebCore::PageOverlay&, WebCore::Page*) override;
>     void didMoveToPage(WebCore::PageOverlay&, WebCore::Page*) override;
>+    void willReattachToPage(WebCore::PageOverlay&, WebCore::Page*) override;
>     void drawRect(WebCore::PageOverlay&, WebCore::GraphicsContext&, const WebCore::IntRect&) override;
>     bool mouseEvent(WebCore::PageOverlay&, const WebCore::PlatformMouseEvent&) override;
> 
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 331d5587819a6b5230ffe71bbffe2fc61774102f..84a7e97dfc9b2c647b01404dd6c47dc48b1b8392 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -186,6 +186,7 @@
> #include <WebCore/Page.h>
> #include <WebCore/PageCache.h>
> #include <WebCore/PageConfiguration.h>
>+#include <WebCore/PageOverlayController.h>
> #include <WebCore/PingLoader.h>
> #include <WebCore/PlatformKeyboardEvent.h>
> #include <WebCore/PlatformMediaSessionManager.h>
>@@ -695,7 +696,7 @@ void WebPage::reinitializeWebPage(WebPageCreationParameters&& parameters)
> 
>     if (m_shouldResetDrawingAreaAfterSuspend) {
>         // 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.
>+        // itself as an IPC::MessageReceiver in its constructor / destructor.
>         m_drawingArea = nullptr;
>         m_shouldResetDrawingAreaAfterSuspend = false;
> 
>@@ -705,6 +706,8 @@ void WebPage::reinitializeWebPage(WebPageCreationParameters&& parameters)
>         m_drawingArea->updatePreferences(parameters.store);
>         m_drawingArea->setPaintingEnabled(true);
>         unfreezeLayerTree(LayerTreeFreezeReason::PageSuspended);
>+
>+        m_page->pageOverlayController().recreatePageOverlayLayers();
>     }
> 
>     setViewLayoutSize(parameters.viewLayoutSize);

Actions: View | Formatted Diff | Diff

Attachments on bug 194845: 362461 | 362462 | 362469 | 362470 | 362474 | 362475 | 362476 | 362515 | 362573 | 362574 | 362578 | 362583