diff --git a/browser/modules/WindowsJumpLists.jsm b/browser/modules/WindowsJumpLists.jsm index c331e6f00a97..4d0b87314dcb 100644 --- a/browser/modules/WindowsJumpLists.jsm +++ b/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() { diff --git a/widget/nsIJumpListBuilder.idl b/widget/nsIJumpListBuilder.idl index 35846f005d46..83b1ca9fdbe6 100644 --- a/widget/nsIJumpListBuilder.idl +++ b/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. diff --git a/widget/tests/unit/test_taskbar_jumplistitems.js b/widget/tests/unit/test_taskbar_jumplistitems.js index c3b5ad0c4589..945a323a64ef 100644 --- a/widget/tests/unit/test_taskbar_jumplistitems.js +++ b/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); diff --git a/widget/windows/JumpListBuilder.cpp b/widget/windows/JumpListBuilder.cpp index 566c41d4ab65..fdd28bb2f004 100644 --- a/widget/windows/JumpListBuilder.cpp +++ b/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 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 mCallback; + RefPtr 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 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 event = + NewNonOwningRunnableMethod> + ("JumpListBuilder::DoCommitListBuild", this, + &JumpListBuilder::DoCommitListBuild, Move(callback)); + Unused << mIOThread->Dispatch(event, NS_DISPATCH_NORMAL); + + return NS_OK; +} + +void JumpListBuilder::DoCommitListBuild(RefPtr 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) diff --git a/widget/windows/JumpListBuilder.h b/widget/windows/JumpListBuilder.h index 553ff765d088..ed5a21276934 100644 --- a/widget/windows/JumpListBuilder.h +++ b/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 sBuildingList; private: RefPtr mJumpListMgr; @@ -51,6 +55,7 @@ private: nsresult TransferIObjectArrayToIMutableArray(IObjectArray *objArray, nsIMutableArray *removedItems); nsresult RemoveIconCacheForItems(nsIMutableArray *removedItems); nsresult RemoveIconCacheForAllItems(); + void DoCommitListBuild(RefPtr aCallback); friend class WinTaskbar; }; diff --git a/widget/windows/WinUtils.cpp b/widget/windows/WinUtils.cpp index 269ca0039dd3..fa4414190624 100644 --- a/widget/windows/WinUtils.cpp +++ b/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 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 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 diff --git a/widget/windows/WinUtils.h b/widget/windows/WinUtils.h index 8655e112b8f5..497551379899 100644 --- a/widget/windows/WinUtils.h +++ b/widget/windows/WinUtils.h @@ -573,6 +573,7 @@ private: int32_t mIcoNoDeleteSeconds; bool mIgnoreRecent; + nsCOMPtr mJumpListCacheDir; }; class FaviconHelper