Changeset 41469 in webkit

Timestamp:

Mar 5, 2009 6:40:36 PM (15 years ago)

Author:

eric@webkit.org

Message:

Reviewed by David Hyatt.

Changes to RenderLayer destruction to hopefully help catch an elusive crasher
​https://bugs.webkit.org/show_bug.cgi?id=24409

Added a new RenderBoxModelObject::destroyLayer() call which is
now the only way which RenderLayers should ever be destroyed.
This ensures that the pointer to the layer is cleared in the
RenderObject after destruction, allowing us to ASSERT in the
RenderBoxModelObject destructor.

• rendering/RenderBox.cpp: (WebCore::RenderBox::calcAbsoluteHorizontalReplaced):
• rendering/RenderBoxModelObject.cpp: (WebCore::RenderBoxModelObject::~RenderBoxModelObject): (WebCore::RenderBoxModelObject::destroyLayer): (WebCore::RenderBoxModelObject::destroy): (WebCore::RenderBoxModelObject::styleDidChange):
• rendering/RenderBoxModelObject.h:
• rendering/RenderLayer.cpp: (WebCore::RenderLayer::stackingContext): (WebCore::RenderLayer::destroy): (WebCore::RenderLayer::removeOnlyThisLayer):
• rendering/RenderLayer.h:
• rendering/RenderObject.cpp: (WebCore::RenderObject::destroy):
• rendering/RenderWidget.cpp: (WebCore::RenderWidget::destroy):

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• rendering/RenderBox.cpp (modified) (1 diff)
• rendering/RenderBoxModelObject.cpp (modified) (3 diffs)
• rendering/RenderBoxModelObject.h (modified) (2 diffs)
• rendering/RenderLayer.cpp (modified) (3 diffs)
• rendering/RenderLayer.h (modified) (2 diffs)
• rendering/RenderObject.cpp (modified) (1 diff)
• rendering/RenderWidget.cpp (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-03-05  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by David Hyatt.
+
+        Changes to RenderLayer destruction to hopefully help catch an elusive crasher
+        https://bugs.webkit.org/show_bug.cgi?id=24409
+        
+        Added a new RenderBoxModelObject::destroyLayer() call which is
+        now the only way which RenderLayers should ever be destroyed.
+        This ensures that the pointer to the layer is cleared in the
+        RenderObject after destruction, allowing us to ASSERT in the
+        RenderBoxModelObject destructor.
+
+        * rendering/RenderBox.cpp:
+        (WebCore::RenderBox::calcAbsoluteHorizontalReplaced):
+        * rendering/RenderBoxModelObject.cpp:
+        (WebCore::RenderBoxModelObject::~RenderBoxModelObject):
+        (WebCore::RenderBoxModelObject::destroyLayer):
+        (WebCore::RenderBoxModelObject::destroy):
+        (WebCore::RenderBoxModelObject::styleDidChange):
+        * rendering/RenderBoxModelObject.h:
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::stackingContext):
+        (WebCore::RenderLayer::destroy):
+        (WebCore::RenderLayer::removeOnlyThisLayer):
+        * rendering/RenderLayer.h:
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::destroy):
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::destroy):
+
 2009-03-05  Eric Seidel  <eric@webkit.org>
 

trunk/WebCore/rendering/RenderBox.cpp

@@ -2355 +2355 @@
             RenderObject* po = parent();
             // 'staticX' should already have been set through layout of the parent.
-            int staticPosition = layer()->staticX() + containerWidth + toRenderBoxModelObject(containerBlock)->borderRight();
+            int staticPosition = layer()->staticX() + containerWidth + containerBlock->borderRight();
             for ( ; po && po != containerBlock; po = po->parent()) {
                 if (po->isBox())

trunk/WebCore/rendering/RenderBoxModelObject.cpp

@@ -51 +51 @@
 RenderBoxModelObject::~RenderBoxModelObject()
 {
+    // Our layer should have been destroyed and cleared by now
+    ASSERT(!hasLayer());
+    ASSERT(!m_layer);
+}
+
+void RenderBoxModelObject::destroyLayer()
+{
+    ASSERT(hasLayer());
+    ASSERT(m_layer);
+    m_layer->destroy(renderArena());
+    m_layer = 0;
+    setHasLayer(false);
 }
 
@@ -58 +70 @@
     if (m_layer)
         m_layer->clearClipRects();
+
+    // RenderObject::destroy calls back to destroyLayer() for layer destruction
     RenderObject::destroy();
 }
@@ -101 +115 @@
         }
     } else if (layer() && layer()->parent()) {
-        
-        RenderLayer* layer = m_layer;
-        m_layer = 0;
-        setHasLayer(false);
         setHasTransform(false); // Either a transform wasn't specified or the object doesn't support transforms, so just null out the bit.
         setHasReflection(false);
-        layer->removeOnlyThisLayer();
+        m_layer->removeOnlyThisLayer(); // calls destroyLayer() which clears m_layer
         if (s_wasFloating && isFloating())
             setChildNeedsLayout(true);

trunk/WebCore/rendering/RenderBoxModelObject.h

@@ -97 +97 @@
     int verticalPosition(bool firstLine) const;
 
+    // Called by RenderObject::destroy() (and RenderWidget::destroy()) and is the only way layers should ever be destroyed
+    void destroyLayer();
+
 protected:
     void calculateBackgroundImageGeometry(const FillLayer*, int tx, int ty, int w, int h, IntRect& destRect, IntPoint& phase, IntSize& tileSize);
@@ -124 +127 @@
 
 // This will catch anyone doing an unnecessary cast.
-void toRenderBoxModelObject(const RenderBox*);
+void toRenderBoxModelObject(const RenderBoxModelObject*);
 
 } // namespace WebCore

trunk/WebCore/rendering/RenderLayer.cpp

@@ -611 +611 @@
 }
 
-RenderLayer *RenderLayer::stackingContext() const
-{
-    RenderLayer* curr = parent();
-    for ( ; curr && !curr->renderer()->isRenderView() && !curr->renderer()->isRoot() &&
-          curr->renderer()->style()->hasAutoZIndex();
-          curr = curr->parent()) { }
-    return curr;
+RenderLayer* RenderLayer::stackingContext() const
+{
+    RenderLayer* layer = parent();
+    while (layer && !layer->renderer()->isRenderView() && !layer->renderer()->isRoot() && layer->renderer()->style()->hasAutoZIndex())
+        layer = layer->parent();
+    return layer;
 }
 
@@ -771 +770 @@
 
 void RenderLayer::destroy(RenderArena* renderArena)
-{    
+{
     delete this;
 
@@ -881 +880 @@
         current = next;
     }
-    
-    destroy(renderer()->renderArena());
+
+    m_renderer->destroyLayer();
 }
 

trunk/WebCore/rendering/RenderLayer.h

@@ -415 +415 @@
     bool has3DTransform() const { return m_transform && !m_transform->isAffine(); }
 
-    void destroy(RenderArena*);
-
      // Overloaded new operator.  Derived classes must override operator new
     // in order to allocate out of the RenderArena.
@@ -520 +518 @@
     friend class RenderLayerBacking;
     friend class RenderLayerCompositor;
+    friend class RenderBoxModelObject;
+
+    // Only safe to call from RenderBoxModelObject::destroyLayer(RenderArena*)
+    void destroy(RenderArena*);
 
 protected:

trunk/WebCore/rendering/RenderObject.cpp

@@ -1840 +1840 @@
     // FIXME: Would like to do this in RenderBoxModelObject, but the timing is so complicated that this can't easily
     // be moved into RenderBoxModelObject::destroy.
-    RenderArena* arena = renderArena();
     if (hasLayer())
-        toRenderBoxModelObject(this)->layer()->destroy(arena);
-    arenaDelete(arena, this);
+        toRenderBoxModelObject(this)->destroyLayer();
+    arenaDelete(renderArena(), this);
 }
 

trunk/WebCore/rendering/RenderWidget.cpp

@@ -92 +92 @@
         setOverrideSize(-1);
 
-    RenderArena* arena = renderArena();
-
-    if (hasLayer())
-        layer()->clearClipRects();
-
     if (style() && (style()->height().isPercent() || style()->minHeight().isPercent() || style()->maxHeight().isPercent()))
         RenderBlock::removePercentHeightDescendant(this);
 
+    if (hasLayer()) {
+        layer()->clearClipRects();
+        destroyLayer();
+    }
+
+    // Grab the arena from node()->document()->renderArena() before clearing the node pointer.
+    // Clear the node before deref-ing, as this may be deleted when deref is called.
+    RenderArena* arena = renderArena();
     setNode(0);
-
-    if (hasLayer())
-        layer()->destroy(arena);
-
     deref(arena);
 }