1882153 - Assertion failure: aspectRatioUsage == AspectRatioUsage::None, at /layout/generic/nsIFrame.cpp:6632

Status: NEW

(Core :: Layout, defect)

Description

[Jason Kratzer [:jkratzer]]

Testcase found while fuzzing mozilla-central rev 689cf8cf7e6b (built with: --enable-debug --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 689cf8cf7e6b --debug --fuzzing  -n firefox
$ python -m grizzly.replay.bugzilla ./firefox/firefox <bugid>

Assertion failure: aspectRatioUsage == AspectRatioUsage::None, at /layout/generic/nsIFrame.cpp:6632

    ==225170==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f4b402d5335 bp 0x7ffe98d60ae0 sp 0x7ffe98d60990 T225170)
    ==225170==The signal is caused by a WRITE memory access.
    ==225170==Hint: address points to the zero page.
        #0 0x7f4b402d5335 in nsIFrame::ComputeSize(gfxContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::StyleSizeOverrides const&, mozilla::EnumSet<mozilla::ComputeSizeFlag, unsigned char>) /layout/generic/nsIFrame.cpp:6632:5
        #1 0x7f4b40266879 in mozilla::ReflowInput::InitConstraints(nsPresContext*, mozilla::Maybe<mozilla::LogicalSize> const&, mozilla::Maybe<mozilla::LogicalMargin> const&, mozilla::Maybe<mozilla::LogicalMargin> const&, mozilla::LayoutFrameType) /layout/generic/ReflowInput.cpp:2406:19
        #2 0x7f4b40262fa1 in mozilla::ReflowInput::Init(nsPresContext*, mozilla::Maybe<mozilla::LogicalSize> const&, mozilla::Maybe<mozilla::LogicalMargin> const&, mozilla::Maybe<mozilla::LogicalMargin> const&) /layout/generic/ReflowInput.cpp:393:3
        #3 0x7f4b40263c2f in mozilla::ReflowInput::ReflowInput(nsPresContext*, mozilla::ReflowInput const&, nsIFrame*, mozilla::LogicalSize const&, mozilla::Maybe<mozilla::LogicalSize> const&, mozilla::EnumSet<mozilla::ReflowInput::InitFlag, unsigned char>, mozilla::StyleSizeOverrides const&, mozilla::EnumSet<mozilla::ComputeSizeFlag, unsigned char>) /layout/generic/ReflowInput.cpp:254:5
        #4 0x7f4b40349e2f in nsGridContainerFrame::ReflowInFlowChild(nsIFrame*, nsGridContainerFrame::GridItemInfo const*, nsSize, mozilla::Maybe<int> const&, nsGridContainerFrame::Fragmentainer const*, nsGridContainerFrame::GridReflowInput const&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&) /layout/generic/nsGridContainerFrame.cpp:7681:15
        #5 0x7f4b4035022d in nsGridContainerFrame::ReflowChildren(nsGridContainerFrame::GridReflowInput&, mozilla::LogicalRect const&, nsSize const&, mozilla::ReflowOutput&, nsReflowStatus&) /layout/generic/nsGridContainerFrame.cpp:8799:7
        #6 0x7f4b403516fc in nsGridContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsGridContainerFrame.cpp:9042:11
        #7 0x7f4b402c6d94 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) /layout/generic/nsContainerFrame.cpp:886:14
        #8 0x7f4b402b4d06 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsCanvasFrame.cpp:729:7
        #9 0x7f4b402c6d94 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) /layout/generic/nsContainerFrame.cpp:886:14
        #10 0x7f4b4030cb17 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput&, bool, bool, mozilla::ReflowOutput*) /layout/generic/nsGfxScrollFrame.cpp:915:3
        #11 0x7f4b4030d9ee in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput&, mozilla::ReflowOutput const&) /layout/generic/nsGfxScrollFrame.cpp:1048:3
        #12 0x7f4b4031262c in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/nsGfxScrollFrame.cpp:1523:3
        #13 0x7f4b402c7281 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) /layout/generic/nsContainerFrame.cpp:927:14
        #14 0x7f4b402875de in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /layout/generic/ViewportFrame.cpp:368:7
        #15 0x7f4b40171344 in mozilla::PresShell::DoReflow(nsIFrame*, bool, mozilla::OverflowChangedTracker*) /layout/base/PresShell.cpp:9785:11
        #16 0x7f4b4019ac3f in mozilla::PresShell::ProcessReflowCommands(bool) /layout/base/PresShell.cpp:9958:22
        #17 0x7f4b4017b803 in DoFlushLayout /layout/base/PresShell.cpp:10005:10
        #18 0x7f4b4017b803 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /layout/base/PresShell.cpp:4361:11
        #19 0x7f4b4013fb3e in FlushPendingNotifications /builds/worker/workspace/obj-build/dist/include/mozilla/PresShell.h:1474:5
        #20 0x7f4b4013fb3e in nsRefreshDriver::TickObserverArray(unsigned int, mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:2531:18
        #21 0x7f4b4013c444 in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsRefreshDriver::IsExtraTick) /layout/base/nsRefreshDriver.cpp:2735:28
        #22 0x7f4b40145f61 in TickDriver /layout/base/nsRefreshDriver.cpp:367:13
        #23 0x7f4b40145f61 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver>>&) /layout/base/nsRefreshDriver.cpp:345:7
        #24 0x7f4b40145e60 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:361:5
        #25 0x7f4b40145cfd in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:951:5
        #26 0x7f4b40144f9c in mozilla::VsyncRefreshDriverTimer::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) /layout/base/nsRefreshDriver.cpp:861:5
        #27 0x7f4b40144209 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsyncTimerOnMainThread() /layout/base/nsRefreshDriver.cpp:592:14
        #28 0x7f4b3f45ffdb in mozilla::dom::VsyncMainChild::RecvNotify(mozilla::VsyncEvent const&, float const&) /dom/ipc/VsyncMainChild.cpp:66:15
        #29 0x7f4b3f75039d in mozilla::dom::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PVsyncChild.cpp:237:78
        #30 0x7f4b3b56e361 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBackgroundChild.cpp:5555:32
        #31 0x7f4b3b50222f in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /ipc/glue/MessageChannel.cpp:1813:25
        #32 0x7f4b3b4fef82 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /ipc/glue/MessageChannel.cpp:1732:9
        #33 0x7f4b3b4ffc02 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /ipc/glue/MessageChannel.cpp:1525:3
        #34 0x7f4b3b500d4f in mozilla::ipc::MessageChannel::MessageTask::Run() /ipc/glue/MessageChannel.cpp:1623:14
        #35 0x7f4b3a804f77 in mozilla::RunnableTask::Run() /xpcom/threads/TaskController.cpp:578:16
        #36 0x7f4b3a7fa6e6 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:905:26
        #37 0x7f4b3a7f8ec7 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /xpcom/threads/TaskController.cpp:728:15
        #38 0x7f4b3a7f9345 in mozilla::TaskController::ProcessPendingMTTask(bool) /xpcom/threads/TaskController.cpp:514:36
        #39 0x7f4b3a808f89 in operator() /xpcom/threads/TaskController.cpp:235:37
        #40 0x7f4b3a808f89 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /xpcom/threads/nsThreadUtils.h:548:5
        #41 0x7f4b3a81e282 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1199:16
        #42 0x7f4b3a8253cd in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:480:10
        #43 0x7f4b3b508143 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:107:5
        #44 0x7f4b3b41e521 in RunHandler /ipc/chromium/src/base/message_loop.cc:363:3
        #45 0x7f4b3b41e521 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:345:3
        #46 0x7f4b3fd73958 in nsBaseAppShell::Run() /widget/nsBaseAppShell.cpp:148:27
        #47 0x7f4b3fe33128 in nsAppShell::Run() /widget/gtk/nsAppShell.cpp:470:33
        #48 0x7f4b41c67d7b in XRE_RunAppShell() /toolkit/xre/nsEmbedFunctions.cpp:712:20
        #49 0x7f4b3b509076 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:235:9
        #50 0x7f4b3b41e521 in RunHandler /ipc/chromium/src/base/message_loop.cc:363:3
        #51 0x7f4b3b41e521 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:345:3
        #52 0x7f4b41c675e2 in XRE_InitChildProcess(int, char**, XREChildData const*) /toolkit/xre/nsEmbedFunctions.cpp:647:34
        #53 0x556d2a3133b6 in content_process_main /browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
        #54 0x556d2a3133b6 in main /browser/app/nsBrowserApp.cpp:375:18
        #55 0x7f4b4f30ed8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
        #56 0x7f4b4f30ee3f in __libc_start_main csu/../csu/libc-start.c:392:3
        #57 0x556d2a2e90e8 in _start (/home/jkratzer/builds/m-c-20240226091922-fuzzing-debug/firefox-bin+0x590e8) (BuildId: 60f7330b0a6d5742590720057ef3b2cf120c0f81)
    
    UndefinedBehaviorSanitizer can not provide additional info.
    SUMMARY: UndefinedBehaviorSanitizer: SEGV /layout/generic/nsIFrame.cpp:6632:5 in nsIFrame::ComputeSize(gfxContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::StyleSizeOverrides const&, mozilla::EnumSet<mozilla::ComputeSizeFlag, unsigned char>)
    ==225170==ABORTING

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: Detailed Crash Information

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

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

Start: 67cef656eb4954a889931ff1b67f21d65a1937da (20231218230000)
End: 4bc62630a431fca569dfb1add5eee214dcdd091c (20231219005617)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=67cef656eb4954a889931ff1b67f21d65a1937da&tochange=4bc62630a431fca569dfb1add5eee214dcdd091c

Comment 4

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

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

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

For more information, please visit BugBot documentation.

Comment 5

[Dianna Smith [:diannaS]]

:dholbert do you think you can provide a priority or severity here? Want to make sure we are okay wont fixing this for 124

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simplified)

Comment 7

[Daniel Holbert [:dholbert]]

Taking a look. The assertion itself is harmless & doesn't indicate anything catastrophic/crashy for end users, but I'm not sure right now to-what-extent it might be a signal of actual sizing issues (it's in nsIFrame::ComputeSize so taking an unexpected path there could mean things end up sized incorrectly).

testcase 2 demonstrates that this is pretty easy to trigger (no need for huge size values or dynamic scripted mutations like in the original testcase), so I'm suspicious that this might be something that we're hitting in the wild and potentially sizing stuff wrong as a result, which leads me to think we should try to get a fix in time for v124.

Comment 8

[Daniel Holbert [:dholbert]]

pernosco recording of this fatal-assertion being tipped by testcase 2:
https://pernos.co/debug/f6RwbPz5Gjv4IB688Kx4AA/index.html#f{m[Abd/,Ckf6_,t[AbQ,AtFb_,f{e[Abd/,Ci9s_,s{af/t1gAAA,bCtg,uErKN+g,oEsf6CQ___/

The assertion is failing because aspectRatioUsage has been previously set by this clause, up higher when we're resolving ISize stuff:
https://searchfox.org/mozilla-central/rev/b55a24116575b1b6c17f1aa56a0b142d86dfa41c/layout/generic/nsIFrame.cpp#6445-6455

if (!stretch && mayUseAspectRatio) {
  // Note: we don't need to handle aspect ratio for inline axis if both
  // width/height are auto. The default ratio-dependent axis is block axis
  // in this case, so we can simply get the block size from the non-auto
  // |styleBSize|.
  auto bSize = nsLayoutUtils::ComputeBSizeValue(
      aCBSize.BSize(aWM), boxSizingAdjust.BSize(aWM),
      styleBSize.AsLengthPercentage());
  result.ISize(aWM) = aspectRatio.ComputeRatioDependentSize(
      LogicalAxis::eLogicalAxisInline, aWM, bSize, boxSizingAdjust);
  aspectRatioUsage = AspectRatioUsage::ToComputeISize;

And then we trip the assertion where we expect this aspectRatioUsage variable to not be set in the bsize section below...

Comment 9

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

Attachment: testcase 3

I found a testcase that doesn't trigger the assertion, but we render it differently from Chrome. In Firefox, the subgrid height is affected by aspect-ratio, but in Google Chrome it is not.

I think Chrome is correct. Per https://drafts.csswg.org/css-grid/#subgrid-box-alignment, the item is subgridded in the block-axis, so the height constraint should be ignore, so should the the height transfer from width via aspect-ratio.

Comment 10

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

Attachment: testcase 3

This is the correct version of testcase 3.

Comment 11

[Daniel Holbert [:dholbert]]

[midaired with comment 9, just capturing some notes from poking in pernosco]
The regressing patch (https://hg.mozilla.org/mozilla-central/rev/20d9f6b14ee3 ) matters here because the testcase's grid item is subgridded in the block axis (i.e. it has grid-template-rows: subgrid), which is a condition that the regressing patch makes use of in its logic.

Previously, this testcase would've taken the first clause in this Compute block-axis size logic flow (where we don't care about the aspect ratio). Now, the isSubgriddedInBlockAxis; check means we take the final clause instead (the clause with the assertion that we're tripping over here).

And I think the assertion is warning us that we're using the aspect ratio in both directions, which is iffy. We're computing the iSize from a tentative bSize, and then we're computing a bSize from that iSize (which itself was dependent on the original tentative bSize). In the case of testcase 2, this doesn't matter much because the iSize and bSize both end up 0; but you could imagine other cases where the repeated aspected-ratio conversion introduces some error (or, at best, is unnecessary -- computing a value that we already know).

Comment 12

[Daniel Holbert [:dholbert]]

RE testcase 3: for what it's worth, its behavior doesn't seem to have changed recently; Nightly from just before the regression and from over a year ago (2023-01-01) match our current rendering of that one. Also, WebKit renders it differently from Chrome, so there's a distinct lack of interop on that one. :)

Comment 13

[Daniel Holbert [:dholbert]]

So for testcase 2 and in particular the code I quoted in comment 8:

• We should be treating the subgrid as if it were stretched in the block axis, per the spec text that TYLin linked ("any specified width/height constraints" should be ignored in the subgridded axis).
• So it's weird/wrong that we're using the specified block-size to compute the inline-size (the code I quoted in comment 8), given that we should be stretching in the block axis (and hence should not be using the specified block-size).
• And it's weird/wrong that we subsequently go and resolve a block size based on the inline-size (in the clause where we assert) given that we're actually going to be using the stretched block-size.

It seems like we shouldn't be doing either of those things. (Maybe that's partly what testcase 3 is getting at.)

Comment 14

[Daniel Holbert [:dholbert]]

Tentatively triaging as S3 given that we've already shipped this regression in v123 and haven't run across real sites that break due to it (perhaps because sites have strayed away from this area of subgrid edge cases due fun interop variability in this area, as illustrated by the different-in-every-browser renderings of testcase 3; or, maybe because the testcases here are doing things that are explicitly supposed to have no effect, like setting an explicit height on a vertical-axis subgrid).

Depending on how targeted vs. broad our fix is, we can see whether uplift makes sense. If the fix here ends up changing our rendering in examples like testcase 3 (where our rendering has been the same for a long time) as part of bringing us into a more coherent state, then it might be wisest to let this ride the trains rather than uplift to 124.

Comment 15

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

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