1551086 - Crash in [@ mozilla::RestyleManager::ContentStateChanged] , Crahs with MS-IME (Japanese IME of Windows10)

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Alice0775 White]

This bug is for crash report bp-6ed96133-9773-4b1a-b83f-e5af80190512.

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:1872
3 xul.dll nsIConstraintValidation::SetValidityState dom/html/nsIConstraintValidation.cpp:206
4 xul.dll void mozilla::dom::HTMLInputElement::UpdateAllValidityStatesButNotElementState dom/html/HTMLInputElement.cpp:6700
5 xul.dll void mozilla::dom::HTMLInputElement::OnValueChanged dom/html/HTMLInputElement.cpp:6798
6 xul.dll mozilla::TextInputListener::OnEditActionHandled dom/html/nsTextEditorState.cpp:980
7 xul.dll nsTextEditorState::UnbindFromFrame dom/html/nsTextEditorState.cpp:1938
8 xul.dll void nsTextControlFrame::DestroyFrom layout/forms/nsTextControlFrame.cpp:138
9 xul.dll nsLineBox::DeleteLineList layout/generic/nsLineBox.cpp:372

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]

[Tracking Requested - why for this release]:

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3f18f4d9dc1e7e0d226267dea1da72765126f809&tochange=95b6997c334afa45b588408bc25b99c9f26b391d

Regressed by:
95b6997c334afa45b588408bc25b99c9f26b391d Emilio Cobos Álvarez — Bug 1525509 - Add release asserts. r=dholbert

Emilio，
Your patch seems to cause the crash. Can you please look into this?
FYI, 67.0b19 does not crash with the STR. Nightly68.0a1 is only crashing.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Thanks for the report Alice.

Do you (or Masayuki, maybe?) know how to enable IME in Linux or such?

This is a pre-existing bug, just uncovered by the assertion in any case. Losing the frame shouldn't affect the validity state of the input.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So I guess the problematic call is this:

https://searchfox.org/mozilla-central/rev/8308eb7ea14318f53b55f3289c2bb9b712265318/dom/html/nsTextEditorState.cpp#1938

I don't understand why UnbindFromFrame needs to call into it. I guess we null out the editor listener on UnbindFromTree so we do need to finish the edit action.

But... that's not quite sound. You cannot quite just change validity states like that from frame destruction.

Masayuki, I wonder why our editor stuff is so tied to the frame tree. Would it be reasonable to change that to happen only when the type attribute changes, and / or the node becomes disconnected?

Comment 4

[Alice0775 White]

For reference:

Fixed window in beta:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=b1cb8bd44deeae0e8e0d94f7755b1abb04ef5653&tochange=05d12183776b26907ebacbc9b0bbf282e148cc1a

Fixed by beta channel only:
bdd03a2c4e54d9bd9f6822ab5bef134d11d67f90 Emilio Cobos Álvarez — Bug 1545842 - Downgrade RestyleManager assertions to DIAGNOSTIC_ASSERT in beta. r=tnikkel a=pascalc

Comment 5

[Alice0775 White]

FYI, I can also reproduce the crash( bp-00c28e79-543c-4f41-87ac-65f7b0190512 ) on Nightly68.0a1 Ubuntu18.04 (ibus-mozc).

Comment 6

[Alice0775 White]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Thanks for the report Alice.

Do you (or Masayuki, maybe?) know how to enable IME in Linux or such?

I am not sure, but see https://moritzmolch.com/2404.
(I am using Ubuntu 18.04 LTS Japanese Remix Release https://www.ubuntulinux.jp/News/ubuntu1804-ja-remix )

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Yay, got it to repro, thanks much!

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Ok, so that site is doing something really weird with two different input fields, which is probably the cause for the other bug you're seeing.

I suspect that what's going on here is that when you type the first s they're switching inputs under the hood, but some state still remains on the old editor. When you remove the characters and switch off IME then write we get confused. I'll try to reduce it a bit.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced-ish test-case

This is roughly what Kitsune is doing. I can get this to crash following the STR in comment 0, but it'd be great to confirm.

Also, there's some other weirdness going on (even without IME)...

The validity state is all wrong if you input text, then remove it, then input again (text is red, but any non-empty value should really be black rather than red).

Comment 10

[Alice0775 White]

I confirmed that Nightly68.01(20190512093034) Windows10 crashes with the testcase attachment 9064337 [details].

Comment 11

[Julien Cristau [:jcristau]]

Sounds pretty bad.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=362abf0bafa2a6018f6a6770f51e551f16deb0a8

I don't think it's terrible (as in, the error was pre-existing, it was just exposed by the assertions).

But we might want to downgrade the assertions to diagnostic asserts more generally in case there are more of these lurking around.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1551086 - Do not finish edit action when unbinding the frame from the editing state. r=masayuki

The code here was introduced to fix bug 1053048, but the test there and the
test-case there no longer need this, and it's generally unsound to do stuff that
changes the state of the input from UnbindFromFrame().

I'll file a new bug for the bogus disabled styling in the test-case in a second.

I don't know how to add a test for this (no less because the testcase crashes on
debug builds regardless of this patch, see bug 1551192).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6b2612701f6
Do not finish edit action when unbinding the frame from the editing state. r=masayuki

Comment 15

[Alice0775 White]

(In reply to Pulsebot from comment #14)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6b2612701f6
Do not finish edit action when unbinding the frame from the editing state.
r=masayuki

The patch ONLY fixed the crash with the testcase attachment 9064337 [details].
However, I can still reproduce the crash with STR in comment#0.

Update Channel nightly-autoland
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0
Built from https://hg.mozilla.org/integration/autoland/rev/f6b2612701f631484e59c793308e955b01eada99

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Do you have a stack of that? I can test that in a bit if not.

Comment 17

[Cristina Coroiu [:ccoroiu]]

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

Comment 18

[Alice0775 White]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

Do you have a stack of that? I can test that in a bit if not.

I have filed a new Bug 1553008.

Comment 19

[Anca Soncutean, Desktop QA]

I’ve managed to triggered the crash both, with the steps from comment 0 and the provided test case on Windows 10 x64 with Fx 68.0a1 (2019-05-12).
I couldn’t reproduce this crash with Fx 69.0b6 on Windows 10 x64 and macOS 10.14. Therefore I will mark this issue as verified fixed.