1553008 - Crash in [@ mozilla::RestyleManager::ContentStateChanged] , Crash with using IME

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Alice0775 White]

Bug 1551086 did not fix this crash.

This bug is for crash report bp-dec8a970-d852-4182-a066-c1fcc0190521.

Top 10 frames of crashing thread:

0 xul.dll mozilla::RestyleManager::ContentStateChanged layout/base/RestyleManager.cpp:3214
1 xul.dll mozilla::dom::Document::ContentStateChanged dom/base/Document.cpp:5253
2 xul.dll mozilla::dom::HTMLFormElement::UpdateValidity dom/html/HTMLFormElement.cpp:1863
3 xul.dll mozilla::dom::HTMLInputElement::UpdateAllValidityStatesButNotElementState dom/html/HTMLInputElement.cpp:6710
4 xul.dll void mozilla::dom::HTMLInputElement::OnValueChanged dom/html/HTMLInputElement.cpp:6798
5 xul.dll nsTextEditorState::SetValue dom/html/nsTextEditorState.cpp:2494
6 xul.dll nsTextEditorState::UnbindFromFrame dom/html/nsTextEditorState.cpp:2043
7 xul.dll void nsTextControlFrame::DestroyFrom layout/forms/nsTextControlFrame.cpp:138
8 xul.dll nsLineBox::DeleteLineList layout/generic/nsLineBox.cpp:372
9 xul.dll nsBlockFrame::DestroyFrom layout/generic/nsBlockFrame.cpp:328

Reproducible: 100%

Steps To Reproduce:

1. Help - Nightly Help
   (i.e, open https://support.mozilla.org/en-US/products/firefox?as=u&utm_source=inproduct )
2. Focus input filed
3. IME turn on (MS-IME)
4. Type 'sa' (without quatation)
   --- observe sあ (this is another bug, see Bug 1524043）
5. Type BackSpace BackSpase
6. IME turn off
7. Type 'sa' (without quatation)

Actual Results:
The tab crashes

Comment 1

[Alice0775 White]

Attachment: about:support

bp-ac47626a-3f1a-46bb-b1d1-320fd0190521

Comment 2

[Alice0775 White]

Also crash on ibus-mozc (ubuntu18.04)
bp-33a41ad1-ab58-4daa-8e17-262550190521

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Here's a stack trace without inlining:

#0  0x00007f1cde1466f1 in mozilla::RestyleManager::ContentStateChanged(nsIContent*, mozilla::EventStates) (this=0x7f1cacc13280, aContent=0x7f1cab91c020, aChangedBits=...)
    at /home/emilio/src/moz/gecko-artifact/layout/base/RestyleManager.cpp:3214
#1  0x00007f1cde14663b in mozilla::PresShell::ContentStateChanged(mozilla::dom::Document*, nsIContent*, mozilla::EventStates)
    (this=0x7f1cad1c4000, aDocument=0x7f1cbe661000, aContent=0x7f1cab91c020, aStateMask=...) at /home/emilio/src/moz/gecko-artifact/layout/base/PresShell.cpp:4282
#2  0x00007f1cdad8c118 in mozilla::dom::Document::ContentStateChanged(nsIContent*, mozilla::EventStates) (this=0x7f1cbe661000, aContent=0x7f1cab91c020, aStateMask=...)
    at /home/emilio/src/moz/gecko-artifact/dom/base/Document.cpp:5293
#3  0x00007f1cdadb16a5 in mozilla::dom::Element::UpdateState(bool) (this=0x7f1cab91c020, aNotify=true) at /home/emilio/src/moz/gecko-artifact/dom/base/Element.cpp:292
#4  0x00007f1cdca390ce in mozilla::dom::HTMLFormElement::UpdateValidity(bool) (this=0x7f1cac70d6f0, aElementValidity=true)
    at /home/emilio/src/moz/gecko-artifact/dom/html/HTMLFormElement.cpp:1863
#5  0x00007f1cdcb59729 in nsIConstraintValidation::SetValidityState(nsIConstraintValidation::ValidityStateType, bool)
    (this=0x7f1cbf0239f8, aState=nsIConstraintValidation::VALIDITY_STATE_VALUE_MISSING, aValue=false) at /home/emilio/src/moz/gecko-artifact/dom/html/nsIConstraintValidation.cpp:206
#6  0x00007f1cdca54863 in mozilla::dom::HTMLInputElement::UpdateValueMissingValidityState() (this=0x7f1cbf0238c0) at /home/emilio/src/moz/gecko-artifact/dom/html/HTMLInputElement.cpp:6664
#7  0x00007f1cdca5d7cb in mozilla::dom::HTMLInputElement::UpdateAllValidityStatesButNotElementState() (this=0x7f1cbf0238c0)
    at /home/emilio/src/moz/gecko-artifact/dom/html/HTMLInputElement.cpp:6703
#8  0x00007f1cdca5c4ec in mozilla::dom::HTMLInputElement::UpdateAllValidityStates(bool)

Root cause is still the same, I think, I'll dig through it a bit.

Comment 4

[Marcia Knous [:marcia]]

Discussing during Triage - Any update on this bug? Thanks!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Ok, so I finally get what's going on here and why this needs IME to happen. The bug is that there's a missing OnValueChanged call. The reason for this is the following:

• We start composing with IME, and set mComposition.
• The keyup listener hides the node, so we remove the event listeners and never get the compositionend event, but critically we never clear out mComposition.
• Then we switch to non-IME mode, while the node is still hidden, then delete content so that the node is shown again. Now we're in a broken state: We have a non-null mComposition, but we're not composing anything.
• We type, editor does the edit action dance, but when we're done we never notify the observers here because mComposition is still non-null. However, we never end up with a compositionend event, and thus we never call OnEditActionHandled, which is what ends up updating the content state at the right time usually, via HTMLInputElement::OnValueChanged.
• So then we just happen to notice the value change via the SetValue done from UnbindFromFrame, which is never actually supposed to change the value (since doing so would be unsound).

Masayuki, any idea of what's the right thing to do here? Clearing out mComposition would probably regress bug 1162818. So chances are that we still need to keep listening to events even if not bound to any frame. Does that seem reasonable?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

If my diagnostic in comment 6 is right this is an editor bug.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Sorry for the delay to reply and thank you for your investigation!! I completely understand what's going on.

This is really interesting case, and this must be a reason why we should fix bug 1556336. This makes it sure that managing state of IME with DOM events is impossible. I think that TextComposition should notify editor of IME composition updates directly.

Comment 9

[Julien Cristau [:jcristau]]

Not a huge volume, P3, fix-optional for 68.

Comment 10

[Alice0775 White]

Attachment: crash_reproducible.html

Comment 11

[Alice0775 White]

The crash is no longer reproduced on Nightly70.0a1(20190710094220) Windows10.

Fixed window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5d8406bfe056b2aee644c856d38b49d682cfdb0a&tochange=006d54dad1f5172a681e4343cb596d8862941850

Fixed by: Bug 1563508

Comment 12

[Emilio Cobos Álvarez (:emilio)]

How can that be? That bug only touched tests.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Ah, could be caused by this bug: https://phabricator.services.mozilla.com/D37061

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Let's mark this as FIXED.