[SPv2] Paint invalidation in PreWalkTreeWalk, plumbed on PaintInvalidationState

third_party/WebKit/Source/core/layout/LayoutObject.h

Index: third_party/WebKit/Source/core/layout/LayoutObject.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.h b/third_party/WebKit/Source/core/layout/LayoutObject.h
index 5c4710f70dcd529f0ad6677e85d0bac7f11f5d6d..a090c7a1d392249fb5f90a8f36ca6692ac7b7cf6 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.h
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.h
@@ -1388,6 +1388,9 @@ public:
         void setPreviousPaintOffset(const LayoutPoint& paintOffset) { m_layoutObject.setPreviousPaintOffset(paintOffset); }
         ObjectPaintProperties& ensureObjectPaintProperties() { return m_layoutObject.ensureObjectPaintProperties(); }
         void clearObjectPaintProperties() { m_layoutObject.clearObjectPaintProperties(); }
+        PaintInvalidationReason invalidatePaintIfNeeded(const PaintInvalidationState& paintInvalidationState) { return m_layoutObject.invalidatePaintIfNeeded(paintInvalidationState); }

[pdr., 2016/06/03 04:43:04]

I think we accidentally backed into a bad design with MutableForPainting.
LayoutObject::invalidatePaintIfNeeded has non local side effects which make me
scared about violating const correctness. As an example, I can imagine someone
writing the following:
  const TableBorderWidth& width = toTable(object).getBorderWidth();
  PaintInvalidationReason reason =
object.getMutableForPainting().invalidatePaintIfNeeded(...);
  if (width > blah) // use-after-free because
LayoutTable::invalidatePaintIfNeeded recalcs table borders

What do you think about changing the invalidation walk to not use const?

I feel a little better about the paint property tree walk using const
LayoutObjects because it's a minor implementation detail that
ObjectPaintProperties is stored on LayoutObject.

[Xianzhu, 2016/06/03 16:46:17]

MutableForPainting is intended to avoid unexpected mutation of most of the
LayoutObject states (e.g. style and layout), while providing a convenient way to
store some data for painting (including paint invalidation) in LayoutObjects.
Another way is to mark the data for painting 'mutable' and the methods accessing
the data 'const', but the MutableForPainting method marks the mutable things for
painting more explicitly.

The side-effect of invalidatePaintIfNeeded() is clear: it should not change any
style/layout states, but only states for paint invalidation which include
previous paint invalidation geometries, paint invalidation flags, etc. Using
const LayoutObject in paint invalidation helps us to avoid unexpected mutation
to other states.

I think you meant collapsed border data in the example. It's safe because it's
protected by m_collapsedBordersValid flag. Extra calls to
recalcCollapsedBordersIfNeeded() will not cause use-after-free because the
function does nothing when m_collapsedBordersValid is true, and we can't access
collapsedBorderValues when m_collapsedBordersValid is false.

The plumbings on PaintInvalidationState and
LayoutObject::invalidatePaintIfNeeded() are temporary. They will be gradually
replaced by new code in PaintInvalidator. Eventually we'll remove
invalidatePaintIfNeeded() from MutableForPainting and add more direct mutation
accessors, then MutableForPainting will contain just accessor methods to
paint-specific data.

+        void clearPaintInvalidationFlags(const PaintInvalidationState& paintInvalidationState) { m_layoutObject.clearPaintInvalidationFlags(paintInvalidationState); }
+        void setShouldDoDelayedFullPaintInvalidation() { m_layoutObject.setShouldDoFullPaintInvalidation(PaintInvalidationDelayedFull); }
 
     private:
         friend class LayoutObject;