Bug 1317322 - Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown, r=jimm
☠☠ backed out by d64711bdb426 ☠ ☠
authorMichael Layzell <michael@thelayzells.com>
Fri, 24 Feb 2017 14:38:09 -0500
changeset 394497 1eb8e1322979bdd375acaa3b8b6ecb257c0729ab
parent 394496 39cba44c517d9a4060215449ddab6956c7abfe86
child 394498 4048d3a531078eb13aed019c5cc5f68da49967c5
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimm
bugs1317322
milestone54.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 1317322 - Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown, r=jimm MozReview-Commit-ID: 90obWnqRSWk
widget/windows/nsClipboard.cpp
widget/windows/nsDataObj.cpp
widget/windows/nsDataObj.h
--- a/widget/windows/nsClipboard.cpp
+++ b/widget/windows/nsClipboard.cpp
@@ -60,17 +60,17 @@ nsClipboard::nsClipboard() : nsBaseClipb
   mIgnoreEmptyNotification = false;
   mWindow         = nullptr;
 
   // Register for a shutdown notification so that we can flush data
  // to the OS clipboard.
   nsCOMPtr<nsIObserverService> observerService =
     do_GetService("@mozilla.org/observer-service;1");
   if (observerService) {
-    observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_FALSE);
+    observerService->AddObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID, PR_FALSE);
   }
 }
 
 //-------------------------------------------------------------------------
 // nsClipboard destructor
 //-------------------------------------------------------------------------
 nsClipboard::~nsClipboard()
 {
--- a/widget/windows/nsDataObj.cpp
+++ b/widget/windows/nsDataObj.cpp
@@ -436,40 +436,111 @@ STDMETHODIMP nsDataObj::QueryInterface(R
 //-----------------------------------------------------
 STDMETHODIMP_(ULONG) nsDataObj::AddRef()
 {
 	++m_cRef;
 	NS_LOG_ADDREF(this, m_cRef, "nsDataObj", sizeof(*this));
 	return m_cRef;
 }
 
+namespace {
+class RemoveTempFileHelper : public nsIObserver
+{
+public:
+  RemoveTempFileHelper(nsIFile* aTempFile)
+    : mTempFile(aTempFile)
+  {
+    MOZ_ASSERT(mTempFile);
+  }
+
+  // The attach method is seperate from the constructor as we may be addref-ing
+  // ourself, and we want to be sure someone has a strong reference to us.
+  void Attach()
+  {
+    // We need to listen to both the xpcom shutdown message and our timer, and
+    // fire when the first of either of these two messages is received.
+    nsresult rv;
+    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return;
+    }
+    mTimer->Init(this, 500, nsITimer::TYPE_ONE_SHOT);
+
+    nsCOMPtr<nsIObserverService> observerService =
+      do_GetService("@mozilla.org/observer-service;1");
+    if (NS_WARN_IF(!observerService)) {
+      mTimer->Cancel();
+      mTimer = nullptr;
+      return;
+    }
+    observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
+  }
+
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSIOBSERVER
+
+private:
+  ~RemoveTempFileHelper()
+  {
+    if (mTempFile) {
+      mTempFile->Remove(false);
+    }
+  }
+
+  nsCOMPtr<nsIFile> mTempFile;
+  nsCOMPtr<nsITimer> mTimer;
+};
+
+NS_IMPL_ISUPPORTS(RemoveTempFileHelper, nsIObserver);
+
+NS_IMETHODIMP
+RemoveTempFileHelper::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* aData)
+{
+  // Let's be careful and make sure that we don't die immediately
+  RefPtr<RemoveTempFileHelper> grip = this;
+
+  // Make sure that we aren't called again by destroying references to ourself.
+  nsCOMPtr<nsIObserverService> observerService =
+    do_GetService("@mozilla.org/observer-service;1");
+  if (observerService) {
+    observerService->RemoveObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
+  }
+
+  if (mTimer) {
+    mTimer->Cancel();
+    mTimer = nullptr;
+  }
+
+  // Remove the tempfile
+  if (mTempFile) {
+    mTempFile->Remove(false);
+    mTempFile = nullptr;
+  }
+  return NS_OK;
+}
+} // namespace
 
 //-----------------------------------------------------
 STDMETHODIMP_(ULONG) nsDataObj::Release()
 {
 	--m_cRef;
 	
 	NS_LOG_RELEASE(this, m_cRef, "nsDataObj");
 	if (0 != m_cRef)
 		return m_cRef;
 
   // We have released our last ref on this object and need to delete the
   // temp file. External app acting as drop target may still need to open the
   // temp file. Addref a timer so it can delay deleting file and destroying
-  // this object. Delete file anyway and destroy this obj if there's a problem.
+  // this object.
   if (mCachedTempFile) {
-    nsresult rv;
-    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
-    if (NS_SUCCEEDED(rv)) {
-      mTimer->InitWithFuncCallback(nsDataObj::RemoveTempFile, this,
-                                   500, nsITimer::TYPE_ONE_SHOT);
-      return AddRef();
-    }
-    mCachedTempFile->Remove(false);
+    RefPtr<RemoveTempFileHelper> helper =
+      new RemoveTempFileHelper(mCachedTempFile);
     mCachedTempFile = nullptr;
+    helper->Attach();
   }
 
 	delete this;
 
 	return 0;
 }
 
 //-----------------------------------------------------
@@ -2146,18 +2217,8 @@ HRESULT nsDataObj::GetFileContents_IStre
   NS_ENSURE_TRUE(pStream, E_FAIL);
 
   aSTG.tymed = TYMED_ISTREAM;
   aSTG.pstm = pStream;
   aSTG.pUnkForRelease = nullptr;
 
   return S_OK;
 }
-
-void nsDataObj::RemoveTempFile(nsITimer* aTimer, void* aClosure)
-{
-  nsDataObj *timedDataObj = static_cast<nsDataObj *>(aClosure);
-  if (timedDataObj->mCachedTempFile) {
-    timedDataObj->mCachedTempFile->Remove(false);
-    timedDataObj->mCachedTempFile = nullptr;
-  }
-  timedDataObj->Release();
-}
--- a/widget/windows/nsDataObj.h
+++ b/widget/windows/nsDataObj.h
@@ -284,13 +284,12 @@ protected:
       STGMEDIUM   stgm;
     } DATAENTRY, *LPDATAENTRY;
 
     nsTArray <LPDATAENTRY> mDataEntryList;
     nsCOMPtr<nsITimer> mTimer;
 
     bool LookupArbitraryFormat(FORMATETC *aFormat, LPDATAENTRY *aDataEntry, BOOL aAddorUpdate);
     bool CopyMediumData(STGMEDIUM *aMediumDst, STGMEDIUM *aMediumSrc, LPFORMATETC aFormat, BOOL aSetData);
-    static void RemoveTempFile(nsITimer* aTimer, void* aClosure);
 };
 
 
 #endif  // _NSDATAOBJ_H_