1475769 - Don't bail out from nsRefreshDriver::Tick after updating mMostRecentRefresh without calling StopTimer

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

I had been wondering how the crash on youtube.com (bug 1466010) happens.  I could finally reproduce the crash just once locally and confirmed that when the crash happens, there is an animation whose Animation:IsRelevant() state is true but both of its Effect::IsCurrent() and Effect::IsInEffect() is false.  That's actually the case what I described in bug 1475155 comment 0.  What a difference is that as for the youtube case, the cashes mostly happen outside of nsRefreshDriver::Tick().

And now I realized there is a case that we update mMostRecentRefresh in nsRefreshDriver::Tick() but don't update any animation state.  The case is  where we bail out from the function in the case where the refresh driver is waiting for paint [1].

Matt, the bailed out code was introduced in bug 854421, is there any strong reasons that we bail out from the function after mMostRecentRefresh is updated?  If there is no reasons, early bailing out is a simple solution for the crash.  I did push a try [2] with such change, it looks good so far (I haven't audited all oranges though).

[1] https://hg.mozilla.org/mozilla-central/file/b79457b703d9/layout/base/nsRefreshDriver.cpp#l1793

[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=da158eb4f29a3ccf26ba39434959227a346d1d30

Comment 1

[Matt Woodrow (:mattwoodrow)]

I can't think of any reason not to move it!

mMostRecentRefresh and mMostRecentTick might be identical now, I think we can combine them.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Thanks!  I will put a patch here soon.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint.

Before this patch, there is a race condition that the refresh driver updates
the most recent refresh time but animations corresponding to the refresh driver
don't update their internal state, that causes the inconsistency that such
animations are regarded as finished on the most recent time whereas their
internal states represent the animations are still in active.  This is the one
of the cause of bug 1466010, i.e. the display item corresponding to the
animation is going to be rebuilt without calling MarkNeedsDisplayItemRebuild.

Review commit: https://reviewboard.mozilla.org/r/257064/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257064/

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8992178 [details]
Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint.

https://reviewboard.mozilla.org/r/257064/#review263888

::: layout/base/nsRefreshDriver.cpp:1800
(Diff revision 1)
>      return;
>    }
> +
> +  TimeStamp previousRefresh = mMostRecentRefresh;
> +  mMostRecentRefresh = aNowTime;
>    mMostRecentTick = aNowTime;

Can we get rid of mMostRecentTick and just use mMostRecentRefresh always now?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8992178 [details]
Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257064/diff/1-2/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 8992178 [details]
> Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating
> mMostRecentRefresh when the refresh driver is waiting for paint.
> 
> https://reviewboard.mozilla.org/r/257064/#review263888
> 
> ::: layout/base/nsRefreshDriver.cpp:1800
> (Diff revision 1)
> >      return;
> >    }
> > +
> > +  TimeStamp previousRefresh = mMostRecentRefresh;
> > +  mMostRecentRefresh = aNowTime;
> >    mMostRecentTick = aNowTime;
> 
> Can we get rid of mMostRecentTick and just use mMostRecentRefresh always now?

Yes, sure. Thanks!

Comment 7

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=247916442954de9a790202e51a37f76db417c893

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb68d406a87d
Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint. r=mattwoodrow

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fb68d406a87d

Comment 10

[Hiroyuki Ikezoe (:hiro)]

There are three reports crashed at MergeState::HasMatchingItemInOldList since this bug landed, but all of the call stacks don't have nsView::WillPaintWindow (e.g bp-293f15f0-a0f0-4f91-984b-60cb60180716). So I think we fixed a case to cause bug 1466010.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8992178 [details]
Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint.

Approval Request Comment
[Feature/Bug causing the regression]: It's a long standing, I'd say bug 854421, retained display list un-wallpapered this 
[User impact if declined]: Crash on nightly and beta
[Is this code covered by automated tests?]: yes, our tests sometimes run through this code, but there is no test case dedicated this. 
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: I don't think we can test this manually
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: low
[Why is the change risky/not risky?]: It might be possible that there are something that rely on the previous code, as a result the appearance of the 'something' slightly will defer but even so the new appearance is the correct one, or the 'something' should be fixed in the first place.
[String changes made/needed]: None

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8992178 [details]
Bug 1475769 - Bail out from nsRefreshDriver::Tick before updating mMostRecentRefresh when the refresh driver is waiting for paint.

Crash fix for some issues uncovered by RDL, let's uplift for beta 10.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/03de02732801