Issue invalidations when squashed layers move later or off of a grouped mapping.

Source/core/layout/compositing/CompositedDeprecatedPaintLayerMapping.cpp

 /*
  * Copyright (C) 2009, 2010, 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 2233 matching lines @@
     layoutObject()->node()->document().compositorPendingAnimations().notifyCompositorAnimationStarted(monotonicTime, group);
 }
 
 IntRect CompositedDeprecatedPaintLayerMapping::pixelSnappedCompositedBounds() const
 {
     LayoutRect bounds = m_compositedBounds;
     bounds.move(m_owningLayer.subpixelAccumulation());
     return pixelSnappedIntRect(bounds);
 }
 
-void CompositedDeprecatedPaintLayerMapping::clearLayerGroupingIfNoPrecedingEntry(size_t indexToClear)
+bool CompositedDeprecatedPaintLayerMapping::invalidateLayerIfNoPrecedingEntry(size_t indexToClear)
 {
     DeprecatedPaintLayer* layerToRemove = m_squashedLayers[indexToClear].paintLayer;
     size_t previousIndex = 0;
     for (; previousIndex < indexToClear; ++previousIndex) {
         if (m_squashedLayers[previousIndex].paintLayer == layerToRemove)
             break;
     }
-    if (previousIndex == indexToClear) {
-        // Assert on incorrect mappings between layers and groups
-        ASSERT(layerToRemove->groupedMapping() == this);

[chrishtr, 2015/08/12 19:29:55]

The new call site to this function may not satisfy this assert, since a layer
could have been removed from squashing and become individually composited and
also earlier in the paint order.

-        layerToRemove->setGroupedMapping(nullptr, DeprecatedPaintLayer::DoNotInvalidateLayerAndRemoveFromMapping);
+    if (previousIndex == indexToClear && layerToRemove->groupedMapping() == this) {
+        compositor()->paintInvalidationOnCompositingChange(layerToRemove);
+        return true;
     }
+    return false;
 }
 
 bool CompositedDeprecatedPaintLayerMapping::updateSquashingLayerAssignment(DeprecatedPaintLayer* squashedLayer, size_t nextSquashedLayerIndex)
 {
     GraphicsLayerPaintInfo paintInfo;
     paintInfo.paintLayer = squashedLayer;
     // NOTE: composited bounds are updated elsewhere
     // NOTE: offsetFromLayoutObject is updated elsewhere
 
     // Change tracking on squashing layers: at the first sign of something changed, just invalidate the layer.
     // FIXME: Perhaps we can find a tighter more clever mechanism later.
     if (nextSquashedLayerIndex < m_squashedLayers.size()) {
         if (paintInfo.paintLayer == m_squashedLayers[nextSquashedLayerIndex].paintLayer)
             return false;
 
         // Must invalidate before adding the squashed layer to the mapping.
         compositor()->paintInvalidationOnCompositingChange(squashedLayer);
 
+        // If the layer which was previously at |nextSquashedLayerIndex| is not earlier in the grouped mapping, invalidate its current
+        // backing now, since it will move later or be removed from the squashing layer.
+        invalidateLayerIfNoPrecedingEntry(nextSquashedLayerIndex);
+
         m_squashedLayers.insert(nextSquashedLayerIndex, paintInfo);

[Stephen Chennney, 2015/08/13 16:31:18]

Because we insert here now ...

     } else {
         // Must invalidate before adding the squashed layer to the mapping.
         compositor()->paintInvalidationOnCompositingChange(squashedLayer);
         m_squashedLayers.append(paintInfo);
     }
     squashedLayer->setGroupedMapping(this, DeprecatedPaintLayer::InvalidateLayerAndRemoveFromMapping);
 
     return true;
 }
 
@@ skipping 23 matching lines @@
     return false;
 }
 #endif
 
 void CompositedDeprecatedPaintLayerMapping::finishAccumulatingSquashingLayers(size_t nextSquashedLayerIndex)
 {
     if (nextSquashedLayerIndex < m_squashedLayers.size()) {
         // Any additional squashed Layers in the array no longer belong here, but they might have been
         // added already at an earlier index. Clear pointers on those that do not appear in the valid set
         // before removing all the extra entries.
-        for (size_t i = nextSquashedLayerIndex; i < m_squashedLayers.size(); ++i)
-            clearLayerGroupingIfNoPrecedingEntry(i);
+        for (size_t i = nextSquashedLayerIndex; i < m_squashedLayers.size(); ++i) {
+            if (invalidateLayerIfNoPrecedingEntry(i))
+                m_squashedLayers[i].paintLayer->setGroupedMapping(nullptr, DeprecatedPaintLayer::DoNotInvalidateLayerAndRemoveFromMapping);

[Stephen Chennney, 2015/08/13 16:31:18]

... it's safe to only null here now. I think that's right but please take a
moment to consider it too before committing.

[chrishtr, 2015/08/13 17:05:59]

I think it's correct because the code here keeps the pointers up to date:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'm not sure what scenario was the original corner case that was missed and
necessitated https://codereview.chromium.org/1209033009, but I don't think CL
introduces any new one, so it's safe.

+        }
+
         m_squashedLayers.remove(nextSquashedLayerIndex, m_squashedLayers.size() - nextSquashedLayerIndex);
     }
 }
 
 String CompositedDeprecatedPaintLayerMapping::debugName(const GraphicsLayer* graphicsLayer)
 {
     String name;
     if (graphicsLayer == m_graphicsLayer.get()) {
         name = m_owningLayer.debugName();
     } else if (graphicsLayer == m_squashingContainmentLayer.get()) {
@@ skipping 31 matching lines @@
     } else if (graphicsLayer == m_scrollingBlockSelectionLayer.get()) {
         name = "Scrolling Block Selection Layer";
     } else {
         ASSERT_NOT_REACHED();
     }
 
     return name;
 }
 
 } // namespace blink