Closed
Bug 1330190
Opened 4 years ago
Closed 4 years ago
SEGV near null in [@ Get]
Categories
(Core :: DOM: Animation, defect, P1)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla53
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)
202 bytes,
text/html
|
Details | |
10.65 KB,
text/plain
|
Details | |
7.06 KB,
text/plain
|
Details | |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
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
Reporter | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
Oh, there is still another case. Thank you, Jesse!
Flags: needinfo?(hikezoe)
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(hikezoe)
Priority: -- → P1
Assignee | ||
Comment 3•4 years ago
|
||
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().
Assignee | ||
Comment 4•4 years ago
|
||
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.
Assignee | ||
Comment 5•4 years ago
|
||
(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.
Assignee | ||
Comment 6•4 years ago
|
||
It turns out that we also need to skip UpdateEffectProperties() in nsStyleSet::GetContext().
Assignee | ||
Comment 7•4 years ago
|
||
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 | ||
Updated•4 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•4 years ago
|
||
A new try with skipping UpdateEffectProperties() (i.e. calling GetContext() with eNoFlags instead of eDoAnimation). https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb7cdd4c273733997141245fa1ea3651677139ce
Assignee | ||
Comment 9•4 years ago
|
||
The previous patches have some mistakes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c54d12c078a72e9b7bd944db437e234d6895ed4e
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•4 years ago
|
||
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 17•4 years ago
|
||
mozreview-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/#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 18•4 years ago
|
||
mozreview-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+
Assignee | ||
Comment 19•4 years ago
|
||
mozreview-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. ;-)
Assignee | ||
Comment 20•4 years ago
|
||
mozreview-review-reply |
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 21•4 years ago
|
||
mozreview-review |
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 22•4 years ago
|
||
mozreview-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 23•4 years ago
|
||
mozreview-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 24•4 years ago
|
||
mozreview-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 25•4 years ago
|
||
mozreview-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 26•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 27•4 years ago
|
||
mozreview-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!
Assignee | ||
Comment 28•4 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d08d408596c8ed857929f7943deb296ae7c52b
Comment 36•4 years ago
|
||
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
Comment 37•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fd18ab48f5c3 https://hg.mozilla.org/mozilla-central/rev/b418519ea4d0 https://hg.mozilla.org/mozilla-central/rev/69a81a6bb0bd https://hg.mozilla.org/mozilla-central/rev/0a9984a0cc54 https://hg.mozilla.org/mozilla-central/rev/a2532fa6577d https://hg.mozilla.org/mozilla-central/rev/8f52299c4c63
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•