1982701 - crash near null in [@ nsIFrame::CorrectStyleParentFrame], with <dialog> showModal()

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing 20250812-68d37f9acbd4 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

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

==137917==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000059 (pc 0x7b86907ac6c8 bp 0x7fff6eebb230 sp 0x7fff6eebb180 T0)
==137917==The signal is caused by a READ memory access.
==137917==Hint: address points to the zero page.
    #0 0x7b86907ac6c8 in HasAnyStateBits /builds/worker/checkouts/gecko/layout/generic/nsIFrame.h:2533:59
    #1 0x7b86907ac6c8 in nsIFrame::CorrectStyleParentFrame(nsIFrame*, mozilla::PseudoStyleType) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:11266:17
    #2 0x7b86908bf764 in GetCorrectedParent(nsIFrame const*) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:11236:10
    #3 0x7b86908bf08a in nsIFrame::DoGetParentComputedStyle(nsIFrame**) const /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:11365:23
    #4 0x7b86903b1479 in mozilla::RestyleManager::DoReparentComputedStyleForFirstLine(nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3858:15
    #5 0x7b86903b1c78 in mozilla::RestyleManager::ReparentFrameDescendants(nsIFrame*, nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3961:9
    #6 0x7b86903b17b2 in mozilla::RestyleManager::DoReparentComputedStyleForFirstLine(nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3947:3
    #7 0x7b86903b1c78 in mozilla::RestyleManager::ReparentFrameDescendants(nsIFrame*, nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3961:9
    #8 0x7b86903b17b2 in mozilla::RestyleManager::DoReparentComputedStyleForFirstLine(nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3947:3
    #9 0x7b86903b1c78 in mozilla::RestyleManager::ReparentFrameDescendants(nsIFrame*, nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3961:9
    #10 0x7b86903b17b2 in mozilla::RestyleManager::DoReparentComputedStyleForFirstLine(nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3947:3
    #11 0x7b86903b13ef in mozilla::RestyleManager::DoReparentComputedStyleForFirstLine(nsIFrame*, mozilla::ServoStyleSet&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3852:7
    #12 0x7b86905a653a in nsCSSFrameConstructor::CheckForFirstLineInsertion(nsIFrame*, nsFrameList&) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:9889:23
    #13 0x7b869059efc0 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:7068:5
    #14 0x7b86903a1173 in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:1686:25
    #15 0x7b86903abca2 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3301:7
    #16 0x7b86903ae19a in mozilla::RestyleManager::ProcessPendingRestyles() /builds/worker/checkouts/gecko/layout/style/RestyleManager.cpp:3391:3
    #17 0x7b869050422d in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:4644:37
    #18 0x7b8689294444 in FlushPendingNotifications /builds/worker/workspace/obj-build/dist/include/mozilla/PresShell.h:1485:5
    #19 0x7b8689294444 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/checkouts/gecko/dom/base/Document.cpp:11547:16
    #20 0x7b868c3f92eb in mozilla::dom::HTMLDialogElement::FocusDialog() /builds/worker/checkouts/gecko/dom/html/HTMLDialogElement.cpp:528:10
    #21 0x7b868c3f9cf2 in mozilla::dom::HTMLDialogElement::ShowModal(mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/html/HTMLDialogElement.cpp:459:3
    #22 0x7b868b022521 in mozilla::dom::HTMLDialogElement_Binding::showModal(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./HTMLDialogElementBinding.cpp:332:24
    #23 0x7b868b0fff1f in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3308:13
    #24 0x7b8691f6fac7 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:501:13
    #25 0x7b8691f6fac7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:597:12
    #26 0x7b86930b3489 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/BaselineIC.cpp:1705:10
    #27 0x30a7965e59b3  ([anon:js-executable-memory]+0x29b3)

Comment 1

[Tyson Smith [:tsmith]]

The attached test case also triggers the following assertion in a debug build:

Assertion failure: aProspectiveParent (Must have a prospective parent), at /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:11242

Comment 2

[Mayank Bansal]

Got a crash from the testcase on Nightly: https://crash-stats.mozilla.org/report/index/74734837-cace-4f83-8bf0-a00a50250813

Comment 3

[Bugmon [:jkratzer for issues]]

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

Start: fa6903bc79db3e595424864c2cc48e6a0c88aa6e (20250811221641)
End: 12583b99aa37020190a98410edcbea026e63fc3c (20250812002550)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fa6903bc79db3e595424864c2cc48e6a0c88aa6e&tochange=12583b99aa37020190a98410edcbea026e63fc3c

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

Comment 4

[Bugmon [:jkratzer for issues]]

A pernosco session for this bug can be found here.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Bugmon [:jkratzer for issues] from comment #3)

The bug appears to have been introduced in the following build range:

In that range, this would be a regression from bug 1941406, given that the testcase uses details (though bug 1941406 was really just a pref-flip, so it's what made the bug possible to trigger by-default).

Comment 6

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

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

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a variant of the testcase.

Notably: in order to trigger the issue,

• The ::first-line rule specifically needs to be targeting the containing-block of the <dialog>. (the div with class "wrapper" in this case)
• ...but the dialog doesn't need to actually be in that first line. (In this case, it's after the second line.)

Comment 8

[Daniel Holbert [:dholbert]]

So if I'm understanding correctly, this code in GetCorrectedParent...
https://searchfox.org/mozilla-central/rev/ce0d41b6033e2a104904327a43edf730245f5241/layout/generic/nsIFrame.cpp#11259-11273
...seems to be assuming that if aFrame and element have the same nontrivial pseudo-type, then both of the following are true:
(A) element must be native anonymous content.
(B) element or some ancestor must return true for IsRootOfNativeAnonymousSubtree

But in this case that's not true.

In Pernosco (with the original testcase), the following is true:

• element (which is aFrame->GetContent()) is the HTMLSlotElement for the ::details-content pseudo; and it returns false from IsRootOfNativeAnonymousSubtree. Walking up the tree from there:
• the parent frame is a nsBlockFrame whose GetContent() is the HTMLDetailsElement
• its parent frame is a nsBlockFrame whose GetContent() is the HTMLDialogElement
• its parent frame is a ScrollContainerFrame whose GetContent() is also the HTMLDialogElement
• its parent frame is the ViewportFrame whose GetContent() is nullptr.
  None of these element's content-nodes return true from IsRootOfNativeAnonymousSubtree(), so we walk until we get a null GetContent() pointer (from the ViewportFrame), and then there's one additional parent = parent->GetInFlowParent(); after the while loop which takes us to the ViewportFrame's parent which is nullptr.

And then we pass that into nsIFrame::CorrectStyleParentFrame and we crash.

I don't have native-anonymous-content invariants paged into my head... but I presume either (B) above is an invalid assumption, or else one of our content nodes here (maybe the HTMLSlotElement element?) should be returning true from IsRootOfNativeAnonymousSubtree.

Comment 9

[Daniel Holbert [:dholbert]]

(And I guess ::first-line is only relevant here because it causes the sort of style-reparenting business that's resulting in us calling GetCorrectedParent here in the first place -- RestyleManager::DoReparentComputedStyleForFirstLine() calls nsIFrame::GetParentComputedStyle() which calls DoGetParentComputedStyle which calls GetCorrectedParent.)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1982701: Exclude element-backed pseudos from NAC ancestor-traversal in GetCorrectedParent. r?emilio,#layout

This is a bit of a band-aid; as noted in the included code-comment, there's
probably a more general check we should be doing here, but this avoids the
crash for now.

Comment 11

[Daniel Holbert [:dholbert]]

Band-aid patch posted which avoids the crash, with both of the attached testcases included as WPT crashtests; I confirmed in a local debug build that both WPTs crash without the fix vs. pass with the fix, when run as ./mach wpt [test].

As noted in the patch, there's probably a better/more-general way to do this, but I'm not sure offhand and I don't want to let the perfect be the enemy of the good, as we get closer to the Nightly143-->Beta143 merge day next week.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/056dd5e03f5a
https://hg.mozilla.org/integration/autoland/rev/05d50287fb8c
Exclude element-backed pseudos from NAC ancestor-traversal in GetCorrectedParent. r=emilio,layout-reviewers

Comment 13

[amarc]

https://hg.mozilla.org/mozilla-central/rev/05d50287fb8c

Comment 14

[Bugmon [:jkratzer for issues]]

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

Comment 15

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

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

Comment 16

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

Upstream PR merged by moz-wptsync-bot