Closed Bug 1334947 Opened 3 years ago Closed 3 years ago

When retrieving a WM_CHAR message, found message might have been removed and gets WM_NULL message instead

Categories

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

All
Windows
defect
Not set
critical

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)

Almost half of remaining crash of mozilla::widget::NativeKey::GetFollowingCharMessage(tagMSG&) is like this case:

> PeekMessage() failed to remove 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=0x100342, 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=0x100342, 
> WM_NULL has been removed: 1, 
> Next key message in all windows: { message=WM_KEYUP, virtual keycode=VK_V, repeat count=1, scancode=0x2F, extended key=false, context code=false, previous key state=true, transition state=true, hwnd=0x100342, time=5862033, 

Before retrieving getting WM_CHAR message from the queue, we peeked following WM_CHAR message. However, at removing a found char message, PeekMessage() returns TRUE but returning WM_NULL and no message in the queue already.

I'm not sure how this can cause, though. (I guess, PeekMessage(PM_REMOVE) is hooked by 3rd party's dll?) Makoto-san, do you have some idea about this issue (returned WM_NULL, instead of specified message)?

I think that when PeekMessage(PM_REMOVE) returns WM_NULL and there is no WM_CHAR message in the queue anymore, we should assume that it's already been handled correctly. I.e., let's treat the WM_KEYDOWN doesn't cause WM_CHAR.
Flags: needinfo?(m_kato)
(In reply to Masayuki Nakano [:masayuki] from comment #0)
> Almost half of remaining crash of
> mozilla::widget::NativeKey::GetFollowingCharMessage(tagMSG&) is like this
> case:
> 
> > PeekMessage() failed to remove 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=0x100342, 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=0x100342, 
> > WM_NULL has been removed: 1, 
> > Next key message in all windows: { message=WM_KEYUP, virtual keycode=VK_V, repeat count=1, scancode=0x2F, extended key=false, context code=false, previous key state=true, transition state=true, hwnd=0x100342, time=5862033, 
> 
> Before retrieving getting WM_CHAR message from the queue, we peeked
> following WM_CHAR message. However, at removing a found char message,
> PeekMessage() returns TRUE but returning WM_NULL and no message in the queue
> already.
> 
> I'm not sure how this can cause, though. (I guess, PeekMessage(PM_REMOVE) is
> hooked by 3rd party's dll?) Makoto-san, do you have some idea about this
> issue (returned WM_NULL, instead of specified message)?
> 
> I think that when PeekMessage(PM_REMOVE) returns WM_NULL and there is no
> WM_CHAR message in the queue anymore, we should assume that it's already
> been handled correctly. I.e., let's treat the WM_KEYDOWN doesn't cause
> WM_CHAR.

Humm, I don't know.  Some crash data seems to use C-DAC software product.  Also bp-c6084363-daf1-4dbb-8a51-b5f962170129 uses keyboard hook dll...
Flags: needinfo?(m_kato)
Thanks. And I also see Indian 3rd party keyboard produce in a crash report. So, I think that we've already handled normal cases completely. I believe that other products create impossible cases with odd hacks...
s/produce/product
In other words, the patch dispatches "consumed" keydown event if WM_*CHAR message is replaced with WM_NULL. Then, keypress won't be fired.

So, if thit happens, user will see no characters inputted into focused editor instead of this crash.
Comment on attachment 8831616 [details]
Bug 1334947 Treat a keydown event as inputting empty text if following char message has gone and gets WM_NULL message at calling PeekMessage() for removing a found char message

https://reviewboard.mozilla.org/r/108166/#review109772
Attachment #8831616 - Flags: review?(m_kato) → review+
Thank you for the review, but I'd like to land and uplift bug 1335306 before this for collecting which keyboard layout causes this crash.
Depends on: 1335306
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0eb11a6b2075
Treat a keydown event as inputting empty text if following char message has gone and gets WM_NULL message at calling PeekMessage() for removing a found char message r=m_kato
https://hg.mozilla.org/mozilla-central/rev/0eb11a6b2075
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8831616 [details]
Bug 1334947 Treat a keydown event as inputting empty text if following char message has gone and gets WM_NULL message at calling PeekMessage() for removing a found 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.
[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 #8831616 - Flags: approval-mozilla-beta?
Attachment #8831616 - 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 8831616 [details]
Bug 1334947 Treat a keydown event as inputting empty text if following char message has gone and gets WM_NULL message at calling PeekMessage() for removing a found char message

We can take this in aurora first and see how it goes before uplifting to beta.  Aurora53+.
Attachment #8831616 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8831616 [details]
Bug 1334947 Treat a keydown event as inputting empty text if following char message has gone and gets WM_NULL message at calling PeekMessage() for removing a found char message

let's take this on beta as well.
Attachment #8831616 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.