Changeset 276513 in webkit

Timestamp:

Apr 23, 2021, 12:51:44 PM (5 years ago)

Author:

Michael Catanzaro

Message:

Remove virtual function calls in GraphicsLayer destructors
​https://bugs.webkit.org/show_bug.cgi?id=180232

Reviewed by Adrian Perez de Castro.

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/GraphicsLayer.cpp (modified) (3 diffs)
• platform/graphics/GraphicsLayer.h (modified) (2 diffs)
• platform/graphics/ca/GraphicsLayerCA.cpp (modified) (2 diffs)
• platform/graphics/ca/GraphicsLayerCA.h (modified) (1 diff)
• platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-04-23  Michael Catanzaro  <mcatanzaro@igalia.com>
+
+        Remove virtual function calls in GraphicsLayer destructors
+        https://bugs.webkit.org/show_bug.cgi?id=180232
+
+        Reviewed by Adrian Perez de Castro.
+
+        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):
+
 2021-04-23  Darin Adler  <darin@apple.com>
 

trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp

@@ -201 +201 @@
 
     removeAllChildren();
-    removeFromParent();
+    removeFromParentInternal();
 }
 
@@ -336 +336 @@
 }
 
-void GraphicsLayer::removeFromParent()
+void GraphicsLayer::removeFromParentInternal()
 {
     if (m_parent) {
@@ -372 +372 @@
     else
         m_childrenTransform = makeUnique<TransformationMatrix>(matrix);
+}
+
+void GraphicsLayer::removeFromParent()
+{
+    // removeFromParentInternal is nonvirtual, for use in willBeDestroyed,
+    // which is called from destructors.
+    removeFromParentInternal();
 }
 

trunk/Source/WebCore/platform/graphics/GraphicsLayer.h

@@ -639 +639 @@
 
     // 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; }
 
@@ -659 +659 @@
 
     virtual bool shouldRepaintOnSizeChange() const { return drawsContent(); }
+
+    void removeFromParentInternal();
 
     // The layer being replicated.

trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp

@@ -452 +452 @@
         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
@@ -489 +483 @@
     removeCloneLayers();
 
-    GraphicsLayer::willBeDestroyed();
+    if (m_parent)
+        downcast<GraphicsLayerCA>(*m_parent).noteSublayersChanged();
+
+    willBeDestroyed();
 }
 

trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h

@@ -191 +191 @@
 private:
     bool isGraphicsLayerCA() const override { return true; }
-
-    WEBCORE_EXPORT void willBeDestroyed() override;
 
     // PlatformCALayerClient overrides

trunk/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp

@@ -168 +168 @@
     if (m_animatedBackingStoreHost)
         m_animatedBackingStoreHost->layerWillBeDestroyed();
+    if (CoordinatedGraphicsLayer* parentLayer = downcast<CoordinatedGraphicsLayer>(parent()))
+        parentLayer->didChangeChildren();
     willBeDestroyed();
 }