1903652 - use-after-poison in [@ nsBlockFrame::MarkIntrinsicISizesDirty]

Status: VERIFIED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing 20240618-1f73c4ee1785 (--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>

==309670==ERROR: AddressSanitizer: use-after-poison on address 0x5250003be528 at pc 0x726618eb09e9 bp 0x7fff5cd856f0 sp 0x7fff5cd856e8
WRITE of size 4 at 0x5250003be528 thread T0 (Isolated Web Co)
    #0 0x726618eb09e8 in nsBlockFrame::MarkIntrinsicISizesDirty() /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:756:31
    #1 0x726618c4bace in mozilla::PresShell::FrameNeedsReflow(nsIFrame*, mozilla::IntrinsicDirty, nsFrameState, mozilla::ReflowRootHandling) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:2703:12
    #2 0x7266190f555a in nsPlaceholderFrame::Destroy(mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsPlaceholderFrame.cpp:156:9
    #3 0x726618f1d9ac in DestroyFrames /builds/worker/checkouts/gecko/layout/generic/nsFrameList.cpp:40:12
    #4 0x726618f1d9ac in nsContainerFrame::Destroy(mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsContainerFrame.cpp:234:11
    #5 0x7266190bcca3 in nsInlineFrame::Destroy(mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsInlineFrame.cpp:177:21
    #6 0x7266190c8d3e in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsFrameList*, mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsLineBox.cpp:369:14
    #7 0x726618eade56 in nsBlockFrame::Destroy(mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:459:3
    #8 0x726618f1d9ac in DestroyFrames /builds/worker/checkouts/gecko/layout/generic/nsFrameList.cpp:40:12
    #9 0x726618f1d9ac in nsContainerFrame::Destroy(mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsContainerFrame.cpp:234:11
    #10 0x7266190c8d3e in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsFrameList*, mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsLineBox.cpp:369:14
    #11 0x726618eade56 in nsBlockFrame::Destroy(mozilla::FrameDestroyContext&) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:459:3
    #12 0x726618edd9db in nsBlockFrame::DoRemoveFrame(mozilla::FrameDestroyContext&, nsIFrame*, unsigned int) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:6920:20
    #13 0x726618ede0d3 in nsBlockFrame::DoRemoveFrame(mozilla::FrameDestroyContext&, nsIFrame*, unsigned int) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:7027:14
    #14 0x726618ede0d3 in nsBlockFrame::DoRemoveFrame(mozilla::FrameDestroyContext&, nsIFrame*, unsigned int) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:7027:14
    #15 0x726618edcda9 in nsBlockFrame::RemoveFrame(mozilla::FrameDestroyContext&, mozilla::FrameChildListID, nsIFrame*) /builds/worker/checkouts/gecko/layout/generic/nsBlockFrame.cpp:6127:5
    #16 0x726618d1e237 in RemoveFrame /builds/worker/checkouts/gecko/layout/base/nsFrameManager.cpp:122:18
    #17 0x726618d1e237 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:7470:5
    #18 0x726618d14dd3 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:8392:7
    #19 0x726618d1ffac in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval(nsIFrame*) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:8265:3
    #20 0x726618d1d0bc in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /builds/worker/checkouts/gecko/layout/base/nsCSSFrameConstructor.cpp:7357:9
    #21 0x726618c65d22 in mozilla::PresShell::ContentRemoved(nsIContent*, nsIContent*) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:4577:22
    #22 0x726611c4fd05 in operator() /builds/worker/checkouts/gecko/dom/base/MutationObservers.cpp:188:19
    #23 0x726611c4fd05 in Notify<(NotifyPresShell)1, (lambda at /builds/worker/checkouts/gecko/dom/base/MutationObservers.cpp:188:19)> /builds/worker/checkouts/gecko/dom/base/MutationObservers.cpp:91:7
    #24 0x726611c4fd05 in mozilla::dom::MutationObservers::NotifyContentRemoved(nsINode*, nsIContent*, nsIContent*) /builds/worker/checkouts/gecko/dom/base/MutationObservers.cpp:187:3
    #25 0x726611eca51e in nsINode::RemoveChildNode(nsIContent*, bool) /builds/worker/checkouts/gecko/dom/base/nsINode.cpp:2299:5
    #26 0x726611ecc9d7 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/nsINode.cpp:2669:18
    #27 0x7266125a2eec in InsertBefore /builds/worker/checkouts/gecko/dom/base/nsINode.h:2201:12
    #28 0x7266125a2eec in AppendChild /builds/worker/checkouts/gecko/dom/base/nsINode.h:2208:12
    #29 0x7266125a2eec in mozilla::dom::Node_Binding::appendChild(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./NodeBinding.cpp:950:60
    #30 0x7266139d3804 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:3268:13
    #31 0x72661a95ce64 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:487:13
    #32 0x72661a95ce64 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:581:12
    #33 0x72661b9c5090 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:1670:10
    #34 0x176dda7e3923  ([anon:js-executable-memory]+0x2923)

Comment 1

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

This is likely regressed by bug 1901515. I notice a similar crash signature in https://bugzilla.mozilla.org/show_bug.cgi?id=1903141#c5.

Comment 2

[Tyson Smith [:tsmith]]

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

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240619213942-7999d1a5d574.
Unable to bisect testcase (Unable to launch the start build!):

Start: a0f7b9f56d1f3dd6b66ba0417bab9acccddc3ad9 (20230622091413)
End: 1f73c4ee1785ebeefb9f926926019cfbcac13f43 (20240618094855)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 4

[Daniel Holbert [:dholbert]]

We probably need to sort this out before letting bug 1901515 part 3 make it to release... TYLin I hope you don't mind if I assign this to you, just to be sure we don't lose track of this. It seems that our optimization in bug 1901515 part 3 ( https://hg.mozilla.org/integration/autoland/rev/8ea9b9da6922 ) isn't sound at this point, because there are codepaths that might exercise the cached first-continuation pointer during destruction; so we need to find a fix or back that out, probably.

We might be able to mitigate this if we could ensure that the cached FirstContinuation() frame remains alive (possibly with some sort of "doomed, don't do anything to me" flag set on it) until we're done destroying all of its next-continuations. I think that would be a slight reordering of things and it might break other invariants (not sure?); but if it's doable, it'd let us make sure that e.g. state-bit-setting on the FirstContinuation is guaranteed to be fine.

Alternately, I can also imagine a more complicated solution where we might morph the cached FirstContinuation() pointer to be behind some layer of indirection, using some sort of shared (refcounted?) struct that just stores a nsIFrame* pointer. The first-continuation-frame itself would also need to have a reference to that struct, so that it could clear the nsIFrame* pointer (which has that frame's own address) when it gets destroyed -- it would have special permission to null out the pointer-value in its own Destroy() function; and then other frames would need to be able to gracefully handle that frame being nullptr (and determine what to do if they find a nullptr there -- whether to ignore that vs. lazily populate it with the actual first-in-flow). This sort of setup would get complicated when a frame list gets split, since the frames after the split would all need to update their FirstContinuation() pointer to point to a new instance of this struct. But that's roughly similar to what we're already doing (just with a raw nsIFrame* rather than this sort of struct)), so maybe it's not that much more work?

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]: Seems worth tracking to make sure we follow what happens here and in bug 1901515 (from comment 4).

Comment 6

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

The bug is marked as tracked for firefox129 (nightly). However, the bug still has low severity.

:fgriffith, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 7

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

Re Comment 4:

You're right that the optimization in bug 1901515 part 3 is not sound because the assumption described in the commit message doesn't hold.

It is challenging to implement the idea to keep the cached first-continuation alive until all its next-continuations are destroyed, as nsBlockFrame::DoRemoveFrame() also need to delete the line structure.

The alternative idea is feasible and can be implemented with our current setup, which stores the first-continuation cache in a plain nsIFrame* frame property. My proposal is that when we remove a frame's first-continuation, we purge all the first-continuation cache in all the next-continuations, which is still O(n). Then, we reintroduce a slow path in FirstContinuation() if the cache does not exist.

Comment 8

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

Attachment: Bug 1903652 Part 1 - Revert Bug 1901515 Part 3 to rework the performance issue. r?dholbert

This patch is a revert of
https://hg.mozilla.org/mozilla-central/rev/8ea9b9da6922 because the correctness
assumption described in the commit message was wrong. Note that this patch does
not revert the documentation improvement for
nsSplittableFrame::RemoveFromFlow() because it is worth keeping.

During frame destruction, nsBlockFrame::MarkIntrinsicISizesDirty() can be
called and it accesses FirstContinuation(). Thus, it is not acceptable to
allow a stale first-continuation cache during frame destruction.

Comment 9

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

Attachment: Bug 1903652 Part 2 - Purge first continuation cache for next continuations during frame destruction. r?dholbert

This patch attempts another approach to avoid the O(n^2) time complexity of
updating the first-continuation cache when removing a frame's continuation chain
from front to back.

Assume F1, F2, and F3 are the first three continuations in the chain. When we
call F2->SetPrevContinuation(nullptr) to disconnect F1 from the chain, we now
purge the first-continuation cache for F2's next-continuations rather than
updating their cache value to F2.

At the time when we call F3->SetPrevContinuation(nullptr) to disconnect F2
from the chain, F3's next-continuations no longer have the cache. Therefor, we
don't need to do extra work to walk the next-continuations.

Because the first-continuation cache might not exist during the frame
destruction, this patch reintroduce the slow path to walk the prev-continuation
chain to reach the first-continuation.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f7e39d93d17
Part 1 - Revert Bug 1901515 Part 3 to rework the performance issue. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/46b26e204660
Part 2 - Purge first continuation cache for next continuations during frame destruction. r=dholbert

Comment 11

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

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

Comment 12

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/3f7e39d93d17
https://hg.mozilla.org/mozilla-central/rev/46b26e204660

Comment 13

[Bugmon [:jkratzer for issues]]

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

Comment 14

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

Upstream PR merged by moz-wptsync-bot

Comment 15

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

Upstream PR merged by moz-wptsync-bot