Bug 1376424 - part0: Backout the patch for bug 1368554 r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 27 Jun 2017 22:02:07 +0900
changeset 601161 226cef11665566e8f3dd1f4938d8be283a0e6948
parent 600694 8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b
child 601162 03891ec1d1db1d584af1151f869e40bdb4b5334e
push id65974
push usermasayuki@d-toybox.com
push dateWed, 28 Jun 2017 06:08:16 +0000
reviewersm_kato
bugs1376424, 1368554
milestone56.0a1
Bug 1376424 - part0: Backout the patch for bug 1368554 r?m_kato TextComposition in the main process is destroyed when the main process sends eCompositionCommit(AsIs) to focused remote process. Therefore, ContentCacheInParent::mCompositionPendingCount is never 2 or more now. It may cause ContentCacheInParent::Assign() setting older composition's start offset to current composition's start offset in the main process. For making uplift the following patch easier, the wrong patch should be backed out first. MozReview-Commit-ID: IHWc7qZBQtc
dom/events/IMEStateManager.cpp
dom/events/TextComposition.cpp
dom/events/TextComposition.h
dom/ipc/TabParent.cpp
dom/ipc/TabParent.h
widget/ContentCache.cpp
widget/ContentCache.h
--- a/dom/events/IMEStateManager.cpp
+++ b/dom/events/IMEStateManager.cpp
@@ -272,19 +272,18 @@ IMEStateManager::OnDestroyPresContext(ns
   if (sTextCompositions) {
     TextCompositionArray::index_type i =
       sTextCompositions->IndexOf(aPresContext);
     if (i != TextCompositionArray::NoIndex) {
       MOZ_LOG(sISMLog, LogLevel::Debug,
         ("  OnDestroyPresContext(), "
          "removing TextComposition instance from the array (index=%" PRIuSIZE ")", i));
       // there should be only one composition per presContext object.
-      RefPtr<TextComposition> composition = sTextCompositions->ElementAt(i);
+      sTextCompositions->ElementAt(i)->Destroy();
       sTextCompositions->RemoveElementAt(i);
-      composition->Destroy();
       if (sTextCompositions->IndexOf(aPresContext) !=
             TextCompositionArray::NoIndex) {
         MOZ_LOG(sISMLog, LogLevel::Error,
           ("  OnDestroyPresContext(), FAILED to remove "
            "TextComposition instance from the array"));
         MOZ_CRASH("Failed to remove TextComposition instance from the array");
       }
     }
@@ -1289,21 +1288,20 @@ IMEStateManager::DispatchCompositionEven
   if ((!aIsSynthesized ||
        composition->WasNativeCompositionEndEventDiscarded()) &&
       aCompositionEvent->CausesDOMCompositionEndEvent()) {
     TextCompositionArray::index_type i =
       sTextCompositions->IndexOf(aCompositionEvent->mWidget);
     if (i != TextCompositionArray::NoIndex) {
       MOZ_LOG(sISMLog, LogLevel::Debug,
         ("  DispatchCompositionEvent(), "
-         "removing TextComposition from the array since eCompositionEnd "
+         "removing TextComposition from the array since NS_COMPOSTION_END "
          "was dispatched"));
-      RefPtr<TextComposition> composition = sTextCompositions->ElementAt(i);
+      sTextCompositions->ElementAt(i)->Destroy();
       sTextCompositions->RemoveElementAt(i);
-      composition->Destroy();
     }
   }
 }
 
 // static
 IMEContentObserver*
 IMEStateManager::GetActiveContentObserver()
 {
--- a/dom/events/TextComposition.cpp
+++ b/dom/events/TextComposition.cpp
@@ -65,40 +65,26 @@ TextComposition::TextComposition(nsPresC
   , mIsRequestingCommit(false)
   , mIsRequestingCancel(false)
   , mRequestedToCommitOrCancel(false)
   , mWasNativeCompositionEndEventDiscarded(false)
   , mAllowControlCharacters(
       Preferences::GetBool("dom.compositionevent.allow_control_characters",
                            false))
   , mWasCompositionStringEmpty(true)
-  , mHasDispatchedCompositionEvents(false)
 {
   MOZ_ASSERT(aCompositionEvent->mNativeIMEContext.IsValid());
 }
 
-TextComposition::~TextComposition()
-{
-  // WARNING: mPresContext may be destroying, so, be careful if you touch it.
-  if (NS_WARN_IF(mTabParent)) {
-    Destroy();
-  }
-}
-
 void
 TextComposition::Destroy()
 {
   mPresContext = nullptr;
   mNode = nullptr;
-  if (mTabParent) {
-    RefPtr<TabParent> tabParent = mTabParent.forget();
-    if (mHasDispatchedCompositionEvents) {
-      tabParent->OnDestroyTextComposition();
-    }
-  }
+  mTabParent = nullptr;
   // TODO: If the editor is still alive and this is held by it, we should tell
   //       this being destroyed for cleaning up the stuff.
 }
 
 bool
 TextComposition::IsValidStateForComposition(nsIWidget* aWidget) const
 {
   return !Destroyed() && aWidget && !aWidget->Destroyed() &&
@@ -160,17 +146,16 @@ void
 TextComposition::DispatchEvent(WidgetCompositionEvent* aDispatchEvent,
                                nsEventStatus* aStatus,
                                EventDispatchingCallback* aCallBack,
                                const WidgetCompositionEvent *aOriginalEvent)
 {
   nsPluginInstanceOwner::GeneratePluginEvent(aOriginalEvent,
                                              aDispatchEvent);
 
-  mHasDispatchedCompositionEvents = true;
   EventDispatcher::Dispatch(mNode, mPresContext,
                             aDispatchEvent, nullptr, aStatus, aCallBack);
 
   OnCompositionEventDispatched(aDispatchEvent);
 }
 
 void
 TextComposition::OnCompositionEventDiscarded(
@@ -259,17 +244,16 @@ TextComposition::DispatchCompositionEven
                    EventDispatchingCallback* aCallBack,
                    bool aIsSynthesized)
 {
   mWasCompositionStringEmpty = mString.IsEmpty();
 
   // If the content is a container of TabParent, composition should be in the
   // remote process.
   if (mTabParent) {
-    mHasDispatchedCompositionEvents = true;
     Unused << mTabParent->SendCompositionEvent(*aCompositionEvent);
     aCompositionEvent->StopPropagation();
     if (aCompositionEvent->CausesDOMTextEvent()) {
       mLastData = aCompositionEvent->mData;
       mLastRanges = aCompositionEvent->mRanges;
       // Although, the composition event hasn't been actually handled yet,
       // emulate an editor to be handling the composition event.
       EditorWillHandleCompositionChangeEvent(aCompositionEvent);
--- a/dom/events/TextComposition.h
+++ b/dom/events/TextComposition.h
@@ -172,17 +172,20 @@ public:
     RefPtr<TextComposition> mComposition;
     CompositionChangeEventHandlingMarker();
     CompositionChangeEventHandlingMarker(
       const CompositionChangeEventHandlingMarker& aOther);
   };
 
 private:
   // Private destructor, to discourage deletion outside of Release():
-  ~TextComposition();
+  ~TextComposition()
+  {
+    // WARNING: mPresContext may be destroying, so, be careful if you touch it.
+  }
 
   // sHandlingSelectionEvent is true while TextComposition sends a selection
   // event to ContentEventHandler.
   static bool sHandlingSelectionEvent;
 
   // This class holds nsPresContext weak.  This instance shouldn't block
   // destroying it.  When the presContext is being destroyed, it's notified to
   // IMEStateManager::OnDestroyPresContext(), and then, it destroy
@@ -254,36 +257,31 @@ private:
   // both composition string and data attribute of compositionupdate
   // and compositionend events.
   bool mAllowControlCharacters;
 
   // mWasCompositionStringEmpty is true if the composition string was empty
   // when DispatchCompositionEvent() is called.
   bool mWasCompositionStringEmpty;
 
-  // mHasDispatchedCompositionEvents is true if the instance has dispatched
-  // one or more composition events.
-  bool mHasDispatchedCompositionEvents;
-
   // Hide the default constructor and copy constructor.
   TextComposition()
     : mPresContext(nullptr)
     , mNativeContext(nullptr)
     , mCompositionStartOffset(0)
     , mTargetClauseOffsetInComposition(0)
     , mIsSynthesizedForTests(false)
     , mIsComposing(false)
     , mIsEditorHandlingEvent(false)
     , mIsRequestingCommit(false)
     , mIsRequestingCancel(false)
     , mRequestedToCommitOrCancel(false)
     , mWasNativeCompositionEndEventDiscarded(false)
     , mAllowControlCharacters(false)
     , mWasCompositionStringEmpty(true)
-    , mHasDispatchedCompositionEvents(false)
   {}
   TextComposition(const TextComposition& aOther);
 
   /**
    * GetEditorBase() returns EditorBase pointer of mEditorBaseWeak.
    */
   already_AddRefed<EditorBase> GetEditorBase() const;
 
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -2068,22 +2068,16 @@ TabParent::SendPasteTransferable(const I
                                  const bool& aIsPrivateData,
                                  const IPC::Principal& aRequestingPrincipal)
 {
   return PBrowserParent::SendPasteTransferable(aDataTransfer,
                                                aIsPrivateData,
                                                aRequestingPrincipal);
 }
 
-void
-TabParent::OnDestroyTextComposition()
-{
-  mContentCache.OnDestroyTextComposition();
-}
-
 /*static*/ TabParent*
 TabParent::GetFrom(nsFrameLoader* aFrameLoader)
 {
   if (!aFrameLoader) {
     return nullptr;
   }
   PBrowserParent* remoteBrowser = aFrameLoader->GetRemoteBrowser();
   return static_cast<TabParent*>(remoteBrowser);
--- a/dom/ipc/TabParent.h
+++ b/dom/ipc/TabParent.h
@@ -504,22 +504,16 @@ public:
   bool SendCompositionEvent(mozilla::WidgetCompositionEvent& aEvent);
 
   bool SendSelectionEvent(mozilla::WidgetSelectionEvent& aEvent);
 
   bool SendPasteTransferable(const IPCDataTransfer& aDataTransfer,
                              const bool& aIsPrivateData,
                              const IPC::Principal& aRequestingPrincipal);
 
-  /**
-   * OnDestroyTextComposition() should be called when TextComposition instance
-   * which dispatched composition events to this instance is being destroyed.
-   */
-  void OnDestroyTextComposition();
-
   static TabParent* GetFrom(nsFrameLoader* aFrameLoader);
 
   static TabParent* GetFrom(nsIFrameLoader* aFrameLoader);
 
   static TabParent* GetFrom(nsITabParent* aTabParent);
 
   static TabParent* GetFrom(PBrowserParent* aTabParent);
 
--- a/widget/ContentCache.cpp
+++ b/widget/ContentCache.cpp
@@ -1157,38 +1157,32 @@ 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",
-     this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck));
+     "aMessage=%s), mPendingEventsNeedingAck=%u, mPendingCompositionCount=%" PRIu8,
+     this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck, mPendingCompositionCount));
+
+  if (WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage)) {
+    MOZ_RELEASE_ASSERT(mPendingCompositionCount > 0);
+    mPendingCompositionCount--;
+  }
 
   MOZ_RELEASE_ASSERT(mPendingEventsNeedingAck > 0);
   if (--mPendingEventsNeedingAck) {
     return;
   }
 
   FlushPendingNotifications(aWidget);
 }
 
-void
-ContentCacheInParent::OnDestroyTextComposition()
-{
-  MOZ_LOG(sContentCacheLog, LogLevel::Info,
-    ("0x%p OnDestroyTextComposition(), "
-     "mPendingEventsNeedingAck=%u, mPendingCompositionCount=%" PRIu8,
-     this, mPendingEventsNeedingAck, mPendingCompositionCount));
-  MOZ_RELEASE_ASSERT(mPendingCompositionCount > 0);
-  mPendingCompositionCount--;
-}
-
 bool
 ContentCacheInParent::RequestIMEToCommitComposition(nsIWidget* aWidget,
                                                     bool aCancel,
                                                     nsAString& aCommittedString)
 {
   MOZ_LOG(sContentCacheLog, LogLevel::Info,
     ("0x%p RequestToCommitComposition(aWidget=%p, "
      "aCancel=%s), mWidgetHasComposition=%s, mCommitStringByRequest=%p",
--- a/widget/ContentCache.h
+++ b/widget/ContentCache.h
@@ -370,23 +370,16 @@ public:
    *
    * WARNING: This may send notifications to IME.  That might cause destroying
    *          TabParent or aWidget.  Therefore, the caller must not destroy
    *          this instance during a call of this method.
    */
   void OnEventNeedingAckHandled(nsIWidget* aWidget, EventMessage aMessage);
 
   /**
-   * OnDestroyTextComposition() should be called when TextComposition instance
-   * which dispatched composition events to the owner of this instance is being
-   * destroyed.
-   */
-  void OnDestroyTextComposition();
-
-  /**
    * RequestIMEToCommitComposition() requests aWidget to commit or cancel
    * composition.  If it's handled synchronously, this returns true.
    *
    * @param aWidget     The widget to be requested to commit or cancel
    *                    the composition.
    * @param aCancel     When the caller tries to cancel the composition, true.
    *                    Otherwise, i.e., tries to commit the composition, false.
    * @param aCommittedString    The committed string (i.e., the last data of