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

VERIFIED FIXED in Firefox 69

Status

()

defect
--
critical
VERIFIED FIXED
3 months ago
Last month

People

(Reporter: alice0775, Assigned: emilio)

Tracking

(Regression, 4 keywords)

68 Branch
mozilla69
Desktop
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 unaffected, firefox67 unaffected, firefox68+ disabled, firefox69 verified)

Details

(crash signature)

Attachments

(2 attachments)

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

[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.

Flags: needinfo?(emilio)
Regressed by: 1525509
Summary: Crash in [@ mozilla::RestyleManager::ContentStateChanged] → Crash in [@ mozilla::RestyleManager::ContentStateChanged] , Crahs with MS-IME (Japanese IME of Windows10)
Has Regression Range: --- → yes
Has STR: --- → yes

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.

Flags: needinfo?(masayuki)
Flags: needinfo?(alice0775)

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?

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

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

OS: Windows 10 → All
Hardware: x86_64 → Desktop

(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 )

Yay, got it to repro, thanks much!

Flags: needinfo?(alice0775)

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.

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).

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

See Also: → 1551192

Sounds pretty bad.

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.

Flags: needinfo?(emilio)

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).

See Also: → 1551196
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6b2612701f6
Do not finish edit action when unbinding the frame from the editing state. r=masayuki

(In reply to Pulsebot from comment #14)

Pushed by ealvarez@mozilla.com:
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

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

Flags: needinfo?(alice0775)
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Assignee: nobody → emilio
Blocks: 1553008

(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.

Flags: needinfo?(alice0775)
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.