1302956 - NativeKey shouldn't run recursively

Status: RESOLVED FIXED

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

Description

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

The main cause of bug 962140 is, PeekMessage() causes the message queue receiving next message (perhaps, due to odd API hook or something).

Basically, NativeKey shouldn't run recursively. NativeKey should manage itself as far as cannot run recursively.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf454c29fac

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d778d994d1bd

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59d9463a2b98

Comment 4

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

Attachment: Bug 1302956 part.1 NativeKey should store latest instance with sLatestInstance

For detecting nested creation of NativeKey instances, NativeKey should manage the latest instance with sLastestInstance for the other instances and previous instance with mLastInstance.

Review commit: https://reviewboard.mozilla.org/r/80696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80696/

Comment 5

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

Attachment: Bug 1302956 part.2 NativeKey shouldn't handle char message if it's created during another instance is trying to remove a char message from the queue

When NativeKey tries to remove a char message from the queue, another NativeKey instance might be created due to odd API hook or something.  In such case, the old instance should handle the received message and the new instance should do nothing for keeping the order of message handling.

This patch makes:

* NativeKey::GetFollowingCharMsg() store removing char message to mRemovingMsg before calling PeekMessage() with PM_REMOVE.
* NativeKey::InitWithChar() set the old instance's mReceivedMsg to the received char message and do nothing even if HandleCharMessage() is called later.
* NativeKey::GetFollowingCharMsg() return received char message if mReceivedMsg is set during a call of PeekMessage().

Review commit: https://reviewboard.mozilla.org/r/80698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/80698/

Comment 6

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

I assume (and hope!) that these patches will fix bug 962140 completely.

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8794147 [details]
Bug 1302956 part.1 NativeKey should store latest instance with sLatestInstance

https://reviewboard.mozilla.org/r/80696/#review79646

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8794148 [details]
Bug 1302956 part.2 NativeKey shouldn't handle char message if it's created during another instance is trying to remove a char message from the queue

https://reviewboard.mozilla.org/r/80698/#review79672

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2903829cac14
part.1 NativeKey should store latest instance with sLatestInstance r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3c4ca5845f2c
part.2 NativeKey shouldn't handle char message if it's created during another instance is trying to remove a char message from the queue r=m_kato

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2903829cac14
https://hg.mozilla.org/mozilla-central/rev/3c4ca5845f2c

Comment 11

[Gerry Chang [:gchang]]

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.

Comment 12

[[:philipp]]

hi, crashes with this signature are still around and increasing in 52.0b1... should we reopen the bug or should a new one be filed for those?

Comment 13

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

(In reply to [:philipp] from comment #12)
> hi, crashes with this signature are still around and increasing in 52.0b1...
> should we reopen the bug or should a new one be filed for those?

No, we've already had bug 992751.

Comment 14

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

And we wasted 4 months for getting the result because such users are not using Nightly nor Aurora, sigh...

Comment 15

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

(In reply to Masayuki Nakano [:masayuki] from comment #13)
> (In reply to [:philipp] from comment #12)
> > hi, crashes with this signature are still around and increasing in 52.0b1...
> > should we reopen the bug or should a new one be filed for those?
> 
> No, we've already had bug 992751.

Oops, bug 962140. I think that bug 992751 was fixed by the patches for this bug.