Prevent synchronous image change notifications during paint

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
Committed: https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46
Cr-Commit-Position: refs/heads/master@{#398147}

[pdr., 2016-06-04 00:33:46]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-04 00:34:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038243002/1

[commit-bot: I haz the power, 2016-06-04 02:12:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-04 02:12:34]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-06-04 02:23:33]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-04 02:23:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038243002/20001

[pdr., 2016-06-04 02:23:48]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-06-04 03:59:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-04 03:59:58]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-06-04 06:39:49]

https://codereview.chromium.org/2038243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2038243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:506: m_frameTimer =
adoptPtr(new Timer<BitmapImage>(this,
&BitmapImage::advanceAnimationWithoutCatchUp));
It seems it might be possible for startAnimation() to be called again (via
draw() or some other call site) before
this timer fires. That seems bad..

[pdr., 2016-06-04 15:05:34]

https://codereview.chromium.org/2038243002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2038243002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:506: m_frameTimer =
adoptPtr(new Timer<BitmapImage>(this,
&BitmapImage::advanceAnimationWithoutCatchUp));
On 2016/06/04 at 06:39:49, chrishtr wrote:
> It seems it might be possible for startAnimation() to be called again (via
draw() or some other call site) before
> this timer fires. That seems bad..

It's safe because startAnimation bails out if there's an active timer.

[chrishtr, 2016-06-06 15:41:48]

lgtm

[chrishtr, 2016-06-06 15:42:18]

Actually, what about a test? You did find pages that tend to trigger this right?

[pdr., 2016-06-06 17:22:56]

On 2016/06/06 at 15:42:18, chrishtr wrote:
> Actually, what about a test? You did find pages that tend to trigger this
right?

svg/filters/filtered-animated-image-crash.html that was marked as passing is
about the best we can get. The added dcheck in this patch fires pretty
aggressively on this test (and all animated gif tests) which will catch
regressions, though false positives will flakily sneak through because it's
timing related.

A better test is difficult to write because one needs to mock out two timing
functions.

[chrishtr, 2016-06-06 17:25:06]

lgtm

[pdr., 2016-06-06 17:27:11]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-06-06 17:27:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2038243002/20001

[commit-bot: I haz the power, 2016-06-06 22:33:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-06 22:36:16]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46
Cr-Commit-Position: refs/heads/master@{#398147}
==========

[commit-bot: I haz the power, 2016-06-06 22:36:17]

Patchset 2 (id:??) landed as
https://crrev.com/45eef1a2a0f721e6782c4c92b28de3835f94ec46
Cr-Commit-Position: refs/heads/master@{#398147}