1648372 - Crash in [@ mozilla::TextInputSelectionController::SetCaretReadOnly]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-de1a3215-c350-4c05-8acb-bf5490200624.

Top 10 frames of crashing thread:

0 xul.dll mozilla::TextInputSelectionController::SetCaretReadOnly dom/html/TextControlState.cpp:521
1 xul.dll mozilla::EditorBase::Init editor/libeditor/EditorBase.cpp:300
2 xul.dll mozilla::TextEditor::Init editor/libeditor/TextEditor.cpp:123
3 xul.dll mozilla::TextControlState::PrepareEditor dom/html/TextControlState.cpp:1799
4 xul.dll mozilla::TextControlState::GetTextEditor dom/html/TextControlState.cpp:1533
5 xul.dll mozilla::dom::HTMLInputElement_Binding::get_editor dom/bindings/HTMLInputElementBinding.cpp:5781
6 xul.dll mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3101
7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:576
8 xul.dll js::CallGetter js/src/vm/Interpreter.cpp:780
9 xul.dll js::NativeGetProperty js/src/vm/NativeObject.cpp:2490

This is a NULL pointer access and not a new crash since we have it all the way back to six months ago which is our retention limit for crash reports. It's odd that it flew under the radar for so long as it seems to affect all platforms.

Comment 1

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

Looks like the mFrameSelection is nullptr. It's cleared by cycle collector and TextInputSelectionController::SetScrollableFrame(). I guess that this is the latter case. It's called by UnbindFrame() and InitializeKeyboardEventListeners(). I don't know why we need to release nsFrameSelection when there is no scrollable frame. This seems like that our TextEditor does not work under invisible state...

Comment 2

[Mirko Brodesser (:mbrodesser-Igalia)]

Masayuki: thanks for the analysis. Fixing the crash could be simple by checking mFrameSelection isn't nullptr. I'll try this next week.

Comment 3

[Mirko Brodesser (:mbrodesser-Igalia)]

Attachment: Bug 1648372: add missing `nullptr` check to `TextInputSelectionController::SetCaretReadOnly`. r=masayuki

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8796bd33fc0
add missing `nullptr` check to `TextInputSelectionController::SetCaretReadOnly`. r=masayuki

Comment 5

[Dorel Luca [:dluca]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Seems like this would be a pretty safe uplift to Beta and ESR78. Did you want to nominate it for approval?

Comment 7

[Mirko Brodesser (:mbrodesser-Igalia)]

Comment on attachment 9161202 [details]
Bug 1648372: add missing nullptr check to TextInputSelectionController::SetCaretReadOnly. r=masayuki

Beta/Release Uplift Approval Request

• User impact if declined:
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): It only adds a nullptr check preventing a crash.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: It's a rare crash. The crash happened because a null-pointer was de-referenced. This fix consists only of adding the corresponding null-pointer check.
• User impact if declined: Rare crashes.
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It only adds a nullptr check preventing a crash.
• String or UUID changes made by this patch:

Comment 8

[Mirko Brodesser (:mbrodesser-Igalia)]

@RyanVM: from the crash report I got the impression that this crash happens very rarely (less than five times per day). That's why I didn't consider uplifting up to now.
But I agree, the code-change should be pretty safe, so we could uplift it.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161202 [details]
Bug 1648372: add missing nullptr check to TextInputSelectionController::SetCaretReadOnly. r=masayuki

Simple null check crash fix. Approved for 79.0b6 and 78.1esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7d0c99d4b3e9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/6c74c44b2c94