Bug 1516323 - Make IMContextWrapper::OnKeyEvent() check whether coming key event is already in the posting event queue. r=m_kato, a=RyanVM
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 26 Dec 2018 08:17:32 +0000
changeset 509206 4cb9d2d57e7d323c9759464a54b6a6dd27144a70
parent 509205 27ce905c5ac017098d33c3e402e974bc8d4444c6
child 509207 dac7e82e8fcf945990335c5efe7d12553e18de60
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, RyanVM
bugs1516323, 1498823
milestone65.0
Bug 1516323 - Make IMContextWrapper::OnKeyEvent() check whether coming key event is already in the posting event queue. r=m_kato, a=RyanVM According to the log of bug 1498823, ibus won't set IBUS_IGNORED_MASK to modifier flags when it synthesizes the event for asynchronous handling in some environments. Currently, we assume that iBus and Fcitx set IBUS_IGNORED_MASK or FcitxKeyState_IgnoredMask. So, we put both real key events and synthesized key events into the posting event queue and that causes using a lot of memory until the editor is blurred. Fortunately, timestamp of synthesized key events are always same as their original events. Therefore, we can look for original event from the positing event queue. Although we have gotten no bug reports about this issue of Fcitx, but this patch adds same hack for Fcitx too because the runtime cost is not expensive but the symptom is really serious. Differential Revision: https://phabricator.services.mozilla.com/D15321
widget/gtk/IMContextWrapper.cpp
widget/gtk/IMContextWrapper.h
--- a/widget/gtk/IMContextWrapper.cpp
+++ b/widget/gtk/IMContextWrapper.cpp
@@ -833,16 +833,34 @@ KeyHandlingState IMContextWrapper::OnKey
     switch (mIMContextID) {
       case IMContextID::eIBus: {
         // See src/ibustypes.h
         static const guint IBUS_IGNORED_MASK = 1 << 25;
         // If IBUS_IGNORED_MASK was set to aEvent->state, the event
         // has already been handled by another process and it wasn't
         // used by IME.
         bool isHandlingAsyncEvent = !!(aEvent->state & IBUS_IGNORED_MASK);
+        if (!isHandlingAsyncEvent) {
+          // On some environments, IBUS_IGNORED_MASK flag is not set as
+          // expected.  In such case, we keep pusing all events into the queue.
+          // I.e., that causes eating a lot of memory until it's blurred.
+          // Therefore, we need to check whether there is same timestamp event
+          // in the queue.  This redundant cost should be low because in most
+          // causes, key events in the queue should be 2 or 4.
+          isHandlingAsyncEvent =
+              mPostingKeyEvents.IndexOf(aEvent) != GdkEventKeyQueue::NoIndex();
+          if (isHandlingAsyncEvent) {
+            MOZ_LOG(gGtkIMLog, LogLevel::Info,
+                    ("0x%p   OnKeyEvent(), aEvent->state does not have "
+                     "IBUS_IGNORED_MASK but "
+                     "same event in the queue.  So, assuming it's a "
+                     "synthesized event",
+                     this));
+          }
+        }
 
         // ibus won't send back key press events in a dead key sequcne.
         if (mMaybeInDeadKeySequence && aEvent->type == GDK_KEY_PRESS) {
           maybeHandledAsynchronously = false;
           if (isHandlingAsyncEvent) {
             isUnexpectedAsyncEvent = true;
             break;
           }
@@ -861,35 +879,55 @@ KeyHandlingState IMContextWrapper::OnKey
         if (mInputContext.mIMEState.mEnabled == IMEState::PASSWORD) {
           maybeHandledAsynchronously = false;
           isUnexpectedAsyncEvent = isHandlingAsyncEvent;
           break;
         }
 
         if (isHandlingAsyncEvent) {
           MOZ_LOG(gGtkIMLog, LogLevel::Info,
-                  ("0x%p   OnKeyEvent(), aEvent->state has "
-                   "IBUS_IGNORED_MASK, so, it won't be handled "
-                   "asynchronously anymore. Removing posted events from "
-                   "the queue",
+                  ("0x%p   OnKeyEvent(), aEvent->state has IBUS_IGNORED_MASK "
+                   "or aEvent is in the "
+                   "posting event queue, so, it won't be handled "
+                   "asynchronously anymore. Removing "
+                   "the posted events from the queue",
                    this));
           maybeHandledAsynchronously = false;
           mPostingKeyEvents.RemoveEvent(aEvent);
           break;
         }
         break;
       }
       case IMContextID::eFcitx: {
         // See src/lib/fcitx-utils/keysym.h
         static const guint FcitxKeyState_IgnoredMask = 1 << 25;
         // If FcitxKeyState_IgnoredMask was set to aEvent->state,
         // the event has already been handled by another process and
         // it wasn't used by IME.
         bool isHandlingAsyncEvent =
             !!(aEvent->state & FcitxKeyState_IgnoredMask);
+        if (!isHandlingAsyncEvent) {
+          // On some environments, FcitxKeyState_IgnoredMask flag *might* be not
+          // set as expected. If there were such cases, we'd keep pusing all
+          // events into the queue.  I.e., that would cause eating a lot of
+          // memory until it'd be blurred.  Therefore, we should check whether
+          // there is same timestamp event in the queue.  This redundant cost
+          // should be low because in most causes, key events in the queue
+          // should be 2 or 4.
+          isHandlingAsyncEvent =
+              mPostingKeyEvents.IndexOf(aEvent) != GdkEventKeyQueue::NoIndex();
+          if (isHandlingAsyncEvent) {
+            MOZ_LOG(gGtkIMLog, LogLevel::Info,
+                    ("0x%p   OnKeyEvent(), aEvent->state does not have "
+                     "FcitxKeyState_IgnoredMask "
+                     "but same event in the queue.  So, assuming it's a "
+                     "synthesized event",
+                     this));
+          }
+        }
 
         // fcitx won't send back key press events in a dead key sequcne.
         if (mMaybeInDeadKeySequence && aEvent->type == GDK_KEY_PRESS) {
           maybeHandledAsynchronously = false;
           if (isHandlingAsyncEvent) {
             isUnexpectedAsyncEvent = true;
             break;
           }
@@ -905,19 +943,20 @@ KeyHandlingState IMContextWrapper::OnKey
         }
 
         // fcitx handles key events asynchronously even if focused
         // editor cannot use IME actually.
 
         if (isHandlingAsyncEvent) {
           MOZ_LOG(gGtkIMLog, LogLevel::Info,
                   ("0x%p   OnKeyEvent(), aEvent->state has "
-                   "FcitxKeyState_IgnoredMask, so, it won't be handled "
-                   "asynchronously anymore. Removing posted events from "
-                   "the queue",
+                   "FcitxKeyState_IgnoredMask or aEvent is in "
+                   "the posting event queue, so, it won't be handled "
+                   "asynchronously anymore. "
+                   "Removing the posted events from the queue",
                    this));
           maybeHandledAsynchronously = false;
           mPostingKeyEvents.RemoveEvent(aEvent);
           break;
         }
         break;
       }
       default:
@@ -995,21 +1034,24 @@ KeyHandlingState IMContextWrapper::OnKey
     // GDK_KEY_RELEASE since it may not be filtered by active keyboard
     // layout even in composition.
     mMaybeInDeadKeySequence = false;
   }
 
   MOZ_LOG(gGtkIMLog, LogLevel::Debug,
           ("0x%p   OnKeyEvent(), succeeded, filterThisEvent=%s "
            "(isFiltered=%s, mFallbackToKeyEvent=%s, "
-           "maybeHandledAsynchronously=%s), mCompositionState=%s, "
-           "mMaybeInDeadKeySequence=%s",
+           "maybeHandledAsynchronously=%s), mPostingKeyEvents.Length()=%zu, "
+           "mCompositionState=%s, mMaybeInDeadKeySequence=%s, "
+           "mKeyboardEventWasDispatched=%s, mKeyboardEventWasConsumed=%s",
            this, ToChar(filterThisEvent), ToChar(isFiltered),
            ToChar(mFallbackToKeyEvent), ToChar(maybeHandledAsynchronously),
-           GetCompositionStateName(), ToChar(mMaybeInDeadKeySequence)));
+           mPostingKeyEvents.Length(), GetCompositionStateName(),
+           ToChar(mMaybeInDeadKeySequence), ToChar(mKeyboardEventWasDispatched),
+           ToChar(mKeyboardEventWasConsumed)));
 
   if (filterThisEvent) {
     return KeyHandlingState::eHandled;
   }
   // If another call of this method has already dispatched eKeyDown event,
   // we should return KeyHandlingState::eNotHandledButEventDispatched because
   // the caller should've stopped handling the event if preceding eKeyDown
   // event was consumed.
--- a/widget/gtk/IMContextWrapper.h
+++ b/widget/gtk/IMContextWrapper.h
@@ -231,17 +231,17 @@ class IMContextWrapper final : public Te
     }
 
     /**
      * 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)) {
+      if (NS_WARN_IF(index == GdkEventKeyQueue::NoIndex())) {
         return;
       }
       RemoveEventsAt(0, index + 1);
     }
 
     /**
      * FirstEvent() returns oldest event in the queue.
      */
@@ -249,26 +249,18 @@ class IMContextWrapper final : public Te
       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);
-    }
-
+    static size_t NoIndex() { return nsTArray<GdkEventKey*>::NoIndex; }
+    size_t Length() const { return mEvents.Length(); }
     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
@@ -278,17 +270,27 @@ class IMContextWrapper final : public Te
           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;
+      return GdkEventKeyQueue::NoIndex();
+    }
+
+   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);
     }
   };
   // OnKeyEvent() append mPostingKeyEvents when it believes that a key event
   // is posted to other IME process.
   GdkEventKeyQueue mPostingKeyEvents;
 
   struct Range {
     uint32_t mOffset;