Closed Bug 1475769 Opened Last year Closed Last year

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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
Flags: needinfo?(matt.woodrow)
Summary: Don't bail out from nsRefreshDriver::Tick after updating mMostRecentRefresh → Don't bail out from nsRefreshDriver::Tick after updating mMostRecentRefresh without calling StopTimer
I can't think of any reason not to move it!

mMostRecentRefresh and mMostRecentTick might be identical now, I think we can combine them.
Flags: needinfo?(matt.woodrow)
Thanks!  I will put a patch here soon.
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?
Attachment #8992178 - Flags: review?(matt.woodrow) → review+
(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!
Pushed by hikezoe@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/fb68d406a87d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee: nobody → hikezoe
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 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
Attachment #8992178 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1466954
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.
Attachment #8992178 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.