1331704 - AddressSanitizer: heap-buffer-overflow [@ nsAutoPtr<nsCSSCompressedDataBlock>::get] with READ of size 8

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase crashes in mozilla-central rev 6a23526fe516

==25439==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x607000451e58 at pc 0x7f723e9ae0fa bp 0x7ffe2c012e70 sp 0x7ffe2c012e68
READ of size 8 at 0x607000451e58 thread T0
    #0 0x7f723e9ae0f9 in get /home/worker/workspace/build/src/obj-firefox/dist/include/nsAutoPtr.h:171:12
    #1 0x7f723e9ae0f9 in operator nsCSSCompressedDataBlock * /home/worker/workspace/build/src/obj-firefox/dist/include/nsAutoPtr.h:185
    #2 0x7f723e9ae0f9 in HasImportantData /home/worker/workspace/build/src/layout/style/Declaration.h:135
    #3 0x7f723e9ae0f9 in GetImportantStyleData /home/worker/workspace/build/src/layout/style/Declaration.h:302
    #4 0x7f723e9ae0f9 in nsStyleSet::AddImportantRules(nsRuleNode*, nsRuleNode*, nsRuleWalker*) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1043
    #5 0x7f723e9af183 in nsStyleSet::FileRules(bool (*)(nsIStyleRuleProcessor*, void*), RuleProcessorData*, mozilla::dom::Element*, nsRuleWalker*) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1265:5
    #6 0x7f723e9b1641 in nsStyleSet::ResolveStyleForInternal(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&, nsStyleSet::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1367:3
    #7 0x7f723e9b0f7c in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1403:10
    #8 0x7f723e9b0f7c in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1350
    #9 0x7f723e7e288c in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.h:121:12
    #10 0x7f723e7e288c in ResolveStyleFor /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StyleSetHandleInlines.h:85
    #11 0x7f723e7e288c in ResolveWithAnimation /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:499
    #12 0x7f723e7e288c in nsComputedDOMStyle::DoGetStyleContextForElementNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType, nsComputedDOMStyle::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:656
    #13 0x7f723e7e2075 in GetStyleContextForElementNoFlush /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:677:10
    #14 0x7f723e7e2075 in nsComputedDOMStyle::GetStyleContextForElement(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:447

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: log.txt

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Regression from bug 1305325.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

According to the stack in comment 0, important rules are also destructed.  It tells us that we shouldn't get base values (resolve the base style) in KeyframeEffectReadOnly::ComposeStyle().  I am inclined to get the base style in UpdateEffectProperties() as I was going to do initially.  It seems a safer place than ComposeStyle() since we have already a *resolved* style there.
Brian, what do you think?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> According to the stack in comment 0, important rules are also destructed. 
> It tells us that we shouldn't get base values (resolve the base style) in
> KeyframeEffectReadOnly::ComposeStyle().  I am inclined to get the base style
> in UpdateEffectProperties() as I was going to do initially.  It seems a
> safer place than ComposeStyle() since we have already a *resolved* style
> there.

Or KeyframeEffectReadOnly::UpdateProperties()?

Comment 5

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> According to the stack in comment 0, important rules are also destructed. 
> It tells us that we shouldn't get base values (resolve the base style) in
> KeyframeEffectReadOnly::ComposeStyle().  I am inclined to get the base style
> in UpdateEffectProperties() as I was going to do initially.  It seems a
> safer place than ComposeStyle() since we have already a *resolved* style
> there.
> Brian, what do you think?

I don't quite follow. UpdateEffectProperties happens just before ComposeStyle. What problem would be fixed by moving the call there?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #5)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > According to the stack in comment 0, important rules are also destructed. 
> > It tells us that we shouldn't get base values (resolve the base style) in
> > KeyframeEffectReadOnly::ComposeStyle().  I am inclined to get the base style
> > in UpdateEffectProperties() as I was going to do initially.  It seems a
> > safer place than ComposeStyle() since we have already a *resolved* style
> > there.
> > Brian, what do you think?
> 
> I don't quite follow. UpdateEffectProperties happens just before
> ComposeStyle. What problem would be fixed by moving the call there?

In UpdateEffectProperties, we don't need to resolve a new style context for getting base style we can just use ResolveStyleByRemovingAnimation with the passing style context.  IIUC, ResolveStyleByRemovingAnimation() does not replace important rules and other rules if we specify eRestyle_AllHintsWithAnimations.

Comment 7

[Marcia Knous [:marcia]]

Tracking 53+ for this crash regression.

Comment 9

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> In UpdateEffectProperties, we don't need to resolve a new style context for
> getting base style we can just use ResolveStyleByRemovingAnimation with the
> passing style context.  IIUC, ResolveStyleByRemovingAnimation() does not
> replace important rules and other rules if we specify
> eRestyle_AllHintsWithAnimations.

Sorry for the delay here. I think I understand what you're saying and it seems to make sense but I always have to look up those ResolveStyleByRemovingAnimation/ResolveStyleByAddingRules/ResolveStyleWithReplacement methods each time to remember what they do.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > In UpdateEffectProperties, we don't need to resolve a new style context for
> > getting base style we can just use ResolveStyleByRemovingAnimation with the
> > passing style context.  IIUC, ResolveStyleByRemovingAnimation() does not
> > replace important rules and other rules if we specify
> > eRestyle_AllHintsWithAnimations.
> 
> Sorry for the delay here. I think I understand what you're saying and it
> seems to make sense but I always have to look up those
> ResolveStyleByRemovingAnimation/ResolveStyleByAddingRules/
> ResolveStyleWithReplacement methods each time to remember what they do.

Me too. Now I am stuck in another problem.  I am trying to resolve base style in KeyframeEffectReadOnly::UpdateProperties(), but in the test case in comment 0 when we call UpdateProperties(), we don't have EffectSet yet, so we can't store the resolved base styles in EffectSet.  This is very common in case of Element.animate():

1. construct KeyframeEffect
 1-1. SetKeyframe
 1-2. UpdateProperties
2. construct Animation
 2-1. SetEffect
 2-2. create an EffectSet

We have to figure out where we should store the resolve style value as cache. I am currently storing them in KeyframeEffectReadOnly class, it works but it seems inefficient.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Rough patch

This patch still needs to be cleaned up but basically is implemented what I commented in this bug.  This patch fixes the test cases both in this bug and bug 1332071.

Jesse, I'd like to fix crashes caused by bug 1305325 completely in this bug.  I guess you still have tons of test cases which cause crashes related to animation code.  Could you please try this patch and see if it fixes those crashes?

Thank you,

Comment 12

[Jesse Schwartzentruber (:truber)]

I wouldn't say tons :) I have >100 but it's not clear to me what makes a unique testcase without reducing each one, which takes a few hours.

I will check against the testcases I have so far, and run the fuzzer against it too.

Comment 13

[Jesse Schwartzentruber (:truber)]

Attachment: fm728433_REDUCED.html

Another crash from inbound rev 80bbd0f4ed81 with patch in attachment 8828265 [details] [diff] [review]

Comment 14

[Jesse Schwartzentruber (:truber)]

Attachment: fm728433_REDUCED.log

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Thank you so much, Jesse!  You are really amazing!  I will look into the test case. Thanks!

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Jesse Schwartzentruber (:truber) from comment #13)
> Created attachment 8828553 [details]
> fm728433_REDUCED.html

This is a case what I've totally missed. What's going on there is:

1) let o_1 = o_0.animate([{}], 283);  // When we process UpdateProperties() for this animation, we don't have a valid effect set, but an effect set is created after SetAnimation() for this animation.
2) let o_2 = o_0.animate([{ "margin": "0.0vh auto" }], 2917); // Now we have an effect set, so we store the base values into the effect set.
3) o_1.finish();
4) o_2.finish(); // At this point, the effect set is destroyed!
5) o_2.reverse(); // Now we have no cached base style at all.

This tells me that we should store the resolved base values in KeyframeEffectReadOnly even if there is a valid effect set.

I greatly appreciate Jesse.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: GetBaseStyle returns resolved nsStyleContext even if there is no EffectSet.

Now GetBaseStyle renamed to ResolveBaseStyle.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Resolve base style values in UpdateProperties() instead of inside ComposeStyle()

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Don't call any functions which result in resolving style while processing composing animation styles.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Drop EnsureBaseStylesForCompositor and GetTargetStyleContextWithoutAnimation from KeyframeEffectReadOnly.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

I did not include the test case in bug 1332071 since the test case causes an NS_ASSERTION in ApplyRenderingChangeToTree() [1] even though the test case causes no crash with these patch.  I will look into it in that bug.

[1] https://hg.mozilla.org/mozilla-central/file/bde3fc40b9b5/layout/base/RestyleManagerBase.cpp#l912

Brian, could you please review them when you have time.
Thank you!

Comment 22

[Jesse Schwartzentruber (:truber)]

With those patches I don't see any more crashes in the set of testcases I have.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

What a fast response!  Super Jesse, thank you!

Comment 25

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> I did not include the test case in bug 1332071 since the test case causes an
> NS_ASSERTION in ApplyRenderingChangeToTree() [1] even though the test case
> causes no crash with these patch.  I will look into it in that bug.

It turns out the assertion is not related missing keyframes (bug 1305325), the assertion happens with normal animations. I filed bug 1332588.

Comment 26

[Brian Birtles (:birtles)]

This looks mostly good but the TransferBaseStyles mechanism seems complicated to me (and error prone if we forget to call it). Can we just generate the EffectSet on-demand if we need to store base styles on it? Then make the condition for dropping the EffectSet some combination of having an Effect and having a base style (perhaps a base style generated during the current animation generation / tick?)

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Extend the life time of EffectSet. r?birtles

Now EffectSet instance is alive during either we have at least one relevant
state effect on the element or base style values.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Resolve base style values in UpdateProperties()

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Don't call any functions which result in resolving style while processing composing animation styles.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Drop GetTargetStyleContextWithoutAnimation from KeyframeEffectReadOnly

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Extend the life time of EffectSet. r?birtles

Now EffectSet instance is alive during either we have at least one relevant
state effect on the element or base style values.

Forgot to destroy in the last patch when we process ClearBaseStyles().

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Thanks Brian!  I did extend the lifetime of EffectSet as you suggested.

I did confirm that all of tests in {dom/animation, layout/styles/test_animations*, layout/styles/crashtests} and web-platform-tests for web-animations work fine with these patches.

Comment 33

[Brian Birtles (:birtles)]

Wow, fast work! Thank you! I'll have another look later tonight.

Comment 34

[Brian Birtles (:birtles)]

Comment on attachment 8830151 [details] [diff] [review]
Part 2: Resolve base style values in UpdateProperties()

Review of attachment 8830151 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is good, but I just want to understand why we call SetNeedsBaseStyle even for properties that are not run on the compositor.

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +285,4 @@
>  
>    nsTArray<AnimationProperty> properties = BuildProperties(aStyleContext);
>  
> +  // We need to update base styles even if any properties are not changed at all

... even if no properties have changed since ...

@@ +445,3 @@
>        // Make this property as needing a base style so that we send the (now
>        // cached) base style to the compositor.
>        SetNeedsBaseStyle(property.mProperty);

Do we want to set this unconditionally for all properties or only for compositor-animatable properties?

::: dom/animation/KeyframeEffectReadOnly.h
@@ +446,4 @@
>    // compositor.
>    void SetNeedsBaseStyle(nsCSSPropertyID aProperty);
>  
> +  // Ensure the base styles is available for |aProperty|.

Ensure base style data is available for |aProperties|.

Comment 35

[Brian Birtles (:birtles)]

Comment on attachment 8830152 [details] [diff] [review]
Part 3: Don't call any functions which result in resolving style while processing composing animation styles.

Review of attachment 8830152 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/EffectCompositor.cpp
@@ +891,5 @@
>  {
> +  EffectSet* effectSet =
> +    EffectSet::GetEffectSet(&aElement, aPseudoType);
> +
> +  MOZ_ASSERT(effectSet, "EffectSet should be a valid");

s/should be a valid/should be valid/

::: dom/animation/EffectCompositor.h
@@ +226,5 @@
> +                                              dom::Element& aElement,
> +                                              CSSPseudoElementType aPseudoType);
> +
> +  // Returns the base style of (pseudo-)element for |aProperty|. Unlike the
> +  // above function this function does not resolve a new style context at all,

Rather than "the above function" we should say "ResolveBaseStyle" since it's quite possible someone will add another declaration between these two (without reading this comment) and then the meaning will change.

Comment 36

[Brian Birtles (:birtles)]

Comment on attachment 8830155 [details] [diff] [review]
Part 1: Extend the life time of EffectSet. r?birtles

Review of attachment 8830155 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/EffectSet.h
@@ +28,5 @@
>  
>  // A wrapper around a hashset of AnimationEffect objects to handle
>  // storing the set as a property of an element.
> +// This lives during either we have at least one relevant state effect or we
> +// have base style values for the element.

// There should be a corresponding EffectSet for each (pseudo-)element that has at least one relevant animation effect targetting it, or which has resolved base style for the element.

@@ +161,4 @@
>    bool IsBeingEnumerated() const { return mActiveIterators != 0; }
>  #endif
>  
> +  bool IsEmpty() const

Should we rename this HasData() ?

(And keep IsEmpty() if any of the call sites are actually checking if there are any effects or not.)

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #36)
> @@ +161,4 @@
> >    bool IsBeingEnumerated() const { return mActiveIterators != 0; }
> >  #endif
> >  
> > +  bool IsEmpty() const
> 
> Should we rename this HasData() ?
> 
> (And keep IsEmpty() if any of the call sites are actually checking if there
> are any effects or not.)

Indeed!  Keeping IsEmpty() and using it makes more efficient. Thanks!

Comment 38

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #34)
> Comment on attachment 8830151 [details] [diff] [review]
> Part 2: Resolve base style values in UpdateProperties()
> 
> Review of attachment 8830151 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is good, but I just want to understand why we call
> SetNeedsBaseStyle even for properties that are not run on the compositor.

Because we also check it inside the function. Should it be renamed to MaybeSetNeedsBaseStyle()?  Anyway I will handle it in a later bug.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Extend the life time of EffectSet

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Resolve base style values in UpdateProperties()

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Don't call any functions which result in resolving style while processing composing animation styles.

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Drop GetTargetStyleContextWithoutAnimation from KeyframeEffectReadOnly

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Test cases

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830230 [details] [diff] [review]
Part 1: Extend the life time of EffectSet

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.  What these patch do is that just moves a procedure (resolving styles) to a different places.  I think it's really hard to create an exploit based on them.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Nothing.

Which older supported branches are affected by this flaw?
On aurora, but the feature that causes this issue is behind a pref which is disabled by default on release and beta channels.

If not all supported branches, which bug introduced the flaw?
Bug 1305325.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
All of these patches can be uplifted to aurora directly.

How likely is this patch to cause regressions; how much testing does it need?
I run all relevant test cases locally. Please see comment 32.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Resolve base style values in UpdateProperties()

[Security approval request comment]
Same as part 1.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830232 [details] [diff] [review]
Part 3: Don't call any functions which result in resolving style while processing composing animation styles.

[Security approval request comment]
Same as the above.

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830233 [details] [diff] [review]
Part 4: Drop GetTargetStyleContextWithoutAnimation from KeyframeEffectReadOnly

[Security approval request comment]
Same as the above.

Comment 48

[Brian Birtles (:birtles)]

Thanks for doing this!

Comment 49

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. You should nominate a patch for beta and aurora as well.

Clearing the redundant sec-approvals. You don't need to ask on each sub-patch.

Comment 50

[Jesse Schwartzentruber (:truber)]

Attachment: log.txt

The original testcase (attachment 8827579 [details]) crashes with a heap-use-after-free.
Tried inbound rev 77d543972ad3 with latest patches.
Part 2 was broken by 367213:04041935086a in dom/animation/KeyframeEffectReadOnly.cpp which I resolved naively.

Comment 53

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8830230 [details] [diff] [review]
Part 1: Extend the life time of EffectSet

Review of attachment 8830230 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Jesse! You are my safety belt!

::: dom/animation/EffectCompositor.cpp
@@ +953,5 @@
>  
>    effectSet->ClearBaseStyles();
> +  if (!effectSet->HasData()) {
> +    EffectSet::DestroyEffectSet(&aElement, aPseudoType);
> +  }

The problem is here.  We don't need to destroy this effect set since when we reach here we have a valid effect set and will use the effect set in UpdateProperties() which will be called soon after.

The automation test (1331704-1.html) has also a problem that there is no 'reftest-wait' since it did crash without waiting for something.  (Whereas I did use 'reftest-wait' in 1331704-2.html).  We should wait for animation ready.

Comment 54

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Extend the life time of EffectSet

Jesse, would you mind if I request re-confirmation with this patch?

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 5: Test cases

Updated. Now 1331704-1.html waits for animation ready.

Comment 56

[Jesse Schwartzentruber (:truber)]

Attachment: assert.html

I don't see any crashes (including Nils' two), but I do now get this diagnostic assertion with the attached testcase (inbound 4b8c1dc925ef):

Assertion failure: !result.IsNull() (The base style should be set), at /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:376
    #0 0x7fca06b1f8f2 in mozilla::dom::KeyframeEffectReadOnly::GetUnderlyingStyle(nsCSSPropertyID, RefPtr<mozilla::AnimValuesStyleRule> const&) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:376:5
    #1 0x7fca06b1fb6e in mozilla::dom::KeyframeEffectReadOnly::CompositeValue(nsCSSPropertyID, RefPtr<mozilla::AnimValuesStyleRule> const&, mozilla::StyleAnimationValue const&, mozilla::dom::CompositeOperation) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:410:12
    #2 0x7fca06af4e48 in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(mozilla::AnimationRule&, nsCSSPropertyIDSet const&) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:574:9
    #3 0x7fca06af418a in mozilla::dom::Animation::ComposeStyle(mozilla::AnimationRule&, nsCSSPropertyIDSet const&) /home/truber/src/m/unified/dom/animation/Animation.cpp:982:7
    #4 0x7fca06b0cd3d in mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) /home/truber/src/m/unified/dom/animation/EffectCompositor.cpp:760:5
    #5 0x7fca06b0c304 in mozilla::EffectCompositor::MaybeUpdateAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) /home/truber/src/m/unified/dom/animation/EffectCompositor.cpp:378:3
    #6 0x7fca06b0d2ee in mozilla::EffectCompositor::GetAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, nsStyleContext*) /home/truber/src/m/unified/dom/animation/EffectCompositor.cpp:414:3
    #7 0x7fca0add9272 in nsStyleSet::RuleNodeWithReplacement(mozilla::dom::Element*, mozilla::dom::Element*, nsRuleNode*, mozilla::CSSPseudoElementType, nsRestyleHint) /home/truber/src/m/unified/layout/style/nsStyleSet.cpp:1578:34
    #8 0x7fca0adda0c9 in nsStyleSet::ResolveStyleWithReplacement(mozilla::dom::Element*, mozilla::dom::Element*, nsStyleContext*, nsStyleContext*, nsRestyleHint, unsigned int) /home/truber/src/m/unified/layout/style/nsStyleSet.cpp:1688:5
    #9 0x7fca0aefe7a6 in mozilla::ElementRestyler::RestyleSelf(nsIFrame*, nsRestyleHint, unsigned int*, nsTArray<mozilla::ElementRestyler::SwapInstruction>&) /home/truber/src/m/unified/layout/base/RestyleManager.cpp:2814:11
    #10 0x7fca0aefb3a7 in mozilla::ElementRestyler::Restyle(nsRestyleHint) /home/truber/src/m/unified/layout/base/RestyleManager.cpp:2137:7

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Thank you Jesse!
This is really annoying case.

  let o_1 = new KeyframeEffect(o_0, [{ "left": "auto" }], 100);  // At this point, we do resolve the base style and store
                                                                 // it in EffectSet, *but* this effect is not appended the
                                                                 // EffectSet itself because this effect does not relevant state. 
  o_0.animate([ { }, { } ]);
  let o_2 = o_0.animate([ { } ]);

  // Before this we call EffectCompositor::UpdateEffectProperties(), as a result the base style is cleared and then unfortunately
  // we don't re-resolve the base style again because the first effect is not in the EffectSet.
  // Then We lost the base style here and have no chance to resolve a new base style.
  o_2.effect = o_1;

Now I would like to back to the original plan that stores base styles in both of EffectSet and KeyframeEffectReadOnly.

Brian, what do you think?

Comment 58

[Brian Birtles (:birtles)]

Thanks so much for your work on this. I still feel uncomfortable with the idea of storing base styles on one object and then transferring it to another object. It seems awkward and error-prone. Is there nothing simpler we can do?

If you can clearly explain why that is the best and simplest approach, that would help. At the moment, I suspect there is something much simpler we can do even if it is less efficient in some cases.

I would also be ok with a complicated approach if we have a plan for doing something simpler in the near future.

Comment 59

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, away most of Jan 21 - Feb 1) from comment #58)
> Thanks so much for your work on this. I still feel uncomfortable with the
> idea of storing base styles on one object and then transferring it to
> another object. It seems awkward and error-prone. Is there nothing simpler
> we can do?

Two ways what I can think of:

1) Store the base styles only in KeyframeEffectReadOnly.
  We will store duplicated values in different KeyframeEffectReadOnly(s) on the same element.
2) Store the base styles in dom::Element (or DOMSlots?).
  We will retain the base styles until the element is detached from the document.  

I'd prefere the first approach for now.

Comment 60

[Brian Birtles (:birtles)]

Ok, I think I need to spend more time to think about this. ni me so I can look at it on Thursday when I get back to the office.

In the meantime, if you have a chance to summarize the issue as clearly as possible, that would help.

Comment 61

[Brian Birtles (:birtles)]

I spent some time looking into this but I don't understand all the background to this, e.g. why we introduced ResolveWithAnimation etc. in the first place. Perhaps I can begin with a very basic and naive summary of the problem and you can explain where it is wrong:

* In order to implement additive animation we need to have base styles.
* As an optimization we DON'T fetch these base styles on every frame, but only once and then we refetch them when they change. (We do this in UpdateProperties which is typically called when the parent context changed. It's not called when the ancestor style context changes but that's an existing bug we need to fix.)
* We need to fetch base styles in some cases even when we don't have any additive animations running at the moment. One example is when we have additive animations that run on the compositor. In that case, we need to fetch the base style in advance and send it to the compositor with the animation so that when it is ready to run it has the necessary style to composite with.
* At some point we added GetStyleContextForElementWithoutAnimation to avoid some cases of recursion?
  What was the recursion?
  Why can't we just use the restyling phases we use for triggering transitions?
  Does GetStyleContextForElementWithoutAnimation skip animation rules for the element only, or for ancestors too?
* In this bug, we hit a situation where we have no base style and that causes us to crash. The patches in this bug have mostly been about making sure that base style exists, but I wonder why do we crash at all? What is assuming we need base style?

Hiro, can you help me understand the problem here?

Comment 62

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #61)

> * At some point we added GetStyleContextForElementWithoutAnimation to avoid
> some cases of recursion?

That's right.  We could avoid *some* cases of recursion. But...

>   What was the recursion?

In this bug case, the test case in comment 0 invokes nsCSSFrameConstructor::MaybeRecreateFramesForElement() and then the function calls ResolveStyleFor() [1]. This is the recursion.  To avoid the recursion we need to use ResolveStyleWithoutAnimation instead of the ResolveStyleFor.  I should note about this recursion in more detail. Actually we already skip recursive calls of ComposeStyle() and UpdateProperties() by checking mIsComposingStyle, but in this case, ComposeStyle() is called against different KeyframeEffectReadOnly instance on the same element.  This is the real problem here.  It breaks style rule data on the element. (as a result this heap-buffer-overflow happens.)

[1] https://hg.mozilla.org/mozilla-central/file/f985243bb630/layout/base/nsCSSFrameConstructor.cpp#l9303

>   Why can't we just use the restyling phases we use for triggering
> transitions?

I guess you mean we should move UpdateEffectProperties() and GetAnimationRule() in nsStyleSet::GetContext() to the phase for transition, but unfortunately in this bug case the problematic filling animation rules call comes from nsStyleSet::ResolveStyleForInternal [2] not in GetContext().

[2] https://hg.mozilla.org/mozilla-central/file/f985243bb630/layout/style/nsStyleSet.cpp#l1367

>   Does GetStyleContextForElementWithoutAnimation skip animation rules for
> the element only, or for ancestors too?

Only for the target element.

> * In this bug, we hit a situation where we have no base style and that
> causes us to crash. The patches in this bug have mostly been about making
> sure that base style exists, but I wonder why do we crash at all? What is
> assuming we need base style?

We have two goals in this bug.

1) Ensure the base style existence.
2) Stop recursive calls of resolving style (filling animation rules) on the same element.

In the case of 1) it causes a crash like in comment 56.
In the case of 2) it causes memory corruption like comment 0.

Please let me know if you have more questions.

Thanks!

Comment 63

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> >   Why can't we just use the restyling phases we use for triggering
> > transitions?
> 
> I guess you mean we should move UpdateEffectProperties() and
> GetAnimationRule() in nsStyleSet::GetContext() to the phase for transition,
> but unfortunately in this bug case the problematic filling animation rules
> call comes from nsStyleSet::ResolveStyleForInternal [2] not in GetContext().

This is slightly lack of explanation.  The rough call stack is below:

Element::Animate()
 nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation()
   nsCSSFrameConstructor::MaybeRecreateFramesForElement
     nsStyleSet::ResolveStyleForInternal

Comment 64

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #63)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #62)
> > >   Why can't we just use the restyling phases we use for triggering
> > > transitions?
> > 
> > I guess you mean we should move UpdateEffectProperties() and
> > GetAnimationRule() in nsStyleSet::GetContext() to the phase for transition,
> > but unfortunately in this bug case the problematic filling animation rules
> > call comes from nsStyleSet::ResolveStyleForInternal [2] not in GetContext().
> 
> This is slightly lack of explanation.  The rough call stack is below:
> 
> Element::Animate()
>  nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation()
>    nsCSSFrameConstructor::MaybeRecreateFramesForElement
>      nsStyleSet::ResolveStyleForInternal

What is the call stack between Element::Animate() and nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation() ?

Thanks for your explanation but I'm still trying to understand what *should* happen so we can work out what the correct fix is and prevent this sort of bug occurring again. I'm concerned that we have had too many bugs in this area which suggests there is something architecturally wrong here. You've explained what is happening but it's still not clear to me why it needs to happen that way.

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Please give me some time to get correct stack.

(In reply to Brian Birtles (:birtles) from comment #64)
> > 
> > This is slightly lack of explanation.  The rough call stack is below:
> > 
> > Element::Animate()
> >  nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation()
> >    nsCSSFrameConstructor::MaybeRecreateFramesForElement
> >      nsStyleSet::ResolveStyleForInternal

Actually this stack copied from my local memo, and this stack was slightly wrong because it's got with a modification of replacing GetTargetStyleContext() with GetTargetStyleContextWithoutAnimations().
Also I don't actually remember which test case I used. 

Anyway, the key point here is that we are composing style, i.e. getting style context for additive animation, resolving styles, for *different* keyframe effect on the same element (same rule data).

Comment 66

[Brian Birtles (:birtles)]

What in Element::Animate() is causing us to compose style?

Comment 67

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #66)
> What in Element::Animate() is causing us to compose style?

In the case in comment 0, 'new KeyframeEffecty()' causes this bug.  Of course in another test case in another bug it's Element::Animate().

Comment 68

[Brian Birtles (:birtles)]

It seems to me we are running into a lot of problems because we:

* Resolve style in a lot of different circumstances
* Tied updating base styles to updating properties
* Make various assumptions about both properties and base styles being available

In terms of simplifying and hardening this setup, I wonder if we could:

* Only update properties and base style as part of restyling (i.e. when we're about to call ComposeStyle)
* Safely handle the case when properties and base style are not available

That seems to be the most simple and intuitive behavior--i.e. these two things: properties and base style, are a product
of restyling. If we haven't done a restyle yet, they probably don't exist, and if we're not doing something that is part
of restyling, we shouldn't expect them to.

So let's see if it is practical.

What is using properties and when?

* KeyframeEffectReadOnly::NotifyAnimationTimingUpdated skips requesting a restyle if we have no properties.

  This could be replaced be a bool indicating if we have animatable properties specified in mKeyframes? It would be
  imperfect, I guess, because we would fail to skip requesting a restyle in the case where all the values in mKeyframes
  are invalid, but that's probably not significant.

* KeyframeEffectReadOnly::GetEffectiveAnimationOfProperty/HasEffectiveAnimationOfProperty looks at the properties to:

  * Find animations for compositor (via FindAnimationsForCompositor / IsMatchForCompositor). This is used in:
    * EffectCompositor::HasAnimationsForCompositor which is called in FrameLayerBuilder and nsDisplayList.
    * EffectCompositor::GetAnimationsForCompositor which, likewise, is called in
      nsLayoutUtils::ComputeSuitableScaleForAnimation and nsDisplayList.
  * Add compositor animations to layers in AddAnimationsForProperty.
  * To check if a layer has animations in ActiveLayerTracker::IsStyleAnimated and
    ElementRestyler::AddLayerChangesForAnimation (via nsLayoutUtils::HasEffectiveAnimation).

  All of the above happen after restyling so the properties should be available.

  * To check if an animation can be throttled in KeyframeEffectReadOnly::CanThrottle().

    This gets called in KeyframeEffectReadOnly::NotifyAnimationTimingUpdated to determine what type of restyle to
    request. Given that this depends on looking up information in the EffectSet, I suspect that if we haven't already
    done a restyle, this is not going to have the right information. That is, if mProperties is empty, we'll end up
    returning the same result as we currently do.

  * To check if an effect / animation is currently running on the compositor or not.

    This is used for the DevTools API where, again, the member is not accurate until we've done restyling.
    It is also used when we do the special "replace transition on the compositor" behavior which, again, doesn't make
    sense to do if we haven't restyled the animation.

* Obviously we also need these members for updating the compositor status in nsDisplayList and FrameLayerBuilder but
  by that point restyling has happened.

* We also have KeyframeEffectReadOnly::ResetIsRunningOnCompositor() which gets called from places like
  KeyframeEffect::SetTarget and KeyframeEffectReadOnly::NotifyAnimationTimingUpdated which are *not* part of restyling
  but if the properties haven't been generated at that point, there's nothing to reset so these should be ok.

* The copy-constructor KeyframeEffectReadOnly::ConstructKeyframeEffect directly copies the properties member.
  Again, if properties have not been generated at this point we should be fine.

* The KeyframeEffectReadOnly::GetProperties (chrome-only) IDL interface obvious uses the properties. I think it would be
  ok to trigger a restyle as part of this method if the properties have not been generated since we know we're not
  already doing a restyle at this point.

* KeyframeEffectReadOnly::CalculateCumulativeChangeHint sets things on properties, but this is part of updating
  properties so it's definitely ok.

* KeyframeEffectReadOnly::MaybeUpdateFrameForCompositor() sets flags on these properties. However, it returns early if
  there is no frame so if we haven't done restyling yet it's already going to return early.

* Finally, ElementPropertyTransition::ToValue() uses the properties to report the transition's to property. This is
  a bit more tricky. We use this in two places:

  a) In nsTransitionManager::ConsiderInitiatingTransition to get the to-value from oldPT. Arguably, we should be using
     CSSTransition::ToValue() here in oldPT's animation since that's what we do elsewhere in this function.

  b) In CSSTransition::SetEffectFromStyle when we initially set the effect of a CSSTransition to initialize its mToValue
     member. We should just pass the |endValue| along here (after dropping Move semantics I guess).

  If we did that, we might be able to drop ElementPropertyTransition::ToValue() altogether.

* KeyframeEffectReadOnly::ComposeStyle, EnsureBaseStylesForCompositor etc. refer to properties but this is part of
  restyling so presumably the properties should be generated by this point.


So, with a few changes I think we could come to an arrangement where we only do UpdateProperties as part of restyling or
a call to GetProperties. i.e. we don't call it when creating a new effect, or calling Element::Animate, or setting
keyframes etc. (although we might need to set a flag in some of those cases to indicate that the properties are
out-of-date).

Based on that, I expect we can do something similar to base styles, i.e. only resolve base styles as part of regular
restyling.

That won't solve all our recursion problems (we'll probably still need the mIsComposingStyle member for a while) but it
should reduce the number of odd permutations where API interactions get into odd states, and, I hope, remove the need to
store base styles on the effect, or generate EffectSets just to store base styles.

Comment 69

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #68)
> That won't solve all our recursion problems (we'll probably still need the
> mIsComposingStyle member for a while)

I think we should solve the recursion anyway.  In my current understandings, there is a possibility that nsRuleNode is broken when 
several recursive call of  nsStyleSet::FileRules happens.  I am not really sure what number of the call of FileRules breaks the rule node (i.e. causes crash). I think this is pretty similar to bug 1277908.  To avoid the FileRules recursion, we need to use ResolveStyleByRemovingAnimation() instead of ResolveStyleWithoutAnimation() to get base styles.  That's because ResolveStyleWithoutAnimation() invokes FileRules whereas ResolveStyleByRemovingAnimation() doesn't. 

I don't still have a good explanation here but I think we need to fix the recursion first and then adjust base style handling.

Comment 70

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #69)
> (In reply to Brian Birtles (:birtles) from comment #68)
> > That won't solve all our recursion problems (we'll probably still need the
> > mIsComposingStyle member for a while)
> 
> I think we should solve the recursion anyway.  In my current understandings,
> there is a possibility that nsRuleNode is broken when 
> several recursive call of  nsStyleSet::FileRules happens.  I am not really
> sure what number of the call of FileRules breaks the rule node (i.e. causes
> crash). I think this is pretty similar to bug 1277908.  To avoid the
> FileRules recursion, we need to use ResolveStyleByRemovingAnimation()
> instead of ResolveStyleWithoutAnimation() to get base styles.  That's
> because ResolveStyleWithoutAnimation() invokes FileRules whereas
> ResolveStyleByRemovingAnimation() doesn't. 

I should note about FilRules here. The broken rule in FileRules() is document rule, not animation rule.

Comment 71

[Hiroyuki Ikezoe (:hiro)]

Attachment: The call stack of recursive FileRules call

This is the call stack when FileRules is recursively called against the same element.

On frame #15, we are calling EffectCompositor::GetAnimationRule which is invoked from FileRules in frame #17.

The GetAnimationRule ends up calling GetStyleContextForElementWithoutAnimation() on frame #5 and then calling FileRules on #0 frame.

Calling FileRules again while processing rule tree in FileRules is apparently a problem. This is the cause of the breakage of rule node.

So, my conclusion here is:

1) Don't call GetStyleContextXX() in functions that are called from GetAnimationRule().
2) Use ResolveStyleByRemovingAnimation() to get base styles.

Comment 74

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #68)
> So, with a few changes I think we could come to an arrangement where we only
> do UpdateProperties as part of restyling

As we discussed this morning, a cumbersome point here is that even if we move UpdateProperties in part of restyling, there are some cases that we have to resolve nsStyleContext, for example, it's RulesMatching() in EffectCompositor.  From what I understand nsStyleSet::FileRules is not allowed to be called recursively.  That means we have to use ResolveStyleByRemovingAnimation (which is not called FileRules as far as I can tell) to get base styles.  And ResolveStyleByRemovingAnimation requires an already resolved nsStyleContext.

In case of CSS animation and transition, this is not a problem since we already have a valid nsStyleContext when we create their instances. 

In case of script animation, we need carefully to resolve nsStyleContext for the ResolveStyleByRemovingAnimation not to resolve the nsStyleContext during restyling.  I think KeyframeEffectReadOnly::SetKeyframes(JSContext*, JSObject*) is a perfect place to resolve the nsStyleContext (this is the place what we currently do) because it's called only from script, not during restyling.

So regarding the recursive problem I think we need to use ResolveStyleByRemovingAnimation() instead of ResolveStyleWithoutAnimation(). As a result of ResolveStyleByRemovingAnimation() needs an nsStyleContext, we need to get base styles in UpdateProperties() because we have a valid nsStyleContext in all cases.

I will reconsider EffectSet lifetime problem.

Comment 75

[Brian Birtles (:birtles)]

I'm still a bit skeptical that resolving properties in SetKeyframes is the right place. If we don't have a target element we can't do it there, so then we need to add the same call to SetEffect. There may be other cases too. Rather than identify those cases, it seems more natural to include this as part of restyling (the only place we should really need this information).

Furthermore, in order to implement CSS animations correctly (which we currently don't), for some cases we really need to re-resolve properties as part of each tick anyway (e.g. when animating font-size on an element that uses em units to animate width). Given that, I think we should start moving towards a model where we can do UpdateProperties as part of restyling.

Comment 76

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #75)
> I'm still a bit skeptical that resolving properties in SetKeyframes is the
> right place. If we don't have a target element we can't do it there, so then
> we need to add the same call to SetEffect. There may be other cases too.
> Rather than identify those cases, it seems more natural to include this as
> part of restyling (the only place we should really need this information).
> 
> Furthermore, in order to implement CSS animations correctly (which we
> currently don't), for some cases we really need to re-resolve properties as
> part of each tick anyway (e.g. when animating font-size on an element that
> uses em units to animate width). Given that, I think we should start moving
> towards a model where we can do UpdateProperties as part of restyling.

I have come up with another idea.  The idea is:

1) Use nsIFrame's nsStyleContext to get base styles in restyles
2) If the nsIFrame has no nsStyleContext at that time, then defer composing styles for additive or accumulative animations, and set a flag to AnimValuesStyleRule that the styles for additive or accumulative animations has not done yet (this flag needs per property).
3) Once we have a valid nsStyleContext (but it does neither contain additive nor accumulative styles), call another ComposeStyle() with this valid nsStyleContext and use it to get base styles.

I guess this mechanism will also work for the font-size & width animation.  But it's not yet clear to me this will work when reframing happens, for example, there is a 'display' animation from 'relative' to 'absolute'.

Comment 77

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store base styles in KeyframeEffectReadOnly instead of EffectSet.

As I mentioned in bug 1336928 comment 13, we'd take the approach to store base styles in each KeyframeEffectReadOnly here.

Comment 78

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Resolve base styles during UpdateProprties

EnsureBaseStyle() requires an already resolved nsStyleContext
and resolves the base style by ResolveStyleByRemovingAnimation().

Comment 79

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 3: Drop all of unused stuff.

Comment 80

[Hiroyuki Ikezoe (:hiro)]

Attachment: transform.html

With part 1 and part 2, all of test cases in duplicated bugs no longer crash.
But the case in bug 1332071 still raises an NS_ASSERTION at the top of ApplyRenderingChangeToTree().
After some trial, it turns out the NS_ASSERTION is not related to this issue (i.e. missing keyframe handling).  Attaching file is a modified test case that still raises the NS_ASSERTION, but does not include missing keyframes at all.  I will open a new bug for it later.

[1] https://hg.mozilla.org/mozilla-central/file/25a94c1047e7/layout/base/RestyleManagerBase.cpp#l912

Comment 81

[Brian Birtles (:birtles)]

Comment on attachment 8835842 [details] [diff] [review]
Part 1: Store base styles in KeyframeEffectReadOnly instead of EffectSet.

Review of attachment 8835842 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed, particularly the change to GetMinAndMaxScaleForAnimationProperty.

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +370,5 @@
> +                                              result);
> +
> +  MOZ_DIAGNOSTIC_ASSERT(success,
> +                        "Should be able to extract computed animation value");
> +  MOZ_DIAGNOSTIC_ASSERT(!result.IsNull(),

As discussed, I wonder if these really need to be diagnostic assertions. Is it worth crashing DevEdition for this?

I think regular assertions are probably enough and we should make all the call sites of this handle (i.e. not access memory unsafely even if the rendered animation ends up being wrong) in the case where base style is null. I believe we already do this in the ContainedAnimatedScale method.

@@ +1763,5 @@
>  
> +    StyleAnimationValue baseStyle = BaseStyle(prop.mProperty);
> +    if (baseStyle.IsNull()) {
> +      // If we failed to get the base style, we consider it has scale value
> +      // here for the safety.

s/for the safety/just to be safe/

::: dom/animation/KeyframeEffectReadOnly.h
@@ +419,4 @@
>    // we need to re-evaluate the cascade of animations when that changes.
>    bool mInEffectOnLastAnimationTimingUpdate;
>  
> +  // The non-animated values for properties in this effect that contains at

s/contains/contain/

::: layout/base/nsLayoutUtils.cpp
@@ +588,5 @@
>        // We need to factor in the scale of the base style if the base style
>        // will be used on the compositor.
> +      StyleAnimationValue baseStyle = effect->BaseStyle(prop.mProperty);
> +      if (baseStyle.IsNull()) {
> +        continue;

Can we really continue here? Don't we still need to do the following loop over the properties?

Comment 82

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store base styles in KeyframeEffectReadOnly instead of EffectSet.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think it's easy.  Actually there are a bunch of test case that created by fuzzing, but all of them are complicated and sophisticated.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Nothing.

Which older supported branches are affected by this flaw?
Firefox 53.

If not all supported branches, which bug introduced the flaw?
Bug 1305325.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes, I've already created it.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.  I did run all of the test cases in duplicated bug of this and run locally our automation tests (mochitest, crashtest, reftest, web-platform-tests) that are relevant to animations.

Comment 83

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8835843 [details] [diff] [review]
Part 2: Resolve base styles during UpdateProprties

[Security approval request comment]
Likewise part 1.

Comment 84

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8835843 [details] [diff] [review]
Part 2: Resolve base styles during UpdateProprties

sec-approval+ for trunk. We'll want Aurora patches made and nominated.

You only need to request sec-approval? once, not for each patch. :-)

Comment 85

[Hiroyuki Ikezoe (:hiro)]

(In reply to Al Billings [:abillings] from comment #84)
> Comment on attachment 8835843 [details] [diff] [review]
> Part 2: Resolve base styles during UpdateProprties
> 
> sec-approval+ for trunk. We'll want Aurora patches made and nominated.
> 
> You only need to request sec-approval? once, not for each patch. :-)

Thanks! I was actually wondering I should do the approval request against each one of the patches.

Comment 86

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f6d5054e20b8f54620d05aced195bb3a3192d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eb3b0501d00e46775fab51d2eb753ebc0603d6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0077d103e8629294d9ffbb7cf4a7cfc6fd4fba5b

Comment 87

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 4: Various crash tests

1331704-2.html passes without any fix at this point, but will fail if base
styles are not preserved once the animation restarts right after the animation
finished.

All other test cases crash.
I added bug numbers in each test file to represent corresponding bug in crashtests.list.

Comment 88

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store base styles in KeyframeEffectReadOnly instead of EffectSet for aurora

This is for aurora branch of part 1 patch. Part 2 and 3 does not need to be modified to aurora.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1305325.
[User impact if declined]: Security risk.
[Is this code covered by automated tests?]: Not at this moment, Part 4 (attachment 8836319 [details] [diff] [review]) will cover it.
[Has the fix been verified in Nightly?]: Not yet, just landed on inbound.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 2 and 3 patch in this bug.
[Is the change risky?]:  low risk.
[Why is the change risky/not risky?]: After these patches, we handle harmlessly failure cases caused by bug 1305325. See below comment from comment 81.
[String changes made/needed]: None.

From comment 81.
 > I think regular assertions are probably enough and we should make all the call sites of
 > this handle (i.e. not access memory unsafely even if the rendered animation ends up being 
 > wrong) in the case where base style is null. I believe we already do this in the 
 > ContainedAnimatedScale method.

Comment 89

[Phil Ringnalda (:philor)]

This was backed out in https://hg.mozilla.org/mozilla-central/rev/b729a810bfe0 https://hg.mozilla.org/mozilla-central/rev/dfd82431a840 https://hg.mozilla.org/mozilla-central/rev/e2f6d5054e20 "for build bustage: unused variable hasProperty at KeyframeEffectReadOnly.h:294." e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=76569140&repo=mozilla-inbound

Comment 90

[Hiroyuki Ikezoe (:hiro)]

Oh, thank you!  I did not notice it when I did opt build locally...

Comment 91

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a53b189dbf463645870e1d43540ca3a5188d5953
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bba0fd5c0aa6a16d4b48b996b389d3c093df218
https://hg.mozilla.org/integration/mozilla-inbound/rev/12492a019ef3eaa12bbc0744b1fd80d1900f8f49

There were two unused variables on opt build.

Comment 92

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Store base styles in KeyframeEffectReadOnly instead of EffectSet for aurora

Fix two warnings on opt builds as well.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1305325.
[User impact if declined]: Security risk.
[Is this code covered by automated tests?]: Not at this moment, Part 4 (attachment 8836319 [details] [diff] [review] [diff] [review]) will cover it.
[Has the fix been verified in Nightly?]: Not yet, just landed on inbound.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 2 and 3 patch in this bug.
[Is the change risky?]:  low risk.
[Why is the change risky/not risky?]: After these patches, we handle harmlessly failure cases caused by bug 1305325. See below comment from comment 81.
[String changes made/needed]: None.

Comment 93

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a53b189dbf46
https://hg.mozilla.org/mozilla-central/rev/3bba0fd5c0aa
https://hg.mozilla.org/mozilla-central/rev/12492a019ef3

Comment 94

[Hiroyuki Ikezoe (:hiro)]

Thank you all involved, especially Brian for reviews including many things that I should learn. And Jesse for sophisticated test cases.  I recently realized that your test cases are really minimized and easier to understand what's going on there. Thanks!

Comment 95

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #80)
> Created attachment 8835847 [details]
> transform.html
> 
> With part 1 and part 2, all of test cases in duplicated bugs no longer crash.
> But the case in bug 1332071 still raises an NS_ASSERTION at the top of
> ApplyRenderingChangeToTree().
> After some trial, it turns out the NS_ASSERTION is not related to this issue
> (i.e. missing keyframe handling).  Attaching file is a modified test case
> that still raises the NS_ASSERTION, but does not include missing keyframes
> at all.  I will open a new bug for it later.

I've already filed for this before. Bug 1332588

Comment 96

[Brian Birtles (:birtles)]

Comment on attachment 8836319 [details] [diff] [review]
Part 4: Various crash tests

Review of attachment 8836319 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/crashtests/crashtests.list
@@ +28,5 @@
> +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1331704-4.html # bug 1311257 (bug 1332093)
> +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1331704-5.html # bug 1311257 (bug 1333544)
> +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1331704-6.html # bug 1311257 (bug 1334040)
> +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1331704-7.html # bug 1311257 (bug 1336794)
> +skip-if(stylo) pref(dom.animations-api.core.enabled,true) load 1331704-8.html # bug 1311257 (bug 1336928)

Do we need the bug number for the duplicates here?

It's a bit confusing that we have:

"<bug number for why we're skipping on stylo> (<duplicate bug number where test case comes from>)"

Maybe just drop the bit in () ?

Comment 97

[Gerry Chang [:gchang]]

Comment on attachment 8836517 [details] [diff] [review]
Part 1: Store base styles in KeyframeEffectReadOnly instead of EffectSet for aurora

Fix a sec-high. Aurora53+.

Comment 98

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40ad75d6e9a5
https://hg.mozilla.org/releases/mozilla-aurora/rev/a0897e0e0d0f
https://hg.mozilla.org/releases/mozilla-aurora/rev/08243c8a9651