The default bug view has changed. See this FAQ.

Manage nested key events for IME

RESOLVED FIXED in mozilla10

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla10
All
Mac OS X
inputmethod
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Attachment #562990 - Flags: review?(smichaud)
(Assignee)

Comment 2

6 years ago
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.
Attachment #562991 - Flags: review?(smichaud)
(Assignee)

Comment 3

6 years ago
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.
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+
(Assignee)

Comment 6

6 years ago
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
Flags: in-testsuite-
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/9c0aad93edc4
https://hg.mozilla.org/mozilla-central/rev/e734bd300268
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
(Assignee)

Comment 8

6 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

6 years ago
oops, the right changes are:

https://hg.mozilla.org/integration/mozilla-inbound/rev/14870847fafa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b7912b04ae
merged backout to central.
(Assignee)

Comment 11

6 years ago
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.
Attachment #562993 - Attachment is obsolete: true
Attachment #564178 - Flags: review?(smichaud)
(Assignee)

Comment 12

6 years ago
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+
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c37f7b456ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/788aeed429a5
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/788aeed429a5
https://hg.mozilla.org/mozilla-central/rev/7c37f7b456ca
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.