Closed Bug 685073 Opened 9 years ago Closed 8 years ago

Manage nested key events for IME


(Core :: Widget: Cocoa, defect)

Not set





(Reporter: masayuki, Assigned: masayuki)



(Keywords: inputmethod, Whiteboard: [inbound])


(2 files, 2 obsolete files)

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.
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.
Attachment #562990 - Flags: review?(smichaud)
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.
Attachment #562991 - Flags: review?(smichaud)
fix a nit in comment and outputs 8 lines around each change.
Attachment #562990 - Attachment is obsolete: true
Attachment #562990 - Flags: review?(smichaud)
Attachment #562993 - Flags: review?(smichaud)
Comment on attachment 562993 [details] [diff] [review]
Patch part.1 Manage nested key events for IME

This looks fine to me.
Attachment #562993 - Flags: review?(smichaud) → review+
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.
Attachment #562991 - Flags: review?(smichaud) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
backedout due to random crash of mochitest-chrome.
Resolution: FIXED → ---
merged backout to central.
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.
Attachment #562993 - Attachment is obsolete: true
Attachment #564178 - Flags: review?(smichaud)
Ah, note that I changed only TextInputHandler.h from the previous patch. So, you don't need to check the other part again.
Attachment #564178 - Flags: review?(smichaud) → review+
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.