Bug 1423456 - ContentCacheInParent::OnEventNeedingAckHandled() should…

…n'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

--HG--
extra : rebase_source : 6de1e2f1302d556d45d19c73b4d1ea3f86b65373

widget/ContentCache.cpp

@@ -522,8 +522,9 @@ ContentCacheInParent::ContentCacheInParent(TabParent& aTabParent)
   , mCompositionStartInChild(UINT32_MAX)
   , mPendingCommitLength(0)
   , mPendingCompositionCount(0)
+  , mPendingCommitCount(0)
   , mWidgetHasComposition(false)
-  , mIsPendingLastCommitEvent(false)
+  , mIsChildIgnoringCompositionEvents(false)
 {
 }
 
@@ -1105,11 +1106,13 @@ ContentCacheInParent::OnCompositionEvent(const WidgetCompositionEvent& aEvent)
     ("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
@@ -1143,7 +1146,7 @@ ContentCacheInParent::OnCompositionEvent(const WidgetCompositionEvent& aEvent)
     if (mPendingCompositionCount == 1) {
       mPendingCommitLength = aEvent.mData.Length();
     }
-    mIsPendingLastCommitEvent = true;
+    mPendingCommitCount++;
   } else if (aEvent.mMessage != eCompositionStart) {
     mCompositionString = aEvent.mData;
   }
@@ -1161,13 +1164,16 @@ ContentCacheInParent::OnCompositionEvent(const WidgetCompositionEvent& aEvent)
     *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;
   }
 
@@ -1184,13 +1190,15 @@ ContentCacheInParent::OnSelectionEvent(
      "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);
@@ -1208,16 +1216,25 @@ ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget,
 
   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();
@@ -1231,27 +1248,60 @@ ContentCacheInParent::OnEventNeedingAckHandled(nsIWidget* aWidget,
                            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 "
@@ -1278,10 +1328,12 @@ ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
 {
   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));
 
@@ -1306,7 +1358,12 @@ ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
   // 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::

widget/ContentCache.h

@@ -478,14 +478,17 @@ class ContentCacheInParent final : public ContentCache
   // 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;