Bug 1405832 - part 2: TextComposition::RequestToCommit() should request IME to commit or cancel composition only when it hasn't been request it yet and hasn't received commit event yet r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 20 Nov 2017 22:59:04 +0900
changeset 393065 e8d7881af680c693612c2ed42d3486c5ad4c8e42
parent 393064 21522127515146fb7742f4e52c5d4c860bc71562
child 393066 3e4e219b8aebf8e439d8ac40738321d7c7c81c6f
push id55827
push usermasayuki@d-toybox.com
push dateWed, 22 Nov 2017 12:53:08 +0000
treeherderautoland@7a3e5d976499 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1405832
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1405832 - part 2: TextComposition::RequestToCommit() should request IME to commit or cancel composition only when it hasn't been request it yet and hasn't received commit event yet r=m_kato According to the log in crash reports, eCompositionCommitRequestHandled is sent to ContentCacheInParent twice or more for a composition. This causes breaking mPendingCompositionCount and mPendingEventsNeedingAck management. Currently, nsIWidget::NotifyIME() should be called only by TextComposition::RequestToCommit(). Therefore, the method should manage if it should request it actually. If the composition has already received eCompositionCommit(AsIs) event, it shouldn't request it because parent process may have already stated new composition and it shouldn't be broken by request for old composition. MozReview-Commit-ID: 2ekSa6EIeRP
dom/events/TextComposition.cpp
dom/events/TextComposition.h
widget/PuppetWidget.cpp
--- a/dom/events/TextComposition.cpp
+++ b/dom/events/TextComposition.cpp
@@ -60,16 +60,17 @@ TextComposition::TextComposition(nsPresC
   , mCompositionStartOffset(0)
   , mTargetClauseOffsetInComposition(0)
   , mIsSynthesizedForTests(aCompositionEvent->mFlags.mIsSynthesizedForTests)
   , mIsComposing(false)
   , mIsEditorHandlingEvent(false)
   , mIsRequestingCommit(false)
   , mIsRequestingCancel(false)
   , mRequestedToCommitOrCancel(false)
+  , mHasReceivedCommitEvent(false)
   , mWasNativeCompositionEndEventDiscarded(false)
   , mAllowControlCharacters(
       Preferences::GetBool("dom.compositionevent.allow_control_characters",
                            false))
   , mWasCompositionStringEmpty(true)
 {
   MOZ_ASSERT(aCompositionEvent->mNativeIMEContext.IsValid());
 }
@@ -241,16 +242,20 @@ void
 TextComposition::DispatchCompositionEvent(
                    WidgetCompositionEvent* aCompositionEvent,
                    nsEventStatus* aStatus,
                    EventDispatchingCallback* aCallBack,
                    bool aIsSynthesized)
 {
   mWasCompositionStringEmpty = mString.IsEmpty();
 
+  if (aCompositionEvent->IsFollowedByCompositionEnd()) {
+    mHasReceivedCommitEvent = true;
+  }
+
   // If this instance has requested to commit or cancel composition but
   // is not synthesizing commit event, that means that the IME commits or
   // cancels the composition asynchronously.  Typically, iBus behaves so.
   // Then, synthesized events which were dispatched immediately after
   // the request has already committed our editor's composition string and
   // told it to web apps.  Therefore, we should ignore the delayed events.
   if (mRequestedToCommitOrCancel && !aIsSynthesized) {
     *aStatus = nsEventStatus_eConsumeNoDefault;
@@ -558,20 +563,22 @@ TextComposition::DispatchCompositionEven
     new CompositionEventDispatcher(this, mNode, aEventMessage, aData,
                                    aIsSynthesizingCommit));
 }
 
 nsresult
 TextComposition::RequestToCommit(nsIWidget* aWidget, bool aDiscard)
 {
   // If this composition is already requested to be committed or canceled,
-  // we don't need to request it again because even if the first request
-  // failed, new request won't success, probably.  And we shouldn't synthesize
-  // events for committing or canceling composition twice or more times.
-  if (mRequestedToCommitOrCancel) {
+  // or has already finished in IME, we don't need to request it again because
+  // request from this instance shouldn't cause committing nor canceling current
+  // composition in IME, and even if the first request failed, new request
+  // won't success, probably.  And we shouldn't synthesize events for
+  // committing or canceling composition twice or more times.
+  if (!CanRequsetIMEToCommitOrCancelComposition()) {
     return NS_OK;
   }
 
   RefPtr<TextComposition> kungFuDeathGrip(this);
   const nsAutoString lastData(mLastData);
 
   {
     AutoRestore<bool> saveRequestingCancel(mIsRequestingCancel);
--- a/dom/events/TextComposition.h
+++ b/dom/events/TextComposition.h
@@ -96,16 +96,25 @@ public:
 
   /**
    * Request to commit (or cancel) the composition to IME.  This method should
    * be called only by IMEStateManager::NotifyIME().
    */
   nsresult RequestToCommit(nsIWidget* aWidget, bool aDiscard);
 
   /**
+   * IsRequestingCommitOrCancelComposition() returns true if the instance is
+   * requesting widget to commit or cancel composition.
+   */
+  bool IsRequestingCommitOrCancelComposition() const
+  {
+    return mIsRequestingCancel || mIsRequestingCommit;
+  }
+
+  /**
    * Send a notification to IME.  It depends on the IME or platform spec what
    * will occur (or not occur).
    */
   nsresult NotifyIME(widget::IMEMessage aMessage);
 
   /**
    * the offset of first composition string
    */
@@ -242,20 +251,24 @@ private:
   // requesting commit or canceling the composition.  In other words, while
   // one of these values is true, we're handling the request.
   bool mIsRequestingCommit;
   bool mIsRequestingCancel;
 
   // mRequestedToCommitOrCancel is true *after* we requested IME to commit or
   // cancel the composition.  In other words, we already requested of IME that
   // it commits or cancels current composition.
-  // NOTE: Before this is set true, both mIsRequestingCommit and
-  //       mIsRequestingCancel are set false.
+  // NOTE: Before this is set to true, both mIsRequestingCommit and
+  //       mIsRequestingCancel are set to false.
   bool mRequestedToCommitOrCancel;
 
+  // Before this dispatches commit event into the tree, this is set to true.
+  // So, this means if native IME already commits the composition.
+  bool mHasReceivedCommitEvent;
+
   // mWasNativeCompositionEndEventDiscarded is true if this composition was
   // requested commit or cancel itself but native compositionend event is
   // discarded by PresShell due to not safe to dispatch events.
   bool mWasNativeCompositionEndEventDiscarded;
 
   // Allow control characters appear in composition string.
   // When this is false, control characters except
   // CHARACTER TABULATION (horizontal tab) are removed from
@@ -274,23 +287,38 @@ private:
     , mCompositionStartOffset(0)
     , mTargetClauseOffsetInComposition(0)
     , mIsSynthesizedForTests(false)
     , mIsComposing(false)
     , mIsEditorHandlingEvent(false)
     , mIsRequestingCommit(false)
     , mIsRequestingCancel(false)
     , mRequestedToCommitOrCancel(false)
+    , mHasReceivedCommitEvent(false)
     , mWasNativeCompositionEndEventDiscarded(false)
     , mAllowControlCharacters(false)
     , mWasCompositionStringEmpty(true)
   {}
   TextComposition(const TextComposition& aOther);
 
   /**
+   * If we're requesting IME to commit or cancel composition, or we've already
+   * requested it, or we've already known this composition has been ended in
+   * IME, we don't need to request commit nor cancel composition anymore and
+   * shouldn't do so if we're in content process for not committing/canceling
+   * "current" composition in native IME.  So, when this returns true,
+   * RequestIMEToCommit() does nothing.
+   */
+  bool CanRequsetIMEToCommitOrCancelComposition() const
+  {
+    return !mIsRequestingCommit && !mIsRequestingCancel &&
+           !mRequestedToCommitOrCancel && !mHasReceivedCommitEvent;
+  }
+
+  /**
    * GetEditorBase() returns EditorBase pointer of mEditorBaseWeak.
    */
   already_AddRefed<EditorBase> GetEditorBase() const;
 
   /**
    * HasEditor() returns true if mEditorBaseWeak holds EditorBase instance
    * which is alive.  Otherwise, false.
    */
--- a/widget/PuppetWidget.cpp
+++ b/widget/PuppetWidget.cpp
@@ -647,16 +647,20 @@ PuppetWidget::RequestIMEToCommitComposit
 
   RefPtr<TextComposition> composition =
     IMEStateManager::GetTextCompositionFor(this);
   // This method shouldn't be called when there is no text composition instance.
   if (NS_WARN_IF(!composition)) {
     return NS_OK;
   }
 
+  MOZ_DIAGNOSTIC_ASSERT(composition->IsRequestingCommitOrCancelComposition(),
+    "Requesting commit or cancel composition should be requested via "
+    "TextComposition instance");
+
   bool isCommitted = false;
   nsAutoString committedString;
   if (NS_WARN_IF(!mTabChild->SendRequestIMEToCommitComposition(
                                aCancel, &isCommitted, &committedString))) {
     return NS_ERROR_FAILURE;
   }
 
   // If the composition wasn't committed synchronously, we need to wait async