Bug 1278084 part.1 Don't release TSF objects during handling a key message r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 06 Jun 2016 21:07:24 +0900
changeset 376346 e19e5a438e8d0ef69ccfe99bf748db6c30a2e4c8
parent 376345 8b13a093f5541c40f7ea94553298ba54f1680704
child 376347 77631aeed6c9df6c07921b0885ddae4f2209e312
child 376351 417897e4d28c4228c8ce8c772475d89094146f49
child 376567 5aa76723d276a4f33c663cd80b254d1a291c2bc2
push id20546
push usermasayuki@d-toybox.com
push dateTue, 07 Jun 2016 19:05:52 +0000
reviewersm_kato
bugs1278084
milestone49.0a1
Bug 1278084 part.1 Don't release TSF objects during handling a key message r?m_kato While TIP is handling a key message, TSFTextStore shouldn't release any TSF objects since it may cause hitting a bug of TIPs. Actually, MS-IME for Japanese on Windows 10 crashes when TSFTextStore is destroyed during composition because probably it accesses some destroyed objects to request to commit composition or query contents. MozReview-Commit-ID: 9CTjHhAvG04
widget/windows/TSFTextStore.cpp
widget/windows/TSFTextStore.h
--- a/widget/windows/TSFTextStore.cpp
+++ b/widget/windows/TSFTextStore.cpp
@@ -1177,16 +1177,17 @@ bool TSFTextStore::sHackQueryInsertForMS
 
 #define TEXTSTORE_DEFAULT_VIEW (1)
 
 TSFTextStore::TSFTextStore()
   : mEditCookie(0)
   , mSinkMask(0)
   , mLock(0)
   , mLockQueued(0)
+  , mHandlingKeyMessage(0)
   , mLockedContent(mComposition, mSelection)
   , mRequestedAttrValues(false)
   , mIsRecordingActionsWithoutLock(false)
   , mPendingOnSelectionChange(false)
   , mHasReturnedNoLayoutError(false)
   , mWaitingQueryLayout(false)
   , mPendingDestroy(false)
   , mDeferClearingLockedContent(false)
@@ -1283,19 +1284,20 @@ TSFTextStore::Init(nsWindowBase* aWidget
   return true;
 }
 
 bool
 TSFTextStore::Destroy()
 {
   MOZ_LOG(sTextStoreLog, LogLevel::Info,
     ("TSF: 0x%p TSFTextStore::Destroy(), mLock=%s, "
-     "mComposition.IsComposing()=%s",
+     "mComposition.IsComposing()=%s, mHandlingKeyMessage=%u",
      this, GetLockFlagNameStr(mLock).get(),
-     GetBoolName(mComposition.IsComposing())));
+     GetBoolName(mComposition.IsComposing()),
+     mHandlingKeyMessage));
 
   mDestroyed = true;
 
   if (mLock) {
     mPendingDestroy = true;
     return true;
   }
 
@@ -1314,35 +1316,56 @@ TSFTextStore::Destroy()
        "ITextStoreACPSink::OnLayoutChange(TS_LC_DESTROY)...",
        this));
     mSink->OnLayoutChange(TS_LC_DESTROY, TEXTSTORE_DEFAULT_VIEW);
   }
 
   mLockedContent.Clear();
   mSelection.MarkDirty();
 
+  // If this is called during handling a keydown or keyup message, we should
+  // put off to release TSF objects until it completely finishes since
+  // MS-IME for Japanese refers some objects without grabbing them.
+  if (!mHandlingKeyMessage) {
+    ReleaseTSFObjects();
+  }
+
+  MOZ_LOG(sTextStoreLog, LogLevel::Info,
+    ("TSF: 0x%p   TSFTextStore::Destroy() succeeded", this));
+
+  return true;
+}
+
+void
+TSFTextStore::ReleaseTSFObjects()
+{
+  MOZ_ASSERT(!mHandlingKeyMessage);
+
+  MOZ_LOG(sTextStoreLog, LogLevel::Info,
+    ("TSF: 0x%p TSFTextStore::ReleaseTSFObjects()", this));
+
   mContext = nullptr;
   if (mDocumentMgr) {
     mDocumentMgr->Pop(TF_POPF_ALL);
     mDocumentMgr = nullptr;
   }
   mSink = nullptr;
   mWidget = nullptr;
   mDispatcher = nullptr;
 
   if (!mMouseTrackers.IsEmpty()) {
     MOZ_LOG(sTextStoreLog, LogLevel::Debug,
-      ("TSF: 0x%p   TSFTextStore::Destroy(), removing a mouse tracker...",
+      ("TSF: 0x%p   TSFTextStore::ReleaseTSFObjects(), "
+       "removing a mouse tracker...",
        this));
     mMouseTrackers.Clear();
   }
 
-  MOZ_LOG(sTextStoreLog, LogLevel::Info,
-    ("TSF: 0x%p   TSFTextStore::Destroy() succeeded", this));
-  return true;
+  MOZ_LOG(sTextStoreLog, LogLevel::Debug,
+    ("TSF: 0x%p   TSFTextStore::ReleaseTSFObjects() completed", this));
 }
 
 STDMETHODIMP
 TSFTextStore::QueryInterface(REFIID riid,
                              void** ppv)
 {
   *ppv=nullptr;
   if ( (IID_IUnknown == riid) || (IID_ITextStoreACP == riid) ) {
@@ -5465,26 +5488,40 @@ TSFTextStore::ProcessRawKeyMessage(const
   }
 
   if (aMsg.message == WM_KEYDOWN) {
     BOOL eaten;
     HRESULT hr = sKeystrokeMgr->TestKeyDown(aMsg.wParam, aMsg.lParam, &eaten);
     if (FAILED(hr) || !eaten) {
       return false;
     }
+    RefPtr<TSFTextStore> textStore(sEnabledTextStore);
+    if (textStore) {
+      textStore->OnStartToHandleKeyMessage();
+    }
     hr = sKeystrokeMgr->KeyDown(aMsg.wParam, aMsg.lParam, &eaten);
+    if (textStore) {
+      textStore->OnEndHandlingKeyMessage();
+    }
     return SUCCEEDED(hr) && eaten;
   }
   if (aMsg.message == WM_KEYUP) {
     BOOL eaten;
     HRESULT hr = sKeystrokeMgr->TestKeyUp(aMsg.wParam, aMsg.lParam, &eaten);
     if (FAILED(hr) || !eaten) {
       return false;
     }
+    RefPtr<TSFTextStore> textStore(sEnabledTextStore);
+    if (textStore) {
+      textStore->OnStartToHandleKeyMessage();
+    }
     hr = sKeystrokeMgr->KeyUp(aMsg.wParam, aMsg.lParam, &eaten);
+    if (textStore) {
+      textStore->OnEndHandlingKeyMessage();
+    }
     return SUCCEEDED(hr) && eaten;
   }
   return false;
 }
 
 // static
 void
 TSFTextStore::ProcessMessage(nsWindowBase* aWindow,
--- a/widget/windows/TSFTextStore.h
+++ b/widget/windows/TSFTextStore.h
@@ -243,16 +243,17 @@ protected:
 
   static bool CreateAndSetFocus(nsWindowBase* aFocusedWidget,
                                 const InputContext& aContext);
   static void MarkContextAsKeyboardDisabled(ITfContext* aContext);
   static void MarkContextAsEmpty(ITfContext* aContext);
 
   bool     Init(nsWindowBase* aWidget);
   bool     Destroy();
+  void     ReleaseTSFObjects();
 
   bool     IsReadLock(DWORD aLock) const
   {
     return (TS_LF_READ == (aLock & TS_LF_READ));
   }
   bool     IsReadWriteLock(DWORD aLock) const
   {
     return (TS_LF_READWRITE == (aLock & TS_LF_READWRITE));
@@ -343,16 +344,31 @@ protected:
   RefPtr<ITextStoreACPSink>  mSink;
   // TS_AS_* mask of what events to notify
   DWORD                        mSinkMask;
   // 0 if not locked, otherwise TS_LF_* indicating the current lock
   DWORD                        mLock;
   // 0 if no lock is queued, otherwise TS_LF_* indicating the queue lock
   DWORD                        mLockQueued;
 
+  uint32_t mHandlingKeyMessage;
+  void OnStartToHandleKeyMessage() { ++mHandlingKeyMessage; }
+  void OnEndHandlingKeyMessage()
+  {
+    MOZ_ASSERT(mHandlingKeyMessage);
+    if (--mHandlingKeyMessage) {
+      return;
+    }
+    // If TSFTextStore instance is destroyed during handling key message(s),
+    // release all TSF objects when all nested key messages have been handled.
+    if (mDestroyed) {
+      ReleaseTSFObjects();
+    }
+  }
+
   class Composition final
   {
   public:
     // nullptr if no composition is active, otherwise the current composition
     RefPtr<ITfCompositionView> mView;
 
     // Current copy of the active composition string. Only mString is
     // changed during a InsertTextAtSelection call if we have a composition.