Closed Bug 1403433 Opened 3 years ago Closed 3 years ago

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

Categories

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

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: tsmith, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(2 files)

Attached file 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>)
Flags: in-testsuite?
Flags: needinfo?(hikezoe)
Component: Layout → CSS Parsing and Computation
Flags: needinfo?(hikezoe)
Summary: Assertion failure: !ServoStyleSet::IsInServoTraversal() → stylo: Assertion failure: !ServoStyleSet::IsInServoTraversal()
Hiro, are you planning on taking this?
Flags: needinfo?(hikezoe)
Priority: -- → P2
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Flags: needinfo?(hikezoe)
See Also: → 1284053
(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 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)
OK, I'd prefer (a).
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!
Attachment #8913554 - Flags: review?(bbirtles) → review+
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)
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)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce2c129f0a87
Add another variant of SchedulePaint that does not call InvalidateRenderingObservers for PendingAnimationTracker. r=birtles
https://hg.mozilla.org/mozilla-central/rev/ce2c129f0a87
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8913554 - Flags: approval-mozilla-beta?
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
Attachment #8913554 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.