Closed Bug 1330190 Opened 5 years ago Closed 5 years ago

SEGV near null in [@ Get]

Categories

(Core :: DOM: Animation, defect, P1)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: truber, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase, Whiteboard: [fuzzblocker])

Attachments

(9 files)

testcase.html
202 bytes, text/html
Details
log.txt
10.65 KB, text/plain
Details
A proof of contept what we need here
7.06 KB, text/plain
Details
Bug 1330190 - Part 1: Add nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1330190 - Part 2: Add a new function named ResolveStyleWithoutAnimation.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1330190 - Part 3: Add ResolvePseudoElementStyleWithoutAnimation. ?heycam
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation().
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1330190 - Part 5: Get style context without all of animation data for base styles.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1330190 - Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file testcase.html — 
The attached testcase causes a crash near null in mozilla-central rev 20094311ab5e

==32475==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x7fd6ac59708f bp 0x7fffb14c63f0 sp 0x7fffb14c63b0 T0)
    #0 0x7fd6ac59708e in Get /home/worker/workspace/build/src/obj-firefox/dist/include/PLDHashTable.h:228:26
    #1 0x7fd6ac59708e in PLDHashTable::Add(void const*, mozilla::fallible_t const&) /home/worker/workspace/build/src/xpcom/glue/PLDHashTable.cpp:535
    #2 0x7fd6aeb65ae5 in PutEntry /home/worker/workspace/build/src/obj-firefox/dist/include/nsTHashtable.h:161:36
    #3 0x7fd6aeb65ae5 in Put /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:150
    #4 0x7fd6aeb65ae5 in Put /home/worker/workspace/build/src/obj-firefox/dist/include/nsBaseHashtable.h:142
    #5 0x7fd6aeb65ae5 in AddValue /home/worker/workspace/build/src/dom/animation/AnimValuesStyleRule.cpp:81
Attached file log.txt — 
Oh, there is still another case. Thank you, Jesse!
Flags: needinfo?(hikezoe)
Flags: needinfo?(hikezoe)
Priority: -- → P1
I have no idea other than fixing bug 1324966.  In case of the test case in comment 0, resolving style context for the base values needs flushing styles (because the target element does not have primary frame yet.), as a result, AnimValuesStyleRule is nullified at EffectCompositor::ComposeAnimationRule().
This patch adds nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation() that returns nsStyleContext without composing any animation styles. (Whereas my intention in bug 1324966 is to return nsStyleContext without composing style for *a given nsCSSPropertyID*)

This patch includes lots of duplicated codes, but anyway it fixes this crash and bug 1322291 too.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> This patch includes lots of duplicated codes, but anyway it fixes this crash
> and bug 1322291 too.

Wrong. The patch just fixed this crash, does not fix bug 1322291 at all.
See Also: → 1330513
It turns out that we also need to skip UpdateEffectProperties() in nsStyleSet::GetContext().
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e0b3a82ff3404bfa8dc382669732fa1f8e53e17
Here is a try with patches that adds nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation() but does not skip UpdateEffectProperties().
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
A new try with skipping UpdateEffectProperties() (i.e. calling GetContext() with eNoFlags instead of eDoAnimation).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb7cdd4c273733997141245fa1ea3651677139ce
Cameron, would you mind reviewing some of these patches?

Background of this issue:
We are using nsCompuedDOMStyle::GetStyleContextForElement() to get nsStyleContext
for gettting base style values (i.e. style values without animating style).
It is processed during composing animating style
(KeyframeEffectReadOnly::ComposeStyle) so that it currently causes recursive
calls of KeyframeEffectReadOnly::ComposeStyle.  Also, when there is an animation
specified in font-size unit (e.g. em) and an animation of font-size, this
recursive call overrides KeyframeEffectReadOnly.mProperties in
KeyframeEffectReadOnly::UpdateProperties
(called from EffectCompositor::UpdateEffectProperties) while we are processing
mProperties in ComposeStyle.
To prevent these symptoms we need a new function that skips all animation rules
and procedure in 'if (aFlags & eDoAnimation)' block in nsStyleSet::GetContext().
The new function is GetStyleContextForElementWithoutAnimation in part 1 patch.
Comment on attachment 8826991 [details]
Bug 1330190 - Part 5: Get style context without all of animation data for base styles.

https://reviewboard.mozilla.org/r/104792/#review105516

r=me with comments addressed

::: dom/animation/KeyframeEffectReadOnly.h:414
(Diff revision 1)
> +  // Similar to the above but ignoring animation rules. We must use this to get
> +  // base styles.

We should explain why we must use this to get base styles. Or alternatively, just say "We use this to get base styles (which don't include animation rules)".

::: dom/animation/KeyframeEffectReadOnly.cpp:923
(Diff revision 1)
> +already_AddRefed<nsStyleContext>
> +KeyframeEffectReadOnly::GetTargetStyleContext()
> +{
> +  return GetTargetStyleContextByGetterFunc(
> +    // Need a lambda here since the forth argument of
> +    // nsComputedDOMStyle::GetStyleContextForElement is a default argument.
> +    [](Element* aElement, nsIAtom* aPseudo, nsIPresShell* aPresShell) {
> +      return nsComputedDOMStyle::GetStyleContextForElement(aElement,
> +                                                           aPseudo,
> +                                                           aPresShell);
> +    }
> +  );
> +}

It seems like just adding a DoGetTargetStyleContext helper that takes a AnimationStyle::{Skip,Include} enum parameter would be simpler that adding templates and lambdas. Any reason to prefer this approach?

::: dom/animation/KeyframeEffectReadOnly.cpp:927
(Diff revision 1)
> +
> +already_AddRefed<nsStyleContext>
> +KeyframeEffectReadOnly::GetTargetStyleContext()
> +{
> +  return GetTargetStyleContextByGetterFunc(
> +    // Need a lambda here since the forth argument of

s/forth/fourth/
Attachment #8826991 - Flags: review?(bbirtles) → review+
Comment on attachment 8826992 [details]
Bug 1330190 - Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing.

https://reviewboard.mozilla.org/r/104794/#review105518

I think the commit message should be something like:

"Based on the other changesets in this series, we don't expect to be calling
UpdateProperties() and ComposeStyle() from within ComposeStyle() itself.
However, in case there is some scenario where that does stil occur, we leave
the mIsComposing check in place and add an equivalent MOZ_DIAGNOSTIC_ASSERT so
that we are alerted if this does occur on debug / Nightly / DevEdition builds,
but handle it gracefully on beta or release builds."

::: dom/animation/KeyframeEffectReadOnly.cpp:276
(Diff revision 1)
>  
> +  MOZ_DIAGNOSTIC_ASSERT(!mIsComposingStyle,
> +                        "Should not be called while processing ComposeStyle()");

Nit: Perhaps drop the blank line between MOZ_ASSERT and MOZ_DIAGNOSTIC_ASSERT to logically group them together? Or move this down together with the early return to logically group it with that--actually, that might be better.
Attachment #8826992 - Flags: review?(bbirtles) → review+
Comment on attachment 8826991 [details]
Bug 1330190 - Part 5: Get style context without all of animation data for base styles.

https://reviewboard.mozilla.org/r/104792/#review105544

::: dom/animation/KeyframeEffectReadOnly.cpp:923
(Diff revision 1)
> +already_AddRefed<nsStyleContext>
> +KeyframeEffectReadOnly::GetTargetStyleContext()
> +{
> +  return GetTargetStyleContextByGetterFunc(
> +    // Need a lambda here since the forth argument of
> +    // nsComputedDOMStyle::GetStyleContextForElement is a default argument.
> +    [](Element* aElement, nsIAtom* aPseudo, nsIPresShell* aPresShell) {
> +      return nsComputedDOMStyle::GetStyleContextForElement(aElement,
> +                                                           aPseudo,
> +                                                           aPresShell);
> +    }
> +  );
> +}

No reason. I guess I was charmed with the magic of lambda. ;-)
Comment on attachment 8826992 [details]
Bug 1330190 - Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing.

https://reviewboard.mozilla.org/r/104794/#review105518

Thank you so much!  I was confused that the beta channel has the same functionality of aurora channel again.
Comment on attachment 8826987 [details]
Bug 1330190 - Part 1: Add nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation.

https://reviewboard.mozilla.org/r/104784/#review105532

::: layout/style/nsComputedDOMStyle.h:92
(Diff revision 1)
>    static already_AddRefed<nsStyleContext>
>    GetStyleContextForElement(mozilla::dom::Element* aElement, nsIAtom* aPseudo,
>                              nsIPresShell* aPresShell,
>                              StyleType aStyleType = eAll);
>  
> +  // Similar to the above but ignore animation rules.

"ignoring" or "without".  Also mention that it is StyleType::eAll, since this function doesn't give you the option.

::: layout/style/nsComputedDOMStyle.h:96
(Diff revision 1)
>  
> +  // Similar to the above but ignore animation rules.
> +  static already_AddRefed<nsStyleContext>
> +  GetStyleContextForElementWithoutAnimation(mozilla::dom::Element* aElement,
> +                                            nsIAtom* aPseudo,
> +                                            nsIPresShell* aPresShell);

Nit: blank line below here.

::: layout/style/nsComputedDOMStyle.cpp:457
(Diff revision 1)
> +enum AnimationFlags {
> +  eWithAnimation,
> +  eWithoutAnimation,
> +};
> +
> +template<AnimationFlags aAnimationFlag>

I don't think it's worth making the AnimationFlags a template parameter, rather than a regular parameter.    There's nothing performance critical here that would benefit from having two versions of DoGetStyleContextForElementNoFlush in the compiled code.

::: layout/style/nsComputedDOMStyle.cpp:458
(Diff revision 1)
> +  eWithAnimation,
> +  eWithoutAnimation,
> +};
> +
> +template<AnimationFlags aAnimationFlag>
> +MOZ_STACK_CLASS class AutoSkipAnimationRules final {

Nit: newline before "{".

I'm not sure it matters, but it seems that most other uses of MOZ_STACK_CLASS put that right after the "class".

::: layout/style/nsComputedDOMStyle.cpp:476
(Diff revision 1)
> +    if (aPresContext->RestyleManager()->IsGecko()) {
> +      mRestyleManager = aPresContext->RestyleManager()->AsGecko();
> +
> +      mOldSkipAnimationRules = mRestyleManager->SkipAnimationRules();
> +      mRestyleManager->SetSkipAnimationRules(true);
> +    }

Can you add an NS_WARNING("stylo: can't skip animation rules yet") in an |else|?

::: layout/style/nsComputedDOMStyle.cpp:494
(Diff revision 1)
> +};
> +}
> +
> +template<AnimationFlags AnimationFlag>
>  already_AddRefed<nsStyleContext>
> -nsComputedDOMStyle::GetStyleContextForElementNoFlush(Element* aElement,
> +DoGetStyleContextForElementNoFlush(Element* aElement,

Maybe just make this a private method of nsComputedDOMStyle, so that we don't have to qualify a few things with "nsComputedDOMStyle::".
Attachment #8826987 - Flags: review?(cam) → review+
Comment on attachment 8826988 [details]
Bug 1330190 - Part 2: Add a new function named ResolveStyleWithoutAnimation.

https://reviewboard.mozilla.org/r/104786/#review105564

::: layout/style/nsStyleSet.h:188
(Diff revision 1)
> +  // Similar to the above, but resolving style without all animation data in
> +  // the first place.
> +  already_AddRefed<nsStyleContext>
> +    ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget,
> +                                 nsStyleContext* aParentContext);

I think it might be confusing that we have two functions named ResolveStyleWithoutAnimation here, but the meaning of the nsStyleContext parameter is different.  Nearly all of the other ResolveStyleXXX functions use the nsStyleContext as the parent style.  One function that doesn't is ResolveStyleByAddingRules, and I think the naming of the function suggests that we're doing something different.

So, I'd like to rename the existing ResolveStyleWithoutAnimation to ResolveStyleByRemovingAnimation.  Then the new function can remain as ResolveStyleWithoutAnimation.

::: layout/style/nsStyleSet.h:539
(Diff revision 1)
> +  template<AnimationFlags AnimationFlag>
> +  already_AddRefed<nsStyleContext>
> +  ResolveStyleForInternal(mozilla::dom::Element* aElement,
> +                          nsStyleContext* aParentContext,
> +                          TreeMatchContext& aTreeMatchContext);

Here, too, I don't think we should make this a template argument.

::: layout/style/nsStyleSet.cpp:1804
(Diff revision 1)
> +  // Here we can call ResolveStyleForInternal() instead of
> +  // ResolveStyleWithReplacement() since we don't need all of animation rules
> +  // (CSS Animations, Transitions and script animations). That's because
> +  // EffectCompositor::GetAnimationRule() skips all of animations rules if
> +  // SkipAnimationRules flag is true.

s/all of animation rules/any animation rules/
Attachment #8826988 - Flags: review?(cam) → review+
Comment on attachment 8826989 [details]
Bug 1330190 - Part 3: Add ResolvePseudoElementStyleWithoutAnimation. ?heycam

https://reviewboard.mozilla.org/r/104788/#review105566

::: layout/style/nsStyleSet.h:553
(Diff revision 1)
> +  template<AnimationFlags AnimationFlag>
> +  already_AddRefed<nsStyleContext>
> +  ResolvePseudoElementStyleInternal(mozilla::dom::Element* aParentElement,

Change template argument to a regular argument.
Attachment #8826989 - Flags: review+
Comment on attachment 8826990 [details]
Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation().

https://reviewboard.mozilla.org/r/104790/#review105570
Attachment #8826990 - Flags: review?(cam) → review+
Comment on attachment 8826990 [details]
Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation().

https://reviewboard.mozilla.org/r/104790/#review105572

(Clicked this one a bit too fast...)
Attachment #8826990 - Flags: review+ → review?(cam)
Comment on attachment 8826990 [details]
Bug 1330190 - Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation().

https://reviewboard.mozilla.org/r/104790/#review105582

::: layout/style/nsComputedDOMStyle.cpp:500
(Diff revision 1)
> +already_AddRefed<nsStyleContext>
> +StyleResolver<eWithoutAnimation>::Resolve(StyleSetHandle aStyleSet,
> +                                          Element* aElement,
> +                                          CSSPseudoElementType aType,
> +                                          nsStyleContext* aParentContext,
> +                                          nsComputedDOMStyle::StyleType aStyleType,
> +                                          bool aInDocWithShell)

I think it's a bit strange to have this template specialization that takes aStyleType but does nothing with it.

It might be clearer if these are two separate functions.  If not, then at least assert inside this function that aStyleType is eAll.
Attachment #8826990 - Flags: review?(cam) → review+
Comment on attachment 8826988 [details]
Bug 1330190 - Part 2: Add a new function named ResolveStyleWithoutAnimation.

https://reviewboard.mozilla.org/r/104786/#review105584

::: layout/style/nsStyleSet.h:188
(Diff revision 1)
> +  // Similar to the above, but resolving style without all animation data in
> +  // the first place.
> +  already_AddRefed<nsStyleContext>
> +    ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget,
> +                                 nsStyleContext* aParentContext);

Great! ResolveStyleByRemovingAnimation sounds really nice, it matches to what that function does!  Thanks!
(In reply to Hiroyuki Ikezoe (:hiro) from comment #27)
> ::: layout/style/nsStyleSet.h:188
> (Diff revision 1)
> > +  // Similar to the above, but resolving style without all animation data in
> > +  // the first place.
> > +  already_AddRefed<nsStyleContext>
> > +    ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget,
> > +                                 nsStyleContext* aParentContext);
> 
> Great! ResolveStyleByRemovingAnimation sounds really nice, it matches to
> what that function does!  Thanks!

Hmm, MozReview is really hard to use for me..

(In reply to Cameron McCormack (:heycam) from comment #22)
> ::: layout/style/nsStyleSet.h:188
> (Diff revision 1)
> > +  // Similar to the above, but resolving style without all animation data in
> > +  // the first place.
> > +  already_AddRefed<nsStyleContext>
> > +    ResolveStyleWithoutAnimation(mozilla::dom::Element* aTarget,
> > +                                 nsStyleContext* aParentContext);
> 
> I think it might be confusing that we have two functions named
> ResolveStyleWithoutAnimation here, but the meaning of the nsStyleContext
> parameter is different.  Nearly all of the other ResolveStyleXXX functions
> use the nsStyleContext as the parent style.  One function that doesn't is
> ResolveStyleByAddingRules, and I think the naming of the function suggests
> that we're doing something different.
> 
> So, I'd like to rename the existing ResolveStyleWithoutAnimation to
> ResolveStyleByRemovingAnimation.  Then the new function can remain as
> ResolveStyleWithoutAnimation.

I wanted to reply to this comment.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd18ab48f5c3
Part 1: Add nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b418519ea4d0
Part 2: Add a new function named ResolveStyleWithoutAnimation. r=heycam
https://hg.mozilla.org/integration/autoland/rev/69a81a6bb0bd
Part 3: Add ResolvePseudoElementStyleWithoutAnimation. ?heycam r=heycam
https://hg.mozilla.org/integration/autoland/rev/0a9984a0cc54
Part 4: Resolve styles without animations in case of GetStyleContextForElementWithoutAnimation(). r=heycam
https://hg.mozilla.org/integration/autoland/rev/a2532fa6577d
Part 5: Get style context without all of animation data for base styles. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8f52299c4c63
Part 6: Add MOZ_DIAGNOSTIC_ASSERT for mIsComposing. r=birtles
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.