1403433 - stylo: Assertion failure: !ServoStyleSet::IsInServoTraversal()

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: !ServoStyleSet::IsInServoTraversal(), at /src/layout/base/ServoRestyleManager.cpp:298

#0 mozilla::ServoRestyleManager::PostRestyleEvent(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) /src/layout/base/ServoRestyleManager.cpp:289:3
#1 nsSVGFilterProperty::DoUpdate() /src/layout/svg/SVGObserverUtils.cpp:365:43
#2 nsSVGRenderingObserverList::InvalidateAll() /src/layout/svg/SVGObserverUtils.cpp:806:19
#3 InvalidateRenderingObservers(nsIFrame*, nsIFrame*) /src/layout/generic/nsFrame.cpp:6416:5
#4 nsIFrame::SchedulePaint(nsIFrame::PaintType) /src/layout/generic/nsFrame.cpp:6686:3
#5 mozilla::PendingAnimationTracker::AddPlayPending(mozilla::dom::Animation&) /src/dom/animation/PendingAnimationTracker.h:33:5
#6 mozilla::dom::Animation::PlayNoUpdate(mozilla::ErrorResult&, mozilla::dom::Animation::LimitBehavior) /src/dom/animation/Animation.cpp:1121:14
#7 mozilla::dom::CSSTransition::PlayFromStyle() /src/layout/style/nsTransitionManager.h:149:5
#8 void nsTransitionManager::ConsiderInitiatingTransition<mozilla::ServoStyleContext const*>(nsCSSPropertyID, mozilla::StyleTransition const&, mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::AnimationCollection<mozilla::dom::CSSTransition>*&, mozilla::ServoStyleContext const*, mozilla::ServoStyleContext const*, bool*, nsCSSPropertyIDSet*) /src/layout/style/nsTransitionManager.cpp:1042:14
#9 bool nsTransitionManager::DoUpdateTransitions<mozilla::ServoStyleContext const*>(nsStyleDisplay const*, mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::AnimationCollection<mozilla::dom::CSSTransition>*&, mozilla::ServoStyleContext const*, mozilla::ServoStyleContext const*) /src/layout/style/nsTransitionManager.cpp:699:9
#10 nsTransitionManager::UpdateTransitions(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::ServoStyleContext const*, mozilla::ServoStyleContext const*) /src/layout/style/nsTransitionManager.cpp:639:10
#11 Gecko_UpdateAnimations /src/layout/style/ServoBindings.cpp:688:7
#12 _$LT$style..gecko..wrapper..GeckoElement$LT$$u27$le$GT$$u20$as$u20$style..dom..TElement$GT$::update_animations::hef5e6c13ef9cecf2 /src/servo/components/style/gecko/wrapper.rs:1238
#13 _$LT$style..context..SequentialTask$LT$E$GT$$GT$::execute::hf0231e0441e6f9d2 /src/servo/components/style/context.rs:491
#14 _$LT$style..context..SequentialTaskList$LT$E$GT$$u20$as$u20$core..ops..Drop$GT$::drop::he67f48a826c677e9 /src/servo/components/style/context.rs:613
#15 core::ptr::drop_in_place::h99b2bed9347083f7 /checkout/src/libcore/ptr.rs:60
#16 core::ptr::drop_in_place::hf7022db091d2eb5a /checkout/src/libcore/ptr.rs:60
#17 style::driver::traverse_dom::h55b444c1310b363a /src/servo/components/style/driver.rs:131
#18 geckoservo::glue::traverse_subtree::h53594e7648a0d56c /src/servo/ports/geckolib/glue.rs:262
#19 Servo_TraverseSubtree /src/servo/ports/geckolib/glue.rs:300
#20 mozilla::ServoStyleSet::StyleDocument(mozilla::ServoTraversalFlags) /src/layout/style/ServoStyleSet.cpp:978:21
#21 mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /src/layout/base/ServoRestyleManager.cpp:1106:20
#22 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4170:41
#23 mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType) /src/layout/base/PresShell.cpp:4043:3
#24 nsComputedDOMStyle::GetStyleContext(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) /src/layout/style/nsComputedDOMStyle.cpp:483:14
#25 nsComputedDOMStyle::UpdateCurrentStyleSources(bool) /src/layout/style/nsComputedDOMStyle.cpp:1020:7
#26 nsComputedDOMStyle::GetPropertyCSSValue(nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /src/layout/style/nsComputedDOMStyle.cpp:1126:3
#27 nsComputedDOMStyle::GetPropertyValue(nsTSubstring<char16_t> const&, nsTSubstring<char16_t>&) /src/layout/style/nsComputedDOMStyle.cpp:449:26
#28 nsICSSDeclaration::GetPropertyValue(nsTSubstring<char16_t> const&, nsTString<char16_t>&, mozilla::ErrorResult&) /src/layout/style/nsICSSDeclaration.h:126:10
#29 mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue(JSContext*, JS::Handle<JSObject*>, nsICSSDeclaration*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/CSSStyleDeclarationBinding.cpp:172:9
#30 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3055:13
#31 0x333c00158ca2  (<unknown module>)

Comment 1

[Bobby Holley (:bholley)]

Hiro, are you planning on taking this?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Yes, I was going to leave some comments here.

I think a proper solution for this assertion is bug 1284053, but it would not be able to be uplifted to beta and I don't know much about DLBI stuff.
A mitigate way is to call SchedulePaintInternal() directly from there (and rename the function).

Also note that the assertion in this test case is not a problem at all, it happens during a sequential task and nsSVGFilterProperty::DoUpdate() posts change hints instead of restyle hints and also the change hints will be processed a second post traversal in the end.  Anyway our current handling for reference elements in SVG is problematic, we should fix as soon as possible.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) PTO on 28 Sep. from comment #2)
> Yes, I was going to leave some comments here.
> 
> I think a proper solution for this assertion is bug 1284053, but it would
> not be able to be uplifted to beta and I don't know much about DLBI stuff.
> A mitigate way is to call SchedulePaintInternal() directly from there (and
> rename the function).

Or we can just call ScheduleViewManagerFlush()?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

nsFrame::SchedulePaint() invokes InvalidateRenderingObservers, and
InvalidateRenderingObservers ends up posting change hints during we are
processing sequential tasks for animations, but we don't allow posting any
change hints while we are in the middle of restyling process.

Though the change hints posted by InvalidateRenderingObservers are not harmful
in this case since the change hints will be processed in a second post
traversal without problems.  That's said, InvalidateRenderingObservers stuff
should be processed in display list based invalidation anyway (bug 1284053).

Review commit: https://reviewboard.mozilla.org/r/184936/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184936/

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

https://reviewboard.mozilla.org/r/184936/#review190028

This API is a bit odd: it requires the caller to be sure it has a display root and doesn't explain in the method name how its behavior differs from SchedulePaint.

Could we instead either:

(a) Add a method like SchedulePaintWithoutInvalidatingObservers() that gets the display root and then calls SchedulePaintInternal, or, if that method name is too long,
(b) Add an enum parameter to SchedulePaint that toggles invalidating rendering observers (and defaults to invalidating them)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

OK, I'd prefer (a).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184936/diff/1-2/

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

https://reviewboard.mozilla.org/r/184936/#review190036

Thank you!

Comment 9

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b8153a783850 -d 858d90b340c2: rebasing 423370:b8153a783850 "Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker. r=birtles" (tip)
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184936/diff/2-3/

Comment 11

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8e9046c1e16e -d 24771058db08: rebasing 423372:8e9046c1e16e "Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker. r=birtles" (tip)
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184936/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce2c129f0a87
Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker. r=birtles

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce2c129f0a87

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

Approval Request Comment
[Feature/Bug causing the regression]: Regressed by stylo
[User impact if declined]: User should not see any visual differences with/without this patch on release builds, will get an assertion on debug builds
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Noe
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Very low
[Why is the change risky/not risky?]: This patch just skips a function call, which caused an assertion on stylo, during animation stuff, the function is fundamentally unnecessary for the animation stuff
[String changes made/needed]: None

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913554 [details]
Bug 1403433 - Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker.

Fix an assert, taking it.
Should be in 57b5

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f4e84e6ed76d

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: Noe

Setting qe-verify- based on Hiroyuki Ikezoe's assessment on manual testing needs and the fact that this fix has automated coverage.