1878188 - Assertion failure: aFrame->GetDepthInFrameTree() == mList[mList.IndexOf(aFrame)].mDepth, at /builds/worker/checkouts/gecko/layout/base/DepthOrderedFrameList.cpp:21

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.zip

Found while fuzzing m-c 20240121-b6f41f48017c (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

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

Assertion failure: aFrame->GetDepthInFrameTree() == mList[mList.IndexOf(aFrame)].mDepth, at /builds/worker/checkouts/gecko/layout/base/DepthOrderedFrameList.cpp:21

#0 0x7f578b190c90 in mozilla::DepthOrderedFrameList::Add(nsIFrame*) /builds/worker/checkouts/gecko/layout/base/DepthOrderedFrameList.cpp:20:5
#1 0x7f578b396347 in nsIFrame::HandlePrimaryFrameStyleChange(mozilla::ComputedStyle*) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:766:11
#2 0x7f578b36392e in nsIFrame::DidSetComputedStyle(mozilla::ComputedStyle*) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:1391:5
#3 0x7f578b1e081f in SetComputedStyle /builds/worker/checkouts/gecko/layout/generic/nsIFrame.h:842:7
#4 0x7f578b1e081f in mozilla::RestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /builds/worker/checkouts/gecko/layout/base/RestyleManager.cpp:2978:10
#5 0x7f578b1e0eb7 in mozilla::RestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /builds/worker/checkouts/gecko/layout/base/RestyleManager.cpp:3024:32
#6 0x7f578b1e0eb7 in mozilla::RestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /builds/worker/checkouts/gecko/layout/base/RestyleManager.cpp:3024:32
#7 0x7f578b1e0eb7 in mozilla::RestyleManager::ProcessPostTraversal(mozilla::dom::Element*, mozilla::ServoRestyleState&, mozilla::ServoPostTraversalFlags) /builds/worker/checkouts/gecko/layout/base/RestyleManager.cpp:3024:32
#8 0x7f578b1e2c82 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/checkouts/gecko/layout/base/RestyleManager.cpp:3237:28
#9 0x7f578b1b5a15 in mozilla::RestyleManager::ProcessPendingRestyles() /builds/worker/checkouts/gecko/layout/base/RestyleManager.cpp:3350:3
#10 0x7f578b1b4b57 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:4332:39
#11 0x7f578b179049 in FlushPendingNotifications /builds/worker/workspace/obj-build/dist/include/mozilla/PresShell.h:1474:5
#12 0x7f578b179049 in nsRefreshDriver::TickObserverArray(unsigned int, mozilla::TimeStamp) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2502:20
#13 0x7f578b175a18 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsRefreshDriver::IsExtraTick) /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:2736:28
#14 0x7f578b18419f in operator() /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:1838:25
#15 0x7f578b18419f in mozilla::detail::RunnableFunction<nsRefreshDriver::EnsureTimerStarted(nsRefreshDriver::EnsureTimerStartedFlags)::$_1>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:548:5
#16 0x7f5785865a77 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:578:16
#17 0x7f578585b1e6 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:905:26
#18 0x7f57858599c7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:728:15
#19 0x7f5785859e45 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:514:36
#20 0x7f5785869a16 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:232:37
#21 0x7f5785869a16 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
#22 0x7f578587ed82 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1199:16
#23 0x7f5785885ecd in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#24 0x7f578655d735 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#25 0x7f57864779d1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#26 0x7f57864779d1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#27 0x7f578adafda8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#28 0x7f578ae6d3e8 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:470:33
#29 0x7f578cc8009b in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:721:20
#30 0x7f578655e616 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:235:9
#31 0x7f57864779d1 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#32 0x7f57864779d1 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#33 0x7f578cc7f902 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:656:34
#34 0x55b527c663b6 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
#35 0x55b527c663b6 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375:18
#36 0x7f579a029d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#37 0x7f579a029e3f in __libc_start_main csu/../csu/libc-start.c:392:3
#38 0x55b527c3c0e8 in _start (/home/user/workspace/browsers/m-c-20240201173838-fuzzing-debug/firefox-bin+0x590e8) (BuildId: 3158970e7ac1bc3dd3d8206f2d8c2427846e3ccf)

Comment 1

[Bugmon [:jkratzer for issues]]

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

Start: f593f07c97724141b65e9eb1b9aa4a3bfdb47b2a (20240117145030)
End: 504ffc20d2ebb35be48357830d614ec96765c802 (20240117093935)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f593f07c97724141b65e9eb1b9aa4a3bfdb47b2a&tochange=504ffc20d2ebb35be48357830d614ec96765c802

Comment 2

[Daniel Holbert [:dholbert]]

Seems to be a regression from bug 1765615. I generated linux64 debug fuzzing builds for that commit and its parent, and grizzly confirms that b2727a598452fb942e0611df5f931d4df0efaceb reproduces the issue and 5608664350d824d3974f3026972afb0b23e5ee75 does not.

Confirmed by running this for bug 1765615's parent revision:

cd /tmp
export REV=5608664350d824d3974f3026972afb0b23e5ee75
python3 -m fuzzfetch -d --autoland --fuzzing -n firefox-$REV  --build $REV
python3 -m grizzly.replay.bugzilla --repeat 5 ./firefox-$REV/firefox 1878188

(no issues detected)
and then running that same command with bug 1765615's own revision:

cd /tmp
export REV=b2727a598452fb942e0611df5f931d4df0efaceb
python3 -m fuzzfetch -d --autoland --fuzzing -n firefox-$REV  --build $REV
python3 -m grizzly.replay.bugzilla --repeat 5 ./firefox-$REV/firefox 1878188

where grizzly saw the assertion-failure happen:
Result: Assertion failure: aFrame->GetDepthInFrameTree() == mList[mList.IndexOf(aFrame)].mDepth, at /builds/worker/checkouts/gecko/layout/base/DepthOrderedFrameList.cpp:21 (f91f2f21:d235c775)

Comment 3

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

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

For more information, please visit BugBot documentation.

Comment 4

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

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

Comment 5

[Frédéric Wang (:fredw)]

Attachment: Slightly reduced testcase

I can reproduce with a build from today with this slightly reduced testcase.

Comment 6

[Frédéric Wang (:fredw)]

Attachment: WIP: Bug 1878188 - Fix invalidation of CSS container-type. r=#layout-reviewers

Comment 7

[Frédéric Wang (:fredw)]

Attachment: Minimized testcase

Comment 8

[Frédéric Wang (:fredw)]

I can confirm this is due to optimization for the CSS container-type property. I attached a minimal testcase that performs only one dynamic change to that property. FWIW, forcing a frame reconstruction works around the crash

diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -2254,6 +2254,10 @@ nsChangeHint nsStyleDisplay::CalcDifference(
     return nsChangeHint_ReconstructFrame;
   }
 
+  if (mContainerType != aNewData.mContainerType) {
+    return nsChangeHint_ReconstructFrame;
+  }
+
   auto oldAppearance = EffectiveAppearance();
   auto newAppearance = aNewData.EffectiveAppearance();
   if (oldAppearance != newAppearance) {

but will investigate with rr later today to see what's happening exactly.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1878188 - Avoid re-registering container query frames unnecessarily. r=fredw,#layout-reviewers

This is kind of a wallpaper, but I think it's sound, read below.

The reason this is asserting is that MathML in particular has some odd
frame re-parenting going on (see
nsCSSFrameConstructor::FlushAccumulatedBlock).

That means that our assert in DepthOrderedFrameList is not really
holding depending on when you add or remove the frame from the list.

However this is not a problem in practice, at least for container
queries, because anonymous frames that can wrap mathML cannot be
containers themselves.

I need to figure out what to do with that assert, but given it's useful
for the other users of DepthOrderedFrameList, I think we should take
this for now.

Comment 10

[Bob Hood [:bhood]]

Frédéric, it would be helpful to have Priority/Severity values assigned to this report. Could you please add those when you get a chance?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7146c4efa63b
Avoid re-registering container query frames unnecessarily. r=fredw

Comment 12

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

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

Comment 13

[Frédéric Wang (:fredw)]

(In reply to Bob Hood [:bhood] from comment #10)

Frédéric, it would be helpful to have Priority/Severity values assigned to this report. Could you please add those when you get a chance?

Per Emilio's analysis, this just seems an internal assert that is failing for an edge case (MathML malignmark element with container-type), so setting low priority per https://wiki.mozilla.org/BMO/UserGuide/BugFields

Comment 14

[Bugmon [:jkratzer for issues]]

Successfully recorded a pernosco session. A link to the pernosco session will be added here shortly.

Comment 15

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7146c4efa63b

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7146c4efa63b

Comment 18

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

Upstream PR merged by moz-wptsync-bot

Comment 19

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

Upstream PR merged by moz-wptsync-bot

Comment 20

[Bugmon [:jkratzer for issues]]

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

Comment 21

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

The patch landed in nightly and beta is affected.
:fredw, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox123 to wontfix.

For more information, please visit BugBot documentation.

Comment 22

[Frédéric Wang (:fredw)]

This is just an internal MOZ_ASSERT to check that the depth has not changed, but it's not clear whether this has any impact with the rest of the code. Per D200634, "this is not a problem in practice, at least for container queries, because anonymous frames that can wrap mathML cannot be
containers themselves.". It does not seem we have any important reason to uplift so setting as wontfix.