Closed Bug 1897255 Opened 5 months ago Closed 2 months ago

crash near null in [@ nsINode::Length]

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox-esr115 --- unaffected
firefox-esr128 --- fixed
firefox126 --- wontfix
firefox127 --- wontfix
firefox128 --- wontfix
firefox129 --- fixed
firefox130 --- verified

People

(Reporter: tsmith, Assigned: masayuki)

References

(Blocks 1 open bug, Regression)

Details

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

Crash Data

Attachments

(2 files)

testcase.html
487 bytes, text/html
Details
Bug 1897255 - Make `nsIFrame::PeekOffsetForLine` won't cross editing host boundary unless the callers allows that r=emilio!
48 bytes, text/x-phabricator-request
dmeehan
: approval-mozilla-beta+
dmeehan
: approval-mozilla-esr128+
Details | Review
Attached file testcase.html —

Found while fuzzing m-c 20240422-d4a6129fdbbe (--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> --repeat 10

==54169==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x7e79ed81d289 bp 0x7ffc01a070c0 sp 0x7ffc01a070b0 T0)
==54169==The signal is caused by a READ memory access.
==54169==Hint: address points to the zero page.
    #0 0x7e79ed81d289 in get /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:314:27
    #1 0x7e79ed81d289 in operator-> /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:344:12
    #2 0x7e79ed81d289 in NodeType /builds/worker/checkouts/gecko/dom/base/nsINode.h:808:38
    #3 0x7e79ed81d289 in nsINode::Length() const /builds/worker/checkouts/gecko/dom/base/nsINode.cpp:3045:11
    #4 0x7e79f3c97c29 in mozilla::TextEditor::ComputeValueFromTextNodeAndBRElement(nsTSubstring<char16_t>&) const /builds/worker/checkouts/gecko/editor/libeditor/TextEditSubActionHandler.cpp:706:18
    #5 0x7e79f3a4dac1 in mozilla::EditorBase::ComputeValueInternal(nsTSubstring<char16_t> const&, unsigned int, nsTSubstring<char16_t>&) const /builds/worker/checkouts/gecko/editor/libeditor/EditorBase.cpp:1430:27
    #6 0x7e79f0633f14 in mozilla::TextEditor::ComputeTextValue(unsigned int, nsTSubstring<char16_t>&) const /builds/worker/workspace/obj-build/dist/include/mozilla/TextEditor.h:197:19
    #7 0x7e79f05fd781 in mozilla::TextControlState::GetValue(nsTSubstring<char16_t>&, bool, bool) const /builds/worker/checkouts/gecko/dom/html/TextControlState.cpp:2537:45
    #8 0x7e79f79ec295 in CollectTextAreaElement /builds/worker/checkouts/gecko/toolkit/components/sessionstore/SessionStoreUtils.cpp:640:15
    #9 0x7e79f79ec295 in mozilla::dom::SessionStoreUtils::CollectFormData(mozilla::dom::Document*, mozilla::dom::sessionstore::FormData&) /builds/worker/checkouts/gecko/toolkit/components/sessionstore/SessionStoreUtils.cpp:860:11
    #10 0x7e79f79cf559 in CollectFormData /builds/worker/checkouts/gecko/toolkit/components/sessionstore/SessionStoreChangeListener.cpp:163:19
    #11 0x7e79f79cf559 in mozilla::dom::SessionStoreChangeListener::FlushSessionStore() /builds/worker/checkouts/gecko/toolkit/components/sessionstore/SessionStoreChangeListener.cpp:249:7
    #12 0x7e79f79f9efd in RecvFlushTabState /builds/worker/checkouts/gecko/toolkit/components/sessionstore/SessionStoreChild.cpp:212:34
    #13 0x7e79f79f9efd in mozilla::dom::PSessionStoreChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PSessionStoreChild.cpp:255:85
    #14 0x7e79f2a6d5ef in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/obj-build/ipc/ipdl/PContentChild.cpp:8047:32
    #15 0x7e79eb65e785 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1820:25
    #16 0x7e79eb65a57f in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::UniquePtr<IPC::Message, mozilla::DefaultDelete<IPC::Message>>) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1739:9
    #17 0x7e79eb65b651 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1530:3
    #18 0x7e79eb65cba3 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/checkouts/gecko/ipc/glue/MessageChannel.cpp:1630:14
    #19 0x7e79e9d3b49a in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:580:16
    #20 0x7e79e9d26b4d in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:907:26
    #21 0x7e79e9d24128 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:730:15
    #22 0x7e79e9d24746 in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:516:36
    #23 0x7e79e9d42b11 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:234:37
    #24 0x7e79e9d42b11 in mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.h:548:5
    #25 0x7e79e9d65734 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1199:16
    #26 0x7e79e9d70ca8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
    #27 0x7e79eb66710e in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
    #28 0x7e79eb4b3a44 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #29 0x7e79eb4b3a44 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #30 0x7e79eb4b3a44 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #31 0x7e79f37b1849 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
    #32 0x7e79f396c31a in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:469:33
    #33 0x7e79f7cdbe6d in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:712:20
    #34 0x7e79eb4b3a44 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:370:10
    #35 0x7e79eb4b3a44 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
    #36 0x7e79eb4b3a44 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
    #37 0x7e79f7cdb455 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:647:34
    #38 0x5aa805df1a60 in content_process_main /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:57:28
    #39 0x5aa805df1a60 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:378:18
    #40 0x7e7a0d629d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #41 0x7e7a0d629e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #42 0x5aa805d18898 in _start (/home/user/workspace/browsers/m-c-20240516162155-fuzzing-asan-opt/firefox+0xda898) (BuildId: 551ab009871d99a9901563226a9b870582fe6d16)
Flags: in-testsuite?

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

Start: de8a5d43ace14a69cfb8731ddc57b1bc45d24381 (20240403065227)
End: 8c92b17c7b2805a83d5a80fa103b82ae132d8320 (20240403080033)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=de8a5d43ace14a69cfb8731ddc57b1bc45d24381&tochange=8c92b17c7b2805a83d5a80fa103b82ae132d8320

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

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

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

For more information, please visit BugBot documentation.

status-firefox126: --- → affected
status-firefox127: --- → affected
status-firefox-esr115: --- → unaffected
Flags: needinfo?(masayuki)

Sounds like a hidden regression of bug 1713334.

Flags: needinfo?(masayuki)
See Also: → 1713334
Severity: -- → S3
Flags: needinfo?(krosylight)

It's not immediately clear to me how this is related to bug 1713334, and I have been away from this that I'm not sure I'm still the right person to investigate 😬

Flags: needinfo?(krosylight)

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #4)

It's not immediately clear to me how this is related to bug 1713334, and I have been away from this that I'm not sure I'm still the right person to investigate 😬

Because of no text node in the anonymous <div>. It seems that there is a path failing to create the text node at restore/reload.

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

Keywords: pernosco-wanted → pernosco

Hmm, here assumes that <textarea> has already initialized its TextEditor. However, looks like it may have not been so yet.

Keywords: pernosco → pernosco-wanted

I guess TextEditor should call EnsureEmptyTextFirstChild from the constructor.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All

Err, no, it's not enough. The text node needs to store the value.

A pernosco session for this bug can be found here.

Oh, I was wrong. The command oddly applies to the text node under the anonymous <div>. So, one of the root causes is in the command handler. The other is in the selection mover.

Oh? I got different stack, they must be caused by same root cause, though.

Assertion failure: aPoint.GetContainer()->IsInclusiveFlatTreeDescendantOf(&aEditingHost), at M:/src/editor/libeditor/HTMLEditUtils.cpp:2301
#01: mozilla::HTMLEditUtils::GetBetterCaretPositionToInsertText<mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>,nsCOMPtr<nsIContent> >,mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>,nsCOMPtr<nsIContent> > > (M:\src\editor\libeditor\HTMLEditUtils.cpp:2300)
#02: mozilla::HTMLEditor::AutoInlineStyleSetter::GetEmptyTextNodeToApplyNewStyle (M:\src\editor\libeditor\HTMLStyleEditor.cpp:558)
#03: mozilla::HTMLEditor::SetInlinePropertiesAroundRanges<1> (M:\src\editor\libeditor\HTMLStyleEditor.cpp:369)
#04: mozilla::HTMLEditor::SetInlinePropertiesAsSubAction<1> (M:\src\editor\libeditor\HTMLStyleEditor.cpp:287)
#05: mozilla::HTMLEditor::SetInlinePropertyAsAction (M:\src\editor\libeditor\HTMLStyleEditor.cpp:181)
#06: mozilla::HighlightColorStateCommand::SetState (M:\src\editor\libeditor\HTMLEditorCommands.cpp:814)
#07: mozilla::MultiStateCommandBase::DoCommandParam (M:\src\editor\libeditor\HTMLEditorCommands.cpp:492)
#08: mozilla::dom::Document::AutoEditorCommandTarget::DoCommandParam<nsTAutoStringN<char16_t,64> > (M:\src\dom\base\Document.cpp:5323)
#09: mozilla::dom::Document::ExecCommand (M:\src\dom\base\Document.cpp:5529)
#10: mozilla::dom::Document_Binding::execCommand (M:\fx64-dbg\dom\bindings\DocumentBinding.cpp:3974)
#11: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions> (M:\src\dom\bindings\BindingUtils.cpp:3270)
#12: CallJSNative (M:\src\js\src\vm\Interpreter.cpp:481)
#13: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:575)
#14: js::Interpret (M:\src\js\src\vm\Interpreter.cpp:3176)
#15: js::RunScript (M:\src\js\src\vm\Interpreter.cpp:453)
#16: js::InternalCallOrConstruct (M:\src\js\src\vm\Interpreter.cpp:607)
#17: js::Call (M:\src\js\src\vm\Interpreter.cpp:674)
#18: JS::Call (M:\src\js\src\vm\CallAndConstruct.cpp:119)
#19: mozilla::dom::EventHandlerNonNull::Call (M:\fx64-dbg\dom\bindings\EventHandlerBinding.cpp:65)
#20: mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> > (M:\fx64-dbg\dist\include\mozilla\dom\EventHandlerBinding.h:82)
#21: mozilla::JSEventHandler::HandleEvent (M:\src\dom\events\JSEventHandler.cpp:200)
#22: mozilla::EventListenerManager::HandleEventSingleListener (M:\src\dom\events\EventListenerManager.cpp:1335)
#23: mozilla::EventListenerManager::HandleEventWithListenerArray (M:\src\dom\events\EventListenerManager.cpp:1653)
#24: mozilla::EventListenerManager::HandleEventInternal (M:\src\dom\events\EventListenerManager.cpp:1550)
#25: mozilla::EventTargetChainItem::HandleEvent (M:\src\dom\events\EventDispatcher.cpp:368)
#26: mozilla::EventTargetChainItem::HandleEventTargetChain (M:\src\dom\events\EventDispatcher.cpp:645)
#27: mozilla::EventDispatcher::Dispatch (M:\src\dom\events\EventDispatcher.cpp:1221)
#28: FocusInOutEvent::Run (M:\src\dom\base\nsFocusManager.cpp:2828)
#29: nsContentUtils::AddScriptRunner (M:\src\dom\base\nsContentUtils.cpp:6259)
#30: nsContentUtils::AddScriptRunner (M:\src\dom\base\nsContentUtils.cpp:6265)
#31: nsFocusManager::FireFocusOrBlurEvent (M:\src\dom\base\nsFocusManager.cpp:2947)
#32: nsFocusManager::SendFocusOrBlurEvent (M:\src\dom\base\nsFocusManager.cpp:2901)
#33: nsFocusManager::WindowHidden (M:\src\dom\base\nsFocusManager.cpp:1092)
#34: nsGlobalWindowInner::PageHidden (M:\src\dom\base\nsGlobalWindowInner.cpp:4468)
#35: nsDocumentViewer::PageHide (M:\src\layout\base\nsDocumentViewer.cpp:1379)
#36: nsDocShell::FirePageHideNotificationInternal (M:\src\docshell\base\nsDocShell.cpp:1224)
#37: nsDocShell::CreateDocumentViewer (M:\src\docshell\base\nsDocShell.cpp:7672)

The problems in the testcase are, nsIFrame::PeekOffsetForLine returns a frame
for any content node outside the editing host and nsIFrame::GetLastLeaf
returns a native anonymous subtree node if the frame is for the native
anonymous subtree root like <input> or <textarea>.

For fixing the former, the methods need to check whether found frame's editable
state and whether the editable node is an inclusive descendant of the
editing host. However, there are complicated cases with inline editing hosts
and elements whose contenteditable is set to false and this is a new
regression for ESR 128. Therefore, we need to fix this without any behavior
changes as far as possible. Therefore, this patch basically checks only whether
the editing state of found one is editable and/or whether the one is an
inclusive descendant of the editing host to avoid Selection moves outside the
editing host. The reaming complicated cases should be handled in new bugs which
blocks bug 1873155.

For fixing the latter, nsIFrame::GetLastLeaf needs to check the given frame's
content is a native anonymous subtree root or not.

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/e90eb44c2da3 Make `nsIFrame::PeekOffsetForLine` won't cross editing host boundary unless the callers allows that r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/47202 for changes under testing/web-platform/tests
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed], [wptsync upstream]

https://hg.mozilla.org/mozilla-central/rev/e90eb44c2da3

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
status-firefox130: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 130 Branch

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

status-firefox129: --- → affected
Upstream PR merged by moz-wptsync-bot
Upstream PR merged by moz-wptsync-bot
status-firefox128: fix-optional → wontfix
status-firefox-esr128: --- → fix-optional
Flags: in-testsuite? → in-testsuite+

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

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

Comment on attachment 9412420 [details]
Bug 1897255 - Make nsIFrame::PeekOffsetForLine won't cross editing host boundary unless the callers allows that r=emilio!

Beta/Release Uplift Approval Request

  • User impact if declined: The testcase indicates that HTMLEditor unexpectedly modifies the native anonymous subtree in <input> and <textarea>. So, TextEditor will hit various unexpected situations after that happen.
  • 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): I wrote this patch as not changing major behavior when user presses ArrowUp/ArrowDown key to make uplifting this safe. Then, Selection won't be moved into the native anonymous subtree outside focused editing host.
  • String changes made/needed: no
  • Is Android affected?: Yes
Attachment #9412420 - Flags: approval-mozilla-beta?

Comment on attachment 9412420 [details]
Bug 1897255 - Make nsIFrame::PeekOffsetForLine won't cross editing host boundary unless the callers allows that r=emilio!

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a new regression from ESR user point of view.
  • User impact if declined: HTMLEditor may modify the native anonymous subtree of <input> and <textarea> after moving selection into them from a focused editing host if web apps try to execute some commands. That would cause unexpected DOM tree for TextEditor. Therefore, TextEditor will meet various unexpected cases.
  • Fix Landed on Version: 130
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): I wrote this patch as not changing the most behavior when user presses ArrowUp and ArrowDown which causes moving Selection into the native anonymous subtree from an editing host as far as possible.
Attachment #9412420 - Flags: approval-mozilla-esr128?

Comment on attachment 9412420 [details]
Bug 1897255 - Make nsIFrame::PeekOffsetForLine won't cross editing host boundary unless the callers allows that r=emilio!

Approved for 129.0b8

Attachment #9412420 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9412420 [details]
Bug 1897255 - Make nsIFrame::PeekOffsetForLine won't cross editing host boundary unless the callers allows that r=emilio!

Approved for 128.1esr.

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

Attachment

General

Creator:
Created:
Updated:
Size: