1475155 - Consider using Animation::IsRelevant instead of 'KeyframeEffect::IsCurrent() || KeyframeEffectIsInEffect()'

Status: NEW

(Core :: DOM: Animation, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

I am not yet 100% sure we should fix by the way what I suggested in the title of this but though.

Anyway, I found another annoying inconsistency while debugging bug 1466010.  That is, after bug 1361067 we dispatch mouse events in a refresh driver tick before we call WillRefresh for DocumentTimeline, which means we do use *not-updated-yet animation timing parameters* for hit testing for the mouse events.  I am still checking what happens by this inconsistency, but I am suspecting this is another cause of bug 1466010.  (It might be safe though)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> I am not yet 100% sure we should fix by the way what I suggested in the
> title of this but though.

*this bug though*.  I did't want to mean 'but though but though..'. :)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I am mostly convinced this is a cause of bug 1466010.  I am figuring out how to write a test case for this.

Comment 3

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> I am mostly convinced this is a cause of bug 1466010.  I am figuring out how
> to write a test case for this.

Oh nice. That value getting out of sync has always been a worry!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: A crash test and some modifications to cause the crash

I did manage to write a crash test to make the inconsistency happen.  To make test test crash I've added some assertions in nsLayoutUtils::HasAnimationOfProperty() and its siblings.

But it doesn't cause bug 1466010 yet.  It presumably needs more complicated/sophisticated gimmics.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

To reproduce bug 1466010, what I am currently guessing is that it requires 'resize' events involved since 'resize' events are also processed before 'update animations' step [1].  Last night I did try to fire 'resize' event in the crash test, but I don't have enough knowledge about the event to make it work. :/

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

Comment 6

[Brian Birtles (:birtles)]

In the past I've spent hours trying to get resize events to fire in crashtests, unsuccessfully. I remember debugging pretty far into how resize events are dispatched but I can't remember what the issue was (it will be in a Bugzilla comment somewhere but I don't recall which one).

I think if you can the inconsistency in a crashtest and you can reproduce the crash interactively, that would be enough.

Comment 7

[Brian Birtles (:birtles)]

Oh, see bug 1407898 comment 4 through to comment 6.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: A call stack from WillPaint to HasEffectiveAnimations

(In reply to Brian Birtles (:birtles) from comment #6)
> In the past I've spent hours trying to get resize events to fire in
> crashtests, unsuccessfully. I remember debugging pretty far into how resize
> events are dispatched but I can't remember what the issue was (it will be in
> a Bugzilla comment somewhere but I don't recall which one).
> 
> I think if you can the inconsistency in a crashtest and you can reproduce
> the crash interactively, that would be enough.

Thank you!  This comment reminds me the past comment. :)  What I wanted to make sure is whether this inconsistency leads certainly to the youtube crash or not.

So, yay, with the assertions on youtube.com I got a call stack hitting one of the assertions from WillPaint to the assertion!  This doesn't lead the crash itself though, I think there still needs some conditions for retained display list.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I filed bug 1475769 for another case where Animation::IsRelevant doesn't match Effect::IsCurrent() or Effect::IsInEffect().  The case is independent from this bug, which means bug 1475769 will not fix this.  That's said, as far as I can tell there is no crash reports for bug 1466010 which has PresShell::HandleEvent symbol [1].

So, I think this bug doesn't cause the crash.  There might be possibly visual faults by this, but I am going to degrade priority of this bug for now. 

[1] https://crash-stats.mozilla.com/signature/?proto_signature=~PresShell%3A%3AHandleEvent&signature=MergeState%3A%3AHasMatchingItemInOldList&date=%3E%3D2018-04-15T02%3A22%3A58.000Z&date=%3C2018-07-15T02%3A22%3A58.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1