Closed Bug 1336028 Opened 3 years ago Closed 3 years ago

WM_*CHAR might be changed its wParam (and sometimes lParam too) while trying to remove found message from the queue

Categories

(Core :: Widget: Win32, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

There are some situations:

Case 1:
https://crash-stats.mozilla.com/report/index/f9c5a204-0066-45cc-a271-d8c772170129
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_A, repeat count=1, scancode=0x1E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0xa011c, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=BACKSPACE (0x0008), repeat count=1, scancode=0x1E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0xa011c, 
> Removed message: { message=WM_CHAR, character code='¢' (0x00A2), repeat count=1, scancode=0x1E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0xa011c, 

When it finds a char message in the queue, the wParam emulates Backspace, then, the wParam is changed to a character. It might be better to remove existing character first, though, we're not sure because nobody reports bugs around this crash.

Perhaps, we should "forget" the found char code in this case because scancode hasn't been changed, so, the newer char message which is actually removed from the queue should be the purpose of the key operation.

Case 2:
https://crash-stats.mozilla.com/report/index/50b6d2da-be56-458b-815e-f86a72170129
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_F, repeat count=1, scancode=0x21, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x300e0, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code='õ' (0x00F5), repeat count=1, scancode=0x21, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x300e0, 
> Removed message: { message=WM_CHAR, character code='F' (0x0046), repeat count=1, scancode=0x21, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x300e0, 

Similar to previous case, but this is changed from a Unicode to an ASCII character. This could be one of following scenarios:
1. The first Unicode character is expected, but due to bug of API hook of the keyboard layout, changed to ASCII character.
2. The second ASCII character is expected, there could be non-native modifier state and when WM_CHAR is removed from the queue, API hook changes the character.

I think that for consistency with Case 1, we should assume this is #2. Anyway, if this is #1, it should be fixed by the keyboard layout vendor.

Case 3:
https://crash-stats.mozilla.com/report/index/918a4307-0449-43e9-97bd-d25132170129
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_A, repeat count=1, scancode=0x1E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x200f4, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code='´' (0x00B4), repeat count=1, scancode=0x1E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x200f4, 
> Removed message: { message=WM_CHAR, character code='¢' (0x00A2), repeat count=1, scancode=0x1E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x200f4, 

https://crash-stats.mozilla.com/report/index/bcc1ddc6-7a82-4c50-993d-4e3f52170127
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_T, repeat count=1, scancode=0x14, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x10b42, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code='þ' (0x00FE), repeat count=1, scancode=0x14, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x10b42, 
> Removed message: { message=WM_CHAR, character code='T' (0x0054), repeat count=1, scancode=0x14, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x10b42, 

Similar to Case 2, but wParam is changed from a Unicode character to another Unicode character.

Case 4:
https://crash-stats.mozilla.com/report/index/17cbaac8-10bc-4b9c-812c-2a8102170128
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_SPACE, repeat count=1, scancode=0x39, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x102f2, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=SPACE (0x0020), repeat count=1, scancode=0x39, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x102f2, 
> Removed message: { message=WM_CHAR, character code='\' (0x005C), repeat count=1, scancode=0x39, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x102f2, 

https://crash-stats.mozilla.com/report/index/451785c0-f555-403f-a1ac-c943a2170128
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_S, repeat count=1, scancode=0x1F, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x1076c, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code='s' (0x0073), repeat count=1, scancode=0x1F, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x1076c, 
> Removed message: { message=WM_CHAR, character code='u' (0x0075), repeat count=1, scancode=0x1F, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x1076c, 

Changed from an ASCII character to another character. Moreover, looks like the first character might be expected. On the other hand, it might be possible that the keyboard layout wants to override usual keyboard layout at removing char message.  Then, the taking the second char message makes sense. (Anyway, I really want a bug report from the user!!)

Case 5:
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_G, repeat count=1, scancode=0x22, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x30144, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code='U' (0x0055), repeat count=1, scancode=0x22, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x30144, 
> Removed message: { message=WM_CHAR, character code='‹' (0x2039), repeat count=1, scancode=0x22, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x30144, 

Changed from an ASCII character to a Unicode character. Although, I don't know which keyboard layout inputs 'u' with KeyG, this could be another case of Case 4.


So, I think that when scan code hasn't been changed, taking newer char message makes sense with these cases.
Case 4:
https://crash-stats.mozilla.com/report/index/2533a35c-0bbf-4bde-a699-970332170201
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_C, repeat count=1, scancode=0x2E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x1019e, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=SPACE (0x0020), repeat count=1, scancode=0x2E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x1019e, 
> Removed message: { message=WM_CHAR, character code='c' (0x0063), repeat count=1, scancode=0x2E, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x1019e, 

https://crash-stats.mozilla.com/report/index/90cf2be0-c396-41f2-b700-06b0a2170201
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_B, repeat count=1, scancode=0x30, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x60366, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=SPACE (0x0020), repeat count=1, scancode=0x30, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x60366, 
> Removed message: { message=WM_CHAR, character code='“' (0x201C), repeat count=1, scancode=0x30, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x60366, 

https://crash-stats.mozilla.com/report/index/f86a45c7-fec5-4ce7-b14c-358ab2170201
> PeekMessage() removed unexpcted char message! 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_N, repeat count=1, scancode=0x31, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0xb03cc, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=SPACE (0x0020), repeat count=1, scancode=0x31, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0xb03cc, 
> Removed message: { message=WM_CHAR, character code='T' (0x0054), repeat count=1, scancode=0x31, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0xb03cc,
Comment on attachment 8832847 [details]
Bug 1336028 NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key

https://reviewboard.mozilla.org/r/109102/#review110508
Attachment #8832847 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b3d2a4cdecd1
NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key r=m_kato
https://hg.mozilla.org/mozilla-central/rev/b3d2a4cdecd1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8832847 [details]
Bug 1336028 NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key

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.

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.
Attachment #8832847 - Flags: approval-mozilla-beta?
Attachment #8832847 - Flags: approval-mozilla-aurora?
Comment on attachment 8832847 [details]
Bug 1336028 NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key

We can take this in aurora first and see how it goes before uplifting to beta.  Aurora53+.
Attachment #8832847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8832847 [details]
Bug 1336028 NativeKey::GetFollowingCharMessage() should take newer char message when found char message and removed message from the queue is different but their scancode indicates same physical key

crash fix for 52.0b4
Attachment #8832847 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.