1240228 - Intermittent ASAN browser_animation_playPauseIframe.js | application terminated with exit code 1 | after SUMMARY: AddressSanitizer: heap-use-after-free EffectSet.h:172 UpdateAnimationRuleRefreshTime

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://treeherder.mozilla.org/logviewer.html#?job_id=19894522&repo=mozilla-inbound

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Hiro, can you please take a look?

Comment 2

[Brian Birtles (:birtles)]

I'm wondering if this is actually from bug 1237467. In that bug we observed that MaybeUpdateAnimationRule can cause the effect set to be deleted (and recreated) but we never worked out exactly where. I wonder if it's happening ComposeStyle. If that's the case we simply need to refetch the EffectSet (and check for null) before calling ComposeStyle.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Thank you, Brian.
I will start investigating this later this week.

Comment 4

[Brian Birtles (:birtles)]

I added some assertions to see if we are destroying the effect set while composing the animation rule and it turns out why sometimes do (but very rarely).

The stack is as follows:

19:38:16     INFO -  Assertion failure: !effectSet->mComposingAnimationRule (Destroying effect set from ComposeAnimationRule), at /builds/slave/try-m64-d-00000000000000000000/build/src/dom/animation/EffectSet.cpp:122
19:38:16     INFO -  #01: mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() [mfbt/RefPtr.h:268]
19:38:16     INFO -  #02: mozilla::dom::Animation::ComposeStyle(RefPtr<mozilla::AnimValuesStyleRule>&, nsCSSPropertySet&) [dom/animation/Animation.cpp:791]
19:38:16     INFO -  #03: mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, nsCSSPseudoElements::Type, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) [mfbt/ReverseIterator.h:82]
19:38:16     INFO -  #04: mozilla::EffectCompositor::MaybeUpdateAnimationRule(mozilla::dom::Element*, nsCSSPseudoElements::Type, mozilla::EffectCompositor::CascadeLevel) [xpcom/glue/nsBaseHashtable.h:156]
19:38:16     INFO -  #05: mozilla::EffectCompositor::GetAnimationRule(mozilla::dom::Element*, nsCSSPseudoElements::Type, mozilla::EffectCompositor::CascadeLevel) [mfbt/Array.h:28]

I had forgotten that Animation::ComposeStyle exists (I thought we always composed style by calling the effect directly) and that it can sometimes change the animation's timing here:

  https://hg.mozilla.org/try/file/65e1e95c94e1/dom/animation/Animation.cpp#l791

I think what we need to do is:

1. Add a flag or find some way of prevent Animation::ComposeStyle from destroying effect sets.
2. Add an assertion to the end of EffectCompositor::ComposeAnimationRule that checks that effects == EffectSet::GetEffectSet(aElement, aPseudoType)

Comment 5

[Brian Birtles (:birtles)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=254290b5e394&selectedJob=15558502

Comment 6

[Brian Birtles (:birtles)]

Attachment: Don't update an effect's timing when tweaking its animation's hold time

In some circumstances when composing style, we tweak the time of the animation
before telling the effect to compose style. This is to avoid visual flicker in
certain situations where the main thread progress is being synchronized with an
animation running on the compositor.

In the past, effects would store their latest sample time locally so when
tweaking the animation time, we would need to call UpdateEffect() after tweaking
the time, and then again after restoring it as otherwise the style composed by
the effect would not reflect the adjusted time.

Now, however, effect's always query their animation for the time so this is no
longer necessary. Furthermore, the actions triggered by UpdateEffect are not
desirable in this case because they can, amongst other things, cause the
associated EffectSet to be destroyed and recreated.

Specifically, Animation::UpdateEffect() calls
KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() which:

  * Calls UpdateTargetRegistration which can trigger EffectSet
    destruction/creation which is undesirable in this case because we intend to
    restore whatever changes we make to the Animation's state and deleting and
    recreating the EffectSet will cause any pointers to it to dangle.

  * Cause us to possibly reset the "is running on compositor" status.
    This too is undesirable since we intend to restore the state of the
    Animation immediately after tweaking the hold time so we don't want to
    act as if any state has changed.

  * Similarly for marking the cascade as possibly needing an update or
    requesting a restyle.

In summary, all the actions performed by NotifyAnimationTimingUpdated are
unnecessary and undesirable in this situation where we are temporarily tweaking
an Animation's current time only to restore it immediately afterwards since the
actions are all involved with recognizing actual changes in state.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd8bbf8f4e4a

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Intermittent Failures Robot]

7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 3
* mozilla-central: 2
* fx-team: 2

Platform breakdown:
* linux64: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1240228&startday=2016-01-18&endday=2016-01-24&tree=all