1443421 - [GTK] IMContextWrapper shouldn't dispatch redundant "keydown" and "keyup" events

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement)

Description

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

As far as I've tested, Linux's IME behaves odd in these days.

I tested on Ubuntu 17.10 with both ibus and fcitx.

With both cases, we may receive two pairs of GDK_KEY_PRESS and GDK_KEY_RELEASE events. Then, gtk_im_context_filter_keypress() returns TRUE for first pair, but does nothing actually. The second pair is handled as usual. It seems that the preceding pair is used for checking something. E.g., whether application handles key events before IME or something.

Anyway, with current implementation, we dispatch redundant eKeyDown event and eKeyUp event for the preceding dummy key event pair and mark them as "processed by IME" since gtk_im_context_filter_keypress() returns TRUE.

Oddly, the event order could be both of the following:

1. GDK_KEY_PRESS
2. GDK_KEY_PRESS
3. GDK_KEY_RELEASE
4. GDK_KEY_RELEASE

vs.

1. GDK_KEY_PRESS
2. GDK_KEY_RELEASE
3. GDK_KEY_PRESS
4. GDK_KEY_RELEASE

I usually see the former order. However, sometimes I saw the latter order.

And first GDK_KEY_PRESS may be consumed by IME as usual. So, simply ignoring the first key event is wrong approach.

Actual logs are here:
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_PRESS, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 SetCursorPosition(aContext=0x0x7fb4d9e43e20), mCompositionTargetRange={ mOffset=4294967295, mLength=4294967295 }mSelection={ mOffset=4294967295, Length()=0, mWritingMode=Horizontal }
> E/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   SetCursorPosition(), FAILED, mCompositionTargetRange and mSelection are invalid
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   MaybeDispatchKeyEventAsProcessedByIME(), keydown or keyup event is dispatched
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=true (isFiltered=true, mFallbackToKeyEvent=false), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_PRESS, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnCommitCompositionNative(aContext=0x0x7fb4d9e43e20), current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, commitString="a", mProcessingKeyEvent=0x0x7fb4bd977ac0, IsComposingOn(aContext)=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnCommitCompositionNative(), we'll send normal key event
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=false (isFiltered=true, mFallbackToKeyEvent=true), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnSelectionChange(aCaller=0x0x7fb4e202c800, aIMENotification={ mSelectionChangeData={ mOffset=1, Length()=0, mReversed=false, mWritingMode=Horizontal, mCausedByComposition=false, mCausedBySelectionEvent=false, mOccurredDuringComposition=false } }), mCompositionState=NotComposing, mIsDeletingSurrounding=false, mRetrieveSurroundingSignalReceived=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_RELEASE, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 SetCursorPosition(aContext=0x0x7fb4d9e43e20), mCompositionTargetRange={ mOffset=4294967295, mLength=4294967295 }mSelection={ mOffset=1, Length()=0, mWritingMode=Horizontal }
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   MaybeDispatchKeyEventAsProcessedByIME(), keydown or keyup event is dispatched
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=true (isFiltered=true, mFallbackToKeyEvent=false), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false
> I/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900 OnKeyEvent(aCaller=0x0x7fb4e202c800, aEvent(0x0x7fb4bd977ac0): { type=GDK_KEY_RELEASE, keyval=a, unicode=0x61 }, aKeyboardEventWasDispatched=false), mMaybeInDeadKeySequence=false, mCompositionState=NotComposing, current context=0x0x7fb4d9e43e20, active context=0x0x7fb4d9e43e20, 
> D/nsGtkIMModuleWidgets 0x0x7fb4cf6cb900   OnKeyEvent(), succeeded, filterThisEvent=false (isFiltered=false, mFallbackToKeyEvent=false), mCompositionState=NotComposing, mMaybeInDeadKeySequence=false

Comment 1

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

But important and good hint is, all native key events are always same instance. Although I'll investigate what happens when I press another key while pressing a key.

Comment 2

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

According to the ibus's source code, ibus may post key event to engine process at first time. Then, returns true for preventing application handles the key event. After that, if the engine doesn't process the key event, same key event will be sent to the application again (held by gdk_event_copy(), therefore, the address is always same). However, it seems that this is ibus specific issue. Can we distinguish if active IM is ibus?

Comment 3

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

This is not ibus specific bug. Fcitx also posts GdkEventKey to different process.

Both of them store original event instance with using gdk_event_copy() (i.e., grabbing it with increasing its refcount) before dispatching the event to another process. Then, both of them adds a bit flag, 1 << 24 (defined as IBUS_HANDLED_MASK and FcitxKeyState_HandledMask), to GdkEventKey::state and return true for gtk_im_context_filter_keypress(). Unfortunately, even if it's handled synchronously, the bit flag is set by both of them. So, we cannot know if the event is dispatched to different process.  If we can know the refcount of GdkEventKey, we can know that, though.

Comment 4

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

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

Comment 5

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

Attachment: Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

ibus and fcitx have asynchronous key event handling mode and it's enabled in
default settings.  That is, when they receive a key event from application via
a call of gtk_im_context_filter_keypress(), they may post the key event
information to other IME process, then does nothing but store the copy of the
event with gdk_event_copy() and returns true for the result of
gtk_im_context_filter_keypress().  When the other IME process handles the
event, returns the result to them in our process.  Then, they send the stored
key event to us again.  Finally, they actually handles the event in our process
actually.

Therefore, we may receive every key event twice.  So, this causes dispatching
eKeyDown event and eKeyUp event twice.  Preceding key event is always marked
as "processed by IME" since gtk_im_context_filter_keypress() returns true
temporarily and following key event is dispatched as expected.  So, we need
to ignore the first event only when gtk_im_context_filter_keypress() returns
true but the event is posted to different process.

Unfortunately, we cannot know if the key event is actually posted to different
process directly.  However, we can know if active IM is ibus, fcitx or another
one and if ibus or fcitx is in asynchronous key handling mode.

The former information is provided by gtk_im_multicontext_get_context_id().
It returns a string which is set to the IM multicontext instance by creator.
We'll get "ibus" if IM is ibus, get "fcitx" if IM is fcitx.

The latter information is not provided.  However, they consider the mode from
env value.  ibus checks IBUS_ENABLE_SYNC_MODE.  fcitx checks both
IBUS_ENABLE_SYNC_MODE and FCITX_ENABLE_SYNC_MODE.

Additionally, we can know if received key event has already been posted to
other IME process.  They use undefined bit of GdkEventKey::state to store
if the key event has already been posted (1 << 25, they called "ignored" flag).
Although their approach is really hacky but we can refer the information at
least for now.

Finally, when we guess a key event is posted to other IME process, let's
IMContextWrapper::OnKeyEvent() not dispatch eKeyDown nor eKeyUp event.
Note that if it's handled synchronously as unexpected, it may causes
dispatching one or more composition events and/or delete content event.
So, in such case, we dispatch a keyboard event for processing key event
anyway.  There is only once case we'll fail to dispatch keyboard event.
If we receive signals to dispatch composition events or delete content
command event when IM receives the result from other IME process but
it doesn't send the key event to us.  This will be fixed by the following
patch.

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

Comment 6

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

Attachment: Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

ibus and fcitx usually post key event to other IME process, then, if it causes
some signals to updating composition string, they may not send the posted
key event to us again.  Then, IMContextWrapper dispatches neither eKeyDown nor
eKeyUp event since mProcessingKeyEvent becomes non-nullptr only while
OnKeyEvent() is called.  So, IMContextWrapper need to store key event if
OnKeyEvent() assumes that given key event is posted to different process.
Then, if IMContextWrapper receives some signals, it should dispatch eKeyDown
and eKeyUp event with stored key event.

Note that we cannot compare the pointer of first event and following event
directly even though usually both events are same address as far as I checked
because according to the source code of ibus, fcitx and GDK, they use
gdk_event_copy() to keep storing original event.  According to the document of
the API, it might just increment refcount.  However, the actual implementation
of the API always creates another instance and return it.  So, it might be
used same address by arena allocation or something accidentally.   Anyway,
we shouldn't compare them.  Instead, we need to compare each information of
two key events.  Unfortunately, we also cannot compare them simply.  Both
ibus and fcitx set unused bits of GdkEventKey::state to true when they send
back the event to us.  Therefore, we should compare some of or all of the
members by ourselves.  I think that matching time must be enough in most
cases since its value of native key events are properly set.  However, for
safer code, this patch also checks type, keyval and part of state.

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

Comment 7

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

Comment on attachment 8957759 [details]
Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226736/diff/1-2/

Comment 8

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

Comment on attachment 8957760 [details]
Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226738/diff/1-2/

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8957759 [details]
Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

https://reviewboard.mozilla.org/r/226736/#review232702

::: widget/gtk/IMContextWrapper.h:105
(Diff revision 2)
> +    // TODO: Typically, new IM comes every several years.  And now, our code
> +    //       becomes really IM behavior dependent.  So, perhaps, we need prefs
> +    //       to control related flags for IM developers.
> +    enum class IMContextID : uint8_t
> +    {
> +      eFcitx,

nits: indent is 4

::: widget/gtk/IMContextWrapper.h:116
(Diff revision 2)
> +
> +    static const char* GetIMContextIDName(IMContextID aIMContextID)
> +    {
> +        switch (aIMContextID) {
> +            case IMContextID::eFcitx:
> +              return "eFcitx";

nits: indent is 4

::: widget/gtk/IMContextWrapper.cpp:280
(Diff revision 2)
> +    // https://github.com/ibus/ibus/blob/86963f2f94d1e4fc213b01c2bc2ba9dcf4b22219/client/gtk2/ibusimcontext.c#L520-L537
> +    if (!env) {
> +        return false;
> +    }
> +    if (!env[0] ||
> +        !nsCRT::strcmp(env, "0") ||

Since you already check whether env is null, you should use strcmp instead of nsCRT::strcmp.  (ex. bug 798840)

Also, you might have to nsDependentString instead of PL_str or str\*.  (If possible, we shouldn't use str* functions due to unsafe.)

```
nsDependentString envStr(env)
if (envStr.IsEmpty() ||
    envStr.EqualLiteral("0") ||
...
```

::: widget/gtk/IMContextWrapper.cpp:299
(Diff revision 2)
> +    const char* env = PR_GetEnv(aEnv);
> +    if (!env) {
> +        return false;
> +    }
> +    if (!env[0] ||
> +        !nsCRT::strcmp(env, "0") ||

Use strcmp or nsDepedentString since env isn't always null

Comment 10

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

Comment on attachment 8957759 [details]
Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226736/diff/2-3/

Comment 11

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

Comment on attachment 8957760 [details]
Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226738/diff/2-3/

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8957760 [details]
Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

https://reviewboard.mozilla.org/r/226738/#review232744

Comment 13

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

Comment on attachment 8957759 [details]
Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226736/diff/3-4/

Comment 14

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

Comment on attachment 8957760 [details]
Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226738/diff/3-4/

Comment 15

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

Added eScim to IMContextID because I succeeded to create test environment with Debian. (But looks like that we don't need to support it aggressively since it's too old and not supported by mozc on Debian.)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edc1455e7082
part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process r=m_kato
https://hg.mozilla.org/integration/autoland/rev/6afa399e604a
part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again r=m_kato

Comment 17

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1443421) for Valgrind leak on Linux x64 opt

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=167886427&repo=autoland

Backout:
https://hg.mozilla.org/integration/autoland/rev/a2a0cf1b3b02f9a6147b79218bf48899937537a5

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6afa399e604a399e09742f16211ae69f22b3ab75

Comment 18

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

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

Comment 19

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

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

Comment 20

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

Comment on attachment 8957759 [details]
Bug 1443421 - part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226736/diff/4-5/

Comment 21

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

Comment on attachment 8957760 [details]
Bug 1443421 - part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226738/diff/4-5/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b93dcde52457
part 1: Make IMContextWrapper not dispatch eKeyDown and eKeyUp event if the native key event is being handled by other IME process r=m_kato
https://hg.mozilla.org/integration/autoland/rev/11c1e1afaaca
part 2: IMContextWrapper should dispatch eKeyDown or eKeyUp event as "processed by IME" even when IM sent some signals without sending key event again r=m_kato

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b93dcde52457
https://hg.mozilla.org/mozilla-central/rev/11c1e1afaaca

Comment 24

[Julien Cristau [:jcristau]]

Can this ride the trains to 61 or is there something we need to uplift?

Comment 25

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

It's not necessary since we don't dispatch keydown nor keyup event during composition in default settings yet.

Comment 26

[Julien Cristau [:jcristau]]

Thanks.