Bug 1423456 - ContentCacheInParent::OnEventNeedingAckHandled() shouldn't decrement mPendingCompositionCount when it receives eCompositionCommit(AsIs) from the remote process but the composition has already been committed by a request to commit composition r=m_kato a=lizzard
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 06 Dec 2017 15:07:41 +0900
changeset 445315 df953324066b58824a34bcdedf7704674452b60d
parent 445314 1dc54b85de4dc4591fa46dab70a31ed79be4e41d
child 445316 23dd8e97b482d748b5c5ebea0a1d9d679bd66866
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, lizzard
bugs1423456, 1420849
milestone58.0
Bug 1423456 - ContentCacheInParent::OnEventNeedingAckHandled() shouldn't decrement mPendingCompositionCount when it receives eCompositionCommit(AsIs) from the remote process but the composition has already been committed by a request to commit composition r=m_kato a=lizzard After fixing bug 1420849, remote process started to ignore composition events after it synthesizes eCompositionCommit event after requesting to commit composition. However, remote process still keeps returning composition events when it receives from the main process. So, if the main process has already sent eCompositionCommit(AsIs) event when it's requested to commit composition from the remote process, ContentCacheInParent::OnEventNeedingAckHandled() receives both eCompositionCommitRequestHandled and eCompositionCommit(AsIs) events for *a* composition. Therefore, mPendingCompositionCount may be decremented twice for a composition. This causes hitting MOZ_DIAGNOSTIC_ASSERT. So, ContentCacheInParent need to manage if sent composition events are ignored or not. Then, ContentCacheInParent::OnEventNeedingAckHandled() stops decrementing mPendingCompositionCount when it receives eCompositionCommit(AsIs) events which are ignored by the remote process. This patch manages it with |mIsChildIgnoringCompositionEvents| and changes |bool mIsPendingLastCommitEvent| to |uint8_t mPendingCommitCount| for making ContentCache be able to manage multiple pending commit events if its remote process is too busy. MozReview-Commit-ID: CYQDeZXl7TJ
widget/ContentCache.cpp
widget/ContentCache.h
--- a/widget/ContentCache.cpp
+++ b/widget/ContentCache.cpp
@@ -519,18 +519,19 @@ ContentCacheInChild::SetSelection(nsIWid
 ContentCacheInParent::ContentCacheInParent(TabParent& aTabParent)
   : ContentCache()
   , mTabParent(aTabParent)
   , mCommitStringByRequest(nullptr)
   , mPendingEventsNeedingAck(0)
   , mCompositionStartInChild(UINT32_MAX)
   , mPendingCommitLength(0)
   , mPendingCompositionCount(0)
+  , mPendingCommitCount(0)
   , mWidgetHasComposition(false)
-  , mIsPendingLastCommitEvent(false)
+  , mIsChildIgnoringCompositionEvents(false)
 {
 }
 
 void
 ContentCacheInParent::AssignContent(const ContentCache& aOther,
                                     nsIWidget* aWidget,
                                     const IMENotification* aNotification)
 {
@@ -1102,26 +1103,28 @@ ContentCacheInParent::GetCaretRect(uint3
 
 bool
 ContentCacheInParent::OnCompositionEvent(const WidgetCompositionEvent& aEvent)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("0x%p OnCompositionEvent(aEvent={ "
      "mMessage=%s, mData=\"%s\" (Length()=%u), mRanges->Length()=%zu }), "
      "mPendingEventsNeedingAck=%u, mWidgetHasComposition=%s, "
-     "mPendingCompositionCount=%u, mCommitStringByRequest=0x%p",
+     "mPendingCompositionCount=%" PRIu8 ", mPendingCommitCount=%" PRIu8 ", "
+     "mIsChildIgnoringCompositionEvents=%s, mCommitStringByRequest=0x%p",
      this, ToChar(aEvent.mMessage),
      GetEscapedUTF8String(aEvent.mData).get(), aEvent.mData.Length(),
      aEvent.mRanges ? aEvent.mRanges->Length() : 0, mPendingEventsNeedingAck,
      GetBoolName(mWidgetHasComposition), mPendingCompositionCount,
+     mPendingCommitCount, GetBoolName(mIsChildIgnoringCompositionEvents),
      mCommitStringByRequest));
 
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
   mDispatchedEventMessages.AppendElement(aEvent.mMessage);
-#endif // #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+#endif // #ifdef defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
   // We must be able to simulate the selection because
   // we might not receive selection updates in time
   if (!mWidgetHasComposition) {
     if (aEvent.mWidget && aEvent.mWidget->PluginHasFocus()) {
       // If focus is on plugin, we cannot get selection range
       mCompositionStart = 0;
     } else if (mCompositionStartInChild != UINT32_MAX) {
@@ -1140,17 +1143,17 @@ ContentCacheInParent::OnCompositionEvent
 
   mWidgetHasComposition = !aEvent.CausesDOMCompositionEndEvent();
 
   if (!mWidgetHasComposition) {
     mCompositionStart = UINT32_MAX;
     if (mPendingCompositionCount == 1) {
       mPendingCommitLength = aEvent.mData.Length();
     }
-    mIsPendingLastCommitEvent = true;
+    mPendingCommitCount++;
   } else if (aEvent.mMessage != eCompositionStart) {
     mCompositionString = aEvent.mData;
   }
 
   // During REQUEST_TO_COMMIT_COMPOSITION or REQUEST_TO_CANCEL_COMPOSITION,
   // widget usually sends a eCompositionChange and/or eCompositionCommit event
   // to finalize or clear the composition, respectively.  In this time,
   // we need to intercept all composition events here and pass the commit
@@ -1158,46 +1161,51 @@ ContentCacheInParent::OnCompositionEvent
   // RequestIMEToCommitComposition().  Then, eCommitComposition event will
   // be dispatched with the committed string in the remote process internally.
   if (mCommitStringByRequest) {
     MOZ_ASSERT(aEvent.mMessage == eCompositionChange ||
                aEvent.mMessage == eCompositionCommit);
     *mCommitStringByRequest = aEvent.mData;
     // We need to wait eCompositionCommitRequestHandled from the remote process
     // in this case.  Therefore, mPendingEventsNeedingAck needs to be
-    // incremented here.
+    // incremented here.  Additionally, we stop sending eCompositionCommit(AsIs)
+    // event.  Therefore, we need to decrement mPendingCommitCount which has
+    // been incremented above.
     if (!mWidgetHasComposition) {
       mPendingEventsNeedingAck++;
+      MOZ_DIAGNOSTIC_ASSERT(mPendingCommitCount);
+      if (mPendingCommitCount) {
+        mPendingCommitCount--;
+      }
     }
-    // Cancel mIsPendingLastCommitEvent because we won't send the commit event
-    // to the remote process.
-    mIsPendingLastCommitEvent = false;
     return false;
   }
 
   mPendingEventsNeedingAck++;
   return true;
 }
 
 void
 ContentCacheInParent::OnSelectionEvent(
                         const WidgetSelectionEvent& aSelectionEvent)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("0x%p OnSelectionEvent(aEvent={ "
      "mMessage=%s, mOffset=%u, mLength=%u, mReversed=%s, "
      "mExpandToClusterBoundary=%s, mUseNativeLineBreak=%s }), "
      "mPendingEventsNeedingAck=%u, mWidgetHasComposition=%s, "
-     "mPendingCompositionCount=%u",
+     "mPendingCompositionCount=%" PRIu8 ", mPendingCommitCount=%" PRIu8 ", "
+     "mIsChildIgnoringCompositionEvents=%s",
      this, ToChar(aSelectionEvent.mMessage),
      aSelectionEvent.mOffset, aSelectionEvent.mLength,
      GetBoolName(aSelectionEvent.mReversed),
      GetBoolName(aSelectionEvent.mExpandToClusterBoundary),
      GetBoolName(aSelectionEvent.mUseNativeLineBreak), mPendingEventsNeedingAck,
-     GetBoolName(mWidgetHasComposition), mPendingCompositionCount));
+     GetBoolName(mWidgetHasComposition), mPendingCompositionCount,
+     mPendingCommitCount, GetBoolName(mIsChildIgnoringCompositionEvents)));
 
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
   mDispatchedEventMessages.AppendElement(aSelectionEvent.mMessage);
 #endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
   mPendingEventsNeedingAck++;
 }
 
@@ -1205,60 +1213,102 @@ void
 ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget,
                                                 EventMessage aMessage)
 {
   // This is called when the child process receives WidgetCompositionEvent or
   // WidgetSelectionEvent.
 
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("0x%p OnEventNeedingAckHandled(aWidget=0x%p, "
-     "aMessage=%s), mPendingEventsNeedingAck=%u, mPendingCompositionCount=%" PRIu8,
-     this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck, mPendingCompositionCount));
+     "aMessage=%s), mPendingEventsNeedingAck=%u, "
+     "mPendingCompositionCount=%" PRIu8 ", mPendingCommitCount=%" PRIu8 ", "
+     "mIsChildIgnoringCompositionEvents=%s",
+     this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck,
+     mPendingCompositionCount, mPendingCommitCount,
+     GetBoolName(mIsChildIgnoringCompositionEvents)));
 
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
   mReceivedEventMessages.AppendElement(aMessage);
 #endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
-  if (WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage) ||
-      aMessage == eCompositionCommitRequestHandled) {
+  bool isCommittedInChild =
+    // Commit requester in the remote process has committed the composition.
+    aMessage == eCompositionCommitRequestHandled ||
+    // The commit event has been handled normally in the remote process.
+    (!mIsChildIgnoringCompositionEvents &&
+     WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage));
 
+  if (isCommittedInChild) {
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
     if (mPendingCompositionCount == 1) {
       RemoveUnnecessaryEventMessageLog();
     }
 #endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
     if (NS_WARN_IF(!mPendingCompositionCount)) {
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
       nsPrintfCString info("\nThere is no pending composition but received %s "
                            "message from the remote child\n\n",
                            ToChar(aMessage));
       AppendEventMessageLog(info);
       CrashReporter::AppendAppNotesToCrashReport(info);
 #endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
       MOZ_DIAGNOSTIC_ASSERT(false,
         "No pending composition but received unexpected commit event");
+
+      // Prevent odd behavior in release channel.
       mPendingCompositionCount = 1;
     }
 
     mPendingCompositionCount--;
+
     // Forget composition string only when the latest composition string is
     // handled in the remote process because if there is 2 or more pending
     // composition, this value shouldn't be referred.
     if (!mPendingCompositionCount) {
       mCompositionString.Truncate();
-      mIsPendingLastCommitEvent = false;
     }
+
     // Forget pending commit string length if it's handled in the remote
     // process.  Note that this doesn't care too old composition's commit
     // string because in such case, we cannot return proper information
     // to IME synchornously.
     mPendingCommitLength = 0;
   }
 
+  if (WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage)) {
+    // After the remote process receives eCompositionCommit(AsIs) event,
+    // it'll restart to handle composition events.
+    mIsChildIgnoringCompositionEvents = false;
+
+    if (NS_WARN_IF(!mPendingCommitCount)) {
+#if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
+      nsPrintfCString info("\nThere is no pending comment events but received "
+                           "%s message from the remote child\n\n",
+                           ToChar(aMessage));
+      AppendEventMessageLog(info);
+      CrashReporter::AppendAppNotesToCrashReport(info);
+#endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
+      MOZ_DIAGNOSTIC_ASSERT(false,
+        "No pending commit events but received unexpected commit event");
+
+      // Prevent odd behavior in release channel.
+      mPendingCommitCount = 1;
+    }
+
+    mPendingCommitCount--;
+  } else if (aMessage == eCompositionCommitRequestHandled &&
+             mPendingCommitCount) {
+    // If the remote process commits composition synchronously after
+    // requesting commit composition and we've already sent commit composition,
+    // it starts to ignore following composition events until receiving
+    // eCompositionStart event.
+    mIsChildIgnoringCompositionEvents = true;
+  }
+
   if (NS_WARN_IF(!mPendingEventsNeedingAck)) {
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
     nsPrintfCString info("\nThere is no pending events but received %s "
                          "message from the remote child\n\n",
                          ToChar(aMessage));
     AppendEventMessageLog(info);
     CrashReporter::AppendAppNotesToCrashReport(info);
 #endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
@@ -1275,20 +1325,22 @@ ContentCacheInParent::OnEventNeedingAckH
 
 bool
 ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
                                                     bool aCancel,
                                                     nsAString& aCommittedString)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("0x%p RequestToCommitComposition(aWidget=%p, "
-     "aCancel=%s), mPendingCompositionCount=%u, "
+     "aCancel=%s), mPendingCompositionCount=%" PRIu8 ", "
+     "mPendingCommitCount=%" PRIu8 ", mIsChildIgnoringCompositionEvents=%s, "
      "IMEStateManager::DoesTabParentHaveIMEFocus(&mTabParent)=%s, "
      "mWidgetHasComposition=%s, mCommitStringByRequest=%p",
      this, aWidget, GetBoolName(aCancel), mPendingCompositionCount,
+     mPendingCommitCount, GetBoolName(mIsChildIgnoringCompositionEvents),
      GetBoolName(IMEStateManager::DoesTabParentHaveIMEFocus(&mTabParent)),
      GetBoolName(mWidgetHasComposition), mCommitStringByRequest));
 
   MOZ_ASSERT(!mCommitStringByRequest);
 
   // If there are 2 or more pending compositions, we already sent
   // eCompositionCommit(AsIs) to the remote process.  So, this request is
   // too late for IME.  The remote process should wait following
@@ -1303,17 +1355,22 @@ ContentCacheInParent::RequestIMEToCommit
     return false;
   }
 
   // If there is no pending composition, we may have already sent
   // eCompositionCommit(AsIs) event for the active composition.  If so, the
   // remote process will receive composition events which causes cleaning up
   // TextComposition.  So, this shouldn't do nothing and TextComposition
   // should handle the request as it's handled asynchronously.
-  if (mIsPendingLastCommitEvent) {
+  // XXX Perhaps, this is wrong because TextComposition in child process
+  //     may commit the composition with current composition string in the
+  //     remote process.  I.e., it may be different from actual commit string
+  //     which user typed.  So, perhaps, we should return true and the commit
+  //     string.
+  if (mPendingCommitCount) {
 #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
     mRequestIMEToCommitCompositionResults.
       AppendElement(RequestIMEToCommitCompositionResult::
                       eToCommittedCompositionReceived);
 #endif // #if defined(MOZ_CRASHREPORTER) && MOZ_DIAGNOSTIC_ASSERT_ENABLED
     return false;
   }
 
--- a/widget/ContentCache.h
+++ b/widget/ContentCache.h
@@ -473,24 +473,27 @@ private:
   // new composition start offset.
   // Note that when mPendingCompositionCount is not 0, i.e., there are 2 or
   // more pending compositions, this cache won't be used because in such case,
   // anyway ContentCacheInParent cannot return proper character rect.
   uint32_t mPendingCommitLength;
   // mPendingCompositionCount is number of compositions which started in widget
   // but not yet handled in the child process.
   uint8_t mPendingCompositionCount;
+  // mPendingCommitCount is number of eCompositionCommit(AsIs) events which
+  // were sent to the child process but not yet handled in it.
+  uint8_t mPendingCommitCount;
   // mWidgetHasComposition is true when the widget in this process thinks that
   // IME has composition.  So, this is set to true when eCompositionStart is
   // dispatched and set to false when eCompositionCommit(AsIs) is dispatched.
   bool mWidgetHasComposition;
-  // mIsPendingLastCommitEvent is true only when this sends
-  // eCompositionCommit(AsIs) event to the remote process but it's not handled
-  // in the remote process yet.
-  bool mIsPendingLastCommitEvent;
+  // mIsChildIgnoringCompositionEvents is set to true if the child process
+  // requests commit composition whose commit has already been sent to it.
+  // Then, set to false when the child process ignores the commit event.
+  bool mIsChildIgnoringCompositionEvents;
 
   ContentCacheInParent() = delete;
 
   /**
    * When following methods' aRoundToExistingOffset is true, even if specified
    * offset or range is out of bounds, the result is computed with the existing
    * cache forcibly.
    */