Bug 1368554 ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Sat, 10 Jun 2017 02:42:16 +0900
changeset 592015 89d16596295f61a506c52df6e65c052c82ea23d4
parent 591816 2d20b365eee19434657f6b365d310e8b70904d2b
child 632699 f1e762fb61cdd4143a8522cb5f963cf6328948b7
push id63257
push usermasayuki@d-toybox.com
push dateFri, 09 Jun 2017 22:03:24 +0000
reviewersm_kato
bugs1368554
milestone55.0a1
Bug 1368554 ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process r?m_kato ContentCacheInParent::mPendingCompositionCount is now managed with composition events which TabParent received. However, TextComposition doesn't dispatch composition events after coming request to commit active composition. Therefore, composition is committed forcibly in a remote process over 255 times, the main process crashes. It's the safest way to use TextComposition to manage ContentCacheInParent::mPendingCompositionCount. MozReview-Commit-ID: DEhzYcK1zcW
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,18 +272,19 @@ 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.
-      sTextCompositions->ElementAt(i)->Destroy();
+      RefPtr<TextComposition> composition = sTextCompositions->ElementAt(i);
       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");
       }
     }
@@ -1288,20 +1289,21 @@ 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 NS_COMPOSTION_END "
+         "removing TextComposition from the array since eCompositionEnd "
          "was dispatched"));
-      sTextCompositions->ElementAt(i)->Destroy();
+      RefPtr<TextComposition> composition = sTextCompositions->ElementAt(i);
       sTextCompositions->RemoveElementAt(i);
+      composition->Destroy();
     }
   }
 }
 
 // static
 IMEContentObserver*
 IMEStateManager::GetActiveContentObserver()
 {
--- a/dom/events/TextComposition.cpp
+++ b/dom/events/TextComposition.cpp
@@ -65,26 +65,40 @@ 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;
-  mTabParent = nullptr;
+  if (mTabParent) {
+    RefPtr<TabParent> tabParent = mTabParent.forget();
+    if (mHasDispatchedCompositionEvents) {
+      tabParent->OnDestroyTextComposition();
+    }
+  }
   // 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() &&
@@ -146,16 +160,17 @@ 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(
@@ -244,16 +259,17 @@ 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
@@ -173,20 +173,17 @@ public:
     RefPtr<TextComposition> mComposition;
     CompositionChangeEventHandlingMarker();
     CompositionChangeEventHandlingMarker(
       const CompositionChangeEventHandlingMarker& aOther);
   };
 
 private:
   // Private destructor, to discourage deletion outside of Release():
-  ~TextComposition()
-  {
-    // WARNING: mPresContext may be destroying, so, be careful if you touch it.
-  }
+  ~TextComposition();
 
   // 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
@@ -257,31 +254,36 @@ 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);
 
   /**
    * GetEditor() returns nsIEditor pointer of mEditorWeak.
    */
   already_AddRefed<nsIEditor> GetEditor() const;
 
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -2062,16 +2062,22 @@ 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
@@ -502,16 +502,22 @@ 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,32 +1157,38 @@ 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));
-
-  if (WidgetCompositionEvent::IsFollowedByCompositionEnd(aMessage)) {
-    MOZ_RELEASE_ASSERT(mPendingCompositionCount > 0);
-    mPendingCompositionCount--;
-  }
+     "aMessage=%s), mPendingEventsNeedingAck=%u",
+     this, aWidget, ToChar(aMessage), mPendingEventsNeedingAck));
 
   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,16 +370,23 @@ 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