Closed Bug 1343446 Opened 7 years ago Closed 7 years ago

Different WM_CHAR message may be removed when there was WM_CHAR whose wParam is NULL and the scancode is 0xFF

Categories

(Core :: Widget: Win32, defect, P3)

All
Windows
defect

Tracking

()

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

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: crash, Whiteboard: tpi:+)

Crash Data

Attachments

(1 file)

https://crash-stats.mozilla.com/report/index/d6c445c3-af30-4716-a1e9-6a2a62170211

> PeekMessage() removed unexpcted char message! 
> Active keyboard layout=0x40094009 (India), 
> Handling message: { message=WM_KEYDOWN, virtual keycode=VK_L, repeat count=1, scancode=0x26, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2030c, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=NULL (0x0000), repeat count=1, scancode=0xFF, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2030c, 
> Removed message: { message=WM_CHAR, character code='k' (0x006B), repeat count=1, scancode=0x26, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x2030c, 

https://crash-stats.mozilla.com/report/index/d337fd68-c458-4341-b21b-83a532170225
https://crash-stats.mozilla.com/report/index/44d9dfff-a0cf-4d5d-aaac-d057d2170225

> Active keyboard layout=0x04090409 (US), 
> 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=0x130c1c, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: { message=WM_CHAR, character code=NULL (0x0000), repeat count=1, scancode=0xFF, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x130c1c, 
> Removed message: { message=WM_CHAR, character code='k' (0x006B), repeat count=1, scancode=0x22, extended key=false, context code=false, previous key state=false, transition state=false, hwnd=0x130c1c, 

Although, these two cases occurred with different keyboard layout, the message queue state is exactly same. So, I guess that this is caused by odd key hook utility.

Perhaps, we should ignore the odd message which was retrieved by PeekMessage(PM_NOREMOVE) but I'm not sure the removed next char message is really next key message in the queue, though.

(Currently, this is only a remaining bug of bug 962140, <https://crash-stats.mozilla.com/signature/?product=Firefox&platform=Windows&version=52.0b4&version=52.0b5&version=52.0b6&version=52.0b7&version=52.0b8&version=52.0b9&version=52.0b99&version=52.0&version=52.0esr&signature=mozilla%3A%3Awidget%3A%3ANativeKey%3A%3AGetFollowingCharMessage&date=%3E%3D2017-02-01T05%3A08%3A00.000Z&date=%3C2017-03-01T05%3A10%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports>)
Status: NEW → ASSIGNED
Comment on attachment 8842325 [details]
Bug 1343446 NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd

Hmm, oddly, it's broken in review repository.
Attachment #8842325 - Attachment is obsolete: true
Attachment #8842325 - Flags: review?(m_kato)
Priority: -- → P3
Whiteboard: tpi:+
Comment on attachment 8842325 [details]
Bug 1343446 NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd

https://reviewboard.mozilla.org/r/116192/#review118186
Attachment #8842325 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c3563703a34d
NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd r=m_kato
https://hg.mozilla.org/mozilla-central/rev/c3563703a34d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8842325 [details]
Bug 1343446 NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd

Approval Request Comment
[Feature/Bug causing the regression]: Avoiding crash when we detect odd keyboard event message in the message queue.
[User impact if declined]:
A few users have reported this crash reports (see comment 0). They may meet this crash randomly (because it's too small numbers if such case occurs permanently).
[Is this code covered by automated tests?]:
No.
[Has the fix been verified in Nightly?]:
No, because we're not sure how to reproduce this crash (with what keyboard utility).
[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?]:
Because this makes only the case avoiding calling MOZ_CRASH().
[String changes made/needed]:
No.
Attachment #8842325 - Flags: approval-mozilla-aurora?
Comment on attachment 8842325 [details]
Bug 1343446 NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This fixes remaining bug of long standing crash bugs due to odd keyboard utility. ESR52 is the last version for WinXP and WinVista. So, taking this into ESR52 may make some users happy.
User impact if declined: 
User may meet this crash, but perhaps, the number of such users is a few.
Fix Landed on Version:
54. Currently, requesting to uplift to 53. And note that other fixes for this series has been fixed in 52. See bug 962140's blockers.
Risk to taking this patch (and alternatives if risky): 
Not risky because this avoids calling MOZ_CRASH() only when we detect the reported case (comment 0).
String or UUID changes made by this patch: 
No.
Attachment #8842325 - Flags: approval-mozilla-esr52?
Comment on attachment 8842325 [details]
Bug 1343446 NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd

Crash fix, let's take it on aurora 53. 
The fix missed the window for the first esr52, but we can keep tracking it in case of a dot release, or for 52.1.0
Attachment #8842325 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8842325 [details]
Bug 1343446 NativeKey::GetFollowingCharMessage() should ignore found message if PeekMessage(PM_REMOVE) retrieves different char message but the found odd char message was odd

crash fix for 52.1.0esr
Attachment #8842325 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.