1916081 - Crash in [@ mozilla::EditorBase::GetDocument]

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/792cac96-184a-48ef-a3e1-5f05a0240831

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames:

0  libxul.so  RefPtr<mozilla::dom::Document>::get() const  mfbt/RefPtr.h:314
0  libxul.so  RefPtr<mozilla::dom::Document>::operator mozilla::dom::Document*() const&  mfbt/RefPtr.h:327
0  libxul.so  mozilla::EditorBase::GetDocument() const  editor/libeditor/EditorBase.h:142
0  libxul.so  mozilla::HTMLEditor::OurWindowHasFocus() const  editor/libeditor/HTMLEditor.cpp:7176
0  libxul.so  mozilla::HTMLEditor::FocusedElementOrDocumentBecomesNotEditable(mozilla::HTML...  editor/libeditor/HTMLEditor.cpp:866
1  libxul.so  mozilla::dom::Document::EditingStateChanged()  dom/base/Document.cpp:6149
2  libxul.so  mozilla::dom::Document::DeferredContentEditableCountChange(mozilla::dom::Elem...  dom/base/Document.cpp:6469
2  libxul.so  mozilla::dom::DeferredContentEditableCountChangeEvent::Run()  dom/base/Document.cpp:6408
3  libxul.so  nsContentUtils::RemoveScriptBlocker()  dom/base/nsContentUtils.cpp:6254
3  libxul.so  mozilla::dom::Document::EndUpdate()  dom/base/Document.cpp:8309

Null deref.

Comment 1

[Andrew McCreight [:mccr8]]

Mostly, but maybe not entirely, on Android.

Comment 2

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Comment 3

[Tyson Smith [:tsmith]]

We are seeing this on Windows and Linux as well.

Comment 4

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

Attachment: Bug 1916081 - Make `HTMLEditor::FocusedElementOrDocumentBecomesNotEditable` stop using `aHTMLEditor` at adjusting IME state r=m_kato!,#dom-core

aHTMLEditor may be nullptr and there may have been an HTMLEditor before
it's called. Therefore, it may need to adjust IME state even if aHTMLEditor
is nullptr. So, it should adjust IME state from aDocument which should be
same as aHTMLEditor->GetDocument() if aHTMLEditor is not nullptr.

Note that if IMEStateManager thinks another nsPresContext has focus, it does
not touch IME state. Therefore, it's safe to skip checking the focus state of
the Document.

Comment 5

[BugBot [:suhaib / :marco]]

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

Comment 6

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20240903210242-7fa1ed101e24.
Unable to bisect testcase (Unable to launch the start build!):

Start: ff7e2e9adbc1753b1f3e01a3f2eb8a91bb911418 (20230906035121)
End: c2e5212ba8d5ef250196cba1614a97edcbcc0209 (20240901214551)
BuildFlags: BuildFlags(asan=True, tsan=False, debug=False, fuzzing=True, coverage=False, valgrind=False, no_opt=False, fuzzilli=False, nyx=False)

Comment 7

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

Attachment: Bug 1916081 - Make `HTMLEditor::FocusedElementOrDocumentBecomesNotEditable` stop using `aHTMLEditor` at adjusting IME state r=m_kato!,#dom-core

aHTMLEditor may be nullptr and there may have been an HTMLEditor before
it's called. Therefore, it may need to adjust IME state even if aHTMLEditor
is nullptr. So, it should adjust IME state from aDocument which should be
same as aHTMLEditor->GetDocument() if aHTMLEditor is not nullptr.

Note that if IMEStateManager thinks another nsPresContext has focus, it does
not touch IME state. Therefore, it's safe to skip checking the focus state of
the Document.

Original Revision: https://phabricator.services.mozilla.com/D220772

Comment 8

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: May see the crash
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Load the test page
• Risk associated with taking this patch: Low
• Explanation of risk level: This touches the case when the last "contenteditable" attribute is removed or the designMode is set to off, and major cases are tested by existing tests
• String changes made/needed: no
• Is Android affected?: yes

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3138300cf7c8
Make `HTMLEditor::FocusedElementOrDocumentBecomesNotEditable` stop using `aHTMLEditor` at adjusting IME state r=m_kato

Comment 10

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

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

Comment 11

[Norisz Fay [:noriszfay]]

Backed out for causing Wc failures on replace_documentElement_children_after_window_closed.html

Backout link

Push with failures

Failure log

Comment 12

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

Upstream PR was closed without merging

Comment 13

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

:masayuki, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 14

[BugBot [:suhaib / :marco]]

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

:hsinyi, 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 15

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

I'll land the patch soon without the test. It seems that the test is not caused by this bug itself. I checked the testcase with debug build without the patch, but I couldn't reproduce it, but it leads a crash in both builds (with/without the patch) if it runs as a WPT crashtest alone.

If I add it as a WPT crashtest within <iframe>, it works, but it leads a time-out at textarea-will-be-blurred-by-focus-event-listener.html which does not use contenteditable nor designMode, so, the path should not run.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f90ebd811545
Make `HTMLEditor::FocusedElementOrDocumentBecomesNotEditable` stop using `aHTMLEditor` at adjusting IME state r=m_kato

Comment 17

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

Attachment: Bug 1916081 - Make `HTMLEditor::FocusedElementOrDocumentBecomesNotEditable` stop using `aHTMLEditor` at adjusting IME state r=m_kato!,#dom-core

aHTMLEditor may be nullptr and there may have been an HTMLEditor before
it's called. Therefore, it may need to adjust IME state even if aHTMLEditor
is nullptr. So, it should adjust IME state from aDocument which should be
same as aHTMLEditor->GetDocument() if aHTMLEditor is not nullptr.

Note that if IMEStateManager thinks another nsPresContext has focus, it does
not touch IME state. Therefore, it's safe to skip checking the focus state of
the Document.

Original Revision: https://phabricator.services.mozilla.com/D220772

Comment 18

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: This fixes a top10 crash
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: I'm not sure how to reproduce this bug. Written in comment 15, the test case seems not related to the crash
• Risk associated with taking this patch: Low
• Explanation of risk level: The crash is caused by nullptr access of null aHTMLEditor. This just avoids to use it, instead, focus check is done by IMEStateManager itself
• String changes made/needed: no
• Is Android affected?: yes

Comment 19

[Cosmin Sabou [:CosminS]]

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

Comment 20

[Bugmon [:jkratzer for issues]]

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

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/65fbdd140d79