While NativeKey retrieves a WM_CHAR message, sometimes a lot of WM_NULL messages are removed

RESOLVED FIXED in Firefox 52

Status

()

Core
Widget: Win32
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({crash})

Trunk
mozilla54
All
Windows
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Similar to bug 1334947. Currently, NativeKey::GetFollowingCharMessage() gives up if PeekMessage(PM_REMOVE) returns WM_NULL message 5 times and makes the process crash.

https://crash-stats.mozilla.com/report/index/785e2b84-9579-44a8-a26a-7518b2170131
> We lost following char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_F17, repeat count=1, scancode=0x00, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x8029a, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=BACKSPACE (0x0008), repeat count=1, scancode=0x0E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x8029a, removed a lot of WM_NULL

I think that when we get WM_NULL, we should look for next WM_CHAR. Then, if there is no WM_CHAR anymore, we should consume the key event like bug 1334947. Otherwise, keep trying to get WM_CHAR more times (but for preventing infinite loop, it should give up to retrieve the WM_CHAR, only dispatch keydown and wait following WM_CHAR).
> but for preventing infinite loop, it should give up to retrieve the WM_CHAR, only dispatch keydown
> and wait following WM_CHAR

Hmm, but in this case, we might receive orphan WM_*CHAR message after that. I'm still not sure how we should handle this case. So, coming patch just use big number for limit of retry. For reducing the cost when removing message has already gone, coming patch checks if there is the message when WM_NULL is returned.
Comment hidden (mozreview-request)
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
Comment hidden (mozreview-request)

Comment 7

a year ago
mozreview-review
Comment on attachment 8832845 [details]
Bug 1335670 NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone

https://reviewboard.mozilla.org/r/108722/#review110472
Attachment #8832845 - Flags: review?(m_kato) → review+

Comment 8

a year ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5d641acb1f4e
NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone r=m_kato

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d641acb1f4e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8832845 [details]
Bug 1335670 NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone

Approval Request Comment
[Feature/Bug causing the regression]: Long standing bug which we don't handle odd keyboard message behavior of Indian keyboard utility.
[User impact if declined]: Some users around India may meet this crash at typing something. See bug 962140 comment 144 for the detail.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No, because we're still not sure how to reproduce this bug but we have not gotten any regression reports.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: When we detect odd case, we have been crashing by ourselves. This patch avoids the crash in the specific case mentioned in comment 0. So, this doesn't change normal path.
[String changes made/needed]: No.
Attachment #8832845 - Flags: approval-mozilla-beta?
Attachment #8832845 - Flags: approval-mozilla-aurora?
FYI: Nobody of Aurora/Nightly channel testers has met this crash. So, the odd keyboard utility users are using only Release and Beta. So, it does not make sense to wait 12 weeks for such users.
Comment on attachment 8832845 [details]
Bug 1335670 NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone

We can take this in aurora first and see how it goes before uplifting to beta.  Aurora53+.
Attachment #8832845 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 13

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/673d97776e64
status-firefox53: affected → fixed
Comment on attachment 8832845 [details]
Bug 1335670 NativeKey should dispatch consumed keydown event when it receives WM_NULL at removing WM_*CHAR from the queue and the original message has gone

let's get this in 52.0b4 and see how things look, there don't seem to be (m?)any of these crashes in aurora
Attachment #8832845 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/fc4e87e2dcd5
status-firefox52: affected → fixed
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.