1410243 - Assertion failure: aStatus.IsEmpty() (Caller should pass a fresh reflow status!) in [@ nsBlockFrame::Reflow]

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: aStatus.IsEmpty() (Caller should pass a fresh reflow status!), at /src/layout/generic/nsBlockFrame.cpp:1091

#0 nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsBlockFrame.cpp:1091:3
#1 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /src/layout/generic/nsContainerFrame.cpp:932:14
#2 nsGridContainerFrame::ReflowInFlowChild(nsIFrame*, nsGridContainerFrame::GridItemInfo const*, nsSize, mozilla::Maybe<int> const&, nsGridContainerFrame::Fragmentainer const*, nsGridContainerFrame::GridReflowInput const&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&) /src/layout/generic/nsGridContainerFrame.cpp:5125:3
#3 nsGridContainerFrame::ReflowChildren(nsGridContainerFrame::GridReflowInput&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&) /src/layout/generic/nsGridContainerFrame.cpp:5697:7
#4 nsGridContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsGridContainerFrame.cpp:6000:11
#5 nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /src/layout/generic/nsBlockReflowContext.cpp:306:11
#6 nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /src/layout/generic/nsBlockFrame.cpp:3476:11
#7 nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /src/layout/generic/nsBlockFrame.cpp:2825:5
#8 nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /src/layout/generic/nsBlockFrame.cpp:2649:11
#9 nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsBlockFrame.cpp:1235:3
#10 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /src/layout/generic/nsContainerFrame.cpp:932:14
#11 nsColumnSetFrame::ReflowChildren(mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) /src/layout/generic/nsColumnSetFrame.cpp:805:7
#12 nsColumnSetFrame::ReflowColumns(mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, nsColumnSetFrame::ReflowConfig&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) /src/layout/generic/nsColumnSetFrame.cpp:502:19
#13 nsColumnSetFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/nsColumnSetFrame.cpp:1240:19
#14 nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, mozilla::ReflowInput const&, nsRect const&, nsAbsoluteContainingBlock::AbsPosReflowFlags, nsIFrame*, nsReflowStatus&, nsOverflowAreas*) /src/layout/generic/nsAbsoluteContainingBlock.cpp:723:14
#15 nsAbsoluteContainingBlock::Reflow(nsContainerFrame*, nsPresContext*, mozilla::ReflowInput const&, nsReflowStatus&, nsRect const&, nsAbsoluteContainingBlock::AbsPosReflowFlags, nsOverflowAreas*) /src/layout/generic/nsAbsoluteContainingBlock.cpp:166:7
#16 mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /src/layout/generic/ViewportFrame.cpp:377:35
#17 mozilla::PresShell::DoReflow(nsIFrame*, bool) /src/layout/base/PresShell.cpp:8939:11
#18 mozilla::PresShell::ProcessReflowCommands(bool) /src/layout/base/PresShell.cpp:9112:24
#19 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /src/layout/base/PresShell.cpp:4184:11
#20 nsRefreshDriver::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:1965:16
#21 mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /src/layout/base/nsRefreshDriver.cpp:307:7
#22 mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:329:5
#23 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:770:5
#24 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:683:35
#25 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /src/layout/base/nsRefreshDriver.cpp:584:9
#26 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /src/layout/ipc/VsyncChild.cpp:67:16
#27 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:155:20
#28 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1755:28
#29 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /src/ipc/glue/MessageChannel.cpp:2119:25
#30 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /src/ipc/glue/MessageChannel.cpp:2049:17
#31 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /src/ipc/glue/MessageChannel.cpp:1895:5
#32 mozilla::ipc::MessageChannel::MessageTask::Run() /src/ipc/glue/MessageChannel.cpp:1928:15
#33 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1037:14
#34 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:512:10
#35 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#36 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#37 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#38 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#39 XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:877:22
#40 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:269:9
#41 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#42 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#43 XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:703:34
#44 content_process_main(mozilla::Bootstrap*, int, char**) /src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
#45 main /src/browser/app/nsBrowserApp.cpp:280:18
#46 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#47 _start (/home/user/workspace/browsers/m-c-1508348667-asan-debug/firefox+0x41ebe4)

Comment 1

[Mats Palmgren (inactive)]

Looks like a grid issue from the stack.  I'll take a look...

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I haven't been able to reproduce the fatal assert on either Win10 or Ubuntu 17.10, but I noticed that it also hits the below non-fatal assertion:
ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0', file /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp, line 1665

That bisected to:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bbd8e6ecf4a00d33736f1df5f906a6331467d966&tochange=1852d46ea730f311b76aeca207d29d6158636927

For whatever that's worth.

Comment 3

[Mats Palmgren (inactive)]

I can't reproduce the fatal assert either.  I do see the non-fatal one
in comment 2, but that seems like a lesser issue.  That's already known
to fail in some cases (bug 574889) so it might not be a grid-specific
issue.

Comment 4

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 5

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/OiRcg7tFnTb9VFNIRJ0STQ/index.html

Comment 6

[Daniel Holbert [:dholbert]]

I don't hit the fatal assertion with this bug's testcase in a current debug build, though I do hit several copies of this non-fatal assertion:

###!!! ASSERTION: overflow containers must be zero-block-size: 'finalSize.BSize(wm) == 0',
file layout/generic/nsBlockFrame.cpp:2292

I do trip this assertion if I load bug 1881375's testcase in a debug build, though. (That bug's testcase triggers a null-deref in opt builds, but it makes debug builds trip this fatal assertion first.)

I think the assertion is pointing us at a legitimate problem: nsGridContainerFrame has one spot where it reflows a child and passes in its own ReflowStatus flag:
https://searchfox.org/mozilla-central/rev/f602853ba8d55ba157e2a74d9b571615f6ed97b8/layout/generic/nsGridContainerFrame.cpp#8832-8833

ReflowInFlowChild(child, info, aContainerSize, Nothing(), nullptr, aState,
                  aContentArea, aDesiredSize, aStatus);

We should probably be declaring a distinct childStatus there for each child, like we do in other calls to ReflowInFlowChild in this file.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1410243: Make nsGridContainerFrame give each child a dedicated ReflowStatus to use for reflow. r?TYLin

This matches what we do in flexbox, here:
https://searchfox.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#2024,2033-2035,2038-2042

We have fuzzer testcases where the child reflow sets the reflow status to
"OverflowIncomplete", which is fine from the perspective of the
immediately-following "IsComplete()" assertion in the contextual code of this
patch; but it's not fine if we then go on to reuse that
already-set-to-OverflowIncomplete status for another child's reflow.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c89fd8ba4e0
Make nsGridContainerFrame give each child a dedicated ReflowStatus to use for reflow. r=TYLin

Comment 9

[Aron Cseh [:acseh]]

https://hg.mozilla.org/mozilla-central/rev/5c89fd8ba4e0

Comment 11

[BugBot [:suhaib / :marco]]

Copying crash signatures from duplicate bugs.

Comment 12

[BugBot [:suhaib / :marco]]

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

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

For more information, please visit BugBot documentation.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 1410243: Make nsGridContainerFrame give each child a dedicated ReflowStatus to use for reflow. r?TYLin

This matches what we do in flexbox, here:
https://searchfox.org/mozilla-central/rev/8c3ca2f5a74e0ba59c3d9dddf5468a2ffab13467/layout/generic/nsFlexContainerFrame.cpp#2024,2033-2035,2038-2042

We have fuzzer testcases where the child reflow sets the reflow status to
"OverflowIncomplete", which is fine from the perspective of the
immediately-following "IsComplete()" assertion in the contextual code of this
patch; but it's not fine if we then go on to reuse that
already-set-to-OverflowIncomplete status for another child's reflow.

Original Revision: https://phabricator.services.mozilla.com/D207094

Comment 14

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: crashes
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: Load testcase in bug 1881375, see if you crash.
• Risk associated with taking this patch: Low
• Explanation of risk level: Very targeted patch, which is a step in the direction of correctness/consistency, and reducing inadvertent leakage of temporary layout state to influence later pieces of layout.
• String changes made/needed: None
• Is Android affected?: yes

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5cdb2f8e60b0

Comment 16

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 126.0b4 on Windows 10x64 by loading the test case from bug 1881375 as stated in comment 14. The Firefox tab crashes with this crash ID. As a side note sometimes I have also got this crash ID.

The issue is verified fixed with Firefox 126.0b5 (20240423115600) from comment 21 and 127.0a1 (20240423214125) on Windows 10x64, macOS 12, and Ubuntu 23.10. Loading the test case from bug 1881375 will no longer crash the Firefox tab.