Inserting Emoji from the picker of Windows 10 into editor which forcibly commits composition immediately may cause crash except in the release channel
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P2)
Tracking
()
People
(Reporter: quiche_on_a_leash, Assigned: masayuki)
References
Details
(Keywords: crashreportid, inputmethod, Whiteboard: [win:stability])
Crash Data
Attachments
(6 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:85.0) Gecko/20100101 Firefox/85.0
Steps to reproduce:
- Load the attached HTML file
- Focus the text input
- Open the emoji picker (win + .)
- Click on emoji a couple of times
- 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•3 years ago
|
||
Hi, please provide the crash ID from about:crashes : https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report
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 4•3 years ago
|
||
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•2 years ago
|
||
I tried to reproduce this, but wasn't able to. Are you still able to reproduce?
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:
- Load the test file
- Focus the text input
- Pick an emoji via the emoji picker
And that's tested with release 97 and the current nightly.
Comment 7•2 years ago
|
||
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
Assignee | ||
Comment 8•2 years ago
|
||
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.
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 9•2 years ago
|
||
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 10•2 years ago
|
||
Oh, there is a useful comment: "I had crash with Google IME input (Windows / Japanese) ".
Comment 11•1 year ago
|
||
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.
Assignee | ||
Comment 12•11 months ago
|
||
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.
Updated•11 months ago
|
Assignee | ||
Comment 13•11 months ago
|
||
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
Assignee | ||
Comment 14•11 months ago
|
||
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
Assignee | ||
Comment 15•11 months ago
|
||
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•11 months ago
|
||
Pushed by masayuki@d-toybox.com: 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•11 months ago
|
||
Backed out for causing build bustages on widget/ContentCache.cpp
Failure log: https://treeherder.mozilla.org/logviewer?job_id=419078527&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/4db7649b842ecb68a337f7f736a479185af0b377
Assignee | ||
Updated•11 months ago
|
Comment 18•11 months ago
|
||
Pushed by masayuki@d-toybox.com: 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•11 months ago
|
||
bugherder |
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
Updated•11 months ago
|
Assignee | ||
Comment 20•10 months ago
|
||
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
eCompositionCommitReceived Event Message Log:
eCompositionStart
eCompositionChange
eCompositionCommit
eCompositionStart
eCompositionCommitRequestHandledResult 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.
Assignee | ||
Comment 21•10 months ago
•
|
||
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.
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 22•10 months ago
|
||
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•10 months ago
|
||
Pushed by masayuki@d-toybox.com: 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•10 months ago
|
||
bugherder |
Comment 25•10 months ago
|
||
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
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Updated•10 months ago
|
Comment 26•10 months ago
|
||
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•9 months ago
|
||
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
Description
•