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

RESOLVED FIXED in Firefox 57

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tsmith, Assigned: hiro)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

57 Branch
mozilla58
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed, firefox58 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Posted 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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(hikezoe)
(Assignee)

Updated

2 years ago
Component: Layout → CSS Parsing and Computation
Flags: needinfo?(hikezoe)
Summary: Assertion failure: !ServoStyleSet::IsInServoTraversal() → stylo: Assertion failure: !ServoStyleSet::IsInServoTraversal()
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
Version: Trunk → 57 Branch
Hiro, are you planning on taking this?
Flags: needinfo?(hikezoe)
Priority: -- → P2
Blocks: 1333171
(Assignee)

Comment 2

2 years ago
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: → bug 1284053
(Assignee)

Comment 3

2 years ago
(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 hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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)
(Assignee)

Comment 6

2 years ago
OK, I'd prefer (a).
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
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+

Comment 9

2 years ago
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 hidden (mozreview-request)

Comment 11

2 years ago
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 hidden (mozreview-request)

Comment 13

2 years ago
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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce2c129f0a87
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 15

2 years ago
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+

Comment 17

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/f4e84e6ed76d
status-firefox57: affected → fixed
Flags: in-testsuite? → in-testsuite+
(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.