Prevent synchronous image change notifications during paint

Image changed notifications are used by animated images to notify
LayoutObject clients that they need to repaint. These notifications
typically result in paint invalidations. Animated bitmap images have
some logic[1] to handle "falling behind" which would synchronously
fire image changed notifications during paint. This results in missed
paint invalidations as well as a changing layout tree out from under
the paint system.

This patch moves the synchronous image change notifications to an
immediate task which occurs after paint has completed.

[1] When painting animated gifs on a heavily loaded system (or a debug
build), pauses in the system can cause the animation to get behind.
When this happens, we want to advance the animation and catch-up but
prevent the next frame from using the same catch-up logic which could
get us in an infinite catch-up loop.

BUG=616700

Review-Url: https://codereview.chromium.org/2038243002
Cr-Commit-Position: refs/heads/master@{#398147}

third_party/WebKit/LayoutTests/TestExpectations

@@ -1323,9 +1323,6 @@ crbug.com/606302 [ Win ] transforms/3d/point-mapping/3d-point-mapping-preserve-3
 crbug.com/606302 [ Linux Debug ] transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html [ Failure ]
 crbug.com/613497 [ Linux Release ] transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html [ Failure ]
 
-# Crashed on Linux ASAN.
-crbug.com/616700 [ Linux ] svg/filters/filtered-animated-image-crash.html [ Crash Failure ]
-
 crbug.com/613659 imported/wpt/quirks-mode/percentage-height-calculation.html [ Failure ]
 crbug.com/613661 imported/wpt/quirks-mode/table-cell-nowrap-minimum-width-calculation.html [ Failure ]
 crbug.com/613663 imported/wpt/quirks-mode/table-cell-width-calculation.html [ Failure ]

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

@@ -3235,6 +3235,12 @@ bool LayoutObject::isInert() const
 void LayoutObject::imageChanged(ImageResource* image, const IntRect* rect)
 {
     ASSERT(m_node);
+
+    // Image change notifications should not be received during paint because
+    // the resulting invalidations will be cleared following paint. This can also
+    // lead to modifying the tree out from under paint(), see: crbug.com/616700.
+    DCHECK(document().lifecycle().state() != DocumentLifecycle::LifecycleState::InPaint);
+
     imageChanged(static_cast<WrappedImagePtr>(image), rect);
 }
 

third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

@@ -499,29 +499,12 @@ void BitmapImage::startAnimation(CatchUpAnimation catchUpIfNecessary)
             nextFrame = frameAfterNext;
         }
 
-        // Draw the next frame immediately.  Note that m_desiredFrameStartTime
+        // Post a task to advance the frame immediately. m_desiredFrameStartTime
         // may be in the past, meaning the next time through this function we'll
         // kick off the next advancement sooner than this frame's duration would
         // suggest.
-        if (internalAdvanceAnimation(false)) {
-            // The image region has been marked dirty, but once we return to our
-            // caller, draw() will clear it, and nothing will cause the
-            // animation to advance again.  We need to start the timer for the
-            // next frame running, or the animation can hang.  (Compare this
-            // with when advanceAnimation() is called, and the region is dirtied
-            // while draw() is not in the callstack, meaning draw() gets called
-            // to update the region and thus startAnimation() is reached again.)
-            // NOTE: For large images with slow or heavily-loaded systems,
-            // throwing away data as we go (see destroyDecodedData()) means we
-            // can spend so much time re-decoding data above that by the time we
-            // reach here we're behind again.  If we let startAnimation() run
-            // the catch-up code again, we can get long delays without painting
-            // as we race the timer, or even infinite recursion.  In this
-            // situation the best we can do is to simply change frames as fast
-            // as possible, so force startAnimation() to set a zero-delay timer
-            // and bail out if we're not caught up.
-            startAnimation(DoNotCatchUp);
-        }
+        m_frameTimer = adoptPtr(new Timer<BitmapImage>(this, &BitmapImage::advanceAnimationWithoutCatchUp));
+        m_frameTimer->startOneShot(0, BLINK_FROM_HERE);
     }
 }
 
@@ -568,6 +551,12 @@ void BitmapImage::advanceAnimation(Timer<BitmapImage>*)
     // startAnimation() again to keep the animation moving.
 }
 
+void BitmapImage::advanceAnimationWithoutCatchUp(Timer<BitmapImage>*)
+{
+    if (internalAdvanceAnimation(false))
+        startAnimation(DoNotCatchUp);
+}
+
 bool BitmapImage::internalAdvanceAnimation(bool skippingFrames)
 {
     // Stop the animation.

third_party/WebKit/Source/platform/graphics/BitmapImage.h

@@ -140,6 +140,12 @@ private:
     void startAnimation(CatchUpAnimation = CatchUp) override;
     void stopAnimation();
     void advanceAnimation(Timer<BitmapImage>*);
+    // Advance the animation and let the next frame get scheduled without
+    // catch-up logic. For large images with slow or heavily-loaded systems,
+    // throwing away data as we go (see destroyDecodedData()) means we can spend
+    // so much time re-decoding data that we are always behind. To prevent this,
+    // we force the next animation to skip the catch up logic.
+    void advanceAnimationWithoutCatchUp(Timer<BitmapImage>*);
 
     // Function that does the real work of advancing the animation.  When
     // skippingFrames is true, we're in the middle of a loop trying to skip over