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 r=m_kato

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.

MozReview-Commit-ID: FZSwN61v0Sd

--HG--
extra : rebase_source : e54284c27a171f899a6cf87a65935669e2d57021

widget/gtk/IMContextWrapper.cpp

@@ -551,6 +551,7 @@ IMContextWrapper::OnDestroyWindow(nsWindow* aWindow)
     mOwnerWindow = nullptr;
     mLastFocusedWindow = nullptr;
     mInputContext.mIMEState.mEnabled = IMEState::DISABLED;
+    mPostingKeyEvents.Clear();
 
     MOZ_LOG(gGtkIMLog, LogLevel::Debug,
         ("0x%p   OnDestroyWindow(), succeeded, Completely destroyed",
@@ -722,9 +723,11 @@ IMContextWrapper::OnKeyEvent(nsWindow* aCaller,
                     MOZ_LOG(gGtkIMLog, LogLevel::Info,
                         ("0x%p   OnKeyEvent(), aEvent->state has "
                          "IBUS_IGNORED_MASK, so, it won't be handled "
-                         "asynchronously anymore",
+                         "asynchronously anymore. Removing posted events from "
+                         "the queue",
                          this));
                     maybeHandledAsynchronously = false;
+                    mPostingKeyEvents.RemoveEvent(aEvent);
                     break;
                 }
                 break;
@@ -747,9 +750,11 @@ IMContextWrapper::OnKeyEvent(nsWindow* aCaller,
                     MOZ_LOG(gGtkIMLog, LogLevel::Info,
                         ("0x%p   OnKeyEvent(), aEvent->state has "
                          "FcitxKeyState_IgnoredMask, so, it won't be handled "
-                         "asynchronously anymore",
+                         "asynchronously anymore. Removing posted events from "
+                         "the queue",
                          this));
                     maybeHandledAsynchronously = false;
+                    mPostingKeyEvents.RemoveEvent(aEvent);
                     break;
                 }
                 break;
@@ -790,12 +795,25 @@ IMContextWrapper::OnKeyEvent(nsWindow* aCaller,
         filterThisEvent = false;
     }
 
-    // If IME handled the key event but we've not dispatched eKeyDown nor
-    // eKeyUp event yet, we need to dispatch here unless the key event is
-    // now being handled by other IME process.
-    if (filterThisEvent && !maybeHandledAsynchronously) {
-        MaybeDispatchKeyEventAsProcessedByIME();
-        // Be aware, the widget might have been gone here.
+    if (filterThisEvent && !mKeyboardEventWasDispatched) {
+        // If IME handled the key event but we've not dispatched eKeyDown nor
+        // eKeyUp event yet, we need to dispatch here unless the key event is
+        // now being handled by other IME process.
+        if (!maybeHandledAsynchronously) {
+            MaybeDispatchKeyEventAsProcessedByIME();
+            // Be aware, the widget might have been gone here.
+        }
+        // If we need to wait reply from IM, IM may send some signals to us
+        // without sending the key event again.  In such case, we need to
+        // dispatch keyboard events with a copy of aEvent.  Therefore, we
+        // need to use information of this key event to dispatch an KeyDown
+        // or eKeyUp event later.
+        else {
+            MOZ_LOG(gGtkIMLog, LogLevel::Info,
+                ("0x%p   OnKeyEvent(), putting aEvent into the queue...",
+                 this));
+            mPostingKeyEvents.PutEvent(aEvent);
+        }
     }
 
     mProcessingKeyEvent = nullptr;
@@ -1135,6 +1153,10 @@ IMContextWrapper::Focus()
 
     sLastFocusedContext = this;
 
+    // Forget all posted key events when focus is moved since they shouldn't
+    // be fired in different editor.
+    mPostingKeyEvents.Clear();
+
     gtk_im_context_focus_in(currentContext);
     mIsIMFocused = true;
     mSetCursorPositionOnKeyEvent = true;
@@ -1679,7 +1701,8 @@ IMContextWrapper::GetCompositionString(GtkIMContext* aContext,
 bool
 IMContextWrapper::MaybeDispatchKeyEventAsProcessedByIME()
 {
-    if (!mProcessingKeyEvent || mKeyboardEventWasDispatched ||
+    if ((!mProcessingKeyEvent && mPostingKeyEvents.IsEmpty()) ||
+        (mProcessingKeyEvent && mKeyboardEventWasDispatched) ||
         !mLastFocusedWindow) {
         return true;
     }
@@ -1694,7 +1717,28 @@ IMContextWrapper::MaybeDispatchKeyEventAsProcessedByIME()
     GtkIMContext* oldComposingContext = mComposingContext;
 
     RefPtr<nsWindow> lastFocusedWindow(mLastFocusedWindow);
-    mKeyboardEventWasDispatched = true;
+    if (mProcessingKeyEvent) {
+        mKeyboardEventWasDispatched = true;
+    }
+
+    // If we're not handling a key event synchronously, the signal may be
+    // sent by IME without sending key event to us.  In such case, we should
+    // dispatch keyboard event for the last key event which was posted to
+    // other IME process.
+    GdkEventKey* sourceEvent =
+        mProcessingKeyEvent ? mProcessingKeyEvent :
+                              mPostingKeyEvents.GetFirstEvent();
+
+    MOZ_LOG(gGtkIMLog, LogLevel::Info,
+        ("0x%p MaybeDispatchKeyEventAsProcessedByIME(), dispatch %s %s "
+         "event: { type=%s, keyval=%s, unicode=0x%X, state=%s, "
+         "time=%u, hardware_keycode=%u, group=%u }",
+         this, ToChar(sourceEvent->type == GDK_KEY_PRESS ? eKeyDown : eKeyUp),
+         mProcessingKeyEvent ? "processing" : "posted",
+         GetEventType(sourceEvent), gdk_keyval_name(sourceEvent->keyval),
+         gdk_keyval_to_unicode(sourceEvent->keyval),
+         GetEventStateName(sourceEvent->state, mIMContextID).get(),
+         sourceEvent->time, sourceEvent->hardware_keycode, sourceEvent->group));
 
     // Let's dispatch eKeyDown event or eKeyUp event now.  Note that only when
     // we're not in a dead key composition, we should mark the eKeyDown and
@@ -1706,13 +1750,22 @@ IMContextWrapper::MaybeDispatchKeyEventAsProcessedByIME()
     //      cannot cancel the following composition event.
     //      Spec bug: https://github.com/w3c/uievents/issues/180
     bool isCancelled;
-    lastFocusedWindow->DispatchKeyDownOrKeyUpEvent(mProcessingKeyEvent,
+    lastFocusedWindow->DispatchKeyDownOrKeyUpEvent(sourceEvent,
                                                    !mMaybeInDeadKeySequence,
                                                    &isCancelled);
-    MOZ_LOG(gGtkIMLog, LogLevel::Debug,
+    MOZ_LOG(gGtkIMLog, LogLevel::Info,
         ("0x%p   MaybeDispatchKeyEventAsProcessedByIME(), keydown or keyup "
          "event is dispatched",
          this));
+
+    if (!mProcessingKeyEvent) {
+        MOZ_LOG(gGtkIMLog, LogLevel::Info,
+            ("0x%p   MaybeDispatchKeyEventAsProcessedByIME(), removing first "
+             "event from the queue",
+             this));
+        mPostingKeyEvents.RemoveEvent(sourceEvent);
+    }
+
     if (lastFocusedWindow->IsDestroyed() ||
         lastFocusedWindow != mLastFocusedWindow) {
         MOZ_LOG(gGtkIMLog, LogLevel::Warning,

widget/gtk/IMContextWrapper.h

@@ -183,6 +183,100 @@ protected:
     // event.
     GdkEventKey* mProcessingKeyEvent;
 
+    /**
+     * GdkEventKeyQueue stores *copy* of GdkEventKey instances.  However, this
+     * must be safe to our usecase since it has |time| and the value should not
+     * be same as older event.
+     */
+    class GdkEventKeyQueue final
+    {
+    public:
+        ~GdkEventKeyQueue() { Clear(); }
+
+        void Clear()
+        {
+            if (!mEvents.IsEmpty()) {
+                RemoveEventsAt(0, mEvents.Length());
+            }
+        }
+
+        /**
+         * PutEvent() puts new event into the queue.
+         */
+        void PutEvent(const GdkEventKey* aEvent)
+        {
+            GdkEventKey* newEvent =
+                reinterpret_cast<GdkEventKey*>(
+                    gdk_event_copy(reinterpret_cast<const GdkEvent*>(aEvent)));
+            newEvent->state &= GDK_MODIFIER_MASK;
+            mEvents.AppendElement(newEvent);
+        }
+
+        /**
+         * RemoveEvent() removes oldest same event and its preceding events
+         * from the queue.
+         */
+        void RemoveEvent(const GdkEventKey* aEvent)
+        {
+            size_t index = IndexOf(aEvent);
+            if (NS_WARN_IF(index == mEvents.NoIndex)) {
+                return;
+            }
+            RemoveEventsAt(0, index + 1);
+        }
+
+        /**
+         * FirstEvent() returns oldest event in the queue.
+         */
+        GdkEventKey* GetFirstEvent() const
+        {
+            if (mEvents.IsEmpty()) {
+                return nullptr;
+            }
+            return mEvents[0];
+        }
+
+        bool IsEmpty() const { return mEvents.IsEmpty(); }
+
+    private:
+        nsTArray<GdkEventKey*> mEvents;
+
+        void RemoveEventsAt(size_t aStart, size_t aCount)
+        {
+            for (size_t i = aStart; i < aStart + aCount; i++) {
+                gdk_event_free(reinterpret_cast<GdkEvent*>(mEvents[i]));
+            }
+            mEvents.RemoveElementsAt(aStart, aCount);
+        }
+
+        size_t IndexOf(const GdkEventKey* aEvent) const
+        {
+            static_assert(!(GDK_MODIFIER_MASK & (1 << 24)),
+                "We assumes 25th bit is used by some IM, but used by GDK");
+            static_assert(!(GDK_MODIFIER_MASK & (1 << 25)),
+                "We assumes 26th bit is used by some IM, but used by GDK");
+            for (size_t i = 0; i < mEvents.Length(); i++) {
+                GdkEventKey* event = mEvents[i];
+                // It must be enough to compare only type, time, keyval and
+                // part of state.   Note that we cannot compaire two events
+                // simply since IME may have changed unused bits of state.
+                if (event->time == aEvent->time) {
+                    if (NS_WARN_IF(event->type != aEvent->type) ||
+                        NS_WARN_IF(event->keyval != aEvent->keyval) ||
+                        NS_WARN_IF(event->state !=
+                                       (aEvent->state & GDK_MODIFIER_MASK))) {
+                        continue;
+                    }
+                }
+                return i;
+            }
+            return mEvents.NoIndex;
+        }
+    };
+    // OnKeyEvent() append mPostingKeyEvents when it believes that a key event
+    // is posted to other IME process.
+    GdkEventKeyQueue mPostingKeyEvents;
+
     struct Range
     {
         uint32_t mOffset;
@@ -520,9 +614,11 @@ protected:
     /**
      * Dispatch an eKeyDown or eKeyUp event whose mKeyCode value is
      * NS_VK_PROCESSKEY and mKeyNameIndex is KEY_NAME_INDEX_Process if
-     * we're not in a dead key sequence, mProcessingKeyEvent is not nullptr
-     * and mKeyboardEventWasDispatched is still false.  If this dispatches
-     * a keyboard event, this sets mKeyboardEventWasDispatched to true.
+     * we're not in a dead key sequence, mProcessingKeyEvent is nullptr
+     * but mPostingKeyEvents is not empty or mProcessingKeyEvent is not
+     * nullptr and mKeyboardEventWasDispatched is still false.  If this
+     * dispatches a keyboard event, this sets mKeyboardEventWasDispatched
+     * to true.
      *
      * @return                      If the caller can continue to handle
      *                              composition, returns true.  Otherwise,