Attachment 369726 Details for Bug 180232 – Patch

[patch] Patch

bug-180232-20190513082751.patch (text/plain), 9.19 KB, created by Michael Catanzaro on 2019-05-13 06:27:52 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Michael Catanzaro

Created: 2019-05-13 06:27:52 PDT

Size: 9.19 KB

patch

obsolete

>Subversion Revision: 245221
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index e8d144433f1e105561f6d4832511776d9c5f2759..0f2f7543544409b164ad7dfaeab2d0752f13057d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,41 @@
>+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):
>+        (WebCore::CoordinatedGraphicsLayer::doRemoveFromParent):
>+        (WebCore::CoordinatedGraphicsLayer::removeFromParent):
>+        * platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:
>+
> 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..a424aae98a141b81e42e3ea7cf855361ee7f74a5 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; }
>@@ -493,6 +491,8 @@ private:
>     bool appendToUncommittedAnimations(const KeyframeValueList&, const FilterOperation*, const Animation*, const String& animationName, int animationIndex, Seconds timeOffset);
>     void appendToUncommittedAnimations(LayerPropertyAnimation&&);
> 
>+    void doRemoveFromParent();
>+
>     enum LayerChange : uint64_t {
>         NoChange                                = 0,
>         NameChanged                             = 1LLU << 1,
>diff --git a/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp b/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
>index a95382c9d747959cb0de24bbd9b261a1c1fc0e7d..0de11982d2f8a3ca39b22596d068e5a427fd6287 100644
>--- a/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
>+++ b/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
>@@ -147,8 +147,11 @@ CoordinatedGraphicsLayer::~CoordinatedGraphicsLayer()
>         purgeBackingStores();
>         m_coordinator->detachLayer(this);
>     }
>+
>     ASSERT(!m_nicosia.imageBacking);
>     ASSERT(!m_nicosia.backingStore);
>+
>+    doRemoveFromParent();
>     willBeDestroyed();
> }
> 
>@@ -219,10 +222,15 @@ bool CoordinatedGraphicsLayer::replaceChild(GraphicsLayer* oldChild, Ref<Graphic
>     return true;
> }
> 
>-void CoordinatedGraphicsLayer::removeFromParent()
>+void CoordinatedGraphicsLayer::doRemoveFromParent()
> {
>     if (CoordinatedGraphicsLayer* parentLayer = downcast<CoordinatedGraphicsLayer>(parent()))
>         parentLayer->didChangeChildren();
>+}
>+
>+void CoordinatedGraphicsLayer::removeFromParent()
>+{
>+    doRemoveFromParent();
>     GraphicsLayer::removeFromParent();
> }
> 
>diff --git a/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h b/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h
>index b5177de8cec528866a1c80a8f69b5ca72dd2b2ec..ad93a085a203659bc59900f881c7ebaaaf57025a 100644
>--- a/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h
>+++ b/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h
>@@ -154,6 +154,8 @@ private:
> 
>     bool filtersCanBeComposited(const FilterOperations&) const;
> 
>+    void doRemoveFromParent();
>+
>     Nicosia::PlatformLayer::LayerID m_id;
>     GraphicsLayerTransform m_layerTransform;
>     TransformationMatrix m_cachedInverseTransform;

Actions: View | Formatted Diff | Diff

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