981958 - PeekMessage() sometimes fails (returns false) at removing found char message on MetroFirefox

Status: RESOLVED FIXED

(Core Graveyard :: Widget: WinRT, defect)

Description

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

When we try to remove found char message with PeekMessage(PM_REMOVE), PeekMessage() sometimes returns false and it causes a call of MOZ_CRASH().

Comment 1

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

This is very wired. We were not sure the found char message had already been removed by another application or still existed in the queue but PeekMessage() fails.

By landing attachment 8381319 [details] [diff] [review], in most cases, it seems that the found char message were removed from the queue. Therefore, we fixed this bug by stopping dispatching keypress event in such case since the removed message may come later or be just canceled.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #96)
> Oh...
> 
> https://crash-stats.mozilla.com/report/index/6df449ed-302f-49e6-bd0e-
> f86a12140224
> https://crash-stats.mozilla.com/report/index/4b6872b7-f710-4a55-8361-
> dce462140224
> https://crash-stats.mozilla.com/report/index/90952072-757c-4213-8149-
> 2d79a2140224
> 
> These are reported only from Metrofox.
> 
> > Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000020, lParam: 0x00390001, InSendMessageEx()=ISMEX_NOSEND, 
> > Found message: WM_CHAR (0x00000102), wParam: 0x00000020, lParam: 0x00390001
> 
> This might indicate we need to work more for this bug...
> # This log indicates PeekMessage(RM_REMOVE) failed due to no message.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #101)
> Created attachment 8381319 [details] [diff] [review]
> Log more details in the queue when PeekMessage() fails to remove found char
> message
> 
> I guess that failure of PeekMessage(PM_REMOVE) is caused by focus move or
> exiting apps. I want to see state of the queue. Let's land this on both m-c
> and Aurora. This is reported only from Metrofox of Aurora. I guess that the
> remaining crash occurs in really rare environment.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #102)
> I don't know if we should take the found char message or just ignore the
> phantom　char message because it might come later. This is reason why I want
> to see the message queue.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #104)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8335da20a00e

(In reply to Carsten Book [:Tomcat] from comment #105)
> https://hg.mozilla.org/mozilla-central/rev/8335da20a00e

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #113)
> Hmm...
> 
> https://crash-stats.mozilla.com/report/index/cad86ed7-093b-4e10-8d2b-
> 16df12140228
> https://crash-stats.mozilla.com/report/index/4108dcdb-aa41-4f2c-8958-
> 5856d2140301
> https://crash-stats.mozilla.com/report/index/6bc3ed50-5e98-4fb3-99a0-
> effd92140228
> https://crash-stats.mozilla.com/report/index/e9519835-bb57-493c-9198-
> c4ff82140228
> https://crash-stats.mozilla.com/report/index/29382059-998e-4bb5-a328-
> 88bfe2140228
> https://crash-stats.mozilla.com/report/index/ba22e1e1-756a-44ca-a85f-
> b0f2d2140302
> https://crash-stats.mozilla.com/report/index/62bcadb9-5485-4586-be72-
> 02c9b2140302
> 
> > Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000020, lParam: 0x00390001, hwnd=0x191258, InSendMessageEx()=ISMEX_NOSEND, 
> > Found message: WM_CHAR (0x00000102), wParam: 0x00000020, lParam: 0x00390001, 
> > WM_NULL has been removed: 1, 
> > There is no key message in any window, 
> > Next message in all windows: Unknown (0x0000C2ED), wParam: 0x00000000, lParam: 0x066F8970, hwnd=0x12124c, time=166724656
> 
> In these cases, found char message has gone after we found WM_NULL in the
> queue.
> 
> I think that we should just ignore the fact of found the message because it
> might be redirected to different thread or process or cancelled.
> 
> Only this report indicates such behavior:
> https://crash-stats.mozilla.com/report/index/9b0156a9-cb0a-4324-80b0-
> ad6c02140302
> 
> > Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000008, lParam: 0x000E0001, hwnd=0x30360, InSendMessageEx()=ISMEX_NOSEND, 
> > Found message: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, 
> > WM_NULL has been removed: 0, 
> > Next key message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x30360, time=10729640, 
> > Next message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x30360, time=10729640

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #114)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #113)
> > Only this report indicates such behavior:
> > https://crash-stats.mozilla.com/report/index/9b0156a9-cb0a-4324-80b0-
> > ad6c02140302
> > 
> > > Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000008, lParam: 0x000E0001, hwnd=0x30360, InSendMessageEx()=ISMEX_NOSEND, 
> > > Found message: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, 
> > > WM_NULL has been removed: 0, 
> > > Next key message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x30360, time=10729640, 
> > > Next message in all windows: WM_DEADCHAR (0x00000103), wParam: 0x00000020, lParam: 0x000E0001, hwnd=0x30360, time=10729640
> 
> Oops, I misread this log. This is really odd. There is the message which we
> want to remove from the queue. But PeekMessage() failed it.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #115)
> Created attachment 8384548 [details] [diff] [review]
> When PeekMessage() fails to remove found char message, just ignore it (not
> to dispatch keyprese event)
> 
> I think that if PeekMessage(PM_REMOVE) fails due to losing message, we
> shouldn't dispatch keypress event because system/user expect that the key
> event shouldn't cause text input.
> 
> Even with this patch, there will be still remaining minor cases. However, I
> guess that it's enough for next release.
> 
> I believe that we need to redesign WM_CHAR message handling in other bug.

(In reply to Jim Mathies [:jimm] (away 4/1-4/21) from comment #116)
> Comment on attachment 8384548 [details] [diff] [review]
> When PeekMessage() fails to remove found char message, just ignore it (not
> to dispatch keyprese event)
> 
> Review of attachment 8384548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.cpp
> @@ +1358,5 @@
> >                                 nextKeyMsg.message, nextKeyMsg.message,
> >                                 PM_REMOVE | PM_NOYIELD)) {
> > +      MSG nextKeyMsgInAllWindows;
> > +      // The char message is redirected to different thread's window by focus
> > +      // move or someting or just calcelled by external application.
> 
> nit - comment needs cleanup: 'someting', 'calcelled'

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #117)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/cb3551d93d56

(In reply to Carsten Book [:Tomcat] from comment #118)
> https://hg.mozilla.org/mozilla-central/rev/cb3551d93d56

Comment 2

[Manuela Muntean [Away]]

It seems that this issue is still reproducing, since bug 962140 currently has the ASSIGNED status and according to Socorro, there are builds with the 20140306171728 and 20140306171728 build IDs that crashed with this signature, in the last 4 weeks:

https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Awidget%3A%3ANativeKey%3A%3AGetFollowingCharMessage%28tagMSG%26%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&hang_type=any&date=2014-03-11+10%3A00%3A00&range_value=4#tab-reports

Comment 3

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

(In reply to Manuela Muntean [:Manuela] [QA] from comment #2)
> It seems that this issue is still reproducing, since bug 962140 currently
> has the ASSIGNED status and according to Socorro, there are builds with the
> 20140306171728 and 20140306171728 build IDs that crashed with this
> signature, in the last 4 weeks:

No, there are a lot of unexpected cases. We're researching what occurs in such cases and fixing one by one. This cannot distinguish from the signature because the crash point is same even the cause is different.

Comment 4

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I see there is a crash on 29.0 Beta (https://crash-stats.mozilla.com/report/index/b98dbda6-04e9-470e-8319-c687f2140321).
Per comment #3 I'm removing the "verifyme" keyword, since it seems there's nothing more we can do on the QA side for now.