1690827 - Inserting Emoji from the picker of Windows 10 into editor which forcibly commits composition immediately may cause crash except in the release channel

Status: VERIFIED FIXED

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

Description

[QOAL]

Attachment: File to help reproduce the crash

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0

Steps to reproduce:

1. Load the attached HTML file
2. Focus the text input
3. Open the emoji picker (win + .)
4. Click on emoji a couple of times
5. Repeat 2 3 4 until it crashes

Actual results:

The browser crashes.
I've also experienced firefox stop responding to keyboard input, in the project I originally triggered the bug in.

Comment 1

[Andre Klapper]

Hi, please provide the crash ID from about:crashes : https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report

Comment 2

[QOAL]

https://crash-stats.mozilla.org/report/index/3084fc1d-b469-4e76-830b-75c440210204

Also I think your MDN link is dead. Should it be this now? https://firefox-source-docs.mozilla.org/contributing/debugging/stacktrace_report.html

Comment 3

[Andre Klapper]

Thanks, indeed.

Comment 4

[Marcela (Please NI request to Brindusa Tot)]

I was able to reproduce it following the steps given by the reporter on Nightly version 87.0a1 (2021-02-08)(64-bit) on Windows 10. It's weird but, I have not been able to reproduce it in Beta version 86.0b7 nor in Release version 85.0.1.
Also, I've executed Mozregression from 2020-11-04 to 2021-02-08 without any good result.

This is the linkt to crash report: https://crash-stats.mozilla.org/report/index/d6df8504-e5c4-4a25-89e4-6e60d0210208
Report ID Date Submitted
bp-d6df8504-e5c4-4a25-89e4-6e60d0210208 2/8/2021, 10:42 AM.
I'll set a component to have a starting point from this and I'll change flags accordingly. Please feel free to change them if they are not correct.
Severity suggested S3

Comment 5

[Stephen A Pohl [:spohl]]

I tried to reproduce this, but wasn't able to. Are you still able to reproduce?

Comment 6

[QOAL]

Yes, I just reproduced it with the current nightly using the steps provided.
https://crash-stats.mozilla.org/report/index/76eb2499-8d6f-4b2a-98c9-409350220211

You'll also find that keyboard input is ignored for address bar, page inputs, if you do the following:

1. Load the test file
2. Focus the text input
3. Pick an emoji via the emoji picker
   And that's tested with release 97 and the current nightly.

Comment 7

[Stephen A Pohl [:spohl]]

Masayuki-san, this appears to crash on Nightly due to a diagnostic assert in ContentCacheInParent::OnEventNeedingAckHandled. I see that you have been active in this area most recently. Would you be able to give your opinion on how to proceed here? The App Notes from the crash report are as follows:

FP(D00-L1000-W00001000-T000) DWrite? DWrite+ WR? WR+
There is no pending composition but received eCompositionCommitRequestHandled message from the remote child

Dispatched Event Message Log:
eCompositionStart
eCompositionCommit
eCompositionStart

Received Event Message Log:
eCompositionStart
eCompositionCommitRequestHandled
eCompositionCommit
eCompositionStart
eCompositionCommit
eCompositionStart
eCompositionCommitRequestHandled

Result of RequestIMEToCommitComposition():
Commit request is handled with stored composition string because BrowserParent has already lost focus
Commit request is handled with stored composition string because BrowserParent has already lost focus

Comment 8

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

Probably, it's caused by a race between processes. I'll take a look, but this is not a recent regression, so it could be not easy to find the cause.

Comment 9

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

I tried to reproduce this bug with emulating same events in automated tests, however, I couldn't reproduce it (although it detects another assertion hit).
https://treeherder.mozilla.org/jobs?repo=try&revision=298d964553e2ca321fe39de3ed90b9c84650926f

This bug does not affect to the normal users. So I think that I should be back when I have much time.

Comment 10

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

Oh, there is a useful comment: "I had crash with Google IME input (Windows / Japanese) ".

Comment 11

[Gabriele Svelto [:gsvelto]]

I found a few comments that point towards IME input:

• 8e26d747-47b0-46d1-83e1-3e34a0230110

  I got an email on outlook and went to switch to that tab from the youtube video I just started 「マッチング真知子先生OP」and it crashed

  Note that the user had outlook.com open in a tab, it wasn't switching to an external mail application, just switching tabs

• aec4bb21-ab93-4fb1-b907-be0a80230128

  was typing in japanese when it crashed

• 08bba100-e252-445d-974b-94a9d0221224

  **** this application I just lost my duolingo process

At least one of the crashes had the Google IME input DLL in the stack.

Comment 12

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

Attachment: Bug 1690827 - part 1: Number each composition for native IME or synthesized in the parent process r=smaug!

For handling (ignoring) "too late" composition commit request from content
process, we need to distinguish a request is for which composition. Therefore,
we need to number each composition originated in the parent process.

This patch makes TextComposition instance which is created at first
composition event for a composition consider a composition ID in the parent
process and set it to composition events which are dispatched into the DOM
tree in the parent or sent to a remote process.

And also this patch adds the composition ID param to the request method of
committing composition and reply methods to notify the parent process of ending
a composition event handling.

The following patches handle them in ContentCacheInParent to consider
whether a request/reply should be ignored or handled.

Comment 13

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

Attachment: Bug 1690827 - part 2: Make `ContentCacheInParent::mWidgetHasComposition` and `ContentCacheInParent::mPendingCompositionCount` should be managed by array of pending composition data r=m_kato!

We need to make ContentCacheInParent store each pending composition data
for ignoring unexpected (including outdated) composition request/notifications
for protecting the main process's data, e.g., current composition.

This patch creates the place (array) and just simply store whether the
pending composition has already sent eCompositionCommit(AsIs) to the
remote process.

Depends on D179310

Comment 14

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

Attachment: Bug 1690827 - part 3: Get rid of `ContentCacheInChild::mPendingCommitCount` r=m_kato!

It's now not necessary. We can check all pending composition data whether
there are some composition which waits commit in the remote process.

Depends on D179311

Comment 15

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

Attachment: Bug 1690827 - part 4: Make `ContentCacheInParent::HandlingCompositionData` store composition ID and some data which needs to be managed per composition r=m_kato!

Then, we can know whether the message notification and commit request is for
proper one or outdated one, etc. Therefore, this patch makes
OnEventNeedingAckHandled() and RequestIMEToCommitComposition() ignores
unexpected and/or outdated calls. This should fix the crash reported in
bug 1466596 too.

Depends on D179312

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2c874809470
part 1: Number each composition for native IME or synthesized in the parent process r=smaug
https://hg.mozilla.org/integration/autoland/rev/bc45f9483150
part 2: Make `ContentCacheInParent::mWidgetHasComposition` and `ContentCacheInParent::mPendingCompositionCount` should be managed by array of pending composition data r=m_kato
https://hg.mozilla.org/integration/autoland/rev/feedd62f908d
part 3: Get rid of `ContentCacheInChild::mPendingCommitCount` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/41105bb6e199
part 4: Make `ContentCacheInParent::HandlingCompositionData` store composition ID and some data which needs to be managed per composition r=m_kato

Comment 17

[Cosmin Sabou [:CosminS]]

Backed out for causing build bustages on widget/ContentCache.cpp

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=8f96604b79a63fe139dc8182dfff1bd83f4029b0&tochange=4db7649b842ecb68a337f7f736a479185af0b377&selectedTaskRun=ezFJ7E0TSXCciOh8ttO5pw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=419078527&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/4db7649b842ecb68a337f7f736a479185af0b377

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d84920bbc530
part 1: Number each composition for native IME or synthesized in the parent process r=smaug
https://hg.mozilla.org/integration/autoland/rev/c385d66839cc
part 2: Make `ContentCacheInParent::mWidgetHasComposition` and `ContentCacheInParent::mPendingCompositionCount` should be managed by array of pending composition data r=m_kato
https://hg.mozilla.org/integration/autoland/rev/acaee36f499c
part 3: Get rid of `ContentCacheInChild::mPendingCommitCount` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/13bcf9807560
part 4: Make `ContentCacheInParent::HandlingCompositionData` store composition ID and some data which needs to be managed per composition r=m_kato

Comment 19

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/d84920bbc530
https://hg.mozilla.org/mozilla-central/rev/c385d66839cc
https://hg.mozilla.org/mozilla-central/rev/acaee36f499c
https://hg.mozilla.org/mozilla-central/rev/13bcf9807560

Comment 20

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

Hmm, it seems that there are still an issue. The crash reports let us:

There is not only the pending verify notifications (2 pending events) but the handling composition is being removed by receiving eCompositionCommitRequestHandled

Dispatched Event Message Log:
eCompositionStart
eCompositionChange
eCompositionCommit
eCompositionStart
eCompositionChange
eCompositionCommit

Received Event Message Log:
eCompositionStart
eCompositionChange
eCompositionCommit
eCompositionStart
eCompositionCommitRequestHandled

Result of RequestIMEToCommitComposition():
Commit request is handled synchronously

Does somebody still reproduce this bug with the latest Nightly builds? I'm not sure whether the crash reports comes with the STR reported in this bug.

Comment 21

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

Ah, okay, the assertion condition is just wrong.

    for (auto& data : mHandlingCompositions) {
      if (&data == handlingCompositionData) {
        if (NS_WARN_IF(data.mPendingEventsNeedingAck != 1u)) {
          nsPrintfCString info(
              "\nThere is not only the pending verify notifications (%" PRIu32
              " pending events) but the handling composition is being removed "
              "by receiving %s\n\n",
              data.mPendingEventsNeedingAck, ToChar(aMessage));
          AppendEventMessageLog(info);
          CrashReporter::AppendAppNotesToCrashReport(info);
          MOZ_DIAGNOSTIC_ASSERT(false,
                                "There is not only the pending event message "
                                "but ending the handling composition");
        }
        break;

As the report, while dispatching eCompositionStart or something, the composition may be requested of commit before the following messages are received by the remote process. Therefore, it does not make sense to check the number of the remaining pending event messages.

Comment 22

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

Attachment: Bug 1690827 - part 5: Remove the assertion checking the number of pending composition event messages when receiving a commit request r=m_kato!

E.g., when a character is inserted with a composition, i.e., without composing
state, like the Emoji picker of Windows, BrowserParent dispatches
eCompositionStart, eCompositionChange and eCompositionCommit message
within a short time. Then, if focus is moved at compositionstart event in
the remote message, eCompositionChange and eCompositionCommit are not yet
handled by the remote process when BrowserParent::OnEventNeedingAckHandled()
received a commit request handled message. Therefore, it's not invalid case
that a commit is handled without acknowledges of multiple pending composition
messages. Therefore, the assertion should be removed, and instead, for making
it easier to understand the log, let's put the information of that. Then, it's
obvious that ignored log of following OnEventNeedingAckHandled() calls is
valid thing.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9ef0fac7c93
part 5: Remove the assertion checking the number of pending composition event messages when receiving a commit request r=m_kato

Comment 24

[Noemi Erli[:noemi_erli]]

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

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox115 to wontfix.

For more information, please visit BugBot documentation.

Comment 26

[Ciprian Georgiu, Desktop QA]

I have reproduced this crash using an affected Nightly build, 2021-02-04, with the test case from comment 0.

The issue is verified as fixed on latest Beta 116.0b3 under Win 10 x64.

Comment 27

[Pulsebot]

Backout by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-release/rev/8dd3ce4ea8f1
Backed out 6 changesets (bug 1690827, bug 1835577, bug 1835578) for causing build bustages on widget/ContentCache.cpp. CLOSED TREE