Attachment 369727 Details for Bug 180232 – Patch

[patch] Patch

bug-180232-20190513083844.patch (text/plain), 7.46 KB, created by Michael Catanzaro on 2019-05-13 06:38:45 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Michael Catanzaro

Created: 2019-05-13 06:38:45 PDT

Size: 7.46 KB

patch

obsolete

>Subversion Revision: 245221
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e8d144433f1e105561f6d4832511776d9c5f2759..081f62d8e00c1d6a2a07e8d87a857169b0d9cbb3 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-05-13  Michael Catanzaro  <mcatanzaro@igalia.com>
>+
>+        Remove virtual function calls in GraphicsLayer destructors
>+        https://bugs.webkit.org/show_bug.cgi?id=180232
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        I notice that ~CoordinatedGraphicsLayer makes a virtual function call to
>+        GraphicsLayer::willBeDestroyed, which makes a virtual function call to
>+        CoordinatedGraphicsLayer::removeFromParent. I think that the functions are being called as
>+        intended, because ~CoordinatedGraphicsLayer has not yet been fully destroyed. However, I'm
>+        reminded of Effective C++ item #9: Never call virtual functions during construction or
>+        destruction ("because such calls will never go to a more derived class than that of the
>+        currently executing constructor or destructor"). This code is almost certain to break if
>+        anyone tries in the future to subclass any of the existing subclasses of GraphicsLayer, so
>+        let's refactor it a bit. This doesn't fix anything, but my hope is that it will make the
>+        code a bit harder to break, and not the opposite.
>+
>+        The main risk here is that some reordering of operations is necessary. The derived class
>+        portion of removeFromParent must now be executed before willBeDestroyed. It can't happen
>+        after, because parent would already be unset by that point. It's hard to be certain that
>+        this won't break anything, but I think it should be fine.
>+
>+        * platform/graphics/GraphicsLayer.cpp:
>+        (WebCore::GraphicsLayer::willBeDestroyed):
>+        (WebCore::GraphicsLayer::removeFromParentInternal):
>+        (WebCore::GraphicsLayer::removeFromParent):
>+        * platform/graphics/GraphicsLayer.h:
>+        * platform/graphics/ca/GraphicsLayerCA.cpp:
>+        (WebCore::GraphicsLayerCA::~GraphicsLayerCA):
>+        (WebCore::GraphicsLayerCA::willBeDestroyed): Deleted.
>+        * platform/graphics/ca/GraphicsLayerCA.h:
>+        * platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:
>+        (WebCore::CoordinatedGraphicsLayer::~CoordinatedGraphicsLayer):
>+
> 2019-05-12  Simon Fraser  <simon.fraser@apple.com>
> 
>         When the set of backing-sharing layers changes, we need to issue a repaint
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.cpp b/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>index 2500fd462f040de4787dc5ce648c95bbcb186c42..1708c3c469764ee1337016616f19053273fa9e2d 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>+++ b/Source/WebCore/platform/graphics/GraphicsLayer.cpp
>@@ -190,7 +190,7 @@ void GraphicsLayer::willBeDestroyed()
>     }
> 
>     removeAllChildren();
>-    removeFromParent();
>+    removeFromParentInternal();
> }
> 
> void GraphicsLayer::clearClient()
>@@ -320,7 +320,7 @@ void GraphicsLayer::removeAllChildren()
>     }
> }
> 
>-void GraphicsLayer::removeFromParent()
>+void GraphicsLayer::removeFromParentInternal()
> {
>     if (m_parent) {
>         GraphicsLayer* parent = m_parent;
>@@ -358,6 +358,13 @@ void GraphicsLayer::setChildrenTransform(const TransformationMatrix& matrix)
>         m_childrenTransform = std::make_unique<TransformationMatrix>(matrix);
> }
> 
>+void GraphicsLayer::removeFromParent()
>+{
>+    // removeFromParentInternal is nonvirtual, for use in willBeDestroyed,
>+    // which is called from destructors.
>+    removeFromParentInternal();
>+}
>+
> void GraphicsLayer::setMaskLayer(RefPtr<GraphicsLayer>&& layer)
> {
>     if (layer == m_maskLayer)
>diff --git a/Source/WebCore/platform/graphics/GraphicsLayer.h b/Source/WebCore/platform/graphics/GraphicsLayer.h
>index de3cf58c8a7bad1aebd5fc95b82b66bd639f93d2..d990437e6f544f4bc29567c839153a78c21e3228 100644
>--- a/Source/WebCore/platform/graphics/GraphicsLayer.h
>+++ b/Source/WebCore/platform/graphics/GraphicsLayer.h
>@@ -623,7 +623,7 @@ protected:
>     WEBCORE_EXPORT explicit GraphicsLayer(Type, GraphicsLayerClient&);
> 
>     // Should be called from derived class destructors. Should call willBeDestroyed() on super.
>-    WEBCORE_EXPORT virtual void willBeDestroyed();
>+    WEBCORE_EXPORT void willBeDestroyed();
>     bool beingDestroyed() const { return m_beingDestroyed; }
> 
>     // This method is used by platform GraphicsLayer classes to clear the filters
>@@ -646,6 +646,8 @@ protected:
> 
>     virtual void setOpacityInternal(float) { }
> 
>+    void removeFromParentInternal();
>+
>     // The layer being replicated.
>     GraphicsLayer* replicatedLayer() const { return m_replicatedLayer; }
>     virtual void setReplicatedLayer(GraphicsLayer* layer) { m_replicatedLayer = layer; }
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>index 90c803c261713f6e042359d8eafbefcc6f87cfeb..74917e369c8ef57e3a2cd3c75a70a985f9bff889 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp
>@@ -436,12 +436,6 @@ GraphicsLayerCA::~GraphicsLayerCA()
>     if (UNLIKELY(isTrackingDisplayListReplay()))
>         layerDisplayListMap().remove(this);
> 
>-    // Do cleanup while we can still safely call methods on the derived class.
>-    willBeDestroyed();
>-}
>-
>-void GraphicsLayerCA::willBeDestroyed()
>-{
>     // We release our references to the PlatformCALayers here, but do not actively unparent them,
>     // since that will cause a commit and break our batched commit model. The layers will
>     // get released when the rootmost modified GraphicsLayerCA rebuilds its child layers.
>@@ -473,7 +467,10 @@ void GraphicsLayerCA::willBeDestroyed()
> 
>     removeCloneLayers();
> 
>-    GraphicsLayer::willBeDestroyed();
>+    if (m_parent)
>+        downcast<GraphicsLayerCA>(*m_parent).noteSublayersChanged();
>+
>+    willBeDestroyed();
> }
> 
> void GraphicsLayerCA::setName(const String& name)
>diff --git a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>index ebbfb52b5cee1e4d19855a4c35b275f2a8c8ec98..57bec6457ea889ca047689b5c1519ec5ccd1c820 100644
>--- a/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>+++ b/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h
>@@ -185,8 +185,6 @@ protected:
> private:
>     bool isGraphicsLayerCA() const override { return true; }
> 
>-    WEBCORE_EXPORT void willBeDestroyed() override;
>-
>     // PlatformCALayerClient overrides
>     void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*) override { }
>     bool platformCALayerRespondsToLayoutChanges() const override { return false; }
>diff --git a/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp b/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
>index a95382c9d747959cb0de24bbd9b261a1c1fc0e7d..6fdb3e67f4d30b356e4da3d463785df32761dc2b 100644
>--- a/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
>+++ b/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
>@@ -147,8 +147,13 @@ CoordinatedGraphicsLayer::~CoordinatedGraphicsLayer()
>         purgeBackingStores();
>         m_coordinator->detachLayer(this);
>     }
>+
>     ASSERT(!m_nicosia.imageBacking);
>     ASSERT(!m_nicosia.backingStore);
>+
>+    if (CoordinatedGraphicsLayer* parentLayer = downcast<CoordinatedGraphicsLayer>(parent()))
>+        parentLayer->didChangeChildren();
>+
>     willBeDestroyed();
> }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 180232: 328066 | 341697 | 355534 | 369726 | 369727 | 426588