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 draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 06 Dec 2017 15:07:41 +0900
changeset 708032 07e5ec7429b17f90db3197c165d1af6c8619f133
parent 707249 f2cf6d1473808039be5ecd8727cc3791d5d7d2d4
child 743087 fdc39e5dc585bc055713bab0de9453b36b4cba0f
push id92274
push usermasayuki@d-toybox.com
push dateWed, 06 Dec 2017 06:13:21 +0000
reviewersm_kato
bugs1423456, 1420849
milestone59.0a1
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 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
@@ -517,18 +517,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)
 {
@@ -1100,21 +1101,23 @@ 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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
   mDispatchedEventMessages.AppendElement(aEvent.mMessage);
 #endif // #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
   // We must be able to simulate the selection because
   // we might not receive selection updates in time
@@ -1138,17 +1141,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
@@ -1156,46 +1159,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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
   mDispatchedEventMessages.AppendElement(aSelectionEvent.mMessage);
 #endif // MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
   mPendingEventsNeedingAck++;
 }
 
@@ -1203,60 +1211,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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
   mReceivedEventMessages.AppendElement(aMessage);
-#endif // MOZ_DIAGNOSTIC_ASSERT_ENABLED
+#endif // #if 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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
     if (mPendingCompositionCount == 1) {
       RemoveUnnecessaryEventMessageLog();
     }
 #endif // #if MOZ_DIAGNOSTIC_ASSERT_ENABLED
 
     if (NS_WARN_IF(!mPendingCompositionCount)) {
 #if 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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
       MOZ_DIAGNOSTIC_ASSERT(false,
         "No pending composition but received unexpected commit event");
+#endif // #if MOZ_DIAGNOSTIC_ASSERT_ENABLED
+
+      // 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 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);
+      MOZ_DIAGNOSTIC_ASSERT(false,
+        "No pending commit events but received unexpected commit event");
+#endif // #if MOZ_DIAGNOSTIC_ASSERT_ENABLED
+
+      // 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 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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
@@ -1273,20 +1323,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
@@ -1301,17 +1353,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 MOZ_DIAGNOSTIC_ASSERT_ENABLED
     mRequestIMEToCommitCompositionResults.
       AppendElement(RequestIMEToCommitCompositionResult::
                       eToCommittedCompositionReceived);
 #endif // #if 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.
    */