Add ASSERTs to avoid querying dirtied z-index or normal flow lists on RenderLayer
authorjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 May 2012 23:56:08 +0000 (23:56 +0000)
committerjchaffraix@webkit.org <jchaffraix@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 May 2012 23:56:08 +0000 (23:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=84920

Reviewed by Simon Fraser.

Covered by existing tests in Debug (at least several time!).

This change adds some ASSERTs on RenderLayer that prevent any use of its lists if they
are dirtied.

On top of this change, we added an invariant that non-stacking contexts should have their
z-index lists NULL (instead of empty or NULL previously). This is enforced at
updateZOrderLists time as we now ensure that it is called in a timely manner.

* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::calculateLayerBounds):
Added call to updateLayersIfNeeded as we will query them later and there is no guarantee
that they are not dirty (we recurse in our children as part of calculateLayerBounds).
This was causing the new ASSERTs to trigger on css3/filter/ tests.

(WebCore::RenderLayer::dirtyZOrderLists):
Added a comment as to why we can't ASSERT that we are in a stacking context here.

(WebCore::RenderLayer::rebuildZOrderLists):
Added an ASSERT that we only rebuild z-index lists for dirtied stacking context.

(WebCore::RenderLayer::updateLayerListsIfNeeded):
Updated to ensure that the reflection layer has its layers updated too. This was triggering
the new ASSERTs on fast/runins/run-in-layer-not-removed-crash.html.

(WebCore::RenderLayer::updateCompositingAndLayerListsIfNeeded):
Updated to use the new isDirtyStackingContext function.

* rendering/RenderLayer.h:
(WebCore::RenderLayer::isDirtyStackingContext):
New helper function. Also made updateLayerListsIfNeeded() the only way
to update layer. That should prevent any misuse.

(WebCore::RenderLayer::posZOrderList):
(WebCore::RenderLayer::negZOrderList):
(WebCore::RenderLayer::normalFlowList):
ASSERT that we don't query any of the previous lists if they are dirty. Also
enforce the invariant that non-stacking contexts should have NULL z-index lists.

(WebCore::RenderLayer::clearZOrderLists):
New function to clearZOrderLists so that we can enfore the previous invariant.

(WebCore::RenderLayer::updateZOrderLists):
Updated to clear the dirty flag and the z-index lists for non-stacking context.

* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::addToOverlapMapRecursive):
(WebCore::RenderLayerCompositor::computeCompositingRequirements):
(WebCore::RenderLayerCompositor::rebuildCompositingLayerTree):
Removed the explicit ASSERTs.

(WebCore::RenderLayerCompositor::updateLayerTreeGeometry):
(WebCore::RenderLayerCompositor::canBeComposited):
Disabled compositing on RenderLayer in flow thread. Because flow thread's
RenderLayer are not collected as part of RenderLayer's lists and could be composited,
this was causing the new ASSERTs to trigger (e.g. on fast/regions/webkit-flow-renderer-layer.html).

* rendering/RenderTreeAsText.cpp:
(WebCore::writeLayers):
Updated to use updateLayerListsIfNeeded().

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@115913 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderLayerCompositor.cpp
Source/WebCore/rendering/RenderTreeAsText.cpp

index 11b6d7c88a2371fa6fd250796ea3cde599ad6332..b2770ce69478fce5a35a5380190bf660055bf203 100644 (file)
@@ -1,3 +1,71 @@
+2012-05-02  Julien Chaffraix  <jchaffraix@webkit.org>
+
+        Add ASSERTs to avoid querying dirtied z-index or normal flow lists on RenderLayer
+        https://bugs.webkit.org/show_bug.cgi?id=84920
+
+        Reviewed by Simon Fraser.
+
+        Covered by existing tests in Debug (at least several time!).
+
+        This change adds some ASSERTs on RenderLayer that prevent any use of its lists if they
+        are dirtied.
+
+        On top of this change, we added an invariant that non-stacking contexts should have their
+        z-index lists NULL (instead of empty or NULL previously). This is enforced at
+        updateZOrderLists time as we now ensure that it is called in a timely manner.
+
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::calculateLayerBounds):
+        Added call to updateLayersIfNeeded as we will query them later and there is no guarantee
+        that they are not dirty (we recurse in our children as part of calculateLayerBounds).
+        This was causing the new ASSERTs to trigger on css3/filter/ tests.
+
+        (WebCore::RenderLayer::dirtyZOrderLists):
+        Added a comment as to why we can't ASSERT that we are in a stacking context here.
+
+        (WebCore::RenderLayer::rebuildZOrderLists):
+        Added an ASSERT that we only rebuild z-index lists for dirtied stacking context.
+
+        (WebCore::RenderLayer::updateLayerListsIfNeeded):
+        Updated to ensure that the reflection layer has its layers updated too. This was triggering
+        the new ASSERTs on fast/runins/run-in-layer-not-removed-crash.html.
+
+        (WebCore::RenderLayer::updateCompositingAndLayerListsIfNeeded):
+        Updated to use the new isDirtyStackingContext function.
+
+        * rendering/RenderLayer.h:
+        (WebCore::RenderLayer::isDirtyStackingContext):
+        New helper function. Also made updateLayerListsIfNeeded() the only way
+        to update layer. That should prevent any misuse.
+
+        (WebCore::RenderLayer::posZOrderList):
+        (WebCore::RenderLayer::negZOrderList):
+        (WebCore::RenderLayer::normalFlowList):
+        ASSERT that we don't query any of the previous lists if they are dirty. Also
+        enforce the invariant that non-stacking contexts should have NULL z-index lists.
+
+        (WebCore::RenderLayer::clearZOrderLists):
+        New function to clearZOrderLists so that we can enfore the previous invariant.
+
+        (WebCore::RenderLayer::updateZOrderLists):
+        Updated to clear the dirty flag and the z-index lists for non-stacking context.
+
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::addToOverlapMapRecursive):
+        (WebCore::RenderLayerCompositor::computeCompositingRequirements):
+        (WebCore::RenderLayerCompositor::rebuildCompositingLayerTree):
+        Removed the explicit ASSERTs.
+
+        (WebCore::RenderLayerCompositor::updateLayerTreeGeometry):
+        (WebCore::RenderLayerCompositor::canBeComposited):
+        Disabled compositing on RenderLayer in flow thread. Because flow thread's
+        RenderLayer are not collected as part of RenderLayer's lists and could be composited,
+        this was causing the new ASSERTs to trigger (e.g. on fast/regions/webkit-flow-renderer-layer.html).
+
+        * rendering/RenderTreeAsText.cpp:
+        (WebCore::writeLayers):
+        Updated to use updateLayerListsIfNeeded().
+
 2012-05-02  Levi Weintraub  <leviw@chromium.org>
 
         Remove unused adjustForAbsoluteZoom method in RenderObject.h
index cbf5241699569905887e356d0f9425170c9d7098..8cf9a1c06938163f62f32af4ef510cee69eddaf3 100644 (file)
@@ -4162,6 +4162,8 @@ IntRect RenderLayer::calculateLayerBounds(const RenderLayer* layer, const Render
         }
     }
 
+    const_cast<RenderLayer*>(layer)->updateLayerListsIfNeeded();
+
     if (RenderLayer* reflection = layer->reflectionLayer()) {
         if (!reflection->isComposited()) {
             IntRect childUnionBounds = calculateLayerBounds(reflection, layer);
@@ -4169,7 +4171,7 @@ IntRect RenderLayer::calculateLayerBounds(const RenderLayer* layer, const Render
         }
     }
     
-    ASSERT(layer->isStackingContext() || (!layer->m_posZOrderList || !layer->m_posZOrderList->size()));
+    ASSERT(layer->isStackingContext() || (!layer->posZOrderList() || !layer->posZOrderList()->size()));
 
 #if !ASSERT_DISABLED
     LayerListMutationDetector mutationChecker(const_cast<RenderLayer*>(layer));
@@ -4456,6 +4458,8 @@ static inline bool compareZIndex(RenderLayer* first, RenderLayer* second)
 void RenderLayer::dirtyZOrderLists()
 {
     ASSERT(m_layerListMutationAllowed);
+    // We cannot assume that we are called on a stacking context as it
+    // is called when we just got demoted from being a stacking context.
 
     if (m_posZOrderList)
         m_posZOrderList->clear();
@@ -4493,6 +4497,7 @@ void RenderLayer::dirtyNormalFlowList()
 void RenderLayer::rebuildZOrderLists()
 {
     ASSERT(m_layerListMutationAllowed);
+    ASSERT(isDirtyStackingContext());
 
 #if USE(ACCELERATED_COMPOSITING)
     bool includeHiddenLayers = compositor()->inCompositingMode();
@@ -4565,13 +4570,18 @@ void RenderLayer::updateLayerListsIfNeeded()
 {
     updateZOrderLists();
     updateNormalFlowList();
+
+    if (RenderLayer* reflectionLayer = this->reflectionLayer()) {
+        reflectionLayer->updateZOrderLists();
+        reflectionLayer->updateNormalFlowList();
+    }
 }
 
 void RenderLayer::updateCompositingAndLayerListsIfNeeded()
 {
 #if USE(ACCELERATED_COMPOSITING)
     if (compositor()->inCompositingMode()) {
-        if ((isStackingContext() && m_zOrderListsDirty) || m_normalFlowListDirty)
+        if (isDirtyStackingContext() || m_normalFlowListDirty)
             compositor()->updateCompositingLayers(CompositingUpdateOnHitTest, this);
         return;
     }
index 1c29b080d8ebe092a6dd3b9fda8edf6b6c6ccbb2..03e5888de919ea4f895185323a0449663fa6e779 100644 (file)
@@ -385,13 +385,26 @@ public:
 
     void dirtyZOrderLists();
     void dirtyStackingContextZOrderLists();
-    void updateZOrderLists();
-    Vector<RenderLayer*>* posZOrderList() const { return m_posZOrderList; }
-    Vector<RenderLayer*>* negZOrderList() const { return m_negZOrderList; }
+
+    Vector<RenderLayer*>* posZOrderList() const
+    {
+        ASSERT(!m_zOrderListsDirty);
+        ASSERT(isStackingContext() || !m_posZOrderList);
+        return m_posZOrderList;
+    }
+
+    Vector<RenderLayer*>* negZOrderList() const
+    {
+        ASSERT(!m_zOrderListsDirty);
+        ASSERT(isStackingContext() || !m_negZOrderList);
+        return m_negZOrderList;
+    }
 
     void dirtyNormalFlowList();
-    void updateNormalFlowList();
-    Vector<RenderLayer*>* normalFlowList() const { return m_normalFlowList; }
+    Vector<RenderLayer*>* normalFlowList() const { ASSERT(!m_normalFlowListDirty); return m_normalFlowList; }
+
+    // Update our normal and z-index lists.
+    void updateLayerListsIfNeeded();
 
     // FIXME: We should ASSERT(!m_visibleContentStatusDirty) here, but see https://bugs.webkit.org/show_bug.cgi?id=71044
     // ditto for hasVisibleDescendant(), see https://bugs.webkit.org/show_bug.cgi?id=71277
@@ -598,7 +611,13 @@ public:
 #endif
 
 private:
+    void updateZOrderLists();
     void rebuildZOrderLists();
+    void clearZOrderLists();
+
+    void updateNormalFlowList();
+
+    bool isDirtyStackingContext() const { return m_zOrderListsDirty && isStackingContext(); }
 
     void computeRepaintRects(LayoutPoint* offsetFromRoot = 0);
     void clearRepaintRects();
@@ -630,7 +649,6 @@ private:
 
     void collectLayers(bool includeHiddenLayers, Vector<RenderLayer*>*&, Vector<RenderLayer*>*&);
 
-    void updateLayerListsIfNeeded();
     void updateCompositingAndLayerListsIfNeeded();
 
     void paintLayer(RenderLayer* rootLayer, GraphicsContext*, const LayoutRect& paintDirtyRect,
@@ -915,10 +933,30 @@ private:
 #endif
 };
 
+inline void RenderLayer::clearZOrderLists()
+{
+    if (m_posZOrderList) {
+        delete m_posZOrderList;
+        m_posZOrderList = 0;
+    }
+
+    if (m_negZOrderList) {
+        delete m_negZOrderList;
+        m_negZOrderList = 0;
+    }
+}
+
 inline void RenderLayer::updateZOrderLists()
 {
-    if (!m_zOrderListsDirty || !isStackingContext())
+    if (!m_zOrderListsDirty)
+        return;
+
+    if (!isStackingContext()) {
+        clearZOrderLists();
+        m_zOrderListsDirty = false;
         return;
+    }
+
     rebuildZOrderLists();
 }
 
index 02f5169060f9a6eab95a4597a2e5cc4ef15f2772..d45e0ad04ccc11d3636f403e216200d897057ed5 100644 (file)
@@ -657,7 +657,6 @@ void RenderLayerCompositor::addToOverlapMapRecursive(OverlapMap& overlapMap, Ren
         }
     }
 
-    ASSERT(!layer->m_normalFlowListDirty);
     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
         size_t listSize = normalFlowList->size();
         for (size_t i = 0; i < listSize; ++i) {
@@ -689,9 +688,8 @@ void RenderLayerCompositor::addToOverlapMapRecursive(OverlapMap& overlapMap, Ren
 void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, OverlapMap* overlapMap, struct CompositingState& compositingState, bool& layersChanged)
 {
     layer->updateLayerPosition();
-    layer->updateZOrderLists();
-    layer->updateNormalFlowList();
-    
+    layer->updateLayerListsIfNeeded();
+
     // Clear the flag
     layer->setHasCompositingDescendant(false);
     
@@ -743,7 +741,6 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, O
 #endif
 
     if (layer->isStackingContext()) {
-        ASSERT(!layer->m_zOrderListsDirty);
         if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {
             size_t listSize = negZOrderList->size();
             for (size_t i = 0; i < listSize; ++i) {
@@ -764,7 +761,6 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, O
         }
     }
     
-    ASSERT(!layer->m_normalFlowListDirty);
     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
         size_t listSize = normalFlowList->size();
         for (size_t i = 0; i < listSize; ++i) {
@@ -811,8 +807,10 @@ void RenderLayerCompositor::computeCompositingRequirements(RenderLayer* layer, O
     }
 
     ASSERT(willBeComposited == needsToBeComposited(layer));
-    if (layer->reflectionLayer())
+    if (layer->reflectionLayer()) {
+        // FIXME: Shouldn't we call computeCompositingRequirements to handle a reflection overlapping with another renderer?
         layer->reflectionLayer()->setMustOverlapCompositedLayers(willBeComposited);
+    }
 
     // Subsequent layers in the parent stacking context also need to composite.
     if (childState.m_subtreeIsCompositing)
@@ -934,8 +932,6 @@ void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, Vect
 #endif
 
     if (layer->isStackingContext()) {
-        ASSERT(!layer->m_zOrderListsDirty);
-
         if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {
             size_t listSize = negZOrderList->size();
             for (size_t i = 0; i < listSize; ++i) {
@@ -949,7 +945,6 @@ void RenderLayerCompositor::rebuildCompositingLayerTree(RenderLayer* layer, Vect
             childList.append(layerBacking->foregroundLayer());
     }
 
-    ASSERT(!layer->m_normalFlowListDirty);
     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
         size_t listSize = normalFlowList->size();
         for (size_t i = 0; i < listSize; ++i) {
@@ -1118,8 +1113,6 @@ void RenderLayerCompositor::updateLayerTreeGeometry(RenderLayer* layer, int dept
 #endif
 
     if (layer->isStackingContext()) {
-        ASSERT(!layer->m_zOrderListsDirty);
-
         if (Vector<RenderLayer*>* negZOrderList = layer->negZOrderList()) {
             size_t listSize = negZOrderList->size();
             for (size_t i = 0; i < listSize; ++i)
@@ -1127,7 +1120,6 @@ void RenderLayerCompositor::updateLayerTreeGeometry(RenderLayer* layer, int dept
         }
     }
 
-    ASSERT(!layer->m_normalFlowListDirty);
     if (Vector<RenderLayer*>* normalFlowList = layer->normalFlowList()) {
         size_t listSize = normalFlowList->size();
         for (size_t i = 0; i < listSize; ++i)
@@ -1411,7 +1403,9 @@ bool RenderLayerCompositor::requiresCompositingLayer(const RenderLayer* layer) c
 
 bool RenderLayerCompositor::canBeComposited(const RenderLayer* layer) const
 {
-    return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer();
+    // FIXME: We disable accelerated compositing for elements in a RenderFlowThread as it doesn't work properly.
+    // See http://webkit.org/b/84900 to re-enable it.
+    return m_hasAcceleratedCompositing && layer->isSelfPaintingLayer() && !layer->renderer()->inRenderFlowThread();
 }
 
 bool RenderLayerCompositor::requiresOwnBackingStore(const RenderLayer* layer, const RenderLayer* compositingAncestorLayer) const
index 3ca64641030ce2a1b94e4d146683733c9682bd25..b26143655a5e27180c35649944c86e110bdb8ef5 100644 (file)
@@ -708,8 +708,7 @@ static void writeLayers(TextStream& ts, const RenderLayer* rootLayer, RenderLaye
     l->calculateRects(rootLayer, 0, paintDirtyRect, layerBounds, damageRect, clipRectToApply, outlineRect, true);
 
     // Ensure our lists are up-to-date.
-    l->updateZOrderLists();
-    l->updateNormalFlowList();
+    l->updateLayerListsIfNeeded();
 
     bool shouldPaint = (behavior & RenderAsTextShowAllLayers) ? true : l->intersectsDamageRect(layerBounds, damageRect.rect(), rootLayer);
     Vector<RenderLayer*>* negList = l->negZOrderList();