Bug 1354143 - Commit jump list on lazy idle thread. r?jimm,florian
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
--- a/browser/modules/WindowsJumpLists.jsm
+++ b/browser/modules/WindowsJumpLists.jsm
@@ -261,19 +261,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,