1336322 - WM_DEADCHAR message might have gone at removing it from the queue

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

https://crash-stats.mozilla.com/report/index/4ba63f64-4f3d-41d0-bc4f-e11102170202
> PeekMessage() failed to remove char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_BACK, repeat count=1, scancode=0x11, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2b01b0, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_DEADCHAR, character code='㕲' (0x3572), repeat count=1, scancode=0x11, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2b01b0, 
> WM_NULL has been removed: 0, 
> Next key message in all windows: { message=WM_DEADCHAR, character code=NULL (0x0000), repeat count=1, scancode=0x11, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2b01b0, time=8413593, 
> Next message in all windows: { message=WM_DEADCHAR, character code=NULL (0x0000), repeat count=1, scancode=0x11, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2b01b0, time=8413593

This is odd case. WM_DEADCHAR message has gone from the queue but next key message is still in the queue but its scancode is different key's.

I think that we should treat this case as consumed case.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d1562a73325

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Coming patch depends on bug 1336080.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19516432cfdd

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Bug 1336322 NativeKey::GetFollowingCharMessage() should treat the char message has gone if PeekMessage() failed to remove found char message and next key message becomes non-char message or different key's char message

This patch depends on bug 1336080.

When PeekMessage() fails to remove found char message, NativeKey::GetFollowingCharMessage() tries to check next key message in the queue again.  Then, when next key message becomes non-char message, such as WM_KEYDOWN or WM_KEYUP, the char message must be removed by odd keyboard layout or something.  Similarly, when next key message is a char message but it's caused by different key, the found char message must be removed by one of them too.

So, in these cases, NativeKey::GetFollowingCharMessage() should treat the key operation is already handled or canceled by the odd keyboard layout or somebody else.  Additionally, in the latter case, following char message should be handled as orphan char message(s) as usual.

Review commit: https://reviewboard.mozilla.org/r/109480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109480/

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8833244 [details]
Bug 1336322 NativeKey::GetFollowingCharMessage() should treat the char message has gone if PeekMessage() failed to remove found char message and next key message becomes non-char message or different key's char message

https://reviewboard.mozilla.org/r/109480/#review110950

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21f71652e491
NativeKey::GetFollowingCharMessage() should treat the char message has gone if PeekMessage() failed to remove found char message and next key message becomes non-char message or different key's char message r=m_kato

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21f71652e491

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Comment on attachment 8833244 [details]
Bug 1336322 NativeKey::GetFollowingCharMessage() should treat the char message has gone if PeekMessage() failed to remove found char message and next key message becomes non-char message or different key's char message

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]: Perhaps, this may depend on the patches for bug 1336080. (It will be fine if you land the uplifts from smaller bug number to larger number of this series of crash bugs.)
[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.

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 9

[Julien Cristau [:jcristau]]

Comment on attachment 8833244 [details]
Bug 1336322 NativeKey::GetFollowingCharMessage() should treat the char message has gone if PeekMessage() failed to remove found char message and next key message becomes non-char message or different key's char message

crash fix for aurora and 52.0b4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee21650b0037

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/066a9139b060