Closed Bug 2042439 Opened 2 months ago Closed 2 months ago

Content process crashes in ActiveScrolledRoot::AssertDepthInvariant when a position: sticky element is painted inside a CSS-transformed scroll container

Categories

(Core :: Panning and Zooming, defect)

Firefox 153
defect

Tracking

()

RESOLVED FIXED
153 Branch
Tracking Status
firefox-esr140 --- unaffected
firefox151 --- wontfix
firefox152 --- wontfix
firefox153 --- fixed

People

(Reporter: pronge, Assigned: tnikkel)

References

(Depends on 1 open bug, Regression)

Details

(Keywords: regression)

Crash Data

Attachments

(3 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:152.0) Gecko/20100101 Firefox/152.0

Steps to reproduce:

  1. Open a page that contains a draggable/responsive grid layout. Each grid item is positioned via
    transform: translate(...) and contains an overflow: auto scroll container with a position: sticky
    child inside it.
  2. Let the page issue an XHR that, when it resolves, causes the grid items to re-render with new
    content.
  3. Wait for the XHR to complete.

Reproduces 100% of the time, also in a private window. Firefox 152.0b2 on macOS 15.

Actual results:

The tab crashed at the next paint after the XHR completed.

Crash ID: bp-b9507e22-1d3a-40cd-a2bd-863170260526
Signature: mozilla::ActiveScrolledRoot::AssertDepthInvariant
MOZ_CRASH reason: MOZ_DIAGNOSTIC_ASSERT(mDepth == (mParent ? mParent->mDepth + 1 : 1)) at
layout/painting/nsDisplayList.cpp:377

Top frames:
ActiveScrolledRoot::AssertDepthInvariant nsDisplayList.cpp:377
ActiveScrolledRoot::LowestCommonAncestor nsDisplayList.cpp:292
WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList :2123
nsDisplayWrapList::CreateWebRenderCommandsNewClipListOption :4762
nsDisplayOwnLayer::CreateWebRenderCommands :5439
nsDisplayStickyPosition::CreateWebRenderCommands :5880
nsDisplayTransform::CreateWebRenderCommands :6869
nsDisplayList::PaintRoot :2353

Not OOM (~520 MB available, no JSLargeAllocationFailure).

Expected results:

The page should repaint without the content process aborting. The combination of a transformed
ancestor, an overflow: auto scroll container inside it, and a position: sticky descendant is valid CSS
and shouldn't trip an ASR depth assertion during WebRender display-list construction.

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.

Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Component: CSS Parsing and Computation → Graphics: WebRender
Component: Graphics: WebRender → Panning and Zooming

This appears to be another useful crash report for this bug
https://crash-stats.mozilla.org/report/index/df4eea6b-71d6-4f0f-a60f-be5800260526

Attached file not-testcase.html (obsolete) —

Thanks for reporting!

Based on your instructions I tried to create a testcase but I was unable to reproduce a problem. I uploaded my first attempt, but after this there are 5 more attempts that also didn't reproduce.

Would you be able to upload a testcase? Or link to one? You could use a site like https://jsfiddle.net/ or https://codepen.io/

Or provide more detailed steps to find a testcase? The framework you are using?

Flags: needinfo?(pronge)
Depends on: 2011906
Crash Signature: [@ mozilla::ActiveScrolledRoot::AssertDepthInvariant ]

I didn't manage to reproduce it in a public testcase, despite a few attempts (plain HTML, then a Vite project using the same libraries as the real app: @tanstack/react-table + @dnd-kit/sortable + Radix ScrollArea, scaled up to 20 widgets × 200 rows × 24 cols). None crash.

Bisecting in the real app got me somewhere though. The crash needs a specific table component that wraps @tanstack/react-table in @dnd-kit/sortable: one <DndContext>, one <SortableContext> per header row, then one <SortableContext> per cell (around a useSortable'd <td>). A 25 × 8 table mounts about 200 SortableContexts and 225 useSortable registrations. Swapping that table for a plain <table> rendering the same rows via flexRender stops the crash.

Versions if it matters: React 18.3.1, @dnd-kit/core 6.3.1, @dnd-kit/sortable 10.0.0, @dnd-kit/utilities 3.2.2, @dnd-kit/modifiers 9.0.0, @tanstack/react-table 8.20.5, @radix-ui/react-scroll-area 1.2.x. Firefox 152.0b4 on macOS.

The more important finding: the crash isn't from the table being rendered, it's from the table being inserted into a parent that already exists. Two paths through the app produce the same final DOM but only one crashes.

  1. Open the page at a small viewport. The widget shows a plain list, not the table. The fetch finishes silently. Then resize the window wider, the widget switches to the table component, which mounts fresh with the data already present. No crash.

  2. Open the page at a wide viewport so the table is rendered from the start. It mounts in a loading state with skeletons inside an outer scroll container. The page sits there for a couple of seconds with a spinner animating while the network request is in flight. The request finishes, the loading state flips, and React inserts a new wrapper (<div style="overflow-x: auto"><table>...</table></div>) into the already-mounted parent. Crash on the next paint, 100%.

So it's specifically splicing a sticky/scroll subtree into a parent that's already been painted. Building the same tree in one initial render is fine. That probably explains why every reduction I tried failed: a setTimeout insertion in a fiddle is still "fresh enough" from Firefox's perspective. The real app sits on a stable, animating, painted parent for several seconds before the insertion.

Sorry this is a bit vague without a public testcase.

Profile from a crash run: https://share.firefox.dev/4vcWPqH

Flags: needinfo?(pronge)

Would sharing a usernane/password to a private site that reproduces with a small set of Firefox developers be a possibility?

The bug has a crash signature, thus the bug will be considered confirmed.

Status: UNCONFIRMED → NEW
Ever confirmed: true

I can see if I can find some credentials you could use. I'll get back to you on that.

I did clone the Firefox source and let an AI agent go through it that had the bug context. I'm not a browser engineer, so I can't make anything useful out of the response. It might provide you with some valuable context though. Hopefully it's not just AI-bloat.


Setup: nsDisplayListBuilder::mActiveScrolledRoots gets cleared every paint in EndFrame (nsDisplayList.cpp:911), but ASRs are also cached on the frame itself via frame properties (ActiveScrolledRootCache, StickyActiveScrolledRootCache). In GetOrCreateASRForFrame and GetOrCreateASRForStickyFrame (nsDisplayList.cpp:168 and :205), aActiveScrolledRoots.AppendElement(asr) only runs inside the if (!asr) branch, so cache hits return an ASR but don't re-append it to the per-paint array.

InsertScrollFrame (nsDisplayList.cpp:603) iterates mActiveScrolledRoots[mDescendantsStartIndex..end). The comment on mDescendantsStartIndex in the header (nsDisplayList.h:1264) says this is intentional, to skip ASRs created in the past. The subtle bit is that "ASRs created in the past" effectively means "ASRs that were cache-hit this paint", not "ASRs created before this scope logically started".

Failure walkthrough:

  1. Paint N. A sticky element gets an ASR S via GetOrCreateASRForStickyFrame. Cache miss, S is appended and cached on the frame. S.parent = X, S.depth = X.depth + 1. End of paint clears the array. S persists on the frame property.

  2. Paint N+1. Same sticky is processed. Cache hit, S is returned but NOT re-appended. S.parent and S.depth are reassigned to the same values. Then SetCurrentActiveScrolledRoot(S) is called, so descendants of the sticky see S as their current ASR.

  3. Inside the sticky's descendants, a new ASR-altering element is encountered for the first time this run. A new ASR Z is created with Z.parent = S. Z is appended to the array. Z.depth = S.depth + 1 = X.depth + 2.

  4. Later, a sibling scroll frame causes ShouldForceLayerForScrollParent to return true on the enclosing scroll, and InsertScrollFrame runs. New ASR Y is created for that scroll, with Y.parent = X. The loop iterates new ASRs in the range. It reaches Z. IsAncestor(X, Z) walks Z → S → X and returns true. Z.IncrementDepth() runs. Z.parent (S) != X, so no re-parent. Z.depth is now X.depth + 3.

  5. S was a cache hit, so it's not in the loop range. S.depth is unchanged at X.depth + 1.

  6. After the loop: Z.depth = X.depth + 3 and Z.parent = S with S.depth = X.depth + 1. Z.depth != S.depth + 1. The invariant is broken. The new assertion at line 627 doesn't fire because it only asserts on the same range. Later, LowestCommonAncestor walks through Z and calls AssertDepthInvariant on it, and it fires.

One possible fix: in the loop, replace descendantASR->IncrementDepth() with descendantASR->mDepth = descendantASR->mParent->mDepth + 1 after the re-parent step. That recomputes depth from the (possibly updated) parent instead of blindly adding one, which would handle the case where the parent's depth didn't move. Probably has its own caveats.

Tentatively triaging as S3, since this doesn't seem widespread. It seems fairly severe for those who experience it, though, so this might go up if this is experienced by a wide set of people.

I'll try to reproduce this, too, once a reproducible example is provided from OP.

Severity: -- → S3

Where can I forward you some login details for the site? The crash happens instantly after logging in, so that would probably be the easiest path forward

After some debugging together via Matrix, Patric determined that the issue only seemed to be reproducing while using a Lenovo ThinkPad Universal USB-C Dock (40AY) from an M3 MacBook Pro with Tahoe 26.5. Otherwise, this reproduced the same between Beta and Nightly.

AIUI, this sort of dock may have separate hardware that might be handled differently in the rendering pipeline. I'm not sure if that (if true) would change the fundamental behavior in causing this crash, but it seems likely.

(In reply to Erich Gubler [:ErichDonGubler] (he/him) from comment #9)

It seems fairly severe for those who experience it, though, so this might go up if this is experienced by a wide set of people.

Note that the assertion that's failing is a MOZ_DIAGNOSTIC_ASSERT. I guess that's enabled on the beta channel these days, but it shouldn't be enabled on the release channel.

See Also: → 2031478

MOZ_DIAGNOSTIC_ASSERT is enabled in early beta (the first week or two of the beta cycle) and nightly.

What was the prompts and input that you gave the AI to generate that info? This would help me understand it better

(In reply to Patric Ronge from comment #8)

Setup: nsDisplayListBuilder::mActiveScrolledRoots gets cleared every paint in EndFrame (nsDisplayList.cpp:911), but ASRs are also cached on the frame itself via frame properties (ActiveScrolledRootCache, StickyActiveScrolledRootCache). In GetOrCreateASRForFrame and GetOrCreateASRForStickyFrame (nsDisplayList.cpp:168 and :205), aActiveScrolledRoots.AppendElement(asr) only runs inside the if (!asr) branch, so cache hits return an ASR but don't re-append it to the per-paint array.

InsertScrollFrame (nsDisplayList.cpp:603) iterates mActiveScrolledRoots[mDescendantsStartIndex..end). The comment on mDescendantsStartIndex in the header (nsDisplayList.h:1264) says this is intentional, to skip ASRs created in the past. The subtle bit is that "ASRs created in the past" effectively means "ASRs that were cache-hit this paint", not "ASRs created before this scope logically started".

InsertScrollFrame should never be called on your machine. In fact, you can see here

https://searchfox.org/firefox-main/rev/e28b34ab33dbf49364999070168cbb7e11e8e5bd/layout/generic/ScrollContainerFrame.cpp#4281

that we insert a critical note into any crashes that might happen if it is ever called, and I looked in your crash reports and there is no such note. So that means it either wasn't called, or ShouldActivateAllScrollFrames is false in your build. Which is very unexpected, but possible. If that was the case we would want to look into why that happened.

Can you provide the contents of about:support from the machine that is affected when it is in the affected configuration (ie in the dock)? That would probably tell us if anything looks wrong that would change the value of ShouldActivateAllScrollFrames.

Erich shared with me the repro site and login info, but I have so far been unable to reproduce the issue. But I can look into the site content for more clues.

Flags: needinfo?(pronge)

Also, running https://mozilla.github.io/mozregression/ in the affected configuration to see exactly what changed caused this to start failing would be quite helpful.

One possible reason the dock is needed is that the you have a mouse connected to the dock and that can affect the type of scrollbars you get on macos. In the mac system settings, search for "scroll bar", and then find the setting "Show scroll bars" that has options "Automatically based on mouse or trackpad" "When scrolling" "Always" and report what your setting is.

I also put a printf in the CSS anchor pos functions that muck with ASRs and it was not hit when I loaded the reproducing website privately provided by the reported. So that means the CSS anchor pos ASR stuff should not come into play, which simplifies the debugging and what could be going wrong.

And also no sticky ASRs created either.

I can reproduce with the private site now, I needed to increase the width of my window.

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

(In reply to Timothy Nikkel (:tnikkel) from comment #17)

One possible reason the dock is needed is that the you have a mouse connected to the dock and that can affect the type of scrollbars you get on macos. In the mac system settings, search for "scroll bar", and then find the setting "Show scroll bars" that has options "Automatically based on mouse or trackpad" "When scrolling" "Always" and report what your setting is.

Ah yes. I had Auto selected. I'm able to replicate the crash 100% of the time in my app when changing that setting from "Automatically based on mouse or trackpad" to "Always", with a viewport of >1600px
No external monitor or dock needed. Reproduced on just my macbook

Flags: needinfo?(pronge)

Got a small reproducing testcase.

Attachment #9590786 - Attachment is obsolete: true

And this is a testcase that is similar but triggers the same assert without using sticky ASRs at all (which changed wholesale in Nov). So this likely shows the bug has existed for a long time (not confirmed yet).

Take an outer scroll container A that is async-inactive because it has zero scroll range. It contains various active scroll frames and active nested scroll frames. Some content inside A (but not related to any of the active descendant scroll frames) is modified so that the size of the scrolled content increases making the scroll range non-zero and now WantAsyncScroll() is true. The size increase triggers a retained display list update. None of the descendant already-active scroll frames are marked modified, so the display list build does not visit the every active descendant scroll frame (even if we were to visit every direct ASR child of A). Visiting the frame of an ASR is the only way that we can update it to changes in the ASR tree, so some ASRs do not get updated, so the depth values get out of sync and we hit the depth invariant assert.

The most obvious way to fix this is to check after reflow if a scroll frame has changed scroll range that changed the return value of WantAsyncScroll and mark it modified. But this is too large of a hammer because the extra display list building the modified frames will trigger, but also because WantAsyncScroll is relatively heavy. As such we limit the check as narrowly as we can. We use a bool that tracks scroll frames that were inactive during the previous paint but that have active scroll frame descendants. Further, previous work in bug 2044231 reduced the cost of WantAsyncScroll by caching GetScrolledRect during ReflowFinished. The main cost of WantAsyncScroll will then be the memory load of fetching the overflow values from the style display struct which might not be cached at this point (even though they are fetched at the start of the reflow of this scroll frame) because this happens after the reflow of the entire frame tree. Lower down in ReflowFinished we call GetScrollRangeForUserInputEvents() inside |if (mVScrollbarBox || mHScrollbarBox)| which will also fetch those values. So hopefully this should not add many memory fetches.

I also checked that anything else that can change WantAsyncScroll's return value should not have this problem.

Assignee: nobody → tnikkel
Status: NEW → ASSIGNED
Pushed by tnikkel@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/082d13fbc6dc https://hg.mozilla.org/integration/autoland/rev/e5537f7e10a5 Handle cases where an inactive scroll frame becomes active and has already-active descendant scroll frames during a retained display list build. r=hiro,layout-reviewers

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

Pushed by agoloman@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/1ab8d1ff4541 https://hg.mozilla.org/integration/autoland/rev/32917493fb41 Revert "Bug 2042439. Handle cases where an inactive scroll frame becomes active and has already-active descendant scroll frames during a retained display list build. r=hiro,layout-reviewers" for causing wpt failures @scrollend-event-fires-to-iframe-window.html.

Backed out for causing wpt failures @scrollend-event-fires-to-iframe-window.html.

Flags: needinfo?(tnikkel)

Upstream PR was closed without merging

Depends on: 2045387

The failure was not actually caused by anything in this patch, but because a field was not init'ed, bug 2042439.

Flags: needinfo?(tnikkel)
Pushed by tnikkel@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/84021b4e80bf https://hg.mozilla.org/integration/autoland/rev/20f108790cfd Handle cases where an inactive scroll frame becomes active and has already-active descendant scroll frames during a retained display list build. r=hiro,layout-reviewers
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 153 Branch

Upstream PR merged by moz-wptsync-bot

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

For more information, please visit BugBot documentation.

Flags: needinfo?(tnikkel)
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: