Bug 1489317: Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell a=pascalc
authorAaron Klotz <aklotz@mozilla.com>
Wed, 12 Sep 2018 19:58:07 +0000
changeset 489848 e7f051b7650e
parent 489847 d9b447bdf5e9
child 489849 fe879d96c14b
push id9801
push userncsoregi@mozilla.com
push dateFri, 14 Sep 2018 19:35:49 +0000
treeherdermozilla-beta@fe879d96c14b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmhowell, pascalc
bugs1489317
milestone63.0
Bug 1489317: Part 3 - Use an agile reference for JumpListBuilder::mJumpListMgr to ensure smooth transition between apartments; r=mhowell a=pascalc Depends on D5318 Differential Revision: https://phabricator.services.mozilla.com/D5321
widget/windows/JumpListBuilder.cpp
widget/windows/JumpListBuilder.h
--- a/widget/windows/JumpListBuilder.cpp
+++ b/widget/windows/JumpListBuilder.cpp
@@ -15,18 +15,21 @@
 #include "WinTaskbar.h"
 #include "nsDirectoryServiceUtils.h"
 #include "nsISimpleEnumerator.h"
 #include "mozilla/Preferences.h"
 #include "nsStringStream.h"
 #include "nsThreadUtils.h"
 #include "mozilla/LazyIdleThread.h"
 #include "nsIObserverService.h"
+#include "mozilla/ScopeExit.h"
 #include "mozilla/Unused.h"
 #include "mozilla/dom/Promise.h"
+#include "mozilla/mscom/COMApartmentRegion.h"
+#include "mozilla/mscom/EnsureMTA.h"
 
 #include <shellapi.h>
 #include "WinUtils.h"
 
 using mozilla::dom::Promise;
 
 // The amount of time, in milliseconds, that our IO thread will stay alive after the last event it processes.
 #define DEFAULT_THREAD_TIMEOUT_MS 30000
@@ -102,20 +105,39 @@ NS_IMPL_ISUPPORTS(DoneCommitListBuildCal
 } // namespace detail
 
 
 JumpListBuilder::JumpListBuilder() :
   mMaxItems(0),
   mHasCommit(false),
   mMonitor("JumpListBuilderMonitor")
 {
-  ::CoInitialize(nullptr);
+  MOZ_ASSERT(NS_IsMainThread());
 
-  CoCreateInstance(CLSID_DestinationList, nullptr, CLSCTX_INPROC_SERVER,
-                   IID_ICustomDestinationList, getter_AddRefs(mJumpListMgr));
+  // Instantiate mJumpListMgr in the multithreaded apartment so that proxied
+  // calls on that object do not need to interact with the main thread's message
+  // pump.
+  mscom::EnsureMTA([this]() {
+    RefPtr<ICustomDestinationList> jumpListMgr;
+    HRESULT hr = ::CoCreateInstance(CLSID_DestinationList, nullptr,
+                                    CLSCTX_INPROC_SERVER,
+                                    IID_ICustomDestinationList,
+                                    getter_AddRefs(jumpListMgr));
+    if (FAILED(hr)) {
+      return;
+    }
+
+    // Since we are accessing mJumpListMgr across different threads
+    // (ie, different apartments), mJumpListMgr must be an agile reference.
+    mJumpListMgr = jumpListMgr;
+  });
+
+  if (!mJumpListMgr) {
+    return;
+  }
 
   // Make a lazy thread for any IO
   mIOThread = new LazyIdleThread(DEFAULT_THREAD_TIMEOUT_MS,
                                  NS_LITERAL_CSTRING("Jump List"),
                                  LazyIdleThread::ManualShutdown);
   Preferences::AddStrongObserver(this, kPrefTaskbarEnabled);
 
   nsCOMPtr<nsIObserverService> observerService =
@@ -124,18 +146,16 @@ JumpListBuilder::JumpListBuilder() :
     observerService->AddObserver(this, TOPIC_PROFILE_BEFORE_CHANGE, false);
     observerService->AddObserver(this, TOPIC_CLEAR_PRIVATE_DATA, false);
   }
 }
 
 JumpListBuilder::~JumpListBuilder()
 {
   Preferences::RemoveObserver(this, kPrefTaskbarEnabled);
-  mJumpListMgr = nullptr;
-  ::CoUninitialize();
 }
 
 NS_IMETHODIMP JumpListBuilder::GetAvailable(int16_t *aAvailable)
 {
   *aAvailable = false;
 
   ReentrantMonitorAutoEnter lock(mMonitor);
   if (mJumpListMgr)
@@ -159,24 +179,29 @@ NS_IMETHODIMP JumpListBuilder::GetMaxLis
 
   *aMaxItems = 0;
 
   if (sBuildingList) {
     *aMaxItems = mMaxItems;
     return NS_OK;
   }
 
+  RefPtr<ICustomDestinationList> jumpListMgr = mJumpListMgr;
+  if (!jumpListMgr) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
   IObjectArray *objArray;
-  if (SUCCEEDED(mJumpListMgr->BeginList(&mMaxItems, IID_PPV_ARGS(&objArray)))) {
+  if (SUCCEEDED(jumpListMgr->BeginList(&mMaxItems, IID_PPV_ARGS(&objArray)))) {
     *aMaxItems = mMaxItems;
 
     if (objArray)
       objArray->Release();
 
-    mJumpListMgr->AbortList();
+    jumpListMgr->AbortList();
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP JumpListBuilder::InitListBuild(JSContext* aCx,
                                              Promise** aPromise)
 {
@@ -207,46 +232,65 @@ NS_IMETHODIMP JumpListBuilder::InitListB
   }
 
   promise.forget(aPromise);
   return NS_OK;
 }
 
 void JumpListBuilder::DoInitListBuild(RefPtr<Promise>&& aPromise)
 {
+  // Since we're invoking COM interfaces to talk to the shell on a background
+  // thread, we need to be running inside a multithreaded apartment.
+  mscom::MTARegion mta;
+  MOZ_ASSERT(mta.IsValid());
+
   ReentrantMonitorAutoEnter lock(mMonitor);
   MOZ_ASSERT(mJumpListMgr);
 
-  if(sBuildingList) {
+  if (sBuildingList) {
     AbortListBuild();
   }
 
+  HRESULT hr = E_UNEXPECTED;
+  auto errorHandler = MakeScopeExit([&aPromise, &hr]() {
+    if (SUCCEEDED(hr)) {
+      return;
+    }
+
+    NS_DispatchToMainThread(NS_NewRunnableFunction("InitListBuildReject",
+                                                   [promise = std::move(aPromise)]() {
+      promise->MaybeReject(NS_ERROR_FAILURE);
+    }));
+  });
+
+  RefPtr<ICustomDestinationList> jumpListMgr = mJumpListMgr;
+  if (!jumpListMgr) {
+    return;
+  }
+
   nsTArray<nsString> urisToRemove;
   RefPtr<IObjectArray> objArray;
-  HRESULT hr = mJumpListMgr->BeginList(&mMaxItems,
-                                       IID_PPV_ARGS(static_cast<IObjectArray**>
-                                                    (getter_AddRefs(objArray))));
+  hr = jumpListMgr->BeginList(&mMaxItems,
+                              IID_PPV_ARGS(static_cast<IObjectArray**>
+                                           (getter_AddRefs(objArray))));
+  if (FAILED(hr)) {
+    return;
+  }
+
   // The returned objArray of removed items are for manually removed items.
   // This does not return items which are removed because they were previously
   // part of the jump list but are no longer part of the jump list.
-  if (SUCCEEDED(hr)) {
-    sBuildingList = true;
-    RemoveIconCacheAndGetJumplistShortcutURIs(objArray, urisToRemove);
+  sBuildingList = true;
+  RemoveIconCacheAndGetJumplistShortcutURIs(objArray, urisToRemove);
 
-    NS_DispatchToMainThread(NS_NewRunnableFunction("InitListBuildResolve",
-                                                   [urisToRemove = std::move(urisToRemove),
-                                                    promise = std::move(aPromise)]() {
-      promise->MaybeResolve(urisToRemove);
-    }));
-  } else {
-    NS_DispatchToMainThread(NS_NewRunnableFunction("InitListBuildReject",
-                                                   [promise = std::move(aPromise)]() {
-      promise->MaybeReject(NS_ERROR_FAILURE);
-    }));
-  }
+  NS_DispatchToMainThread(NS_NewRunnableFunction("InitListBuildResolve",
+                                                 [urisToRemove = std::move(urisToRemove),
+                                                  promise = std::move(aPromise)]() {
+    promise->MaybeResolve(urisToRemove);
+  }));
 }
 
 // Ensures that we have no old ICO files left in the jump list cache
 nsresult JumpListBuilder::RemoveIconCacheForAllItems()
 {
   // Construct the path of our jump list cache
   nsCOMPtr<nsIFile> jumpListCacheDir;
   nsresult rv = NS_GetSpecialDirectory("ProfLDS",
@@ -289,16 +333,21 @@ NS_IMETHODIMP JumpListBuilder::AddListTo
   nsresult rv;
 
   *_retval = false;
 
   ReentrantMonitorAutoEnter lock(mMonitor);
   if (!mJumpListMgr)
     return NS_ERROR_NOT_AVAILABLE;
 
+  RefPtr<ICustomDestinationList> jumpListMgr = mJumpListMgr;
+  if (!jumpListMgr) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
   switch(aCatType) {
     case nsIJumpListBuilder::JUMPLIST_CATEGORY_TASKS:
     {
       NS_ENSURE_ARG_POINTER(items);
 
       HRESULT hr;
       RefPtr<IObjectCollection> collection;
       hr = CoCreateInstance(CLSID_EnumerableObjectCollection, nullptr,
@@ -333,32 +382,32 @@ NS_IMETHODIMP JumpListBuilder::AddListTo
 
       // We need IObjectArray to submit
       RefPtr<IObjectArray> pArray;
       hr = collection->QueryInterface(IID_IObjectArray, getter_AddRefs(pArray));
       if (FAILED(hr))
         return NS_ERROR_UNEXPECTED;
 
       // Add the tasks
-      hr = mJumpListMgr->AddUserTasks(pArray);
+      hr = jumpListMgr->AddUserTasks(pArray);
       if (SUCCEEDED(hr))
         *_retval = true;
       return NS_OK;
     }
     break;
     case nsIJumpListBuilder::JUMPLIST_CATEGORY_RECENT:
     {
-      if (SUCCEEDED(mJumpListMgr->AppendKnownCategory(KDC_RECENT)))
+      if (SUCCEEDED(jumpListMgr->AppendKnownCategory(KDC_RECENT)))
         *_retval = true;
       return NS_OK;
     }
     break;
     case nsIJumpListBuilder::JUMPLIST_CATEGORY_FREQUENT:
     {
-      if (SUCCEEDED(mJumpListMgr->AppendKnownCategory(KDC_FREQUENT)))
+      if (SUCCEEDED(jumpListMgr->AppendKnownCategory(KDC_FREQUENT)))
         *_retval = true;
       return NS_OK;
     }
     break;
     case nsIJumpListBuilder::JUMPLIST_CATEGORY_CUSTOMLIST:
     {
       NS_ENSURE_ARG_POINTER(items);
 
@@ -415,17 +464,17 @@ NS_IMETHODIMP JumpListBuilder::AddListTo
 
       // We need IObjectArray to submit
       RefPtr<IObjectArray> pArray;
       hr = collection->QueryInterface(IID_IObjectArray, (LPVOID*)&pArray);
       if (FAILED(hr))
         return NS_ERROR_UNEXPECTED;
 
       // Add the tasks
-      hr = mJumpListMgr->AppendCategory(reinterpret_cast<const wchar_t*>(catName.BeginReading()), pArray);
+      hr = jumpListMgr->AppendCategory(reinterpret_cast<const wchar_t*>(catName.BeginReading()), pArray);
       if (SUCCEEDED(hr))
         *_retval = true;
 
       // Get rid of the old icons
       nsCOMPtr<nsIRunnable> event =
         new mozilla::widget::AsyncDeleteAllFaviconsFromDisk(true);
       mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
 
@@ -437,17 +486,22 @@ NS_IMETHODIMP JumpListBuilder::AddListTo
 }
 
 NS_IMETHODIMP JumpListBuilder::AbortListBuild()
 {
   ReentrantMonitorAutoEnter lock(mMonitor);
   if (!mJumpListMgr)
     return NS_ERROR_NOT_AVAILABLE;
 
-  mJumpListMgr->AbortList();
+  RefPtr<ICustomDestinationList> jumpListMgr = mJumpListMgr;
+  if (!jumpListMgr) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
+  jumpListMgr->AbortList();
   sBuildingList = false;
 
   return NS_OK;
 }
 
 NS_IMETHODIMP JumpListBuilder::CommitListBuild(nsIJumpListCommittedCallback* aCallback)
 {
   ReentrantMonitorAutoEnter lock(mMonitor);
@@ -466,49 +520,69 @@ NS_IMETHODIMP JumpListBuilder::CommitLis
        &JumpListBuilder::DoCommitListBuild, std::move(callback));
   Unused << mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
 
   return NS_OK;
 }
 
 void JumpListBuilder::DoCommitListBuild(RefPtr<detail::DoneCommitListBuildCallback> aCallback)
 {
+  // Since we're invoking COM interfaces to talk to the shell on a background
+  // thread, we need to be running inside a multithreaded apartment.
+  mscom::MTARegion mta;
+  MOZ_ASSERT(mta.IsValid());
+
   ReentrantMonitorAutoEnter lock(mMonitor);
   MOZ_ASSERT(mJumpListMgr);
   MOZ_ASSERT(aCallback);
 
-  HRESULT hr = mJumpListMgr->CommitList();
+  HRESULT hr = E_UNEXPECTED;
+  auto onExit = MakeScopeExit([&hr, &aCallback]() {
+    // XXX We might want some specific error data here.
+    aCallback->SetResult(SUCCEEDED(hr));
+    Unused << NS_DispatchToMainThread(aCallback);
+  });
+
+  RefPtr<ICustomDestinationList> jumpListMgr = mJumpListMgr;
+  if (!jumpListMgr) {
+    return;
+  }
+
+  hr = jumpListMgr->CommitList();
   sBuildingList = false;
 
   if (SUCCEEDED(hr)) {
     mHasCommit = true;
   }
-
-  // XXX We might want some specific error data here.
-  aCallback->SetResult(SUCCEEDED(hr));
-  Unused << NS_DispatchToMainThread(aCallback);
 }
 
 NS_IMETHODIMP JumpListBuilder::DeleteActiveList(bool *_retval)
 {
   *_retval = false;
 
   ReentrantMonitorAutoEnter lock(mMonitor);
   if (!mJumpListMgr)
     return NS_ERROR_NOT_AVAILABLE;
 
-  if(sBuildingList)
+  if (sBuildingList) {
     AbortListBuild();
+  }
 
   nsAutoString uid;
   if (!WinTaskbar::GetAppUserModelID(uid))
     return NS_OK;
 
-  if (SUCCEEDED(mJumpListMgr->DeleteList(uid.get())))
+  RefPtr<ICustomDestinationList> jumpListMgr = mJumpListMgr;
+  if (!jumpListMgr) {
+    return NS_ERROR_UNEXPECTED;
+  }
+
+  if (SUCCEEDED(jumpListMgr->DeleteList(uid.get()))) {
     *_retval = true;
+  }
 
   return NS_OK;
 }
 
 /* internal */
 
 bool JumpListBuilder::IsSeparator(nsCOMPtr<nsIJumpListItem>& item)
 {
@@ -549,17 +623,17 @@ void JumpListBuilder::RemoveIconCacheAnd
 
     wchar_t buf[MAX_PATH];
     HRESULT hres = pLink->GetArguments(buf, MAX_PATH);
     if (SUCCEEDED(hres)) {
       LPWSTR *arglist;
       int32_t numArgs;
 
       arglist = ::CommandLineToArgvW(buf, &numArgs);
-      if(arglist && numArgs > 0) {
+      if (arglist && numArgs > 0) {
         nsString spec(arglist[0]);
         aURISpecs.AppendElement(std::move(spec));
         ::LocalFree(arglist);
       }
     }
 
     int iconIdx = 0;
     hres = pLink->GetIconLocation(buf, MAX_PATH, &iconIdx);
@@ -594,16 +668,19 @@ NS_IMETHODIMP JumpListBuilder::Observe(n
   NS_ENSURE_ARG_POINTER(aTopic);
   if (strcmp(aTopic, TOPIC_PROFILE_BEFORE_CHANGE) == 0) {
     nsCOMPtr<nsIObserverService> observerService =
       do_GetService("@mozilla.org/observer-service;1");
     if (observerService) {
       observerService->RemoveObserver(this, TOPIC_PROFILE_BEFORE_CHANGE);
     }
     mIOThread->Shutdown();
+    // Clear out mJumpListMgr, as MSCOM services won't be available soon.
+    ReentrantMonitorAutoEnter lock(mMonitor);
+    mJumpListMgr = nullptr;
   } else if (strcmp(aTopic, "nsPref:changed") == 0 &&
              nsDependentString(aData).EqualsASCII(kPrefTaskbarEnabled)) {
     bool enabled = Preferences::GetBool(kPrefTaskbarEnabled, true);
     if (!enabled) {
 
       nsCOMPtr<nsIRunnable> event =
         new mozilla::widget::AsyncDeleteAllFaviconsFromDisk();
       mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
--- a/widget/windows/JumpListBuilder.h
+++ b/widget/windows/JumpListBuilder.h
@@ -18,16 +18,17 @@
 #include "nsIMutableArray.h"
 
 #include "nsIJumpListBuilder.h"
 #include "nsIJumpListItem.h"
 #include "JumpListItem.h"
 #include "nsIObserver.h"
 #include "nsTArray.h"
 #include "mozilla/Attributes.h"
+#include "mozilla/mscom/AgileReference.h"
 #include "mozilla/ReentrantMonitor.h"
 
 namespace mozilla {
 namespace widget {
 
 namespace detail {
 class DoneCommitListBuildCallback;
 } // namespace detail
@@ -43,17 +44,17 @@ public:
   NS_DECL_NSIOBSERVER
 
   JumpListBuilder();
 
 protected:
   static Atomic<bool> sBuildingList;
 
 private:
-  RefPtr<ICustomDestinationList> mJumpListMgr;
+  mscom::AgileReference mJumpListMgr;
   uint32_t mMaxItems;
   bool mHasCommit;
   nsCOMPtr<nsIThread> mIOThread;
   ReentrantMonitor mMonitor;
 
   bool IsSeparator(nsCOMPtr<nsIJumpListItem>& item);
   void RemoveIconCacheAndGetJumplistShortcutURIs(IObjectArray *aObjArray, nsTArray<nsString>& aURISpecs);
   void DeleteIconFromDisk(const nsAString& aPath);