981963 - PeekMessage() may remove char message which is really different from found message (wParam of WM_DEADCHAR becomes 0)

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

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

This looks like rare case...

https://crash-stats.mozilla.com/report/index/cce66a0e-f415-4c95-aa7b-0e3342140226

> Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00001300, lParam: 0x00000000, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_DEADCHAR (0x00000103), wParam: 0x00001300, lParam: 0x00240001, 
> Removed message: WM_DEADCHAR (0x00000103), wParam: 0x00000000, lParam: 0x00000000

https://crash-stats.mozilla.com/report/index/7aff1c12-ef74-4e4b-afc0-d63642140227

> Handling message: WM_KEYDOWN (0x00000100), wParam: 0x00000030, lParam: 0x000B0001, InSendMessageEx()=ISMEX_NOSEND, 
> Found message: WM_CHAR (0x00000102), wParam: 0x00000030, lParam: 0x000B0001, 
> Removed message: WM_CHAR (0x00000102), wParam: 0x00000065, lParam: 0x00120001

Comment 1

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

I think that we should avoid only the former case for now. I don't see the latter report so much.

Comment 2

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

Attachment: Patch

Just ignore the strange char message if wParam of the char message is 0 (i.e., inputting null character).

I'll file new bug for the latter case. However, I believe that we need to redesign for fixing it. (We should collect following char messages in the constructor)

Comment 3

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5333bf0898ed

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5333bf0898ed

Comment 5

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

Comment on attachment 8394103 [details] [diff] [review]
Patch

[Approval Request Comment]
User impact if declined: This avoids to crash by ignoring strange message (inputting null character).
Testing completed (on m-c, etc.): Landed on m-c.
Risk to taking this patch (and alternatives if risky): Nothing. This change affects the path to call MOZ_CRASH().
String or IDL/UUID changes made by this patch: Nothing.

Comment 6

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

Comment on attachment 8394103 [details] [diff] [review]
Patch

[Approval Request Comment]
See comment 5.

Comment 7

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/dce0c66c6636

Comment 8

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

https://hg.mozilla.org/releases/mozilla-beta/rev/ca84c1c8ce16

Comment 9

[u279076]

I'm still seeing reports of this crash with builds where the fix landed:

Firefox 29: https://crash-stats.mozilla.com/report/index/8e63dafc-cd4a-4ada-b5b1-a64d32140415
Firefox 30: https://crash-stats.mozilla.com/report/index/05f34808-7742-4730-be96-414d22140413
Firefox 31: https://crash-stats.mozilla.com/report/index/be4791d5-d781-47e3-9a38-7bcef2140415

Should this be reopened or a follow-up bug be filed?

Comment 10

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

They are really different cases. See their App Notes. And perhaps, bug 992751 is the root cause of this series of bugs.

Comment 11

[u279076]

Thanks Masayuki. I'll assume this verified fixed based on the latest data and follow-up in bug 992751.