1925331 - crash near null in [@ mozilla::HTMLEditor::EnsureNoFollowingUnnecessaryLineBreak]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found while fuzzing 20241016-462f654f2467 (--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>

==114136==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000001c (pc 0x792693b6bae2 bp 0x7ffec2fc64f0 sp 0x7ffec2fc63a0 T0)
==114136==The signal is caused by a READ memory access.
==114136==Hint: address points to the zero page.
    #0 0x792693b6bae2 in GetBoolFlag /gecko/dom/base/nsINode.h:2016:12
    #1 0x792693b6bae2 in IsContent /gecko/dom/base/nsINode.h:2026:35
    #2 0x792693b6bae2 in FromNode<const nsCOMPtr<nsINode> &> /gecko/dom/base/nsIContent.h:89:3
    #3 0x792693b6bae2 in ContainerAs<nsIContent> /builds/worker/workspace/obj-build/dist/include/mozilla/EditorDOMPoint.h:214:5
    #4 0x792693b6bae2 in mozilla::HTMLEditor::EnsureNoFollowingUnnecessaryLineBreak(mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent>> const&, mozilla::dom::Element const&) /gecko/editor/libeditor/HTMLEditor.cpp:4388:34
    #5 0x792693c4b1c7 in mozilla::HTMLEditor::AutoDeleteRangesHandler::HandleDeleteCollapsedSelectionAtWhiteSpaces(mozilla::HTMLEditor&, short, mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>, nsCOMPtr<nsIContent>> const&, mozilla::dom::Element const&) /gecko/editor/libeditor/HTMLEditorDeleteHandler.cpp:2303:31
    #6 0x792693c43a45 in mozilla::HTMLEditor::AutoDeleteRangesHandler::HandleDeleteAroundCollapsedRanges(mozilla::HTMLEditor&, short, short, mozilla::AutoRangeArray&, mozilla::WSRunScanner const&, mozilla::WSScanResult const&, mozilla::dom::Element const&) /gecko/editor/libeditor/HTMLEditorDeleteHandler.cpp:2020:9
    #7 0x792693c3df73 in mozilla::HTMLEditor::AutoDeleteRangesHandler::Run(mozilla::HTMLEditor&, short, short, mozilla::AutoRangeArray&, mozilla::dom::Element const&) /gecko/editor/libeditor/HTMLEditorDeleteHandler.cpp:1836:11
    #8 0x792693c3c6b1 in mozilla::HTMLEditor::HandleDeleteSelection(short, short) /gecko/editor/libeditor/HTMLEditorDeleteHandler.cpp:1306:61
    #9 0x792693b012b7 in mozilla::EditorBase::DeleteSelectionAsSubAction(short, short) /gecko/editor/libeditor/EditorBase.cpp:4573:9
    #10 0x792693af816e in mozilla::EditorBase::DeleteSelectionAsAction(short, short, nsIPrincipal*) /gecko/editor/libeditor/EditorBase.cpp:4536:8
    #11 0x79268d23a3e9 in mozilla::dom::Document::ExecCommand(nsTSubstring<char16_t> const&, bool, nsTSubstring<char16_t> const&, nsIPrincipal&, mozilla::ErrorResult&) /gecko/dom/base/Document.cpp:5610:37
    #12 0x79268eb74472 in mozilla::dom::Document_Binding::execCommand(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/./DocumentBinding.cpp:4160:36
    #13 0x79268efd7c94 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /gecko/dom/bindings/BindingUtils.cpp:3266:13
    #14 0x792695969d24 in CallJSNative /gecko/js/src/vm/Interpreter.cpp:527:13
    #15 0x792695969d24 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /gecko/js/src/vm/Interpreter.cpp:623:12
    #16 0x792696907620 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /gecko/js/src/jit/BaselineIC.cpp:1677:10

Comment 1

[Tyson Smith [:tsmith]]

The test case also triggers the following assertion in a debug build: Assertion failure: aNextOrAfterModifiedPoint.IsInContentNode()

Comment 2

[Bugmon [:jkratzer for issues]]

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

Start: 11ab9645846ff996343f1c5bdb0c65165724a1b6 (20241016095056)
End: af719c3f4b8acbca58ebfb0ab2e933ce7565c39b (20241016082039)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=11ab9645846ff996343f1c5bdb0c65165724a1b6&tochange=af719c3f4b8acbca58ebfb0ab2e933ce7565c39b

Comment 3

[Mayank Bansal]

Got a crash from the testcase on the latest Nightly: https://crash-stats.mozilla.org/report/index/0ab21bb6-0a3a-4ab7-a751-735700241018

Bisection:
Bug 1923251 - part 2: Make HTMLEditor clean up unnecessary padding line breaks when deleting something immediately before it r=m_kato
Differential Revision: https://phabricator.services.mozilla.com/D225039

Comment 4

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

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1925331 - Make the delete handlers of `HTMLEditor` guarantee that the point is in content node at calling `EnsureNoFollowingUnnecessaryLineBreak` r=m_kato!

The point may not be set if the selection move is prevented or not suggested.
Therefore, it should check the caret position before calling
EnsureNoFollowingUnnecessaryLineBreak. On the other hand,
HandleDeleteCollapsedSelectionAtWhiteSpaces should use Selection instead
if caret position is not suggested. It should be handled in bug 1925424.

Comment 6

[Bugmon [:jkratzer for issues]]

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

Start: 11ab9645846ff996343f1c5bdb0c65165724a1b6 (20241016095056)
End: af719c3f4b8acbca58ebfb0ab2e933ce7565c39b (20241016082039)
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=11ab9645846ff996343f1c5bdb0c65165724a1b6&tochange=af719c3f4b8acbca58ebfb0ab2e933ce7565c39b

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00271a7486f0
Make the delete handlers of `HTMLEditor` guarantee that the point is in content node at calling `EnsureNoFollowingUnnecessaryLineBreak` r=m_kato

Comment 8

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

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

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/00271a7486f0

Comment 10

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

Upstream PR merged by moz-wptsync-bot

Comment 11

[Bugmon [:jkratzer for issues]]

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

Comment 12

[Daniel Holbert [:dholbert]]

We backed out this bug's patch as well as the regressor's patch (bug 1923251), to investigate another regression from that bug. (See bug 1923251 comment 23 and the few comments before that.)

Even though this bug's own patch was backed out, I think we can still leave this bug closed, because (as noted above) the regressor was backed-out, so this should still not be reproducible on trunk.

When bug 1923251 re-lands, it'll be worth re-validating that this is still fixed (i.e. that we re-land this bug's patch as part of re-landing the patch for bug 1923251.)

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yep, the test is still in the tree. So, relanded patches will be tested this automatically.