1993379 - crash near null in [@ mozilla::TextRenderedRunIterator::~TextRenderedRunIterator]

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing 20251005-9d455cb6a47f (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

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

==230353==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000001d0 (pc 0x7fffdeaaec44 bp 0x7fffffff1730 sp 0x7fffffff1710 T0)
==230353==The signal is caused by a READ memory access.
==230353==Hint: address points to the zero page.
    #0 0x7fffdeaaec44 in isSome /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:472:42
    #1 0x7fffdeaaec44 in reset /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:802:9
    #2 0x7fffdeaaec44 in ForgetCachedProvider /builds/worker/workspace/obj-build/dist/include/mozilla/SVGTextFrame.h:629:49
    #3 0x7fffdeaaec44 in mozilla::TextRenderedRunIterator::~TextRenderedRunIterator() /builds/worker/checkouts/gecko/layout/svg/SVGTextFrame.cpp:1788:55
    #4 0x7fffdeab1af1 in mozilla::SVGTextFrame::GetFrameForPoint(mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, double> const&) /builds/worker/checkouts/gecko/layout/svg/SVGTextFrame.cpp:3337:1
    #5 0x7fffdea48023 in mozilla::SVGClipPathFrame::PointIsInsideClipPath(nsIFrame*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, double> const&) /builds/worker/checkouts/gecko/layout/svg/SVGClipPathFrame.cpp:291:21
    #6 0x7fffdead4cdd in mozilla::SVGUtils::HitTestClip(nsIFrame*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, double> const&) /builds/worker/checkouts/gecko/layout/svg/SVGUtils.cpp:755:27
    #7 0x7fffdea77cf0 in mozilla::SVGIntegrationUtils::HitTestFrameForEffects(nsIFrame*, nsPoint const&) /builds/worker/checkouts/gecko/layout/svg/SVGIntegrationUtils.cpp:420:10
    #8 0x7fffdecb4541 in mozilla::nsDisplayEffectsBase::HitTest(mozilla::nsDisplayListBuilder*, nsRect const&, mozilla::nsDisplayItem::HitTestState*, nsTArray<nsIFrame*>*) /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:8025:7
    #9 0x7fffdec702ee in mozilla::nsDisplayList::HitTest(mozilla::nsDisplayListBuilder*, nsRect const&, mozilla::nsDisplayItem::HitTestState*, nsTArray<nsIFrame*>*) const /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:2454:11
    #10 0x7fffdec92c1f in HitTest /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:4635:13
    #11 0x7fffdec92c1f in mozilla::nsDisplayOpacity::HitTest(mozilla::nsDisplayListBuilder*, nsRect const&, mozilla::nsDisplayItem::HitTestState*, nsTArray<nsIFrame*>*) /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:4778:22
    #12 0x7fffdec702ee in mozilla::nsDisplayList::HitTest(mozilla::nsDisplayListBuilder*, nsRect const&, mozilla::nsDisplayItem::HitTestState*, nsTArray<nsIFrame*>*) const /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:2454:11
    #13 0x7fffdec9f817 in mozilla::nsDisplayAsyncZoom::HitTest(mozilla::nsDisplayListBuilder*, nsRect const&, mozilla::nsDisplayItem::HitTestState*, nsTArray<nsIFrame*>*) /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:6171:9
    #14 0x7fffdec702ee in mozilla::nsDisplayList::HitTest(mozilla::nsDisplayListBuilder*, nsRect const&, mozilla::nsDisplayItem::HitTestState*, nsTArray<nsIFrame*>*) const /builds/worker/checkouts/gecko/layout/painting/nsDisplayList.cpp:2454:11
    #15 0x7fffde556833 in nsLayoutUtils::GetFramesForArea(mozilla::RelativeTo, nsRect const&, nsTArray<nsIFrame*>&, nsLayoutUtils::FrameForPointOptions const&) /builds/worker/checkouts/gecko/layout/base/nsLayoutUtils.cpp:2571:8
    #16 0x7fffde55601f in nsLayoutUtils::GetFrameForPoint(mozilla::RelativeTo, nsPoint, nsLayoutUtils::FrameForPointOptions const&) /builds/worker/checkouts/gecko/layout/base/nsLayoutUtils.cpp:2512:8
    #17 0x7fffde42d5b2 in mozilla::FindFrameTargetedByInputEvent(mozilla::WidgetGUIEvent*, mozilla::RelativeTo, nsPoint const&, unsigned int) /builds/worker/checkouts/gecko/layout/base/PositionedEventTargeting.cpp:754:22
    #18 0x7fffde475d4e in mozilla::PresShell::EventHandler::GetFrameToHandleNonTouchEvent(AutoWeakFrame&, mozilla::WidgetGUIEvent*) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:8182:7
    #19 0x7fffde46e161 in ComputeEventTargetFrameAndPresShellAtEventPoint /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:8230:7
    #20 0x7fffde46e161 in mozilla::PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret(AutoWeakFrame&, mozilla::WidgetGUIEvent*, nsEventStatus*) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:8409:10
    #21 0x7fffde46da1e in mozilla::PresShell::EventHandler::HandleEvent(AutoWeakFrame&, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:7770:7
    #22 0x7fffde46a5ef in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) /builds/worker/checkouts/gecko/layout/base/PresShell.cpp:7623:23
    #23 0x7fffddb52dcc in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) /builds/worker/checkouts/gecko/view/nsViewManager.cpp:611:18
    #24 0x7fffddb528d9 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) /builds/worker/checkouts/gecko/view/nsView.cpp:983:9
    #25 0x7fffddbec749 in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) /builds/worker/checkouts/gecko/widget/PuppetWidget.cpp:309:37
    #26 0x7fffd5912396 in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) /builds/worker/checkouts/gecko/gfx/layers/apz/util/APZCCallbackHelper.cpp:538:21
    #27 0x7fffdcccce48 in DispatchWidgetEventViaAPZ /builds/worker/checkouts/gecko/dom/ipc/BrowserChild.cpp:1841:10
    #28 0x7fffdcccce48 in mozilla::dom::BrowserChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/checkouts/gecko/dom/ipc/BrowserChild.cpp:1798:3
    #29 0x7fffdccd03a4 in mozilla::dom::BrowserChild::RecvRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/checkouts/gecko/dom/ipc/BrowserChild.cpp:1749:3
    #30 0x7fffdccd05b7 in mozilla::dom::BrowserChild::RecvSynthMouseMoveEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/checkouts/gecko/dom/ipc/BrowserChild.cpp:1707:8
    #31 0x7fffdcec027a in mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PBrowserChild.cpp:5305:80
    #32 0x7fffdcf9c737 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentChild.cpp:8445:32
    #33 0x7fffd483e4b5 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1797:25
    #34 0x7fffd483a9b9 in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, std::unique_ptr<IPC::Message, std::default_delete<IPC::Message>>) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1723:9
    #35 0x7fffd483b7d7 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1512:3
    #36 0x7fffd483cce3 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1614:14
    #37 0x7fffd312092a in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:705:16
    #38 0x7fffd3110878 in mozilla::TaskController::RunTask(mozilla::Task*) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:196:19
    #39 0x7fffd311795d in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:1325:20
    #40 0x7fffd3115498 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:1148:15
    #41 0x7fffd3115ab6 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:641:36
    #42 0x7fffd3130094 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:336:37
    #43 0x7fffd3130094 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_1>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:549:5
    #44 0x7fffd3150f9b in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1157:16
    #45 0x7fffd315be48 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:462:10
    #46 0x7fffd71fc93c in SpinEventLoopUntil<(mozilla::ProcessFailureBehavior)1, (lambda at /builds/worker/checkouts/gecko/dom/base/FuzzingFunctions.cpp:408:22)> /builds/worker/workspace/obj-build/dist/include/mozilla/SpinEventLoopUntil.h:176:25
    #47 0x7fffd71fc93c in mozilla::dom::FuzzingFunctions::SpinEventLoopFor(mozilla::dom::GlobalObject const&, unsigned int) /builds/worker/checkouts/gecko/dom/base/FuzzingFunctions.cpp:407:3
    #48 0x7fffd8ed4704 in mozilla::dom::FuzzingFunctions_Binding::spinEventLoopFor(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/obj-build/dom/bindings/./FuzzingFunctionsBinding.cpp:280:3
    #49 0x7fffe0103237 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:490:13
    #50 0x7fffe0103237 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:586:12
    #51 0x7fffe123c169 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:1695:10
    #52 0x7fff44f8aa03  ([anon:js-executable-memory]+0x2a03)

Comment 1

[Tyson Smith [:tsmith]]

This has also been reported by live site testing.

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/vrUH2bGsj-aetkyrC6JDRw/index.html

Comment 3

[Daniel Holbert [:dholbert]]

As discussed separately, it seems like the issue here is that ~TextRenderedRunIterator() assumes mFrameIterator.Root() is non-null; but we don't have that guarantee.

In this case, Tyson let me know that the testcase hits bug 1467887's assertion in debug builds, which means FrameIfAnonymousChildReflowed( will be returning nullptr in opt builds; and so we'll be passing nullptr to initialize mFrameIterator (as its first arg) here:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1756-1759

explicit TextRenderedRunIterator(SVGTextFrame* aSVGTextFrame,
                                 RenderedRunFilter aFilter = eAllFrames,
                                 const nsIFrame* aSubtree = nullptr)
    : mFrameIterator(FrameIfAnonymousChildReflowed(aSVGTextFrame), aSubtree),

It handles a null mRootFrame nicely internally, e.g. here:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1568-1571

void Init() {
  if (!mRootFrame) {
    return;
  }

So it needs to handle null mRootFrame nicely at destruction-time too.

I propose:
(1) We null-check Root() before using it, in the TextRenderedRunIterator destructor.
(2) Perhaps we we rename TextFrameIterator::Root() and Current() and Next() to have a "Get" prefix, since they return pointers which need to be null-checked (cannot be assumed to be non-null), per our naming convention.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1993379 part 1: Add a null-check to TextRenderedRunIterator and CharIterator destructors. r?jfkthame,longsonr,#layout

mFrameIterator.Root() can be nullptr, as noted in its own Init() method:
https://searchfox.org/firefox-main/rev/dc1c78e9c37aba6ed05a4ec47c4bfcb16f57b51d/layout/svg/SVGTextFrame.cpp#1568-1571

So we need to null-check it before dereferencing it.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1993379 part 2: Add "Get" prefix to various SVGTextFrame iterator methods that can return nullptr. r?jfkthame,longsonr,#layout

This doesn't change behavior; it's just a function rename. All of these
functions are allowed to return nullptr, so we include "Get" in the name as a
clue about that.

Note that some of the callsites do already include a nullcheck; others do not.
Presumably/hopefully those that don't null-check are downstream of earlier
checks that ensure that the pointer is non-null. (Or if they're not, then the
new method-names will give us a better clue about the need for a null check, if
we find ways to trigger a null-deref.)

Comment 6

[Bugmon [:jkratzer for issues]]

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

Start: a099475af71c851f2352f87c9a380b9ed156a392 (20251004194052)
End: f46e72e667a10cfae64d9e5156d5ae3e54b7070e (20251005001655)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a099475af71c851f2352f87c9a380b9ed156a392&tochange=f46e72e667a10cfae64d9e5156d5ae3e54b7070e

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/35205629af47
https://hg.mozilla.org/integration/autoland/rev/ed39bb7b27a2
part 1: Add a null-check to TextRenderedRunIterator and CharIterator destructors. r=longsonr,firefox-svg-reviewers
https://github.com/mozilla-firefox/firefox/commit/059eda0de09f
https://hg.mozilla.org/integration/autoland/rev/43c357deb8ff
part 2: Add "Get" prefix to various SVGTextFrame iterator methods that can return nullptr. r=longsonr,firefox-svg-reviewers

Comment 8

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

Setting Regressed by field after analyzing regression range found by bugmon in comment #6.

Comment 9

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

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

Comment 10

[Daniel Holbert [:dholbert]]

As I noted in phab, the attached testcase doesn't trigger the crash for me (nor does it trigger bug 1467887's assertion for me).

But the testcase that Tyson just posted in bug 1467887 comment 6 does trigger the crash for me (in an opt build[1] without this bug's patch).

[1] In a debug build, that testcase triggers bug 1467887's assertion; but if I comment out that assertion, then it hits this crash here, if I revert this bug's patch.

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/ed39bb7b27a2
https://hg.mozilla.org/mozilla-central/rev/43c357deb8ff