685073 - Manage nested key events for IME

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

We're now managing only one key event during keydown events.  However, IME might send two or more keydown events when we call interpretKeyEvents.  So, we should manage the processing key events by an array.

Comment 1

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

Attachment: Patch part.1 Manage nested key events for IME

I don't find actual problem of this issue. But current implementation is actually wrong and this isn't risky change. I think we should take this.

Comment 2

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

Attachment: Patch part.2 Consume key event which causes nested key event

If an interpretKeyEvents call causes multiple key events, newer event should be consume the older events because the older events shouldn't be processed by our editor or web applications.

Comment 3

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

Attachment: Patch part.1 Manage nested key events for IME

fix a nit in comment and outputs 8 lines around each change.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 562993 [details] [diff] [review]
Patch part.1 Manage nested key events for IME

This looks fine to me.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 562991 [details] [diff] [review]
Patch part.2 Consume key event which causes nested key event

This makes sense ... though any change of this nature is at least a little risky.

All the better to land it early in the release cycle and see if it causes trouble.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e734bd300268
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0aad93edc4

try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=42637a15103b

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9c0aad93edc4
https://hg.mozilla.org/mozilla-central/rev/e734bd300268

Comment 8

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

backedout due to random crash of mochitest-chrome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e734bd300268
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0aad93edc4

Comment 9

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

oops, the right changes are:

https://hg.mozilla.org/integration/mozilla-inbound/rev/14870847fafa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b7912b04ae

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

merged backout to central.

Comment 11

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

Attachment: Patch part.1 Manage nested key events for IME

The random crash is caused by accessing an invalid pointer of current key event.

nsTArray doesn't guarantee that the pointer is always valid. When InsertElementAt() or RemoveElementAt() is called, it re-allocates memory. At that time, stored pointer (in currentKeyEvent in each methods of TextInputHandler) may become invalid.

For resolving this issue, this patch sets KeyEventState pointers to the array instead of the copy instances. I.e., TextInputHandler allocates memory for them by itself. Therefore, the stored pointer is always valid even after KeydownEventHandler() calls interpretKeyEvents.

However, it might cause making memory fragment every keydown. That's too bad. Therefore, this patch adds mFirstKeyEvent which is always used for mCurrentKeyEvent[0].  I.e., if IME doesn't use nested key events, TextInputHandler never allocates another KeyEventState in heap.

I tried to create a test for nested key events. However, I realized that it's impossible. Gecko has "script blocker" mechanism which prevents that a javascript's event handler causes another DOM event dispatching. Currently, it doesn't assume that javascript event handler can cause a native event dispatching. Therefore, it can make second nested event accidentally and test_bug428405.xul does it. However, that fails emulating actual path (the test must not assume that the nested event uses another path). So, I should fix test_bug428405.xul and I should make nsDOMWindowUtils::SendKeyEvent() return error if event is nested.

Comment 12

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

Ah, note that I changed only TextInputHandler.h from the previous patch. So, you don't need to check the other part again.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c37f7b456ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/788aeed429a5

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/788aeed429a5
https://hg.mozilla.org/mozilla-central/rev/7c37f7b456ca