Bug 1354143 - Commit jump list on lazy idle thread. r=florian,jimm
authorWei-Cheng Pan <wpan@mozilla.com>
Wed, 24 May 2017 16:38:57 +0800
changeset 411539 1ce78807187af4d8243ee751d3176f77d3a30068
parent 411538 9858be1746b67131a36b60dbae910e60c00a3040
child 411540 6cb63e92bc902cb701f8ad65a32218d1e6ee66bf
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersflorian, jimm
bugs1354143
milestone55.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 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
browser/modules/WindowsJumpLists.jsm
widget/nsIJumpListBuilder.idl
widget/tests/unit/test_taskbar_jumplistitems.js
widget/windows/JumpListBuilder.cpp
widget/windows/JumpListBuilder.h
widget/windows/WinUtils.cpp
widget/windows/WinUtils.h
--- a/browser/modules/WindowsJumpLists.jsm
+++ b/browser/modules/WindowsJumpLists.jsm
@@ -246,19 +246,25 @@ this.WinTaskbarJumpList =
       // Prior to building, delete removed items from history.
       this._clearHistory(removedItems);
       return true;
     }
     return false;
   },
 
   _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() {
     var items = Cc["@mozilla.org/array;1"].
                 createInstance(Ci.nsIMutableArray);
     this._tasks.forEach(function(task) {
       if ((this._shuttingDown && !task.close) || (!this._shuttingDown && !task.open))
         return;
--- a/widget/nsIJumpListBuilder.idl
+++ b/widget/nsIJumpListBuilder.idl
@@ -3,16 +3,22 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsISupports.idl"
 
 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
 {
   /**
    * JumpLists
    *
    * Jump lists are built and then applied. Modifying an applied jump list is not
    * permitted. Callers should begin the creation of a new jump list using
@@ -131,19 +137,21 @@ interface nsIJumpListBuilder : nsISuppor
   /**
    * Aborts and clears the current jump list build.
    */
   void abortListBuild();
 
   /**
    * 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.
    * Common uses would be the enabling of a privacy mode and uninstallation.
    *
    * @returns true if the operation completed successfully.
    *
    * @throw NS_ERROR_UNEXPECTED on internal errors.
--- a/widget/tests/unit/test_taskbar_jumplistitems.js
+++ b/widget/tests/unit/test_taskbar_jumplistitems.js
@@ -169,17 +169,17 @@ function test_shortcuts()
     sc.app = handlerApp;
     do_check_eq(sc.app.detailedDescription, "TestApp detailed description.");
     do_check_eq(sc.app.name, "TestApp");
     do_check_true(sc.app.parameterExists("-test"));
     do_check_false(sc.app.parameterExists("-notset"));
   }
 }
 
-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
   // those in these tests. We'll init the jump list for the xpc shell harness,
   // add a task item, and commit it.
  
   // not compiled in
   if (Ci.nsIWinTaskbar == null)
@@ -220,42 +220,55 @@ function test_jumplist()
   handlerApp.detailedDescription = "Testing detailed description.";
 
   var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
                   getService(Ci.nsIProperties).
                   QueryInterface(Ci.nsIDirectoryService);
   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);
 
     var removed = Cc["@mozilla.org/array;1"]
                   .createInstance(Ci.nsIMutableArray);
     do_check_true(builder.initListBuild(removed));
     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();
   }
 }
 
 function run_test()
 {
   if (mozinfo.os != "win") {
     return;
   }
   test_basics();
   test_separator();
   test_hashes();
   test_links();
   test_shortcuts();
-  test_jumplist();
+
+  run_next_test();
 }
+
+add_task(test_jumplist);
--- a/widget/windows/JumpListBuilder.cpp
+++ b/widget/windows/JumpListBuilder.cpp
@@ -15,39 +15,98 @@
 #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/Unused.h"
 
 #include "WinUtils.h"
 
 // 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
 
 namespace mozilla {
 namespace widget {
 
 static NS_DEFINE_CID(kJumpListItemCID,     NS_WIN_JUMPLISTITEM_CID);
 static NS_DEFINE_CID(kJumpListLinkCID,     NS_WIN_JUMPLISTLINK_CID);
 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)
 {
   ::CoInitialize(nullptr);
 
   CoCreateInstance(CLSID_DestinationList, nullptr, CLSCTX_INPROC_SERVER,
                    IID_ICustomDestinationList, getter_AddRefs(mJumpListMgr));
@@ -402,33 +461,51 @@ NS_IMETHODIMP JumpListBuilder::AbortList
     return NS_ERROR_NOT_AVAILABLE;
 
   mJumpListMgr->AbortList();
   sBuildingList = false;
 
   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)
 {
   *_retval = false;
 
   if (!mJumpListMgr)
     return NS_ERROR_NOT_AVAILABLE;
--- a/widget/windows/JumpListBuilder.h
+++ b/widget/windows/JumpListBuilder.h
@@ -21,41 +21,46 @@
 #include "nsIJumpListItem.h"
 #include "JumpListItem.h"
 #include "nsIObserver.h"
 #include "mozilla/Attributes.h"
 
 namespace mozilla {
 namespace widget {
 
+namespace detail {
+class DoneCommitListBuildCallback;
+} // namespace detail
+
 class JumpListBuilder : public nsIJumpListBuilder, 
                         public nsIObserver
 {
   virtual ~JumpListBuilder();
 
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIJUMPLISTBUILDER
   NS_DECL_NSIOBSERVER
 
   JumpListBuilder();
 
 protected:
-  static bool sBuildingList; 
+  static Atomic<bool> sBuildingList;
 
 private:
   RefPtr<ICustomDestinationList> mJumpListMgr;
   uint32_t mMaxItems;
   bool mHasCommit;
   nsCOMPtr<nsIThread> mIOThread;
 
   bool IsSeparator(nsCOMPtr<nsIJumpListItem>& item);
   nsresult TransferIObjectArrayToIMutableArray(IObjectArray *objArray, nsIMutableArray *removedItems);
   nsresult RemoveIconCacheForItems(nsIMutableArray *removedItems);
   nsresult RemoveIconCacheForAllItems();
+  void DoCommitListBuild(RefPtr<detail::DoneCommitListBuildCallback> aCallback);
 
   friend class WinTaskbar;
 };
 
 } // namespace widget
 } // namespace mozilla
 
 #endif /* __JumpListBuilder_h__ */
--- a/widget/windows/WinUtils.cpp
+++ b/widget/windows/WinUtils.cpp
@@ -1456,30 +1456,34 @@ AsyncDeleteIconFromDisk::~AsyncDeleteIco
 
 AsyncDeleteAllFaviconsFromDisk::
   AsyncDeleteAllFaviconsFromDisk(bool aIgnoreRecent)
   : mIgnoreRecent(aIgnoreRecent)
 {
   // 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
   do {
     bool hasMore = false;
     if (NS_FAILED(entries->HasMoreElements(&hasMore)) || !hasMore)
       break;
 
--- a/widget/windows/WinUtils.h
+++ b/widget/windows/WinUtils.h
@@ -568,16 +568,17 @@ public:
 
   explicit AsyncDeleteAllFaviconsFromDisk(bool aIgnoreRecent = false);
 
 private:
   virtual ~AsyncDeleteAllFaviconsFromDisk();
 
   int32_t mIcoNoDeleteSeconds;
   bool mIgnoreRecent;
+  nsCOMPtr<nsIFile> mJumpListCacheDir;
 };
 
 class FaviconHelper
 {
 public:
   static const char kJumpListCacheDir[];
   static const char kShortcutCacheDir[];
   static nsresult ObtainCachedIconFile(nsCOMPtr<nsIURI> aFaviconPageURI,