Last Comment Bug 685073 - Manage nested key events for IME
: Manage nested key events for IME
Status: RESOLVED FIXED
[inbound]
: inputmethod
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Markus Stange [:mstange]
Mentors:
Depends on:
Blocks: 477291
  Show dependency treegraph
 
Reported: 2011-09-06 23:29 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-10-05 05:10 PDT (History)
4 users (show)
masayuki: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part.1 Manage nested key events for IME (10.02 KB, patch)
2011-09-27 23:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch part.2 Consume key event which causes nested key event (4.72 KB, patch)
2011-09-27 23:55 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review
Patch part.1 Manage nested key events for IME (16.16 KB, patch)
2011-09-28 00:15 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review
Patch part.1 Manage nested key events for IME (16.32 KB, patch)
2011-10-03 07:11 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-06 23:29:08 PDT
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] (Mozilla Japan) 2011-09-27 23:52:05 PDT
Created attachment 562990 [details] [diff] [review]
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] (Mozilla Japan) 2011-09-27 23:55:14 PDT
Created attachment 562991 [details] [diff] [review]
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] (Mozilla Japan) 2011-09-28 00:15:49 PDT
Created attachment 562993 [details] [diff] [review]
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) 2011-09-29 12:35:33 PDT
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) 2011-09-29 12:47:57 PDT
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 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-30 18:15:31 PDT
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] (Mozilla Japan) 2011-09-30 18:16:47 PDT
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] 2011-10-01 02:50:26 PDT
merged backout to central.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-03 07:11:59 PDT
Created attachment 564178 [details] [diff] [review]
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] (Mozilla Japan) 2011-10-03 07:13:51 PDT
Ah, note that I changed only TextInputHandler.h from the previous patch. So, you don't need to check the other part again.

Note You need to log in before you can comment on or make changes to this bug.