Bug 1354143 - Commit jump list on lazy idle thread. r=florian,jimm

Since committing will do IO on the main thread, it would be better to do it on
an idle thread instead. We have to change JavaScript code too because now the
API is asynchrous.

This patch also updates its xpcshell test.

Now mozilla::widget::AsyncDeleteAllFaviconsFromDisk will get profile directory
on the main thread to prevent it happens on off-main-threads, thus prevents
off-main-thread assertion.

MozReview-Commit-ID: CWcR0B2BC3n

--HG--
extra : rebase_source : 3685a07f9f4476bc94bdf92937734b78fb3fe309

browser/modules/WindowsJumpLists.jsm

@@ -251,9 +251,15 @@ this.WinTaskbarJumpList =
   },
 
   _commitBuild: function WTBJL__commitBuild() {
-    if (!this._hasPendingStatements() && !this._builder.commitListBuild()) {
-      this._builder.abortListBuild();
+    if (this._hasPendingStatements()) {
+      return;
     }
+
+    this._builder.commitListBuild(succeed => {
+      if (!succeed) {
+        this._builder.abortListBuild();
+      }
+    });
   },
 
   _buildTasks: function WTBJL__buildTasks() {

widget/nsIJumpListBuilder.idl

@@ -8,6 +8,12 @@
 interface nsIArray;
 interface nsIMutableArray;
 
+[scriptable, function, uuid(5131a62a-e99f-4631-9138-751f8aad1ae4)]
+interface nsIJumpListCommittedCallback : nsISupports
+{
+  void done(in boolean result);
+};
+
 [scriptable, uuid(1FE6A9CD-2B18-4dd5-A176-C2B32FA4F683)]
 interface nsIJumpListBuilder : nsISupports
 {
@@ -136,9 +142,11 @@ interface nsIJumpListBuilder : nsISupports
   /**
    * Commits the current jump list build to the Taskbar.
    *
-   * @returns true if the operation completed successfully.
+   * @param callback
+   *        Receives one argument, which is true if the operation completed
+   *        successfully, otherwise it is false.
    */
-  boolean commitListBuild();
+  void commitListBuild([optional] in nsIJumpListCommittedCallback callback);
 
   /**
    * Deletes any currently applied taskbar jump list for this application.

widget/tests/unit/test_taskbar_jumplistitems.js

@@ -174,7 +174,7 @@ function test_shortcuts()
   }
 }
 
-function test_jumplist()
+async function test_jumplist()
 {
   // Jump lists can't register links unless the application is the default
   // protocol handler for the protocol of the link, so we skip off testing
@@ -225,6 +225,10 @@ function test_jumplist()
   var notepad = dirSvc.get("WinD", Ci.nsIFile);
   notepad.append("notepad.exe");
   if (notepad.exists()) {
+    // To ensure "profile-before-change" will fire before
+    // "xpcom-shutdown-threads"
+    do_get_profile();
+
     handlerApp.executable = notepad;
     sc.app = handlerApp;
     items.appendElement(sc);
@@ -235,13 +239,19 @@ function test_jumplist()
     do_check_true(builder.addListToBuild(builder.JUMPLIST_CATEGORY_TASKS, items));
     do_check_true(builder.addListToBuild(builder.JUMPLIST_CATEGORY_RECENT));
     do_check_true(builder.addListToBuild(builder.JUMPLIST_CATEGORY_FREQUENT));
-    do_check_true(builder.commitListBuild());
+    let rv = new Promise((resolve) => {
+      builder.commitListBuild(resolve);
+    });
+    do_check_true(await rv);
 
     builder.deleteActiveList();
 
     do_check_true(builder.initListBuild(removed));
-    do_check_true(builder.addListToBuild(builder.JUMPLIST_CATEGORY_CUSTOM, items, "Custom List"));
-    do_check_true(builder.commitListBuild());
+    do_check_true(builder.addListToBuild(builder.JUMPLIST_CATEGORY_CUSTOMLIST, items, "Custom List"));
+    rv = new Promise((resolve) => {
+      builder.commitListBuild(resolve);
+    });
+    do_check_true(await rv);
 
     builder.deleteActiveList();
   }
@@ -257,5 +267,8 @@ function run_test()
   test_hashes();
   test_links();
   test_shortcuts();
-  test_jumplist();
+
+  run_next_test();
 }
+
+add_task(test_jumplist);

widget/windows/JumpListBuilder.cpp

@@ -20,6 +20,7 @@
 #include "nsThreadUtils.h"
 #include "mozilla/LazyIdleThread.h"
 #include "nsIObserverService.h"
+#include "mozilla/Unused.h"
 
 #include "WinUtils.h"
 
@@ -36,13 +37,71 @@ static NS_DEFINE_CID(kJumpListShortcutCID, NS_WIN_JUMPLISTSHORTCUT_CID);
 // defined in WinTaskbar.cpp
 extern const wchar_t *gMozillaJumpListIDGeneric;
 
-bool JumpListBuilder::sBuildingList = false;
+Atomic<bool> JumpListBuilder::sBuildingList(false);
 const char kPrefTaskbarEnabled[] = "browser.taskbar.lists.enabled";
 
 NS_IMPL_ISUPPORTS(JumpListBuilder, nsIJumpListBuilder, nsIObserver)
 #define TOPIC_PROFILE_BEFORE_CHANGE "profile-before-change"
 #define TOPIC_CLEAR_PRIVATE_DATA "clear-private-data"
 
+
+namespace detail {
+
+class DoneCommitListBuildCallback : public nsIRunnable
+{
+  NS_DECL_THREADSAFE_ISUPPORTS
+
+public:
+  DoneCommitListBuildCallback(nsIJumpListCommittedCallback* aCallback,
+                              JumpListBuilder* aBuilder)
+    : mCallback(aCallback)
+    , mBuilder(aBuilder)
+    , mResult(false)
+  {
+  }
+
+  NS_IMETHOD Run() override
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+    if (mCallback) {
+      Unused << mCallback->Done(mResult);
+    }
+    // Ensure we are releasing on the main thread.
+    Destroy();
+    return NS_OK;
+  }
+
+  void SetResult(bool aResult)
+  {
+    mResult = aResult;
+  }
+
+private:
+  ~DoneCommitListBuildCallback()
+  {
+    // Destructor does not always call on the main thread.
+    MOZ_ASSERT(!mCallback);
+    MOZ_ASSERT(!mBuilder);
+  }
+
+  void Destroy()
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+    mCallback = nullptr;
+    mBuilder = nullptr;
+  }
+
+  // These two references MUST be released on the main thread.
+  RefPtr<nsIJumpListCommittedCallback> mCallback;
+  RefPtr<JumpListBuilder> mBuilder;
+  bool mResult;
+};
+
+NS_IMPL_ISUPPORTS(DoneCommitListBuildCallback, nsIRunnable);
+
+} // namespace detail
+
+
 JumpListBuilder::JumpListBuilder() :
   mMaxItems(0),
   mHasCommit(false)
@@ -407,23 +466,41 @@ NS_IMETHODIMP JumpListBuilder::AbortListBuild()
   return NS_OK;
 }
 
-NS_IMETHODIMP JumpListBuilder::CommitListBuild(bool *_retval)
+NS_IMETHODIMP JumpListBuilder::CommitListBuild(nsIJumpListCommittedCallback* aCallback)
 {
-  *_retval = false;
-
   if (!mJumpListMgr)
     return NS_ERROR_NOT_AVAILABLE;
 
+  // Also holds a strong reference to this to prevent use-after-free.
+  RefPtr<detail::DoneCommitListBuildCallback> callback =
+    new detail::DoneCommitListBuildCallback(aCallback, this);
+
+  // The builder has a strong reference in the callback already, so we do not
+  // need to do it for this runnable again.
+  RefPtr<nsIRunnable> event =
+    NewNonOwningRunnableMethod<RefPtr<detail::DoneCommitListBuildCallback>>
+      ("JumpListBuilder::DoCommitListBuild", this,
+       &JumpListBuilder::DoCommitListBuild, Move(callback));
+  Unused << mIOThread->Dispatch(event, NS_DISPATCH_NORMAL);
+
+  return NS_OK;
+}
+
+void JumpListBuilder::DoCommitListBuild(RefPtr<detail::DoneCommitListBuildCallback> aCallback)
+{
+  MOZ_ASSERT(mJumpListMgr);
+  MOZ_ASSERT(aCallback);
+
   HRESULT hr = mJumpListMgr->CommitList();
   sBuildingList = false;
 
-  // XXX We might want some specific error data here.
   if (SUCCEEDED(hr)) {
-    *_retval = true;
     mHasCommit = true;
   }
 
-  return NS_OK;
+  // XXX We might want some specific error data here.
+  aCallback->SetResult(SUCCEEDED(hr));
+  Unused << NS_DispatchToMainThread(aCallback);
 }
 
 NS_IMETHODIMP JumpListBuilder::DeleteActiveList(bool *_retval)

widget/windows/JumpListBuilder.h

@@ -26,6 +26,10 @@
 namespace mozilla {
 namespace widget {
 
+namespace detail {
+class DoneCommitListBuildCallback;
+} // namespace detail
+
 class JumpListBuilder : public nsIJumpListBuilder, 
                         public nsIObserver
 {
@@ -39,7 +43,7 @@ public:
   JumpListBuilder();
 
 protected:
-  static bool sBuildingList; 
+  static Atomic<bool> sBuildingList;
 
 private:
   RefPtr<ICustomDestinationList> mJumpListMgr;
@@ -51,6 +55,7 @@ private:
   nsresult TransferIObjectArrayToIMutableArray(IObjectArray *objArray, nsIMutableArray *removedItems);
   nsresult RemoveIconCacheForItems(nsIMutableArray *removedItems);
   nsresult RemoveIconCacheForAllItems();
+  void DoCommitListBuild(RefPtr<detail::DoneCommitListBuildCallback> aCallback);
 
   friend class WinTaskbar;
 };

widget/windows/WinUtils.cpp

@@ -1461,20 +1461,24 @@ AsyncDeleteAllFaviconsFromDisk::
   // We can't call FaviconHelper::GetICOCacheSecondsTimeout() on non-main
   // threads, as it reads a pref, so cache its value here.
   mIcoNoDeleteSeconds = FaviconHelper::GetICOCacheSecondsTimeout() + 600;
+
+  // Prepare the profile directory cache on the main thread, to ensure we wont
+  // do this on non-main threads.
+  Unused << NS_GetSpecialDirectory("ProfLDS",
+    getter_AddRefs(mJumpListCacheDir));
 }
 
 NS_IMETHODIMP AsyncDeleteAllFaviconsFromDisk::Run()
 {
+  if (!mJumpListCacheDir) {
+    return NS_ERROR_FAILURE;
+  }
   // Construct the path of our jump list cache
-  nsCOMPtr<nsIFile> jumpListCacheDir;
-  nsresult rv = NS_GetSpecialDirectory("ProfLDS", 
-    getter_AddRefs(jumpListCacheDir));
-  NS_ENSURE_SUCCESS(rv, rv);
-  rv = jumpListCacheDir->AppendNative(
+  nsresult rv = mJumpListCacheDir->AppendNative(
       nsDependentCString(FaviconHelper::kJumpListCacheDir));
   NS_ENSURE_SUCCESS(rv, rv);
   nsCOMPtr<nsISimpleEnumerator> entries;
-  rv = jumpListCacheDir->GetDirectoryEntries(getter_AddRefs(entries));
+  rv = mJumpListCacheDir->GetDirectoryEntries(getter_AddRefs(entries));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Loop through each directory entry and remove all ICO files found

widget/windows/WinUtils.h

@@ -573,6 +573,7 @@ private:
 
   int32_t mIcoNoDeleteSeconds;
   bool mIgnoreRecent;
+  nsCOMPtr<nsIFile> mJumpListCacheDir;
 };
 
 class FaviconHelper