1825452 - Hit MOZ_CRASH(damage: GeckoRestyleDamage(nsChangeHint(0)), hint: RESTYLE_SELF | RESTYLE_DESCENDANTS, flags: (empty) }) at servo/ports/geckolib/glue.rs:5811 (due to font-size transition affecting query container)

Status: NEW

(Core :: CSS Transitions and Animations, defect, P3)

Description

[2366719611]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/111.0.0.0 Safari/537.36

Steps to reproduce:

open the attached html file with firefox debug version.

Actual results:

the firefox crashed.
log:
Hit MOZ_CRASH(Resolving style on <output id="htmlvar00029" form="htmlvar00008" for="htmlvar00006" disabled="disabled" style="mso-style-next: Normal; margin-inline..." size="59" direction="ltr" face="verdana" selected="selected" dirname="++ehWCJ"> (0x7f345e011350) without current styles: ElementData { styles: ElementStyles { primary: Some(Some(0x7f345e3f0d80)), pseudos: EagerPseudoStyles(None) }, damage: GeckoRestyleDamage(nsChangeHint(0)), hint: RESTYLE_SELF | RESTYLE_DESCENDANTS, flags: (empty) }) at servo/ports/geckolib/glue.rs:5811
#01: ??? RustMozCrash
#02: ??? mozglue_static::panic_hook::ha65425cbd41d959b
#03: ??? core::ops::function::Fn::call::h49ab1da6b2955792
#04: std::panicking::rust_panic_with_hook::hfd45b6b6c12d9fa5 std::panicking::rust_panic_with_hook::hfd45b6b6c12d9fa5
#05: ??? std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::hf591e8609a75bd4b
#06: ??? std::sys_common::backtrace::__rust_end_short_backtrace::h81899558795e4ff7
#07: rust_begin_unwind rust_begin_unwind
#08: core::panicking::panic_fmt::h4235fa9b4675b332 core::panicking::panic_fmt::h4235fa9b4675b332
#09: ??? Servo_ResolveStyle
#10: ??? nsCSSFrameConstructor::ResolveComputedStyle(nsIContent*)
#11: ??? nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, mozilla::ComputedStyle const&, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&, mozilla::EnumSet<nsCSSFrameConstructor::ItemFlag, unsigned char>)
#12: ??? nsCSSFrameConstructor::BuildInlineChildItems(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, bool, bool)
#13: ??? nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, bool, mozilla::ComputedStyle*, mozilla::EnumSet<nsCSSFrameConstructor::ItemFlag, unsigned char>, nsCSSFrameConstructor::FrameConstructionItemList&)
#14: ??? nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, mozilla::ComputedStyle const&, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&, mozilla::EnumSet<nsCSSFrameConstructor::ItemFlag, unsigned char>)
#15: ??? nsCSSFrameConstructor::ContentAppended(nsIContent*, nsCSSFrameConstructor::InsertionKind)
#16: ??? mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&)
#17: ??? mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags)
#18: ??? mozilla::RestyleManager::ProcessPendingRestyles()
#19: ??? mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)
#20: ??? mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)
#21: ??? mozilla::SMILAnimationController::DoSample(bool)
#22: ??? mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)
#23: ??? mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)
#24: ??? nsComputedDOMStyle::Flush(mozilla::dom::Document&, mozilla::FlushType)
#25: ??? nsComputedDOMStyle::UpdateCurrentStyleSources(nsCSSPropertyID)
#26: ??? nsComputedDOMStyle::GetPropertyValue(nsCSSPropertyID, nsTSubstring<char> const&, nsTSubstring<char>&)
#27: ??? mozilla::dom::CSSStyleDeclaration_Binding::getPropertyValue(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)
#28: ??? bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)
#29: ??? CallJSNative(JSContext*, bool ()(JSContext, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)
#30: ??? js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
#31: ??? Interpret(JSContext*, js::RunState&)
#32: ??? js::RunScript(JSContext*, js::RunState&)
#33: ??? js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
#34: ??? js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason)
#35: ??? js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)
#36: ??? bool GetExistingProperty<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::PropertyKey, (js::AllowGC)1>::HandleType, js::PropertyInfoBase<unsigned int>, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType)
#37: ??? bool NativeGetPropertyInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::PropertyKey, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType)
#38: ??? js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>)
#39: ??? js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>)
#40: ??? Interpret(JSContext*, js::RunState&)
#41: ??? js::RunScript(JSContext*, js::RunState&)
#42: ??? js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
#43: ??? js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)
#44: ??? (???:???)

Expected results:

the firefox should not crash.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::CSS Parsing and Computation' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Mayank Bansal]

Can you attach the testcase?

Comment 3

[2366719611]

Attachment: the html that causes the crash

Comment 4

[Darkspirit]

mozregression --good 2022-08-01 --bad 2023-03-30 -B debug -P stdout -a https://bugzilla.mozilla.org/attachment.cgi?id=9325881

11:51.01 INFO: Last good revision: 59656096ae33d73fcccf04b73eff52d0393570ba
11:51.01 INFO: First bad revision: 335d18658070d72e6a8e7752c5a776361149eb65
11:51.01 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=59656096ae33d73fcccf04b73eff52d0393570ba&tochange=335d18658070d72e6a8e7752c5a776361149eb65

335d18658070d72e6a8e7752c5a776361149eb65 Emilio Cobos Álvarez — Bug 1801123 - Enable container queries on nightly. r=dshin

(Shipped by bug 1809720.)

Comment 5

[Emilio Cobos Álvarez [:emilio]]

It'd be awesome to have some more reduced test-case here, the test case is quite massive.

Comment 6

[2366719611]

Attachment: minimized_1819.html

Comment 7

[2366719611]

I tried to minimize the html that will cause the crash, please take a look at it.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Thanks!

Comment 9

[David Shin[:dshin]]

Attachment: Minimum reproducible test case

Took a look because it reminded me a lot of bug 1727579. Managed to reduced as far as I could take.
Curiously, body inside foreignObject seemingly ends up as the body of the document...?

Comment 10

[Emilio Cobos Álvarez [:emilio]]

So the issue here is somewhat differently, the main components are:

• Having a font transition that affects a size container via inheritance.
• That means that we re-match the size container's descendants due to the font-size change here.
• But during the animation-only traversal we don't re-selector-match so we leave the restyle hint in the DOM.

Hiro, Boris, do you recall why do we only re-cascade (and not re-selector match) in the animation-only traversal?

The other question here is "why is there a transition at all in this test-case". The transition happens even without container-type so that seems pre-existing.

Comment 11

[Boris Chiou [:boris]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

So the issue here is somewhat differently, the main components are:

• Having a font transition that affects a size container via inheritance.
• That means that we re-match the size container's descendants due to the font-size change here.
• But during the animation-only traversal we don't re-selector-match so we leave the restyle hint in the DOM.

Hiro, Boris, do you recall why do we only re-cascade (and not re-selector match) in the animation-only traversal?

IIRC, the purpose of animation-only traversal is to do interpolation for animations and transitions (which are created from the 1st traversal, or restyle from animation/transition tick), and apply the results to animation rules or transition rules, for the elements which specify transition property and/or animation property. So we only do "re-cascade" in the animation-only traversal. That's why we skip a lot of things in animation-only traversal. Which means transitions shouldn't trigger any further style changes.

The other question here is "why is there a transition at all in this test-case". The transition happens even without container-type so that seems pre-existing.

Not sure how container-type works about this. We have non-zero transition-duration (and the default transition-property is all) but no one changes the style in the test case. Perhaps this is a bug?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

So the issue here is somewhat differently, the main components are:

• Having a font transition that affects a size container via inheritance.
• That means that we re-match the size container's descendants due to the font-size change here.
• But during the animation-only traversal we don't re-selector-match so we leave the restyle hint in the DOM.

I am afraid I can't recall details, but I think Boris is right. Any animation resyling didn't require selector matching at the era we were developing stylo. Though I am super unfamiliar with container queries stuff, if it requires selector matching only on descendants of the animating element, doing it during animation-only styling should be fine.

The other question here is "why is there a transition at all in this test-case". The transition happens even without container-type so that seems pre-existing.

There's a style specified as;

#htmlvar00006 { font: italic normal large serif; transition-duration: 1s; }

Presumably the #htmlvar00006 element has been styled (once or more) before resolving this selector, thus the transition is triggered. That sounds pretty normal for me. (we could maybe? optimize it though)

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Oh wait! Re-selector matching could trigger new transitions? If it's true, we can't allow it during animation-only restyling. :/