Closed Bug 1872410 Opened 1 year ago Closed 1 year ago

crash near null in [@ FindLineContaining]

Categories

(Core :: DOM: Selection, defect, P2)

Product:
Core
Component:
DOM: Selection
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
123 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox121 --- unaffected
firefox122 + fixed
firefox123 + verified

People

(Reporter: tsmith, Assigned: masayuki)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [bugmon:bisected,confirmed], [wptsync upstream])

Attachments

(2 files)

testcase.html
348 bytes, text/html
Details
Bug 1872410 - Make `nsTableRowGroupFrame::FindLineContaining` allow callers to set aFrame to an out-of-flow child r=emilio!
48 bytes, text/x-phabricator-request
dmeehan
: approval-mozilla-beta+
Details | Review
Attached file testcase.html —

Found while fuzzing m-c 20231228-10aa74237898 (--enable-address-sanitizer --enable-fuzzing)

To reproduce via Grizzly Replay:

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

==231349==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fac1b19fbf8 bp 0x7ffc818b3850 sp 0x7ffc818b3830 T0)
==231349==The signal is caused by a READ memory access.
==231349==Hint: address points to the zero page.
    #0 0x7fac1b19fbf8 in FindLineContaining /builds/worker/checkouts/gecko/layout/tables/nsTableRowGroupFrame.cpp:1680:39
    #1 0x7fac1b19fbf8 in non-virtual thunk to nsTableRowGroupFrame::FindLineContaining(nsIFrame*, int) /builds/worker/checkouts/gecko/layout/tables/nsTableRowGroupFrame.cpp
    #2 0x7fac1ad952d5 in nsFrameList::GetPrevVisualFor(nsIFrame*) const /builds/worker/checkouts/gecko/layout/generic/nsFrameList.cpp:279:22
    #3 0x7fac1ab60e19 in nsFrameIterator::GetPrevSibling(nsIFrame*) /builds/worker/checkouts/gecko/layout/base/nsFrameTraversal.cpp:241:14
    #4 0x7fac1ab60b2d in nsFrameIterator::Prev() /builds/worker/checkouts/gecko/layout/base/nsFrameTraversal.cpp:141:16
    #5 0x7fac1aeeb29b in Traverse /builds/worker/checkouts/gecko/layout/base/nsFrameTraversal.h:30:7
    #6 0x7fac1aeeb29b in nsIFrame::GetFrameFromDirection(nsDirection, mozilla::EnumSet<mozilla::PeekOffsetOption, unsigned short> const&) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:9676:36
    #7 0x7fac1aee3bbc in operator() /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:9078:30
    #8 0x7fac1aee3bbc in nsIFrame::PeekOffsetForWord(mozilla::PeekOffsetStruct*, int) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:9071:33
    #9 0x7fac1aee92a7 in nsIFrame::PeekOffset(mozilla::PeekOffsetStruct*) /builds/worker/checkouts/gecko/layout/generic/nsIFrame.cpp:9373:14
    #10 0x7fac1ac776c4 in mozilla::SelectionMovementUtils::PeekOffsetForCaretMove(nsIContent*, unsigned int, nsDirection, mozilla::CaretAssociationHint, mozilla::intl::BidiEmbeddingLevel, nsSelectionAmount, nsPoint const&, mozilla::EnumSet<mozilla::PeekOffsetOption, unsigned short>) /builds/worker/checkouts/gecko/layout/generic/SelectionMovementUtils.cpp:55:39
    #11 0x7fac1ad9baab in nsFrameSelection::PeekOffsetForCaretMove(nsDirection, bool, nsSelectionAmount, nsFrameSelection::CaretMovementStyle, nsPoint const&) const /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:944:10
    #12 0x7fac1ad99ee7 in nsFrameSelection::MoveCaret(nsDirection, bool, nsSelectionAmount, nsFrameSelection::CaretMovementStyle) /builds/worker/checkouts/gecko/layout/generic/nsFrameSelection.cpp:812:47
    #13 0x7fac12e57fcd in mozilla::dom::Selection::Modify(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/Selection.cpp:3720:24
    #14 0x7fac140654e2 in mozilla::dom::Selection_Binding::modify(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./SelectionBinding.cpp:1073:24
    #15 0x7fac152de144 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:3258:13
    #16 0x7fac1f5b39b5 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:479:13
    #17 0x7fac1f5b39b5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:573:12
    #18 0x7fac1f5d88b0 in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:640:10
    #19 0x7fac1f5d88b0 in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:645:10
    #20 0x7fac1f5d88b0 in js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3060:16
    #21 0x7fac1f5b2737 in MaybeEnterInterpreterTrampoline /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:393:10
    #22 0x7fac1f5b2737 in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:451:13
    #23 0x7fac1f5b3b1e in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:605:13
    #24 0x7fac1f5b5aa6 in InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:640:10
    #25 0x7fac1f5b5aa6 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:672:8
    #26 0x7fac1f72cd2b in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:119:10
    #27 0x7fac14d25c42 in mozilla::dom::EventHandlerNonNull::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/./EventHandlerBinding.cpp:65:37
    #28 0x7fac1618b322 in void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget>>(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/EventHandlerBinding.h:82:12
    #29 0x7fac16188d75 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /builds/worker/checkouts/gecko/dom/events/JSEventHandler.cpp:199:12
    #30 0x7fac16133f49 in mozilla::EventListenerManager::HandleEventSingleListener(mozilla::EventListenerManager::Listener*, nsAtom*, mozilla::WidgetEvent*, mozilla::dom::Event*, mozilla::dom::EventTarget*, bool) /builds/worker/checkouts/gecko/dom/events/EventListenerManager.cpp:1348:22
    #31 0x7fac16136b5a in mozilla::EventListenerManager::HandleEventWithListenerArray(mozilla::EventListenerManager::ListenerArray*, nsAtom*, mozilla::EventMessage, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, bool) /builds/worker/checkouts/gecko/dom/events/EventListenerManager.cpp:1663:12
    #32 0x7fac16135596 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /builds/worker/checkouts/gecko/dom/events/EventListenerManager.cpp:1560:35
    #33 0x7fac16118952 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /builds/worker/checkouts/gecko/dom/events/EventDispatcher.cpp:364:17
    #34 0x7fac16115e58 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /builds/worker/checkouts/gecko/dom/events/EventDispatcher.cpp:611:18
    #35 0x7fac1611d91a in mozilla::EventDispatcher::Dispatch(mozilla::dom::EventTarget*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /builds/worker/checkouts/gecko/dom/events/EventDispatcher.cpp:1232:11
    #36 0x7fac16126958 in mozilla::EventDispatcher::DispatchDOMEvent(mozilla::dom::EventTarget*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) /builds/worker/checkouts/gecko/dom/events/EventDispatcher.cpp
    #37 0x7fac13047c7b in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/base/nsINode.cpp:1404:17
    #38 0x7fac127134e5 in nsContentUtils::DispatchEvent(mozilla::dom::Document*, mozilla::dom::EventTarget*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch) /builds/worker/checkouts/gecko/dom/base/nsContentUtils.cpp:4756:29
    #39 0x7fac12713194 in nsContentUtils::DispatchTrustedEvent(mozilla::dom::Document*, mozilla::dom::EventTarget*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, bool*) /builds/worker/checkouts/gecko/dom/base/nsContentUtils.cpp:4722:10
    #40 0x7fac179fa044 in mozilla::dom::nsSourceErrorEventRunner::Run() /builds/worker/checkouts/gecko/dom/media/utils/MediaElementEventRunners.cpp:96:10
    #41 0x7fac0ec9833a in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:568:16
    #42 0x7fac0ec7e1bb in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:895:26
    #43 0x7fac0ec7ad98 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:718:15
    #44 0x7fac0ec7b499 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:504:36
    #45 0x7fac0eca0431 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:222:37
    #46 0x7fac0eca0431 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
    #47 0x7fac0ecc83af in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1199:16
    #48 0x7fac0ecd60ea in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
    #49 0x7fac1095ec8e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
    #50 0x7fac10786f8a in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #51 0x7fac10786f8a in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #52 0x7fac10786f8a in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #53 0x7fac1a0cc619 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
    #54 0x7fac1a2d1012 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:470:33
    #55 0x7fac1f1608de in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:721:20
    #56 0x7fac10786f8a in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #57 0x7fac10786f8a in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #58 0x7fac10786f8a in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #59 0x7fac1f15fe83 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:656:34
    #60 0x55dd81599d9c in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #61 0x55dd81599d9c in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:375:18
    #62 0x7fac36629d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #63 0x7fac36629e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #64 0x55dd814be0a8 in _start (/home/user/workspace/browsers/m-c-20231229205714-fuzzing-asan-opt/firefox+0xdc0a8) (BuildId: eaf06add5484a8c1b6944c1826727fe419512961)
Flags: in-testsuite?

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

Start: 7a6f17636cf1b0a6bc8afd1eeffe26cfbc03c29d (20231121054432)
End: fddad0a59b2d028b2147493bf9ba8cc953d9e3eb (20231121083833)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7a6f17636cf1b0a6bc8afd1eeffe26cfbc03c29d&tochange=fddad0a59b2d028b2147493bf9ba8cc953d9e3eb

Keywords: regression
Whiteboard: [bugmon:bisected,confirmed]
Regressed by: 1820290

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

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

For more information, please visit BugBot documentation.

status-firefox121: --- → unaffected
status-firefox122: --- → affected
status-firefox-esr115: --- → unaffected
Flags: needinfo?(masayuki)

:hsinyi could this be triaged for severity?
Next week is the final week of beta, maybe Masayuki is on PTO. Wondering if there's a user impact and we need a fix for Fx122

Flags: needinfo?(htsai)

I'll take a look this first because of actual crash bug.

Assignee: nobody → masayuki
Severity: -- → S2
Status: NEW → ASSIGNED
Flags: needinfo?(masayuki)
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All

I'm still not sure how this crash is caused by the change of bug 1820290 though...

In that case, nsIFrame::GetFrameFromDirection starts scanning from nsTextFrame containing "1". The frame ancestors are:

  • nsTableFrame
    • nsTableRowGroupFrame
      • nsBlockFrame
        • nsTextFrame

Then, first nsFrameIterator::GetPrevSibling() call in nsFrameIterator::Prev() returns nullptr because no previous sibling of the text frame. Then, second call of it with nsBlockFrame hits the crash. During the second call, nsFrameList::GetPrevVisualFor() is called by here. aFrame is the nsBlockFrame and the mFirstChild is nsTableRowFrame. They have same nsTableRowGroupFrame as parent. Then, the line iterator is the nsTableRowGroupFrame itself. Finally, nsTableRowGroupFrame::FindLineContaining() is called with the nsBlockFrame. Therefore, the query frame returns nullptr and crash.

So I believe that this is not a bug of nsFrameIterator, but a bug of nsFrameList::GetPrevVisualFor (and nsFrameList::GetNextVisualFor).

What do you think, Emilio? And which part should I fix?

Flags: needinfo?(emilio)

FYI: nsTableRowGroupFrame has the nsTableRowFrame as a child instead of nsBlockFrame. nsBlockFrame does not have siblings.

Oh, nsTableRowGroupFrame has nsTableRowFrame, nsTableRowFrame has nsTableCellFrame, nsTableCellFrame has nsBlockFrame for the <dialog>, and the nsBlockFrame has nsPlaceholderFrame whose real frame is nsBlockFrame for the container of the nsTextFrame... Really tricky structure.

Flags: needinfo?(htsai)

Fixing nsTableRowGroupFrame::FindLineContaining sounds good, but I'm not sure the behavior you're implementing in comment 8 matches what nsBlockFrame does, which is IIUC just returning -1 for out of flows, unless I'm missing something.

I think nsTableRowGroupFrame::FindLineContaining should just return -1 instead of crashing if what you give it is not a row frame.

Flags: needinfo?(emilio)

Tracking since this was triaged as an S2

tracking-firefox122: --- → +
tracking-firefox123: --- → +

If the table-row-group is styled by CSS and it does not have proper table row
children, anonymous table rows and cells are generated and the real content is
rendered in a placeholder. In this case, the parent of out-of-flow child frame
is the table-row-group frame even though the table-row-group frame has only
anonymous table-row child frame. Therefore, it fails to query nsTableRowFrame
and access nsIFrame methods with nullptr.

This patch allows the method users to set an out-of-flow child frame and
makes the method return "not found".

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/5018a6e4e125 Make `nsTableRowGroupFrame::FindLineContaining` allow callers to set aFrame to an out-of-flow child r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/43913 for changes under testing/web-platform/tests
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/5018a6e4e125

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox123: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 123 Branch

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

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox122 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(masayuki)

Comment on attachment 9371667 [details]
Bug 1872410 - Make nsTableRowGroupFrame::FindLineContaining allow callers to set aFrame to an out-of-flow child r=emilio!

Beta/Release Uplift Approval Request

  • User impact if declined: user may meet the crash if web apps use display: table-row-group for elements whose children are not styled with display: table-row.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Only adding nullptr check into the crash point.
  • String changes made/needed:
  • Is Android affected?: Yes
Flags: needinfo?(masayuki)
Attachment #9371667 - Flags: approval-mozilla-beta?
Upstream PR merged by moz-wptsync-bot

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

Status: RESOLVED → VERIFIED
status-firefox123: fixed → verified
Keywords: bugmon

Comment on attachment 9371667 [details]
Bug 1872410 - Make nsTableRowGroupFrame::FindLineContaining allow callers to set aFrame to an out-of-flow child r=emilio!

Approved for 122.0b9

Attachment #9371667 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: