1826272 - Crash in [@ mozilla::TextInputListener::SettingValue]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Makoto Kato [:m_kato]]

I don't know how to reproduce this, but mTextInputListener seems to be null.

Crash report: https://crash-stats.mozilla.org/report/index/367ccea6-10a7-4c31-89e8-b1e4c0230404

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0  xul.dll  mozilla::TextInputListener::SettingValue  dom/html/TextInputListener.h:34
0  xul.dll  mozilla::AutoTextControlHandlingState::WillSetValueWithTextEditor  dom/html/TextControlState.cpp:1219
0  xul.dll  mozilla::TextControlState::SetValueWithTextEditor  dom/html/TextControlState.cpp:2764
1  xul.dll  mozilla::TextControlState::SetValue  dom/html/TextControlState.cpp:2697
2  xul.dll  mozilla::dom::HTMLInputElement::SetValueInternal  dom/html/HTMLInputElement.cpp:2682
2  xul.dll  mozilla::dom::HTMLInputElement::SetValue  dom/html/HTMLInputElement.cpp:1658
3  xul.dll  mozilla::dom::HTMLInputElement_Binding::set_value  dom/bindings/HTMLInputElementBinding.cpp:2976
4  xul.dll  mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>  dom/bindings/BindingUtils.cpp:3266
5  xul.dll  CallJSNative  js/src/vm/Interpreter.cpp:459
5  xul.dll  js::InternalCallOrConstruct  js/src/vm/Interpreter.cpp:547

Comment 1

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

AutoTextControlHandlingState::mTextInputListener is never cleared and it's initialized only by the constructors. They initialize it with aTextControlState.mTextListener. It's initialized in BindToFrame and cleared by Clear (called in the CC or the destructor) and UnbindFromFrame. In the reported stack, it's initialized in TextControlState::SetValue, and going into SetValueWithTextEditor means that either mTextEditor or mBoundFrame is not nullptr...

Comment 2

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

In UnbindFromFrame, mBoundFrame is always cleared when mTextListener is cleared. And in Clear, mBoundFrame and/or mTextEditor is also cleared. Therefore, SetValueWithTextEditor shouldn't be called...

Comment 3

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

Attachment: Bug 1826272 - Make the content process crash if `TextControlState` becomes odd state r=m_kato!

If TextControlState:mTextListener is set to nullptr, at least one of
TextControlState::mBoundFrame or TextControlFrame::mTextEditor must be
set to nullptr. Therefore, when TextControlState realizes the odd state,
it should crash immediately. Then, we can investigate how to reproduce the
situation from crash reports.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/948cf466f3f2
Make the content process crash if `TextControlState` becomes odd state r=m_kato

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/948cf466f3f2

Comment 6

[Kris Maglione [:kmag]]

Fairly low volume crash. Interestingly, most of the crashes appear to be on Android.

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Kris Maglione [:kmag] from comment #6)

Fairly low volume crash. Interestingly, most of the crashes appear to be on Android.

I guess that reflow occurs more than desktop browser.

Comment 8

[BugBot [:suhaib / :marco]]

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

• Top 10 AArch64 and ARM crashes on release

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

For more information, please visit BugBot documentation.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Do we have no URLs in android crash reports? Gabriele, do you know why?

Comment 10

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

TextControlState::mTextListener is now cleared by TextControlState::DeinitSelection() too. It's called when the text frame is destroyed or the type attribute of the <input> from a text control value to a non-text control one. However, DeinitSelection() always calls DestoryEditor() which clears mEditorInitialized if it's true before clearing mTextListener. So, in this case, it should be safe.

And also, Clear() which is the other clearer of mTextListener does the same...

Oh, in SetValueWithTextEditor, AutoRestoreEditorState is created before calling WillSetValueWithTextEditor(). AutoRestorEditorState may update the editor flags which may run script. Looks like IMEStateManager::UpdateIMEState is the cause of marked as that. Probably, it's marked as MOZ_CAN_RUN_SCRIPT because it may call OS API or initializing IMEContentObserver. The former is not important here because the crash is about the content process. In these days, the actual reason of that must be IMEContentObserver::InitWithEditor because IME notifications will be sent asynchronously. Oh, but I have no idea how IMEContentObserver::InitWithEditor may run script in these days...

Edited: IMEContentObserver::InitWithEditor does not run script. Therefore, I removed the annotations from the related methods in bug 2029538.

Comment 11

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

Oh, IMEStateManager::UpdateIMEState may cause composition events. However, to make it, IMEState::mEnabled becomes Enabled or Password to Disabled. On the other hand, it runs only when the original state is readonly because AutoResotreEditorState deletes the readonly flag temporarily and if it's already not readonly, SetFlag() does nothing...

Oh, setting ime-state style might cause committing the composition because IME may not be available in the password fields on some platforms.

Comment 12

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

Oh, but the composition should've already been committed, hmm...

Comment 13

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

Ah, okay, probably, I was investigating wrong position. This is probably a bug of AutoTextControlHandlingState. It initializes mTextInputListener only in the constructor and never clear it. Additionally, when it's initialized and it's nullptr, asserts aTextControlState.mEditorInitialized being false. (Although it seems that the assertion is not in wrong constructor.) However, mEditorInitialized may be set to true later. Then, AutoTextControlHandlingState keeps the old state but the latest state is checked before using it. So, we need to make PrepareEditor manage mHandlingState->mTextInputListener too.

Comment 14

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

Perhaps, to reproduce it, we need to make:

1. Start composition
2. Reframe the text control frame
3. Set value
4. Trigger preparing TextEditor from compositionend or input event listener

I didn't have an idea of #4, but it seems that It's done automatically at dispatching compositionend.

I'll try to write a mochitest.

Comment 15

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

Hmm, I'm wrong. During setting the new value, cannot create prepare editor...
https://searchfox.org/firefox-main/rev/5ff69e076a8d8cc38707e29d5a8d7c42d5798a8b/dom/html/TextControlState.cpp#1485-1487

Comment 16

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

Attachment: Bug 1826272 - Make `TextControlState::mTextListener` live longer r=emilio!

I've not understand why the crash occurs. To reproduce the crash, we
need the following conditions:

1. mTextListener is nullptr when SetValue() is called [1]
2. mEditorInitialized is true when setting the value [2]

mTextListener is initialized when the text control frame is
initialized [3]. So, there are 2 possible scenarios:

1. PrepareEditor() is called before the frame is initialized
2. PrepareEditor() creates the editor during a call of SetValue()

However, the second scenario should be impossible because
PrepareEditor() does nothing during setting value [4].

Therefore, it seems that PrepareEditor() needs to guarantee that
handling state will have a TextInputListener when it sets
mEditorInitialized to true.

1. https://searchfox.org/firefox-main/rev/5ff69e076a8d8cc38707e29d5a8d7c42d5798a8b/dom/html/TextControlState.cpp#2288-2289
2. https://searchfox.org/firefox-main/rev/5ff69e076a8d8cc38707e29d5a8d7c42d5798a8b/dom/html/TextControlState.cpp#2288-2289,2407-2408
3. https://searchfox.org/firefox-main/rev/5ff69e076a8d8cc38707e29d5a8d7c42d5798a8b/layout/forms/nsTextControlFrame.cpp#413-414,427-428
4. https://searchfox.org/firefox-main/rev/5ff69e076a8d8cc38707e29d5a8d7c42d5798a8b/dom/html/TextControlState.cpp#1484-1487

Comment 17

[Emilio Cobos Álvarez [:emilio]]

It seems there are no crashes on 150+, so the bug is probably fixed on trunk.

Comment 18

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

Oh, that's good point. Indeed, it could be fixed. However, the reports are rare on non-release channels. So, this could be reproducible only in specific sites which are not used by the most our testers.

Comment 19

[Gabriele Svelto [:gsvelto]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)

Do we have no URLs in android crash reports? Gabriele, do you know why?

We should, but bug 1811650 has never been fixed so I have a theory of why we might be missing them. In desktop crash reports the URL crash annotation is opt-in and the user must tick the appropriate checkbox when submitting a crash report. On Android we don't have a mechanism for that yet, which is the same reason why we don't have a mechanism to let users send comments alongside a crash report. Jeff, is that correct?

Comment 20

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

It seems that anyway my patch may fix some hidden bugs. EditorBase::SetTextInputListener() is called only by TextControlElement::PrepareEditor() and it's done only when the editor has not been initialized yet or it's being reinitialized. So, if still have a case that TextControlState::PrepareEditor is called before the first TextControlState::InitializeSelection call, the editor instance cannot notify the command handlers and TextControlElement of the state changes. E.g., undo/redo are never available in such editor.

Comment 21

[Jeff Boek [:boek]]

:gvselto that is correct. We could add it but the crash reporting flow on mobile is still pretty bare-bone and would need to some time to decide how we would want to improve it.

Comment 22

[Gabriele Svelto [:gsvelto]]

URLs and comments are very high value for us. Not just for manual crash triage and analysis but also because our automation uses them to detect if a particular crash signature is worth looking into (a signature with lots of comments is usually important to users, even if the crashes are few). We've got more resources this year so we could help you out with the implementation if your team could provide us with scoping and guidance on the topic.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ad61492d2b47
https://hg.mozilla.org/integration/autoland/rev/65fa00dae7a5
Make `TextControlState::mTextListener` live longer r=emilio

Comment 24

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/65fa00dae7a5

Comment 25

[BMO Github Automation]

Authored by https://github.com/masayuki-nakano
https://github.com/mozilla/enterprise-firefox/commit/ad61492d2b47a7b4b65ccaa3523385e5c1e6292a
[enterprise-main] Bug 1826272 - Make TextControlState::mTextListener live longer r=emilio