981960 - PeekMessage(PM_REMOVE) sometimes fails to remove found char message (returns false) even if the message still in the queue

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://crash-stats.mozilla.com/report/index/d2cd4736-3fa9-4736-a8c8-b86ef2140310
https://crash-stats.mozilla.com/report/index/e8165a55-6747-4b10-8a1b-df5c82140310

> Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000008, lParam: 0x000E0001, hwnd=0x1500b6, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x1500b6, 
> WM_NULL has been removed: 0, 
> Next key message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x1500b6, time=7724890, 
> Next message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x1500b6, time=7724890

> Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000008, lParam: 0x000E0001, hwnd=0x1202e0, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x1202e0, 
> WM_NULL has been removed: 0, 
> Next key message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x1202e0, time=18573562, 
> Next message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x1202e0, time=18573562

This is very wired. PeekMessage() fails to remove found char message which still exists in the queue...

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://crash-stats.mozilla.com/report/index/a03ed936-3cba-4b56-83f5-3828f2140309

> Handling message: WM_SYSKEYDOWN (0x00000104), wParam: 0x00000035, lParam: 0x20060001, hwnd=0x106f2, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_CHAR (0x00000102), wParam: 0x000000BD, lParam: 0x20060001, hwnd=0x106f2, 
> WM_NULL has been removed: 0, 
> Next key message in all windows: WM_CHAR (0x00000102), wParam: 0x000000BD, lParam: 0x20060001, hwnd=0x106f2, time=120333875, 
> Next message in all windows: WM_CHAR (0x00000102), wParam: 0x000000BD, lParam: 0x20060001, hwnd=0x106f2, time=120333875

This is a WM_CHAR case.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://crash-stats.mozilla.com/report/index/34c5bb82-77f7-4e77-8c9f-2108c2140310

> Handling message: WM_SYSKEYDOWN (0x00000104), wParam: 0x0000000D, lParam: 0x201C0001, hwnd=0x102f4, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_CHAR (0x00000102), wParam: 0x00000000, lParam: 0x201C0001, hwnd=0x102f4, 
> WM_NULL has been removed: 0, 
> Next key message in all windows: WM_CHAR (0x00000102), wParam: 0x00000000, lParam: 0x201C0001, hwnd=0x102f4, time=1520703, 
> Next message in all windows: WM_CHAR (0x00000102), wParam: 0x00000000, lParam: 0x201C0001, hwnd=0x102f4, time=1520703

This is very rare. The wParam (input character) is a null character.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I realized that these crashes occurred GetFollowingCharMsg() is called by DispatchPluginEventsAndDiscardsCharMessages().

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Retry to remove found char message with GetMessage() when PeekMessage() fails to remove the message from the queue

This is really experimental patch.

When the message queue has the found char message but PeekMessage() with PM_REMOVE fails to remove it, let's retry with GetMessage().

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that if the patch cannot fix this bug and we don't find other approaches for this, we should not dispatch keypress event in this case because the char message must come later.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6df586c17279

We need to uplift this since no Nightly testers report this crash :-(

Comment 7

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6df586c17279
> 
> We need to uplift this since no Nightly testers report this crash :-(

Is this showing up on desktop?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #7)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/6df586c17279
> > 
> > We need to uplift this since no Nightly testers report this crash :-(
> 
> Is this showing up on desktop?

Yes.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6df586c17279

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

As this is a Metro topcrash and we're not shipping Metro yet in FF28, we will not need to track this one.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Lukas Blakk [:lsblakk] from comment #10)
> As this is a Metro topcrash and we're not shipping Metro yet in FF28, we
> will not need to track this one.

Yeah, but the crash also occurs on desktop Firefox. So, when I request uplift the patch(es), is it possible to be allowed? Unfortunately, no Nightly testers report this crash. So, we need to uplift the experimental patch and if it'll fail to fix this bug, we should land next patch which completely avoids the crash in this situation.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I mean, not for 28, it's too late. I think that we should fix this bug at least on 29.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> I mean, not for 28, it's too late. I think that we should fix this bug at
> least on 29.

Yes, nominate for uplift to Aurora if it's a  low risk landing.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8389496 [details] [diff] [review]
Retry to remove found char message with GetMessage() when PeekMessage() fails to remove the message from the queue

[Approval Request Comment]
User impact if declined: If we're lucky, this patch can fix this bug. The cause of this bug is, Win32 API's undocumented behavior. This patch retry with another API when we meet the unexpected situation. Unluckily, we have no Nightly testers who can reproduce this bug. We need to uplift this patch to check if this patch can fix this bug.
Testing completed (on m-c, etc.): Landed on m-c.
Risk to taking this patch (and alternatives if risky): The risk must be low. This change doesn't affect to normal path.
String or IDL/UUID changes made by this patch: No.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2b9f2c873ff4

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: It seems that only one tester meets this crash on Aurora. The latest crash report takes 5 days from previous report. I think that we need to watch a couple of weeks if it's actually fixed. Fortunately, Aurora becomes Beta next week. So, we probably see new crash report in next week if it's not fixed.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Very interesting. The patch improves the crash frequency actually. However, there are following crash reports:

https://crash-stats.mozilla.com/report/index/67ffc2e6-9768-466e-9fd5-4eb202140322
> PeekMessage() failed to remove char message! 
> Handling message: WM_KEYDOWN (0x00000100), wParam: 0x0000000D, lParam: 0x001C0001, hwnd=0x1301e8, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_CHAR (0x00000102), wParam: 0x0000000D, lParam: 0x001C0001, hwnd=0x1301e8, 
> WM_NULL has been removed: 1, 
> Next key message in all windows: WM_KEYUP (0x00000101), wParam: 0x0000000D, lParam: 0xC01C0001, hwnd=0x1301e8, time=152910656, 
> Next message in all windows: Unknown (0x0000C1B3), wParam: 0x00000000, lParam: 0x070FAB00, hwnd=0x1703c8, time=152910734

https://crash-stats.mozilla.com/report/index/d1246ae5-6d8f-41ee-850f-395d92140324
> PeekMessage() failed to remove char message! 
> Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000020, lParam: 0x00390001, hwnd=0x30320, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_CHAR (0x00000102), wParam: 0x00000020, lParam: 0x00390001, hwnd=0x30320, 
> WM_NULL has been removed: 1, 
> Next key message in all windows: WM_KEYUP (0x00000101), wParam: 0x00000020, lParam: 0xC0390001, hwnd=0x30320, time=7563968, 
> Next message in all windows: Unknown (0x0000C183), wParam: 0x00000000, lParam: 0x065FAAB0, hwnd=0x902f4, time=7563968

In these reports, the found WM_CHAR message are really lost.

So, this bug has been fixed by the patch. But we need more work for the remaining case.