SEGV on unknown address [@ fetch_add]

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Animation
P1
critical
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: truber, Assigned: hiro)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla53
crash, csectype-wildptr, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 fixed)

Details

(Whiteboard: [post-critsmash-triage])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments, 2 obsolete attachments)

2.75 KB, text/html
Details
14.23 KB, text/plain
Details
13.90 KB, text/plain
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details | Review
58 bytes, text/x-review-board-request
birtles
: review+
Details | Review
794 bytes, text/html
Details
15.63 KB, text/plain
Details
411 bytes, text/html
Details
2.36 KB, patch
birtles
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
1.89 KB, patch
birtles
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 months ago
Created attachment 8817058 [details]
testcase.html

The attached testcase crashes on an unknown high address in mozilla-central rev ad993783599a.

This is not very reduced because changing almost anything produces a SEGV near-NULL instead [@ GetUnit] (attached as log2.txt).

==18289==ERROR: AddressSanitizer: SEGV on unknown address 0x7fa56d6ce454 (pc 0x7fa551c1b30c bp 0x7fa538cbd350 sp 0x7fa538cbcc40 T24)
    #0 0x7fa551c1b30b in fetch_add /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../include/c++/4.8.5/bits/atomic_base.h:614:16
    #1 0x7fa551c1b30b in add /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Atomics.h:254
    #2 0x7fa551c1b30b in inc /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Atomics.h:286
    #3 0x7fa551c1b30b in operator++ /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Atomics.h:609
    #4 0x7fa551c1b30b in operator++ /home/worker/workspace/build/src/obj-firefox/dist/include/nsISupportsImpl.h:302
    #5 0x7fa551c1b30b in AddRef /home/worker/workspace/build/src/obj-firefox/dist/include/nsCSSValue.h:1216
    #6 0x7fa551c1b30b in AddRef /home/worker/workspace/build/src/gfx/layers/../../mfbt/RefPtr.h:37
    #7 0x7fa551c1b30b in AddRef /home/worker/workspace/build/src/gfx/layers/../../mfbt/RefPtr.h:396
    #8 0x7fa551c1b30b in RefPtr /home/worker/workspace/build/src/gfx/layers/../../mfbt/RefPtr.h:111
    #9 0x7fa551c1b30b in FrameTransformProperties /home/worker/workspace/build/src/obj-firefox/dist/include/nsDisplayList.h:4234
    #10 0x7fa551c1b30b in ApplyAnimatedValue /home/worker/workspace/build/src/gfx/layers/composite/AsyncCompositionManager.cpp:663
(Reporter)

Comment 1

11 months ago
Created attachment 8817060 [details]
log.txt
(Reporter)

Comment 2

11 months ago
Created attachment 8817061 [details]
log2.txt
(Assignee)

Updated

11 months ago
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 3

10 months ago
Note that KeyframeEffectReadOnly::mNeedsBaseStyleSet is not set when this segv happens, as a result, the base style is null on the compositor.   That's because, as far as I can see, we are sending the transform animation without processing KeyframeEffectReadOnly::ComposeStyle() (we are setting mNeedsBaseStyleSet inside this function).

Leaving ni? to me for now.
(Assignee)

Updated

10 months ago
Blocks: 1305325
(Assignee)

Comment 4

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Note that KeyframeEffectReadOnly::mNeedsBaseStyleSet is not set when this
> segv happens, as a result, the base style is null on the compositor.  
> That's because, as far as I can see, we are sending the transform animation
> without processing KeyframeEffectReadOnly::ComposeStyle() (we are setting
> mNeedsBaseStyleSet inside this function).

The reason why we don't call KeyframeEffectReadOnly::ComposeStyle(), the animation in the test case is still delay phase at that point.  So if all of animations which will be sent to the compositor are in delay phase , we should set the base style and set a bit in mNeedsBaseStyleSet somehow.

There are 3 places we can do that:

1. EffectCompositor::ComposeAnimationRule()
2. FindAnimationsForCompositor()
3. AddAnimationsForProperty() in nsDisplayList.cpp

2 seems not to be appropriate place because the function name does not match to what we want to do at all.
3 seems to me that it's the best place because, at that place, we have only properties that can be run on the compositor, so we don't need to iterate all effects to check whether the base style has been set or not (whereas we need to iterate all effects if we do it in ComposeAnimationRule).  Actually I prefer to do this kind of process in dom/animation though.

Anyway, I will try it in AddAnimationsForProperty().
Flags: needinfo?(hiikezoe)
(Assignee)

Comment 5

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ef7b4ba312cb1968fd9a65ed5b26ce2bbb9aa2c

Updated

10 months ago
Priority: -- → P1
(Assignee)

Comment 6

10 months ago
Gosh!  I made two mistakes in modification of descriptions int a test right before pushing the try...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1847b973c429fb50ffc46899dd105eb4df97f93
Comment hidden (mozreview-request)

Comment 8

10 months ago
mozreview-review
Comment on attachment 8818847 [details]
Bug 1322291 - Part 2: Make sure that the base style is set even if additive or accumulates animations are in the delay phase.

https://reviewboard.mozilla.org/r/98808/#review99622

I think the commit message is not accurate. It says we don't call ComposeStyle for animations in their delay phase but I'm pretty sure we do for backwards-filling animations in their delay phase, right?

Given that, how bad would it be if we just set mNeedsBaseStyle in KeyframeEffectReadOnly::ComposeStyle for animations in their delay phase? i.e. where we have the early return if if computedTiming.mProgress.IsNull(), just do the work to set mNeedsBaseStyle if the animation would need a base style if it were filling backwards at that point?

It seems like that would be simpler but what would be the cost? We'd end up filling in the base style in some cases where we don't need it but we'd only do it for compositor-animatable properties I think? And probably >50% of the time we'll end up sending those animations to the compositor anyway so we'd need to do that work anyway?

What do you think?

Clearing the review request now because I'd like to know what you think about this before doing a more thorough review (although I've added a couple of comments below in case we decide to stick with the current approach).

::: dom/animation/EffectCompositor.h:235
(Diff revision 1)
> +  // Set the base style corresponding to |aFrame| if necessary.
> +  // If we have 'additive' or 'accumulates' animation in |aAnimations| and
> +  // we have no base style for |aProperty|, set the base style in
> +  // EffectSet::mBaseStyleValues.
> +  static void MaybeSetBaseStyle(
> +    nsCSSPropertyID aProperty,
> +    const nsIFrame* aFrame,
> +    const nsTArray<RefPtr<dom::Animation>>& aAnimations);

I think I'd prefer to call this EnsureBaseStyle and just explain in the comment that this only does something when we actually need a base style?

::: dom/animation/EffectCompositor.cpp:969
(Diff revision 1)
> +    for (const AnimationProperty& property : keyframeEffect->Properties()) {
> +      if (property.mProperty != aProperty) {
> +        continue;
> +      }
> +
> +      // Bailout if we've already set the flag and have the base style.

This comment doesn't make sense. We don't have the base style since earlier in this function effects->HasBaseStyle(aProperty) returned false.

Should this be an assertion?

::: dom/animation/EffectCompositor.cpp:988
(Diff revision 1)
> +        Unused << EffectCompositor::GetBaseStyle(aProperty, aFrame);
> +        keyframeEffect->SetNeedsBaseStyle(aProperty);

(This feels a little fragile--having the EffectCompositor fill in data in the EffectSet and tweak flags on the effect so that the check in nsDisplayList passes. Which is why, if possible, it would be nice if we can handle this case within the effect itself.)
Attachment #8818847 - Flags: review?(bbirtles)
(Assignee)

Comment 9

10 months ago
(In reply to Brian Birtles (:birtles) from comment #8)
> Comment on attachment 8818847 [details]
> Bug 1322291 - Make sure that the base style is set even if additive or
> accumulates animations are in the delay phase.
> 
> https://reviewboard.mozilla.org/r/98808/#review99622
> 
> I think the commit message is not accurate. It says we don't call
> ComposeStyle for animations in their delay phase but I'm pretty sure we do
> for backwards-filling animations in their delay phase, right?

Thanks! That's right.  The message was apparently missing the fill:backwards case.  I will fix it.

> Given that, how bad would it be if we just set mNeedsBaseStyle in
> KeyframeEffectReadOnly::ComposeStyle for animations in their delay phase?

We also need to resolve the base style, actually we need to do it just once per element/frame though.
To be more precise, we skipped animations in delay phase at Animation::ComposeStyle(), there are IsInEffect() check. So we need some tweaks (dropping the IsInEffect check?) for the delayed animations in Animation::ComposeStyle(), I thought it's bit unfavorable.

> i.e. where we have the early return if if computedTiming.mProgress.IsNull(),
> just do the work to set mNeedsBaseStyle if the animation would need a base
> style if it were filling backwards at that point?

I am sorry, I don't quite understand this 'if it were filling backwards at that point'.  You mean the case that we don't actually need the base style? i.e. setting the base style for fill:backwards animations?

> It seems like that would be simpler but what would be the cost? We'd end up
> filling in the base style in some cases where we don't need it but we'd only
> do it for compositor-animatable properties I think? And probably >50% of the
> time we'll end up sending those animations to the compositor anyway so we'd
> need to do that work anyway?

Yes, right. I was bit worried about the cost. But I actually do not think it's a bad idea to fill the base style for unnecessary case here.

Side note: I have been thinking that the base style should be passed to the compositor along with layer (instead of an animation) some day.  Although I don't do it soon because I've heard that layers on the compositor will be replaced with display list-ish things for webrender. So in my current plan, it will be done after the webrender is integrated in mozilla-central.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> (In reply to Brian Birtles (:birtles) from comment #8)
> > Comment on attachment 8818847 [details]
> > Bug 1322291 - Make sure that the base style is set even if additive or
> > accumulates animations are in the delay phase.
> > 
> > https://reviewboard.mozilla.org/r/98808/#review99622
> > 
> > I think the commit message is not accurate. It says we don't call
> > ComposeStyle for animations in their delay phase but I'm pretty sure we do
> > for backwards-filling animations in their delay phase, right?
> 
> Thanks! That's right.  The message was apparently missing the fill:backwards
> case.  I will fix it.
> 
> > Given that, how bad would it be if we just set mNeedsBaseStyle in
> > KeyframeEffectReadOnly::ComposeStyle for animations in their delay phase?
> 
> We also need to resolve the base style, actually we need to do it just once
> per element/frame though.
> To be more precise, we skipped animations in delay phase at
> Animation::ComposeStyle(), there are IsInEffect() check. So we need some
> tweaks (dropping the IsInEffect check?) for the delayed animations in
> Animation::ComposeStyle(), I thought it's bit unfavorable.

Yeah, you're right. Perhaps we could just drop that call to IsInEffect? Given that it will not return false while we're pending, that early return only saves us:

* A call to PlayState() -- fairly cheap (although it does involve a virtual function call to mTimeline->GetCurrentTime within Animation::GetCurrentTime)
* AutoRestore on mHoldTime -- cheap
* A virtual function call to mEffect->AsKeyframeEffect()

From there on it should be much the same.

> > i.e. where we have the early return if if computedTiming.mProgress.IsNull(),
> > just do the work to set mNeedsBaseStyle if the animation would need a base
> > style if it were filling backwards at that point?
> 
> I am sorry, I don't quite understand this 'if it were filling backwards at
> that point'.  You mean the case that we don't actually need the base style?
> i.e. setting the base style for fill:backwards animations?

I mean, if we have an animation with no backwards fill in its delay phase, then we fill in the base style and set mNeedsBaseStyle as if fill=backwards/both. That is, we don't do it for all effects, only those that have a non-replace property value for a compositor-animatable property.

I wonder what the correct thing is to do for properties in |aPropertiesToSkip|? Do they get sent to the compositor? If not, we're fine.

I also wonder if need to check for underlying animations or not. If there is an underlying 'replace' animation then can we assume that when it finishes we will update the animations on the layer and fill in the base value at that point? Or do we need to have the base style so we can continue running correctly even when the underlying animation has finished (assuming it does not fill forwards)?
(Assignee)

Comment 11

10 months ago
(In reply to Brian Birtles (:birtles) from comment #10)
> > > i.e. where we have the early return if if computedTiming.mProgress.IsNull(),
> > > just do the work to set mNeedsBaseStyle if the animation would need a base
> > > style if it were filling backwards at that point?
> > 
> > I am sorry, I don't quite understand this 'if it were filling backwards at
> > that point'.  You mean the case that we don't actually need the base style?
> > i.e. setting the base style for fill:backwards animations?
> 
> I mean, if we have an animation with no backwards fill in its delay phase,
> then we fill in the base style and set mNeedsBaseStyle as if
> fill=backwards/both. That is, we don't do it for all effects, only those
> that have a non-replace property value for a compositor-animatable property.

Thanks.  Now I've got to understand.

> I wonder what the correct thing is to do for properties in
> |aPropertiesToSkip|? Do they get sent to the compositor? If not, we're fine.

Correct, no.  We should skip the properties in aPropertiesToSkip.

> I also wonder if need to check for underlying animations or not. If there is
> an underlying 'replace' animation then can we assume that when it finishes
> we will update the animations on the layer and fill in the base value at
> that point? Or do we need to have the base style so we can continue running
> correctly even when the underlying animation has finished (assuming it does
> not fill forwards)?

I was considering a bit about it before.  I thought ideally we don't need the base style if there is an underlying replace animation, but I can imagine that we will see graphical glitches if the main thread is under busy while the animation is finished. Also I think we will see unwanted style if the underlying replace animation has positive endDelay.

Anyway, I will drop IsInEffect() in Animation::ComposeStyle(). Thanks!
(Assignee)

Comment 12

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e5adfc340ade399c52f9047f73b374c7a9cc782
(Assignee)

Updated

10 months ago
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 13

10 months ago
The try result tells me that calling GetTargetStyleContext()[1] from KeyframeEffectReadOnly::ComposeStyle() for the case when mProgress is null causes infinite calls of ResolveStyleFor() if the animation is CSS animation.

[1] https://hg.mozilla.org/try/rev/96d6d397467548b26b8129942f6225187f7f7ef3#l2.17

The GetTargetStyleContext() call should be moved after the check that composite mode is not 'replace'.
(Assignee)

Comment 14

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> The try result tells me that calling GetTargetStyleContext()[1] from
> KeyframeEffectReadOnly::ComposeStyle() for the case when mProgress is null
> causes infinite calls of ResolveStyleFor() if the animation is CSS animation.

Though I can't yet write any test cases that cause this infinite calls of ResolveStyleFor() in case of script animations, I think we should introduce a mechanism to avoid the infinite calls using AutoRestore.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acae1901435987bb35824a66e6f91679268c60d8
(Assignee)

Comment 15

10 months ago
The try looks good with the mechanism to avoid the infinite calls.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

10 months ago
mozreview-review
Comment on attachment 8818847 [details]
Bug 1322291 - Part 2: Make sure that the base style is set even if additive or accumulates animations are in the delay phase.

https://reviewboard.mozilla.org/r/98808/#review100428

r=me with the following comments addressed.

::: dom/animation/KeyframeEffectReadOnly.h:430
(Diff revision 2)
> +  // Ensure the base styles for properties that can be run on the compositor
> +  // except properties in |aPropertiesToSkip|.

Ensure the base style is available for any properties that can be run on the compositor and which are not included in |aPropertiesToSkip|.

::: dom/animation/KeyframeEffectReadOnly.cpp:419
(Diff revision 2)
> +      }
> +
> +      if (!styleContext) {
> +        styleContext = GetTargetStyleContext();
> +      }
> +      MOZ_ASSERT(styleContext);

How certain are we that this will never be null? If we're not completely sure, I wonder if this should be a release assert?

::: dom/animation/KeyframeEffectReadOnly.cpp:425
(Diff revision 2)
> +
> +      Unused << EffectCompositor::GetBaseStyle(property.mProperty,
> +                                               styleContext,
> +                                               *mTarget->mElement,
> +                                               mTarget->mPseudoType);
> +      SetNeedsBaseStyle(property.mProperty);

Perhaps this should have a comment:

'Mark this property as needing a base style so that we send the (now cached) base style to the compositor'

?

::: dom/animation/KeyframeEffectReadOnly.cpp:450
(Diff revision 2)
> +    // In case of properties that can be run on the compositor, we need the base
> +    // styles for such properties because those animation will be sent to
> +    // compositor while they are in delay phase so that we can composite this
> +    // animation on the compositor once the animation is out of the delay phase
> +    // on the compositor.

This is really good, but might be a little more accurate as:

"If we are not in-effect this effect might still be sent to the compositor and later become in-effect (e.g. if it is in the delay phase). In that case, we might need the base style in order to perform additive/accumulative animation on the compositor."

Is it only the delay phase we care about? Or could we have a finished animation with a negative playback rate where this situation might arise? If it is only the delay phase, then we should probably check computedTiming.mPhase == ComputedTiming::AnimationPhase::Before before calling EnsureBaseStylesForCompositor.

::: dom/animation/test/crashtests/1322291-1.html:14
(Diff revision 2)
> +  // We need to wait for being the animation composited on the compositor, i.e.
> +  // the delay phase end.

This comment doesn't make sense--it says we're waiting for the end of the delay but it looks like we're waiting for the end of the active interval?
Attachment #8818847 - Flags: review?(bbirtles) → review+

Comment 19

10 months ago
mozreview-review
Comment on attachment 8820187 [details]
Bug 1322291 - Part 1: Block nested calls of KeyframeEffectReadOnly::ComposeStyle().

https://reviewboard.mozilla.org/r/99718/#review100432

I can see from the commit message that this situation can sometimes arise, but I wonder for what sort of content does this occur?

I wonder if there's some other way we should be avoiding this situation? When it does arise, is simply not composing style the correct thing to do?
(Assignee)

Comment 20

10 months ago
(In reply to Brian Birtles (:birtles) from comment #19)
> Comment on attachment 8820187 [details]
> Bug 1322291 - Part 1: Block nested calls of
> KeyframeEffectReadOnly::ComposeStyle().
> 
> https://reviewboard.mozilla.org/r/99718/#review100432
> 
> I can see from the commit message that this situation can sometimes arise,
> but I wonder for what sort of content does this occur?

As far as I can tell, the situation will never happen for now.  But once we have additive or accumulate composite for CSS animations (bug 1293490), I am convinced that the situation happens.

> I wonder if there's some other way we should be avoiding this situation?

I think we can avoid this specific case if we move GetBaseStyle call (which leads nsStyleSet::ResolveStyleFor) into EffectCompositor::UpdateEffectProperties (or something related to it).  But, even so, we will see another similar situation.

A call stack of nsStyleSet::ResolveStyleFor() is something like this:

nsStyleSet::ResolveStyleFor() calls
 nsStyleSet::FileRules() calls
   EffectCompositor::GetAnimationRule() calls
     KeyframeEffectReadOnly::ComposeStyle()
 nsStyleSet::GetContext()

nsStyleSet::GetContext() calls
 EffectCompositor::UpdateEffectProperties()

So, I think there is a fundamentally possibility that we will end up nested calls of ComposeStyle() or UpdateEffectProperties().
There is no way what I can think of now in my understandings.

> When it does arise, is simply not composing style the correct thing to do?

I think so.
After the first composing style, we don't need any additional style any more.

Comment 21

10 months ago
mozreview-review
Comment on attachment 8820187 [details]
Bug 1322291 - Part 1: Block nested calls of KeyframeEffectReadOnly::ComposeStyle().

https://reviewboard.mozilla.org/r/99718/#review100452

::: dom/animation/KeyframeEffectReadOnly.cpp:395
(Diff revision 1)
> +  if (mIsComposingStyle) {
> +    return;
> +  }
> +
> +  AutoRestore<bool> isComposingStyle(mIsComposingStyle);
> +  mIsComposingStyle = true;

As discussed, this feels like a bit of a hack. In theory, it shouldn't be necessary to compute the animated style for an element in order to get the base style (since that's the definition of the base style: no animations) so we shouldn't end up calling ComposeStyle on animation objects recursively just because we requested the base style.

We discussed passing the style context from EffectCompositor::UpdateEffectProperties down the chain and using that in EnsureBaseStylesForCompositor but it seems like there will be cases where that style context will be null.

It seems like we need a way to get the base style without triggering a call into animation code (or at least not for the same element). That might be a bit of work so I'm ok with adding this functionality now as long as we have a comment to that effect and a bug filed for doing that work. I'd like to avoid multiplying workarounds and instead refactor things to behave as expected.
Attachment #8820187 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 22

10 months ago
(In reply to Brian Birtles (:birtles) from comment #21)
> Comment on attachment 8820187 [details]
> Bug 1322291 - Part 1: Block nested calls of
> KeyframeEffectReadOnly::ComposeStyle().
> 
> https://reviewboard.mozilla.org/r/99718/#review100452
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp:395
> (Diff revision 1)
> > +  if (mIsComposingStyle) {
> > +    return;
> > +  }
> > +
> > +  AutoRestore<bool> isComposingStyle(mIsComposingStyle);
> > +  mIsComposingStyle = true;
> 
> As discussed, this feels like a bit of a hack. In theory, it shouldn't be
> necessary to compute the animated style for an element in order to get the
> base style (since that's the definition of the base style: no animations) so
> we shouldn't end up calling ComposeStyle on animation objects recursively
> just because we requested the base style.
> 
> We discussed passing the style context from
> EffectCompositor::UpdateEffectProperties down the chain and using that in
> EnsureBaseStylesForCompositor but it seems like there will be cases where
> that style context will be null.
> 
> It seems like we need a way to get the base style without triggering a call
> into animation code (or at least not for the same element). That might be a
> bit of work so I'm ok with adding this functionality now as long as we have
> a comment to that effect and a bug filed for doing that work. I'd like to
> avoid multiplying workarounds and instead refactor things to behave as
> expected.

Thanks for really great suggestion.  Filed bug 1324966.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84235ea3de409c9f58cace5230431c78692b2a08

Comment 26

10 months ago
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/autoland/rev/a09000ada715
Part 1: Block nested calls of KeyframeEffectReadOnly::ComposeStyle(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/c469088d05e4
Part 2: Make sure that the base style is set even if additive or accumulates animations are in the delay phase.  r=birtles

Comment 27

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a09000ada715
https://hg.mozilla.org/mozilla-central/rev/c469088d05e4
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

10 months ago
Group: dom-core-security
(Reporter)

Updated

10 months ago
Keywords: csectype-wildptr
(Reporter)

Comment 28

10 months ago
Created attachment 8821425 [details]
testcase_post_fix.html

This testcase causes the same crash at a different address in rev 8460203bc93b.
(Reporter)

Comment 29

10 months ago
Created attachment 8821426 [details]
log_post_fix.txt
(Reporter)

Updated

10 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 30

10 months ago
Created attachment 8821491 [details]
transform with iterationStart

Thank you, Jesse!  The new sample noticed me that we need to set base styles explicitly even if computedTiming.mProgress is not null.

I am thinking that we need to add a stack base class to call EnsureBaseStylesForCompositor in its dtor.
(Assignee)

Comment 31

10 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #30)
> Created attachment 8821491 [details]
> transform with iterationStart

This example has iterationStart:0.5 and the initial frame from 0.0 to 1/3 offset is a missing keyframe.  So when we compose the first style, we don't set the base style at all.
(Assignee)

Comment 32

10 months ago
Created attachment 8821836 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor.

AnimationProperty has mHasAdditiveOrAccumulative, which is updated in           
UpdateProperties(), to avoid iteration over all segments in ComposeStyle().
(Assignee)

Comment 33

10 months ago
Brian, what do you think of attachment 8821836 [details] [diff] [review]?
Flags: needinfo?(bbirtles)
(Assignee)

Comment 34

10 months ago
Created attachment 8821853 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor v2

After discussion with Brian, we'd like to drop mHasAdditiveOrAccumulative flag to avoid mistakes to set/clear such cached flag.
Attachment #8821836 - Attachment is obsolete: true

Comment 35

10 months ago
Adding Atte to help with verification. He has multiple test cases.
Comment on attachment 8821853 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor v2

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

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +417,5 @@
>          continue;
>        }
> +      if (NeedsBaseStyle(property.mProperty)) {
> +        continue;
> +      }

Shouldn't this go in the outer loop? i.e. before we iterate over the segments?

(I also wonder if we need to use NeedsBaseStyle or if we can just go straight to |mNeedsBaseStyleSet| since we've already checked that we have a compositor-animatable property. Perhaps we can just assert that the property is in LayerAnimationInfo::sRecords.)

@@ +466,4 @@
>      // compositor while they are in delay phase so that we can composite this
>      // animation on the compositor once the animation is out of the delay phase
>      // on the compositor.
> +    EnsureBaseStylesForCompositor(aPropertiesToSkip);

I think the comment before this might need some adjusting to describe the case brought up here (i.e. comment 31).

@@ +571,5 @@
>      }
>    }
> +
> +  // For properties that can be run on the compositor, we need base styles on
> +  // the compositor even if the objective sgment for properties does have

s/sgment/segment/

Not sure what 'objective' means here?

s/does have neither ... or .../does not have either ... or .../

Updated

10 months ago
Flags: needinfo?(bbirtles)
(Assignee)

Comment 37

10 months ago
(In reply to Brian Birtles (:birtles) from comment #36)
> Comment on attachment 8821853 [details] [diff] [review]
> Part 3: Ensure base styles if there is an additive or accumulate segment for
> propertis that can be run on the compositor v2
> 
> Review of attachment 8821853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> @@ +417,5 @@
> >          continue;
> >        }
> > +      if (NeedsBaseStyle(property.mProperty)) {
> > +        continue;
> > +      }
> 
> Shouldn't this go in the outer loop? i.e. before we iterate over the
> segments?

Ah, exactly. I will do it. Thanks!

> (I also wonder if we need to use NeedsBaseStyle or if we can just go
> straight to |mNeedsBaseStyleSet| since we've already checked that we have a
> compositor-animatable property. Perhaps we can just assert that the property
> is in LayerAnimationInfo::sRecords.)

My intention was to skip iteration over all mSegments once we set NeedsBaseStyle() for the case we have the same properties on different effect. E.g.:

 element.animate({ transform: 100px }, { duration: 1000, delay: 1000 });
 element.animate([{ transform: scale: 2 },
                  { transform: scale: 4 }], {duration: 2000, composite: 'add' });

> @@ +571,5 @@
> >      }
> >    }
> > +
> > +  // For properties that can be run on the compositor, we need base styles on
> > +  // the compositor even if the objective sgment for properties does have
> 
> s/sgment/segment/
> 
> Not sure what 'objective' means here?

'current processing segment'?
(Assignee)

Comment 38

9 months ago
(In reply to Brian Birtles (:birtles) from comment #36)

> @@ +466,4 @@
> >      // compositor while they are in delay phase so that we can composite this
> >      // animation on the compositor once the animation is out of the delay phase
> >      // on the compositor.
> > +    EnsureBaseStylesForCompositor(aPropertiesToSkip);
> 
> I think the comment before this might need some adjusting to describe the
> case brought up here (i.e. comment 31).

The case in comment 31 (with a halfway iterationStart) gets caught by EnsureBaseStylesForCompositor() at the last of ComposeStyle(), not here.

Apart from this, I thought we send negative playback animation with endDelay to the compositor even it's in endDelay phase, but actually we don't now.  That's because in the endDelay phase the animation phase is 'after'. so IsCurrent() is false at that time.  We can leave "computedTiming.mPhase == ComputedTiming::AnimationPhase::Before" check at this moment but I am going to drop the check in this bug with a comment(for the future).  I just filed bug 1330498 for the endDelay issue.

Thank you for the review and giving me a time to check current behavior.
(Assignee)

Comment 39

9 months ago
Created attachment 8826039 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor v3
Attachment #8821853 - Attachment is obsolete: true
Attachment #8826039 - Flags: review?(bbirtles)
(Assignee)

Comment 40

9 months ago
Created attachment 8826040 [details] [diff] [review]
Part 4: Additional crash test
Attachment #8826040 - Flags: review?(bbirtles)
Comment on attachment 8826039 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor v3

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

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +430,4 @@
>        continue;
>      }
>  
> +    if (NeedsBaseStyle(property.mProperty)) {

Does this need a comment? It seems a little counter-intuitive that when you call "EnsureBaseStyleForCompositor" we do an early return if we *do* need a base style. Perhaps we should say, "We only call SetNeedsBaseStyle after calling GetBaseStyle so if NeedsBaseStyle is true the base style should be already filled-in."

@@ +486,5 @@
> +    // compositor while they are in delay phase or endDelay phase with negative
> +    // playback rate so that we can composite this animation on the compositor
> +    // once the animation is out of the phase on the compositor.
> +    // NOTE: Actually we don't send negative playback animation in its endDelay
> +    // phase to the compositor, it will be fixed in bug 1330498.

I think we don't need this second comment paragraph. There is one before it that says much the same thing. I think we should just adjust that previous comment to something like:

    // If we are not in-effect, this effect might still be sent to the
    // compositor and later become in-effect (e.g. if it is in the delay phase,
    // or, if it is in the end delay phase but with a negative playback rate).
    // In that case, we might need the base style in order to perform
    // additive/accumulative animation on the compositor.
    //
    // Note, however, that we don't actually send animations with a negative
    // playback rate in their end delay phase to the compositor at this stage
    // (bug 1330498).

@@ +599,5 @@
> +  // For properties that can be run on the compositor, we need base styles on
> +  // the compositor even if the current processing segment for properties does
> +  // not have either additive or accumulative composite, even if the animations
> +  // is not in-effect. That's because the animation will progress to the segment
> +  // which has additive or accumulative composite on the compositor later.

// For properties that can be run on the compositor, we need may need to
  // prepare base styles to send to the compositor even if the current
  // processing segment for properties does not have either an additive or
  // accumulative composite mode, and even if the animation is not in-effect.
  // That's because the animation may later progress to a segment which has
  // an additive or accumulative composite mode.
Attachment #8826039 - Flags: review?(bbirtles) → review+
Attachment #8826040 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 42

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6fee753b7ea7a6f0148e1aaafe7aeaa7b54990
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c7c8b14e0bb86eae9339bfb2ca602b00e83abee

Thanks a lot!
https://hg.mozilla.org/mozilla-central/rev/6e6fee753b7e
https://hg.mozilla.org/mozilla-central/rev/3c7c8b14e0bb
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago9 months ago
Resolution: --- → FIXED
Blocks: 1324063
hiro: please request sec-approval? before landing patches for security bugs.
See https://wiki.mozilla.org/Security/Bug_Approval_Process

Part of that process is answering some questions auto-populated when you add the flag. Please do that now retroactively so we can make a call about the severity and need to land on other supported branches.
status-firefox51: --- → ?
status-firefox52: --- → ?
status-firefox-esr45: --- → ?
Flags: needinfo?(hikezoe)
Keywords: sec-high
(Assignee)

Comment 45

9 months ago
This is also a regression by bug 1305325.  This does not need to be uplifted.
status-firefox50: --- → unaffected
status-firefox51: ? → unaffected
status-firefox52: ? → unaffected
status-firefox-esr45: ? → unaffected
Flags: needinfo?(hikezoe)
Hiro, I think Daniel would still like you to apply for sec-approval for completeness.
(Assignee)

Comment 47

9 months ago
Comment on attachment 8826039 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor v3

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
A test case included in the patch has been already landed.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
A test case included in the patch has been already landed.

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?
No, this does not need backports.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.  It's been for six days after landing the patch.
Attachment #8826039 - Flags: sec-approval?
Comment on attachment 8826039 [details] [diff] [review]
Part 3: Ensure base styles if there is an additive or accumulate segment for propertis that can be run on the compositor v3

sec-approval+ for trunk.
Attachment #8826039 - Flags: sec-approval? → sec-approval+
Group: dom-core-security → core-security-release
(Reporter)

Updated

9 months ago
Blocks: 1334591
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.