From add4f2919daac22804778ff290bad75a9db37bd5 Mon Sep 17 00:00:00 2001
From: Michael Layzell <michael@thelayzells.com>
Date: Fri, 24 Feb 2017 14:38:09 -0500
Subject: [PATCH] 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 |  2 +-
 widget/windows/nsDataObj.cpp   | 99 +++++++++++++++++++++++++++-------
 widget/windows/nsDataObj.h     |  1 -
 3 files changed, 81 insertions(+), 21 deletions(-)

diff --git a/widget/windows/nsClipboard.cpp b/widget/windows/nsClipboard.cpp
index 334caeb955bd..95092f462d54 100644
--- a/widget/windows/nsClipboard.cpp
+++ b/widget/windows/nsClipboard.cpp
@@ -65,7 +65,7 @@ nsClipboard::nsClipboard() : nsBaseClipboard()
   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);
   }
 }
 
diff --git a/widget/windows/nsDataObj.cpp b/widget/windows/nsDataObj.cpp
index 96f1aae93337..a3b7b1250831 100644
--- a/widget/windows/nsDataObj.cpp
+++ b/widget/windows/nsDataObj.cpp
@@ -441,6 +441,82 @@ STDMETHODIMP_(ULONG) nsDataObj::AddRef()
 	return m_cRef;
 }
 
+namespace {
+class RemoveTempFileHelper : public nsIObserver
+{
+public:
+  explicit 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()
@@ -454,17 +530,12 @@ STDMETHODIMP_(ULONG) nsDataObj::Release()
   // 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;
@@ -2151,13 +2222,3 @@ HRESULT nsDataObj::GetFileContents_IStream(FORMATETC& aFE, STGMEDIUM& aSTG)
 
   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();
-}
diff --git a/widget/windows/nsDataObj.h b/widget/windows/nsDataObj.h
index 0b12e83cb8d9..35f20fc043fd 100644
--- a/widget/windows/nsDataObj.h
+++ b/widget/windows/nsDataObj.h
@@ -289,7 +289,6 @@ protected:
 
     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);
 };