1289701 - Crash in mozilla::AnimValuesStyleRule::AddValue

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

This bug was filed from the Socorro interface and is 
report bp-1954f7ec-ab07-4791-96ef-1ebf42160725.
=============================================================

This is #4 crashes on Mac, Nightly build 20160725030248. The first crash appeared on 2016-07-25.

Probably a regression from bug 1288586, Hiroyuki san, do you have any idea?

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

There are some crashes under Linux with different signature:
bp-0be371fe-8bed-49e1-b8b1-53b7b2160725
bp-f42f88b0-f406-4714-b8a1-58b352160725

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I am really surprised the change causes crashes because the change basically just moved the call of CalculateCumulativeChangeHint() from inside GetAnimationPropertiesFromKeyframes[1] to [2].  I see nothing in particular between them.  Actually I have no idea what happened at the top of the stack[3].

[1] http://hg.mozilla.org/mozilla-central/file/78dd94ba93c7/dom/animation/KeyframeEffect.cpp#l560
[2] http://hg.mozilla.org/mozilla-central/file/78dd94ba93c7/dom/animation/KeyframeEffect.cpp#l599
[3] https://hg.mozilla.org/mozilla-unified/annotate/7c669d5d63ef/dom/animation/AnimValuesStyleRule.h#l44

One thing I noticed is that all of the stack traces have mozilla::ElementRestyler::RestyleUndisplayedNodes.  We've been suffering from various crashes triggered by undisplayed nodes.  E.g. bug 1245260, bug 1279819, bug 1247914.  These crashes depend on timing.  So, I suspect that moving the call of CalculateCumulativeChangeHint() causes timing change, as a result this crash appeared.

Leaving ni? to me.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I can reproduce this crash with a test case in bug 1279819, and confirmed that a patch what I tried but not checked-in in bug 1279819 fixes this crash.

An security issue has certainly been fixed in bug 1279819, but there has been still another potential issue which causes this crash.  The patch for bug 1288586 revealed the another issue.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

There is one crash from 48.0b10 20160721144529. Is it related? Should we mark 48 as affected?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Yes, I think so.
The one has a bit different stack, but the build has the fix for bug 1279819, it's the same as current trunk, but does not have the fix for bug 1288586, so I guess the difference is caused by it.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Brian, can you mark this bug as security sensitive one?

The test case in bug 1279819 causes another heap-use-after free.

=================================================================
==9802==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000755388 at pc 0x7f61bf8272b3 bp 0x7ffdbb571750 sp 0x7ffdbb571748
READ of size 4 at 0x615000755388 thread T0
    #0 0x7f61bf8272b2 in mozilla::dom::KeyframeEffectReadOnly::CalculateCumulativeChangeHint(nsStyleContext*) /home/zoe/mozilla-inbound/dom/animation/KeyframeEffect.cpp:1504:45


I should have tried the test case on ASAN build first.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Thank you, Brian.

A problem here is also nested calls of GetContext(). From ASAN error log;

0x615000037088 is located 8 bytes inside of 512-byte region [0x615000037080,0x615000037280)
freed by thread T0 here:
    #0 0x4c0652 in free (/home/zoe/mozilla-inbound/obj-asan/dist/bin/firefox+0x4c0652)
    #1 0x7f8880676e29 in nsTArrayFallibleAllocator::Free(void*) /home/zoe/mozilla-inbound/obj-asan/dist/include/nsTArray.h:170:34
    #2 0x7f8880676e29 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::ShrinkCapacity(unsigned long, unsigned long) /home/zoe/mozilla-inbound/obj-asan/dist/include/nsTArray-inl.h:230
    #3 0x7f8882e72401 in nsTArray_Impl<mozilla::AnimationProperty, nsTArrayInfallibleAllocator>::Clear() /home/zoe/mozilla-inbound/obj-asan/dist/include/nsTArray.h:1591:18
    #4 0x7f8882e72401 in nsTArray_Impl<mozilla::AnimationProperty, nsTArrayInfallibleAllocator>::operator=(nsTArray_Impl<mozilla::AnimationProperty, nsTArrayInfallibleAllocator>&&) /home/zoe/mozilla-inbound/obj-asan/dist/include/nsTArray.h:903
    #5 0x7f8882e51868 in nsTArray<mozilla::AnimationProperty>::operator=(nsTArray<mozilla::AnimationProperty>&&) /home/zoe/mozilla-inbound/obj-asan/dist/include/nsTArray.h:2100:5
    #6 0x7f8882e51868 in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /home/zoe/mozilla-inbound/dom/animation/KeyframeEffect.cpp:590
    #7 0x7f8882e5ca7a in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(nsTArray<mozilla::Keyframe>&&, nsStyleContext*) /home/zoe/mozilla-inbound/dom/animation/KeyframeEffect.cpp:489:5
    #8 0x7f8886a2e50c in UpdateOldAnimationPropertiesWithNew(mozilla::dom::CSSAnimation&, mozilla::TimingParams&, nsTArray<mozilla::Keyframe>&, bool, nsStyleContext*) /home/zoe/mozilla-inbound/layout/style/nsAnimationManager.cpp:343:5
    #9 0x7f8886a2e50c in CSSAnimationBuilder::Build(nsPresContext*, mozilla::StyleAnimation const&, nsCSSKeyframesRule const*) /home/zoe/mozilla-inbound/layout/style/nsAnimationManager.cpp:627
    #10 0x7f8886a2db25 in nsAnimationManager::BuildAnimations(nsStyleContext*, mozilla::dom::Element*, mozilla::AnimationCollection<mozilla::dom::CSSAnimation>*, nsTArray<RefPtr<mozilla::dom::CSSAnimation> >&) /home/zoe/mozilla-inbound/layout/style/nsAnimationManager.cpp:1090:33
    #11 0x7f8886a2d15d in nsAnimationManager::UpdateAnimations(nsStyleContext*, mozilla::dom::Element*) /home/zoe/mozilla-inbound/layout/style/nsAnimationManager.cpp:405:5
    #12 0x7f8886b9c1d8 in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) /home/zoe/mozilla-inbound/layout/style/nsStyleSet.cpp:949:7
    #13 0x7f8886b9f8e3 in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) /home/zoe/mozilla-inbound/layout/style/nsStyleSet.cpp:1367:10
    #14 0x7f8886b9f32a in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*) /home/zoe/mozilla-inbound/layout/style/nsStyleSet.cpp:1325:10
    #15 0x7f8886b35124 in mozilla::StyleSetHandle::Ptr::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*) /home/zoe/mozilla-inbound/obj-asan/dist/include/mozilla/StyleSetHandleInlines.h:85:3
    #16 0x7f8886b35124 in CalcLengthWith(nsCSSValue const&, int, nsStyleFont const*, nsStyleContext*, nsPresContext*, bool, bool, mozilla::RuleNodeCacheConditions&) /home/zoe/mozilla-inbound/layout/style/nsRuleNode.cpp:517
    #17 0x7f8886bed4d1 in SetFontSizeCalcOps::ComputeLeafValue(nsCSSValue const&) /home/zoe/mozilla-inbound/layout/style/nsRuleNode.cpp:3223:14
    #18 0x7f8886b7c2e7 in nsRuleNode::SetFontSize(nsPresContext*, nsRuleData const*, nsStyleFont const*, nsStyleFont const*, int*, nsFont const&, int, int, bool, bool, mozilla::RuleNodeCacheConditions&) /home/zoe/mozilla-inbound/layout/style/nsRuleNode.cpp:3317:14
    #19 0x7f8886b80163 in nsRuleNode::SetFont(nsPresContext*, nsStyleContext*, unsigned char, nsRuleData const*, nsStyleFont const*, nsStyleFont*, bool, mozilla::RuleNodeCacheConditions&) /home/zoe/mozilla-inbound/layout/style/nsRuleNode.cpp:3903:3
    #20 0x7f8886b3be6d in nsRuleNode::ComputeFontData(void*, nsRuleData const*, nsStyleContext*, nsRuleNode*, nsRuleNode::RuleDetail, mozilla::RuleNodeCacheConditions) /home/zoe/mozilla-inbound/layout/style/nsRuleNode.cpp:4162:5
    #21 0x7f8886b38c7a in nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*) /home/zoe/mozilla-inbound/layout/style/nsRuleNode.cpp:2499:11
    #22 0x7f8886b39243 in nsStyleContext::StyleData(nsStyleStructID) /home/zoe/mozilla-inbound/layout/style/nsStyleContext.cpp:456:15
    #23 0x7f8882e6418b in mozilla::dom::CreateStyleContextForAnimationValue(nsCSSProperty, mozilla::StyleAnimationValue const&, nsStyleContext*) /home/zoe/mozilla-inbound/dom/animation/KeyframeEffect.cpp:1486:3
    #24 0x7f8882e5fe8d in mozilla::dom::KeyframeEffectReadOnly::CalculateCumulativeChangeHint(nsStyleContext*) /home/zoe/mozilla-inbound/dom/animation/KeyframeEffect.cpp:1500:9
    #25 0x7f8882e519a4 in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /home/zoe/mozilla-inbound/dom/animation/KeyframeEffect.cpp:599:3
    #26 0x7f8882e51174 in mozilla::EffectCompositor::UpdateEffectProperties(nsStyleContext*, mozilla::dom::Element*, mozilla::CSSPseudoElementType) /home/zoe/mozilla-inbound/dom/animation/EffectCompositor.cpp:240:5
    #27 0x7f8886b9c257 in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) /home/zoe/mozilla-inbound/layout/style/nsStyleSet.cpp:951:7
    #28 0x7f8886b9f8e3 in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) /home/zoe/mozilla-inbound/layout/style/nsStyleSet.cpp:1367:10
    #29 0x7f8886c90930 in mozilla::ElementRestyler::RestyleUndisplayedNodes(nsRestyleHint, mozilla::UndisplayedNode*, nsIContent*, nsStyleContext*, unsigned char) /home/zoe/mozilla-inbound/layout/base/RestyleManager.cpp:4615:9
    #30 0x7f8886c8f50d in mozilla::ElementRestyler::DoRestyleUndisplayedDescendants(nsRestyleHint, nsIContent*, nsStyleContext*) /home/zoe/mozilla-inbound/layout/base/RestyleManager.cpp:4560:3

While we iterate mProperties in CalculateCumulativeChangeHint() [1], we end up calling another UpdateProperties() inside the loop, then iterating mProperties is freed.  It's the problem.

[1] http://hg.mozilla.org/mozilla-central/file/ddeb0295df69/dom/animation/KeyframeEffect.cpp#l1497

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: A guard to block nested calls of animation processes in GetContext

I'd suggest to introduce a guard to avoid the nested calls of GetContext() on the same element.

One thing I am not convinced is that there is no case that we need such nested calls on the same element.  I mean, once an animation style is updated, then as a result of the updated style, another animation style is updated, then the first animation style needs to be updated.  I guess calc<> or houdini can cause such recursive call, but I have no idea yet. 
Should we restrict this guard only for root element?  Brian?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

With 1 crash during the full beta 48 cycle, we can probably release with this crash.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 8775862 [details] [diff] [review]
A guard to block nested calls of animation processes in GetContext

I think this is probably ok but I'll leave it up to whoever reviews this on the style side to make that call.

Currently we've fixed UpdateProperties so that it copies keyframes in case it is called recursively. Now it looks like we also need to do the same for the properties. However, that seems expensive so I guess that's why you decided to add the guard instead?

What about adding the guard to UpdateProperties instead?

Also, if we have a guard, then I guess we don't need to copy the keyframes in UpdateProperties anymore.

A few comments for if we do decide to keep mInResolvingAnimationStyles:

* The member variable needs a comment where it is declared explaining what it's for (and perhaps should be called mResolvingAnimationStyles?)

* I think we still want to detect this sort of situation and fix things so we don't try to call GetContext recursively. That is, we should add an assertion before checking mInResolvingAnimationStyles to check that it is false.

* We should use mozilla::AutoRestore to restore the member (in case we add early returns in the middle, for example).

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #10)
> Currently we've fixed UpdateProperties so that it copies keyframes in case
> it is called recursively. Now it looks like we also need to do the same for
> the properties. However, that seems expensive so I guess that's why you
> decided to add the guard instead?

I did not consider its cost.  I was just concerned that, even if we introcude the guard for animation properties, we may fall into a similar pit caused by GetContext calls.

> What about adding the guard to UpdateProperties instead?
> 
> Also, if we have a guard, then I guess we don't need to copy the keyframes
> in UpdateProperties anymore.

Right.  One we have the guard in attachment 8775862 [details] [diff] [review], we may don't need mIsProcessingRestyles in RestyleManager either.

> A few comments for if we do decide to keep mInResolvingAnimationStyles:
> 
> * The member variable needs a comment where it is declared explaining what
> it's for (and perhaps should be called mResolvingAnimationStyles?)

Sure.

> * We should use mozilla::AutoRestore to restore the member (in case we add
> early returns in the middle, for example).

Unfortunately AutoRestore can be used against a value with bit filed.  Actually I wanted to use AutoRestore.

> * I think we still want to detect this sort of situation and fix things so
> we don't try to call GetContext recursively. That is, we should add an
> assertion before checking mInResolvingAnimationStyles to check that it is
> false.

Yeah, agree.  One place we know already is ResolveStyleFor() for docElement in CalcLengthWith().
http://hg.mozilla.org/mozilla-central/file/08f8a5aacd83/layout/style/nsRuleNode.cpp#l517

Another possible place I found is ResolveStyleFor() for docElement in GetPropagatedScrollbarStylesForViewport().  (I just searched callers of ResolveStyleFor() for docElement)
http://hg.mozilla.org/mozilla-central/file/4a18b5cacb1b/layout/style/nsStyleContext.cpp#l734

Your advice noticed me that we can try the assertion on try server.  Here it is;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db758f0ef33&selectedJob=24843065

Yay!  The second one was caught by the assertion.
https://treeherder.mozilla.org/logviewer.html#?job_id=24843065&repo=try#L85053

The test case, added for bug 1277908, is also generated by fuzzing.  As we all have noticed that we human being can not write such test case.  It will be really hard to know all cases which causes recursive GetContext calls.

Hello Jesse,  I need your help.  If you know any other use-after-free or use-after-poison cases that ASAN output includes nsStyleSet::GetContext symbol, please let me know.
Thank you,

Comment 12

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > What about adding the guard to UpdateProperties instead?

What do you think about this? i.e. we would add the member to KeyframeEffectReadOnly instead.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #12)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > (In reply to Brian Birtles (:birtles) from comment #10)
> > > What about adding the guard to UpdateProperties instead?
> 
> What do you think about this? i.e. we would add the member to
> KeyframeEffectReadOnly instead.

I think the member won't help nsAnimationManager::UpdateAnimations().

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Hi, mats.

We are suffering from nested calls of GetContext again.

As far as I can tell, the starting point of the nested calls (i.e. the second call of GetContext against the same element) is ResolveStyleFor() for documentElement in CalcLengthWith()[1].

[1] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/style/nsRuleNode.cpp#533

But I am not 100% sure there is no other trigger of the nested calls.  So I'd suggest to introduce a guard inside eDoAnimation if block[2] in GetContext() to skip processes related to animations triggered by the second and subsequent calls of GetContext().

[2] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/style/nsStyleSet.cpp#965

What do you think of this?

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8775862 [details] [diff] [review]
A guard to block nested calls of animation processes in GetContext

Looks reasonable to me.

nit:
s/mInResolvingAnimationStyles/mIsResolvingAnimationStyles/
(In -> Is)
and maybe you need to make it a plain 'bool' so that you
can use AutoRestore?

+ birtles nits in comment 10.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: A guard to block nestes calls of animation processes in GetContext

Addressed both of comments.

The comment of the flag for the guard is less descriptive.  I will add more comments when I land a test case something like this;

  // While resolving animation style, we need to obtain another style           
  // by ResolveStyleByAddingRules, it sometimes leads to resolve the            
  // style for the same target.  This flag aims to avoid this re-resolving      
  // style for the same target.

Comment 17

[Brian Birtles (:birtles)]

Comment on attachment 8782714 [details] [diff] [review]
A guard to block nestes calls of animation processes in GetContext

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

r=me with the assertion added (unless we decided we don't need it)

::: layout/style/nsStyleSet.cpp
@@ +962,5 @@
>      NS_ASSERTION(result->GetPseudo() == aPseudoTag, "Unexpected pseudo");
>    }
>  
> +  if ((aFlags & eDoAnimation) &&
> +      !mIsResolvingAnimationStyles) {

Were we going to add an assertion here that !mIsResolvingAnimationStyles so we can still investigate when this does happen? Or at least warning?

::: layout/style/nsStyleSet.h
@@ +574,5 @@
>    // whether font feature values lookup object needs initialization
>    RefPtr<gfxFontFeatureValueSet> mFontFeatureValuesLookup;
> +
> +  // True if we are resolving animations styles in GetContext() to
> +  // avoid nested calls of GetContext().

This might be a little easier to understand as two sentence:

// True if we are resolving animation styles in GetContext().
// We use this to avoid nested calls to GetContext().

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #17)
> Comment on attachment 8782714 [details] [diff] [review]
> A guard to block nestes calls of animation processes in GetContext
> 
> Review of attachment 8782714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the assertion added (unless we decided we don't need it)
> 
> ::: layout/style/nsStyleSet.cpp
> @@ +962,5 @@
> >      NS_ASSERTION(result->GetPseudo() == aPseudoTag, "Unexpected pseudo");
> >    }
> >  
> > +  if ((aFlags & eDoAnimation) &&
> > +      !mIsResolvingAnimationStyles) {
> 
> Were we going to add an assertion here that !mIsResolvingAnimationStyles so
> we can still investigate when this does happen? Or at least warning?

I will add NS_ASSERTION there.  Actually we can't use MOZ_ASSERT since layout/style/crashtests/1277908-2.html hits the condition.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Brian Birtles (:birtles) from comment #17)
> > Comment on attachment 8782714 [details] [diff] [review]
> > A guard to block nestes calls of animation processes in GetContext
> > 
> > Review of attachment 8782714 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me with the assertion added (unless we decided we don't need it)
> > 
> > ::: layout/style/nsStyleSet.cpp
> > @@ +962,5 @@
> > >      NS_ASSERTION(result->GetPseudo() == aPseudoTag, "Unexpected pseudo");
> > >    }
> > >  
> > > +  if ((aFlags & eDoAnimation) &&
> > > +      !mIsResolvingAnimationStyles) {
> > 
> > Were we going to add an assertion here that !mIsResolvingAnimationStyles so
> > we can still investigate when this does happen? Or at least warning?
> 
> I will add NS_ASSERTION there.  Actually we can't use MOZ_ASSERT since
> layout/style/crashtests/1277908-2.html hits the condition.

Hmm, adding NS_ASSERTION needs asserts(count) in crashtests.list, the assertion count on my laptop is 68! But I am not sure the number is the same on other platforms.  It will need some trials on try server.  I will add the NS_ASSERTION in a followup bug.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8782714 [details] [diff] [review]
A guard to block nestes calls of animation processes in GetContext

>+  // True if we are resolving animations styles in GetContext() to
>+  // avoid nested calls of GetContext().

Nit: the explanation here is slightly misleading; we're not avoiding
nested GetContext() calls, just the "resolving animations styles"
block in it.  Would it be possible to re-phrase it to make that clear?

@dbaron or @heycam should probably have a look at this too to make
sure skipping nested resolution of animations is OK from a style
correctness POV.

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8782714 [details] [diff] [review]
A guard to block nestes calls of animation processes in GetContext

I am pretty unsure about whether we should never be re-entering the "resolving animations styles" section for the same element.  That sounds plausible, but I'd like to defer to dbaron.

Comment 23

[David Baron :dbaron:]

My initial inclination here is that 'rem' units have given us enough problems at this point, and we should be fixing this by changing the code in CalcLengthWith to walk up the style context tree to find the root element instead of finding the root element, getting its primary frame, and potentially (as in this case) calling ResolveStyleFor.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Thank you, dbaron.  The idea really sounded nice, so I tried.  Unfortunately in the test case of bug 1279819, CalcLengthWith() is called with a null aStyleContext, so we can't walk up style context tree.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

I don't quite understand when we should pass aStyleFont to CalcLengthWith() and when we should pass aStyleContext to CalcLengthWith(), but should we pass aStyleContext whenever we need 'rem' unit size?

If we can do such change, won't it be a big change?  I mean, can it be uplifted? dbaron?

Note that in the test case of bug 1279819, CalcLengthWith() is called from nsRuleNode::SetFont() [1].  It seems to me that we can pass a nsStyleContext to in the case.
[1] http://hg.mozilla.org/mozilla-central/file/506facea6316/layout/style/nsRuleNode.cpp#l3945

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: Find root style context to walk up the style context tree in case of 'rem' units

I managed to write a patch to walk up the style context tree in case of 'rem' units.

A StyleContext pointer is passed to CalcLengthWith(), the assertion in AutoCheckDependency() at [1] is hit when StyleFont() is called at [2], so I have to move the line into 'if (aUseProvidedRootEmSize)' and 'else if (aStyleContext && !aStyleContext->GetParent())' blocks.

[1] http://hg.mozilla.org/mozilla-central/file/b7f7ae14590a/layout/style/nsStyleContext.h#l618
[2] http://hg.mozilla.org/mozilla-central/file/506facea6316/layout/style/nsRuleNode.cpp#l502

I've confirmed that this patch fixes this heap-use-after free and the assertion that !mIsResolvingAnimationStyles checks can be added in attachment 8782714 [details] [diff] [review].

dbaron, what do you think of?

Comment 27

[David Baron :dbaron:]

Comment on attachment 8787065 [details] [diff] [review]
Find root style context to walk up the style context tree in case of 'rem' units

I didn't look very closely, but this mostly seems like the right approach.  Two comments:


>       } else if (aStyleContext && !aStyleContext->GetParent()) {
>+        const nsStyleFont *styleFont =
>+          aStyleFont ? aStyleFont : aStyleContext->StyleFont();

This should probably have:

  // NOTE: See comment above about being careful with |styleFont|.

>   result_type ComputeLeafValue(const nsCSSValue& aValue)
>   {
>     nscoord size;
>     if (aValue.IsLengthUnit()) {
>-      // Note that font-based length units use the parent's size
>-      // unadjusted for scriptlevel changes. A scriptlevel change
>-      // between us and the parent is simply ignored.
>-      size = CalcLengthWith(aValue, mParentSize,
>-                            mParentFont,
>-                            nullptr, mPresContext, mAtRoot,
>-                            true, mConditions);
>+      if (aValue.GetUnit() == eCSSUnit_RootEM &&
>+          !mAtRoot && mStyleContext->GetParent()) {
>+        // In case of 'rem' units and the style context is not root
>+        // we should pass the style context instead of style font
>+        // so that we can find the root style to walk up the style
>+        // context tree.
>+        size = CalcLengthWith(aValue, mParentSize,
>+                              nullptr,
>+                              mStyleContext, mPresContext,
>+                              mAtRoot,
>+                              true, mConditions);
>+      } else {
>+        // Note that font-based length units use the parent's size
>+        // unadjusted for scriptlevel changes. A scriptlevel change
>+        // between us and the parent is simply ignored.
>+        size = CalcLengthWith(aValue, mParentSize,
>+                              mParentFont,
>+                              nullptr, mPresContext, mAtRoot,
>+                              true, mConditions);
>+      }

Is there a need to duplicate this rather than just pass both parameters through?

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Find root style context to walk up the style context tree in case of 'rem' units

Now an nsStyleContext is passed to CalcLengthWith() in case of 'rem' units
instead of passing nsStyleFont. As a result, in such case, StyleFont() for
the style context is called and causes an assertion in AutoCheckDependency()
of nsStyleContext class. So the caller of StyleFont() had to be confined
if-else blocks.

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #27)
> >   result_type ComputeLeafValue(const nsCSSValue& aValue)
> >   {
> >     nscoord size;
> >     if (aValue.IsLengthUnit()) {
> >-      // Note that font-based length units use the parent's size
> >-      // unadjusted for scriptlevel changes. A scriptlevel change
> >-      // between us and the parent is simply ignored.
> >-      size = CalcLengthWith(aValue, mParentSize,
> >-                            mParentFont,
> >-                            nullptr, mPresContext, mAtRoot,
> >-                            true, mConditions);
> >+      if (aValue.GetUnit() == eCSSUnit_RootEM &&
> >+          !mAtRoot && mStyleContext->GetParent()) {
> >+        // In case of 'rem' units and the style context is not root
> >+        // we should pass the style context instead of style font
> >+        // so that we can find the root style to walk up the style
> >+        // context tree.
> >+        size = CalcLengthWith(aValue, mParentSize,
> >+                              nullptr,
> >+                              mStyleContext, mPresContext,
> >+                              mAtRoot,
> >+                              true, mConditions);
> >+      } else {
> >+        // Note that font-based length units use the parent's size
> >+        // unadjusted for scriptlevel changes. A scriptlevel change
> >+        // between us and the parent is simply ignored.
> >+        size = CalcLengthWith(aValue, mParentSize,
> >+                              mParentFont,
> >+                              nullptr, mPresContext, mAtRoot,
> >+                              true, mConditions);
> >+      }
> 
> Is there a need to duplicate this rather than just pass both parameters
> through?

I don't know exactly reason but we have an assertion at the top of CalcLengthWidth();

  NS_ASSERTION(!aStyleFont || !aStyleContext,
               "Duplicate sources of data");

This assertion was introduced in bug 478321, and the patch author is you, dbaron, so it could be removed, but I'd want to handle it in a follow-up bug.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add a note about the possibility of reentrant call of nsStyleSet::GetContext()

Attachment 8787491 [details] [diff] was less descriptive about this bug because I am afraid that this vulnerability my leak.  I am going to land this patch once this bug can be open.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

Attachment: A guard to block nestes calls of animation processes in GetContext

This patch is a modification of attachment 8782714 [details] [diff] [review].  Now we can use MOZ_ASSERT() there.   I'd also land this patch once this bug is open.

Comment 31

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This will miss 49. We could consider it for a dot release.

Comment 32

[David Baron :dbaron:]

Comment on attachment 8787491 [details] [diff] [review]
Find root style context to walk up the style context tree in case of 'rem' units

>-      // Note that font-based length units use the parent's size
>-      // unadjusted for scriptlevel changes. A scriptlevel change
>-      // between us and the parent is simply ignored.
>-      size = CalcLengthWith(aValue, mParentSize,
>-                            mParentFont,
>-                            nullptr, mPresContext, mAtRoot,
>-                            true, mConditions);
>+      if (aValue.GetUnit() == eCSSUnit_RootEM &&
>+          !mAtRoot && mStyleContext->GetParent()) {
>+        // In case of 'rem' units and the style context is not root
>+        // we should pass the style context instead of style font
>+        // so that we can find the root style to walk up the style
>+        // context tree.
>+        size = CalcLengthWith(aValue, mParentSize,
>+                              nullptr,
>+                              mStyleContext, mPresContext,
>+                              mAtRoot,
>+                              true, mConditions);
>+      } else {
>+        // Note that font-based length units use the parent's size
>+        // unadjusted for scriptlevel changes. A scriptlevel change
>+        // between us and the parent is simply ignored.
>+        size = CalcLengthWith(aValue, mParentSize,
>+                              mParentFont,
>+                              nullptr, mPresContext, mAtRoot,
>+                              true, mConditions);
>+      }

Instead of this, please:
 * always pass both mParentFont and mStyleContext
 * replace the assertion at the top of CalcLengthWith that one of aParentFont and one of aStyleContext is non-null with an assertion that aStyleContext is non-null
 * remove your equivalent assertion inside the rem-unit case in CalcLengthWith
 * document above CalcLengthWith that if aStyleFont is null, aStyleContext->StyleFont() is used


You also need to fix nsRuleNode::CalcLengthWithInitialFont, which you would have discovered due to the assertions if you'd already done that (as I asked you before).  Without such a fix, this patch will make 'rem' units crash in media queries and responsive images.  You should also add tests for the cases that crash with this version of the patch.

Comment 33

[David Baron :dbaron:]

I should also note that I would normally expect that, as part of writing a patch like this, you would look at all callers of CalcLengthWith to make sure they pass a non-null style context.  I did that as part of my review, but my doing it during the code review should not be the first time in the process that it happens.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Attachment: Find root style context to walk up the style context tree in case of 'rem' units v3

(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #32)
> Comment on attachment 8787491 [details] [diff] [review]
> Find root style context to walk up the style context tree in case of 'rem'
> units
> 
> >-      // Note that font-based length units use the parent's size
> >-      // unadjusted for scriptlevel changes. A scriptlevel change
> >-      // between us and the parent is simply ignored.
> >-      size = CalcLengthWith(aValue, mParentSize,
> >-                            mParentFont,
> >-                            nullptr, mPresContext, mAtRoot,
> >-                            true, mConditions);
> >+      if (aValue.GetUnit() == eCSSUnit_RootEM &&
> >+          !mAtRoot && mStyleContext->GetParent()) {
> >+        // In case of 'rem' units and the style context is not root
> >+        // we should pass the style context instead of style font
> >+        // so that we can find the root style to walk up the style
> >+        // context tree.
> >+        size = CalcLengthWith(aValue, mParentSize,
> >+                              nullptr,
> >+                              mStyleContext, mPresContext,
> >+                              mAtRoot,
> >+                              true, mConditions);
> >+      } else {
> >+        // Note that font-based length units use the parent's size
> >+        // unadjusted for scriptlevel changes. A scriptlevel change
> >+        // between us and the parent is simply ignored.
> >+        size = CalcLengthWith(aValue, mParentSize,
> >+                              mParentFont,
> >+                              nullptr, mPresContext, mAtRoot,
> >+                              true, mConditions);
> >+      }
> 
> Instead of this, please:
>  * always pass both mParentFont and mStyleContext
>  * replace the assertion at the top of CalcLengthWith that one of
> aParentFont and one of aStyleContext is non-null with an assertion that
> aStyleContext is non-null
>  * remove your equivalent assertion inside the rem-unit case in
> CalcLengthWith
>  * document above CalcLengthWith that if aStyleFont is null,
> aStyleContext->StyleFont() is used

Thank you,  I did not notice that passing both of mParentFont and mStyleContext to CalcLengthWith() does not cause the assertion in nsStyleContext::AutoCheckDependency.

(In reply to David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) from comment #33)
> I should also note that I would normally expect that, as part of writing a
> patch like this, you would look at all callers of CalcLengthWith to make
> sure they pass a non-null style context.  I did that as part of my review,
> but my doing it during the code review should not be the first time in the
> process that it happens.

I am sorry, but I could not find any way to pass nsStyleConext* to CalcLengthWith() from CalcLengthWithInitialFont() and I still haven't find the way.  I did try to get it from documentElement there but it did not help.  Could you please tell me how to get nsStyleContext* in CalcLengthWithInitialFont()?

In case of calling from CalcLengthWithInitialFont(), a way I can think of is to pass a null style context and add an assertion to check aUseProvidedRootEmSize is true.  Attaching patch does in this way.

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test cases to check CalcLength() calls

1289701-1.html checks a case that SetFontSizeCalcOps::ComputeLeafValue
does not hit any assertions in CalcLengthWith().

1289701-2.html and 1289701-3.html check that CalcLengthWithInitialFont()
does not hit any assertions.

2 and 3 are the test cases for media queries and responsive images.  1 is for the crash case with the previous patch.

Comment 36

[David Baron :dbaron:]

>+  NS_ASSERTION(aStyleContext || aUseProvidedRootEmSize,
>+               "Must have style context or specify aUseProvidedRootEmSize");

This is fine too, as long as all of the callers satisfy the condition and the function works correctly.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Attachment: Find root style context to walk up the style context tree in case of 'rem' units v4

Thank you, dbaron for quick reply!

Updated commit message and a comment for CalcLengthWith() function.

Can anybody review this patch and test cases (attachment 8791005 [details] [diff] [review]) because dbaron seems to have been busy until September 25?

Comment 38

[David Baron :dbaron:]

Comment on attachment 8791051 [details] [diff] [review]
Find root style context to walk up the style context tree in case of 'rem' units v4

>Bug 1289701 - Find the root style context to walk up the style context tree instead of calling ResolveStyleFor or getting from root element's primarity frame. r?dbaron

"to walk up" -> "by walking up"

"getting from" -> "getting it from"

"primarity" -> "primary"

>CalcLengthWithInitialFont(). The CalcLengthWithInitialFont() calls

Remove "The ".

>CalcLengthWith() along with a valid nsStyleFont and

Remove "along ".

>aUseProvidedRootEmSize with a true value, so we can get rem unit font size

"with a true value" -> "true"

"get rem unit" -> "get the rem unit"

>from the nsStyleFont in the case of CalcLengthWithInitialFont().

"in the case of" -> "when called from"

>+// In case that |aValue| is rem unit, if |aStyleContext| is nullptr,
>+// callers must be specify a valid |aStyleFont| and |aUseProvidedRootEmSize|
>+// must be true so that we can get the length from |aStyleFont|.
>+// If |aStyleFont| is nullptr, aStyleContext->StyleFont() is used.

"is nullptr" -> "is null"

"must be specify" -> "must specify"

I think it would also be better to:
 * have the 4th line (above) first
 * have a blank "//" line between the two sentences.


r=dbaron with that.

Thanks for making all these revisions, and sorry for the delay getting to this.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: Find root style context to walk up the style context tree in case of 'rem' units for aurora/beta/release branches

Thank you, dbaron!  I understand you've been very busy and doing tremendous jobs.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
No so easy.  

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. To attack this vulnerability, there needs to a hidden document, I don't leave any comments about hidden documents.

Which older supported branches are affected by this flaw?
49.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch can be applied to release branch without any modifications.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.  I run all of mochitest and crashtest in layout/style, there is no suspicion.

Comment 40

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

Release management, is it too late to take this for 50?

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Al Billings [:abillings] from comment #40)
> Release management, is it too late to take this for 50?

This is sec-high and we still have another week before we hit RC so I'd be ok to take this one before I gtb 50.0b10 (Monday Oct 24th) noon PST.

Comment 42

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

Crap. I missed this email and we missed the window. I'd rather not take this with no betas. We should delay until November 14, a week into the new cycle.

So, sec-approval+ for November 14 checkin into Trunk. At that point, we'd want Aurora and Beta patches made and nominated.

Comment 43

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

Changing sec-approval to 11/29 as we moved the release out a week.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing so it can land. Getting this back on the radar for 50.1 as well.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Thanks Ryan.  I did not notice it at all.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e6d1dafee00c988288f4b8e3023d4df7bf792fc

Comment 47

[Hiroyuki Ikezoe (:hiro)]

Attachment: Find root style context to walk up the style context tree in case of 'rem' units (rebased)

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta/Release approval ASAP so we can hopefully get it uplifted in time for 51b6.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803188 [details] [diff] [review]
Find root style context to walk up the style context tree in case of 'rem' units for aurora/beta/release branches

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1288586
[User impact if declined]:  A security hole by heap-use-after-free.
[Is this code covered by automated tests?]: A test case for bug 1279819 covers this bug, but has not yet landed since this bug is not in public.
[Has the fix been verified in Nightly?]: Not yet landed in nightly.
[Needs manual test from QE? If yes, steps to reproduce]: I don't think so.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Now we do just walk up style context tree to find the root style context instead of re-resolving style context, it's much simpler/safer way.  
[String changes made/needed]: None

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e6d1dafee00

Comment 51

[Marcia Knous [:marcia]]

Tracking 53- as this is resolved fixed.

Comment 52

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8803188 [details] [diff] [review]
Find root style context to walk up the style context tree in case of 'rem' units for aurora/beta/release branches

Sec-high, approved for all branches

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d172a5a5da1
https://hg.mozilla.org/releases/mozilla-beta/rev/600eda864ff4
https://hg.mozilla.org/releases/mozilla-release/rev/451a4adf2b05

Comment 54

[Marcia Knous [:marcia]]

Tracking 53+ for this sec high issue.

Comment 55

[Hiroyuki Ikezoe (:hiro)]

Landed attachment 8787492 [details] [diff] [review].
https://hg.mozilla.org/integration/mozilla-inbound/rev/756b4447cae3fa1877d1252526384988d9e570f2

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/756b4447cae3

Comment 57

[Hiroyuki Ikezoe (:hiro)]

The test case that caused this crash was now landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4fc4070d3bdd62cab93649da36bca4314421b3