Title: [276513] trunk/Source/WebCore
- Revision
- 276513
- Author
- [email protected]
- Date
- 2021-04-23 12:51:44 -0700 (Fri, 23 Apr 2021)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (276512 => 276513)
--- trunk/Source/WebCore/ChangeLog 2021-04-23 19:34:35 UTC (rev 276512)
+++ trunk/Source/WebCore/ChangeLog 2021-04-23 19:51:44 UTC (rev 276513)
@@ -1,3 +1,38 @@
+2021-04-23 Michael Catanzaro <[email protected]>
+
+ 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 <[email protected]>
Remove decoder memory allocations based on untrusted data (sizes) in the stream; related changes
Modified: trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp (276512 => 276513)
--- trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp 2021-04-23 19:34:35 UTC (rev 276512)
+++ trunk/Source/WebCore/platform/graphics/GraphicsLayer.cpp 2021-04-23 19:51:44 UTC (rev 276513)
@@ -200,7 +200,7 @@
}
removeAllChildren();
- removeFromParent();
+ removeFromParentInternal();
}
void GraphicsLayer::clearClient()
@@ -335,7 +335,7 @@
}
}
-void GraphicsLayer::removeFromParent()
+void GraphicsLayer::removeFromParentInternal()
{
if (m_parent) {
GraphicsLayer* parent = m_parent;
@@ -373,6 +373,13 @@
m_childrenTransform = makeUnique<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)
Modified: trunk/Source/WebCore/platform/graphics/GraphicsLayer.h (276512 => 276513)
--- trunk/Source/WebCore/platform/graphics/GraphicsLayer.h 2021-04-23 19:34:35 UTC (rev 276512)
+++ trunk/Source/WebCore/platform/graphics/GraphicsLayer.h 2021-04-23 19:51:44 UTC (rev 276513)
@@ -638,7 +638,7 @@
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
@@ -659,6 +659,8 @@
virtual bool shouldRepaintOnSizeChange() const { return drawsContent(); }
+ void removeFromParentInternal();
+
// The layer being replicated.
GraphicsLayer* replicatedLayer() const { return m_replicatedLayer; }
virtual void setReplicatedLayer(GraphicsLayer* layer) { m_replicatedLayer = layer; }
Modified: trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp (276512 => 276513)
--- trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp 2021-04-23 19:34:35 UTC (rev 276512)
+++ trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp 2021-04-23 19:51:44 UTC (rev 276513)
@@ -451,12 +451,6 @@
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.
@@ -488,7 +482,10 @@
removeCloneLayers();
- GraphicsLayer::willBeDestroyed();
+ if (m_parent)
+ downcast<GraphicsLayerCA>(*m_parent).noteSublayersChanged();
+
+ willBeDestroyed();
}
void GraphicsLayerCA::setName(const String& name)
Modified: trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h (276512 => 276513)
--- trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h 2021-04-23 19:34:35 UTC (rev 276512)
+++ trunk/Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h 2021-04-23 19:51:44 UTC (rev 276513)
@@ -191,8 +191,6 @@
private:
bool isGraphicsLayerCA() const override { return true; }
- WEBCORE_EXPORT void willBeDestroyed() override;
-
// PlatformCALayerClient overrides
void platformCALayerLayoutSublayersOfLayer(PlatformCALayer*) override { }
bool platformCALayerRespondsToLayoutChanges() const override { return false; }
Modified: trunk/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp (276512 => 276513)
--- trunk/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp 2021-04-23 19:34:35 UTC (rev 276512)
+++ trunk/Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp 2021-04-23 19:51:44 UTC (rev 276513)
@@ -167,6 +167,8 @@
ASSERT(!m_nicosia.backingStore);
if (m_animatedBackingStoreHost)
m_animatedBackingStoreHost->layerWillBeDestroyed();
+ if (CoordinatedGraphicsLayer* parentLayer = downcast<CoordinatedGraphicsLayer>(parent()))
+ parentLayer->didChangeChildren();
willBeDestroyed();
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
