Bug 1368554 ContentCacheInParent::mPendingCompositionCount should be decreased when TextCompositin which has dispatched composition events to corresponding remote process r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Sat, 10 Jun 2017 02:42:16 +0900
changeset 413907 401127e8ba3bf0890d843c33173f9d1fb182e643
parent 413906 8047b74cb02259717a3e0714b75a4a181e1b15c0
child 413908 70bfa8c83b16b1e3dd4477a6f9aa7c8d03ea6072
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1368554
milestone55.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 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