2008625 - Assertion failure: !mFinishedBuildingColumns (Should only call once!), at /builds/worker/checkouts/gecko/layout/generic/ColumnSetWrapperFrame.cpp:107

Status: VERIFIED FIXED

(Core :: Layout: Columns, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing m-c 20260104-39305265f6c1 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework --upgrade
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Assertion failure: !mFinishedBuildingColumns (Should only call once!), at /builds/worker/checkouts/gecko/layout/generic/ColumnSetWrapperFrame.cpp:107

#0 0x7fffec81cbf0 in MOZ_CrashSequence /builds/worker/workspace/obj-build/dist/include/mozilla/Assertions.h:237:3
#1 0x7fffec81cbf0 in mozilla::ColumnSetWrapperFrame::AppendFrames(mozilla::FrameChildListID, nsFrameList&&) /builds/worker/checkouts/gecko/layout/generic/ColumnSetWrapperFrame.cpp:107:3
#2 0x7fffec79cac6 in nsCSSFrameConstructor::FinishBuildingColumns(nsFrameConstructorState&, nsContainerFrame*, nsContainerFrame*, nsFrameList&) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:10239:22
#3 0x7fffec7a25b8 in nsCSSFrameConstructor::AppendFramesToParent(nsFrameConstructorState&, nsContainerFrame*, nsFrameList&, nsIFrame*, bool) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:5656:5
#4 0x7fffec7a5837 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:6618:7
#5 0x7fffec695a15 in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:1620:27
#6 0x7fffec69cf0d in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3250:7
#7 0x7fffec69e181 in mozilla::RestyleManager::ProcessPendingRestyles() /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3340:3
#8 0x7fffec74cd17 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:4471:37
#9 0x7fffe8679515 in FlushPendingNotifications /builds/worker/workspace/obj-build/dist/include/mozilla/PresShell.h:1526:5
#10 0x7fffe8679515 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/checkouts/gecko/dom/base/Document.cpp:11635:16
#11 0x7fffe84cfa8f in FlushPendingNotifications /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:7104:11
#12 0x7fffe84cfa8f in nsGlobalWindowOuter::GetScrollXY(bool) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:3786:5
#13 0x7fffe84cfbad in nsGlobalWindowOuter::GetScrollXOuter() /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:3807:56
#14 0x7fffe849255b in nsGlobalWindowInner::GetScrollX(mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.cpp:3697:3
#15 0x7fffe962ecc2 in GetPageXOffset /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowInner.h:800:12
#16 0x7fffe962ecc2 in mozilla::dom::Window_Binding::get_pageXOffset(JSContext*, JS::Handle<JSObject*>, void*, JSJitGetterCallArgs) /builds/worker/workspace/obj-build/dom/bindings/./WindowBinding.cpp:5129:38
#17 0x7fffe9b24364 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::MaybeCrossOriginObjectThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3178:13
#18 0x7fffee6c7bc4 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:490:13
#19 0x7fffee6c746f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:586:12
#20 0x7fffee6c886c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:685:8
#21 0x7fffee6c99d3 in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:807:10
#22 0x7fffeea4d76f in CallGetter /builds/worker/checkouts/gecko/js/src/vm/NativeObject.cpp:2139:12
#23 0x7fffeea4d76f in 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) /builds/worker/checkouts/gecko/js/src/vm/NativeObject.cpp:2167:12
#24 0x7fffeea4def2 in 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) /builds/worker/checkouts/gecko/js/src/vm/NativeObject.cpp:2310:14
#25 0x7fffee6b2141 in js::ForwardingProxyHandler::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const /builds/worker/checkouts/gecko/js/src/proxy/Wrapper.cpp:147:10
#26 0x7fffe84ba10d in nsOuterWindowProxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const /builds/worker/checkouts/gecko/dom/base/nsGlobalWindowOuter.cpp:957:23
#27 0x7fffee6a10e8 in js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/proxy/Proxy.cpp:526:19
#28 0x7fffee6a0c4f in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/proxy/Proxy.cpp:534:10
#29 0x7fffed51d6f3 in js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, js::PropertyName*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/ObjectOperations-inl.h:120:10
#30 0x7fffee6e8430 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:4617:10
#31 0x7fffedcd4a33 in js::jit::DoGetPropFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/BaselineIC.cpp:1318:8
#32 0x1335af3b5c4a  ([anon:js-executable-memory]+0xcc4a)

Comment 1

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20260105211017-f3c4f3651e2d.
The bug appears to have been introduced in the following build range:

Start: 791d47406d61d6ee6e99e1e84b896b227a775c8b (20260103085132)
End: 4dc50e8367a1d9082bc8f8fa8296481ec97a2ba4 (20260103093014)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=791d47406d61d6ee6e99e1e84b896b227a775c8b&tochange=4dc50e8367a1d9082bc8f8fa8296481ec97a2ba4

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 2008148

:emilio, since you are the author of the regressor, bug 2008148, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Ting-Yu, is this assert correct? My understanding is that before my patch you could end up in the same situation if instead of doing an append you do multiple of them at the DOM level. This is in the nsCSSFrameConstructor::AppendFramesToParent codepath for columns.

For now I'll restore the old behavior for this test-case, but I think the assert is probably wrong. I don't see what would prevent us from hitting FinishBuildingColumns multiple times on the same frame here.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 2008625 - Don't unconditionally fall back to single inserts for ::first-letter. r=TYLin

This fixes the regression making us go through the same codepath as
before, but I think this assertion is fishy, see details on the bug.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 2008625 - Add web-platform reftests for appending element into containers that have pseudo-elements. r?emilio

• multicol-dynamic-add-002.html is adapted from bug 1645714 comment 0.
• multicol-dynamic-add-003.html is adapted from bug 1645714 comment 6.
• multicol-dynamic-add-004.html is adapted from bug 1645714 comment 7.

Note: multicol-dynamic-add-004.html (inserting float element) still crashes when
running on CI.

Comment 6

[Phabricator Automation]

Comment on attachment 9536255 [details]
Bug 2008625 - Add web-platform reftests for appending element into containers that have pseudo-elements. r?emilio

Revision D278219 was moved to bug 2009082. Setting attachment 9536255 [details] to obsolete.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)

Ting-Yu, is this assert correct? My understanding is that before my patch you could end up in the same situation if instead of doing an append you do multiple of them at the DOM level. This is in the nsCSSFrameConstructor::AppendFramesToParent codepath for columns.

I believe the assertion is reasonable. It is used to allow ColumnSetWrapperFrame to finish building the multicol frame tree structure [1], which is expected to be called once. For complex scenario like inserting a column-span element in the subtree in the column content, we jut reframe the entire multicol [2][3]. For appending non-column-span DOM element into the last column content multiple times, yes, this is handled by nsCSSFrameConstructor::AppendFramesToParent, but it shouldn't needed to call FinishBuildingColumns(), because there is no needed to create column-span-wrapper frame [4]. For this bug, I suspect the new nsCSSFrameConstructor::ContentRangeInserted probably didn't handle the multicol reframe case like the old nsCSSFrameConstructor::ContentAppended did.

[1] https://searchfox.org/firefox-main/rev/5a0e415cfc3fb41872974e47db748854155a2456/layout/base/nsCSSFrameConstructor.cpp#10239-10240
[2] https://searchfox.org/firefox-main/rev/5a0e415cfc3fb41872974e47db748854155a2456/layout/base/nsCSSFrameConstructor.cpp#10756-10762
[3] https://searchfox.org/firefox-main/rev/5a0e415cfc3fb41872974e47db748854155a2456/layout/base/nsCSSFrameConstructor.cpp#11041-11069
[4] https://searchfox.org/firefox-main/rev/5a0e415cfc3fb41872974e47db748854155a2456/layout/base/nsCSSFrameConstructor.cpp#5639-5640

Is it possible that before your fix in bug 2008148, the testcase in this bug just go to the reframe path in ContentAppended?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b25bbe45423b
https://hg.mozilla.org/integration/autoland/rev/7c7cb9e2ce5c
Don't unconditionally fall back to single inserts for ::first-letter. r=TYLin,layout-reviewers

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/57068 for changes under testing/web-platform/tests

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7c7cb9e2ce5c

Comment 11

[Bugmon [:jkratzer for issues]]

Verified bug as fixed on rev mozilla-central 20260108165448-9af669f3df03.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot