Bug 1396888 - Handle async moving of bookmarks in the caller for the move dialog, to avoid going out of scope whilst async actions are happening. r=mak

MozReview-Commit-ID: JhETMnmxNOD

--HG--
extra : rebase_source : ff7b201900d06c7d9feafb791dea03d19ec4bdcd

browser/components/places/content/controller.js

@@ -282,7 +282,7 @@ PlacesController.prototype = {
       this.showBookmarkPropertiesForSelection();
       break;
     case "placesCmd_moveBookmarks":
-      this.moveSelectedBookmarks();
+      this.moveSelectedBookmarks().catch(Components.utils.reportError);
       break;
     case "placesCmd_reload":
       this.reloadSelectedLivemark();
@@ -778,10 +778,38 @@ PlacesController.prototype = {
   /**
    * Opens a dialog for moving the selected nodes.
    */
-  moveSelectedBookmarks: function PC_moveBookmarks() {
+  async moveSelectedBookmarks() {
+    let args = {
+      // The guid of the folder to move bookmarks to. This will only be
+      // set in the useAsyncTransactions case.
+      moveToGuid: null,
+      // nodes is passed to support !useAsyncTransactions.
+      nodes: this._view.selectedNodes,
+    };
     window.openDialog("chrome://browser/content/places/moveBookmarks.xul",
                       "", "chrome, modal",
-                      this._view.selectedNodes);
+                      args);
+
+    if (!args.moveToGuid) {
+      return;
+    }
+
+    let transactions = [];
+
+    for (let node of this._view.selectedNodes) {
+      // Nothing to do if the node is already under the selected folder.
+      if (node.parent.bookmarkGuid == args.moveToGuid) {
+        continue;
+      }
+      transactions.push(PlacesTransactions.Move({
+        guid: node.bookmarkGuid,
+        newParentGuid: args.moveToGuid,
+      }));
+    }
+
+    if (transactions.length) {
+      await PlacesTransactions.batch(transactions);
+    }
   },
 
   /**

browser/components/places/content/moveBookmarks.js

@@ -15,7 +15,7 @@ var gMoveBookmarksDialog = {
   },
 
   init() {
-    this._nodes = window.arguments[0];
+    this._nodes = window.arguments[0].nodes;
 
     this.foldersTree.place =
       "place:excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1&folder=" +
@@ -45,16 +45,9 @@ var gMoveBookmarksDialog = {
       return;
     }
 
-    PlacesTransactions.batch(async () => {
-      let newParentGuid = await PlacesUtils.promiseItemGuid(selectedFolderId);
-      for (let node of this._nodes) {
-        // Nothing to do if the node is already under the selected folder.
-        if (node.parent.itemId == selectedFolderId)
-          continue;
-        await PlacesTransactions.Move({ guid: node.bookmarkGuid,
-                                        newParentGuid }).transact();
-      }
-    }).catch(Components.utils.reportError);
+    // Async transactions must do the move in the caller to avoid going out of
+    // scope whilst the dialog is still closing.
+    window.arguments[0].moveToGuid = selectedNode.bookmarkGuid;
   },
 
   newFolder: function MBD_newFolder() {

browser/components/places/tests/browser/browser.ini

@@ -45,6 +45,7 @@ subsuite = clipboard
 [browser_library_left_pane_middleclick.js]
 [browser_library_left_pane_select_hierarchy.js]
 [browser_library_middleclick.js]
+[browser_library_move_bookmarks.js]
 [browser_library_open_leak.js]
 [browser_library_openFlatContainer.js]
 [browser_library_panel_leak.js]

browser/components/places/tests/browser/browser_library_move_bookmarks.js

@@ -0,0 +1,79 @@
+/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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/. */
+
+/**
+ *  Test enabled commands in the left pane folder of the Library.
+ */
+
+registerCleanupFunction(async function() {
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesTestUtils.clearHistory();
+});
+
+add_task(async function test_moveBookmarks() {
+  let children = [{
+    title: "TestFolder",
+    type: PlacesUtils.bookmarks.TYPE_FOLDER,
+  }];
+
+  for (let i = 0; i < 10; i++) {
+    children.push({
+      title: `test${i}`,
+      url: `http://example.com/${i}`,
+    });
+  }
+
+  let bookmarks = await PlacesUtils.bookmarks.insertTree({
+    guid: PlacesUtils.bookmarks.unfiledGuid,
+    children
+  });
+
+  let folderId = await PlacesUtils.promiseItemId(bookmarks[0].guid);
+
+  let itemIds = [];
+  let promiseMoveNotifications = [];
+  for (let i = 0; i < 10; i++) {
+    // + 1 due to the folder being inserted first.
+    let guid = bookmarks[i + 1].guid;
+
+    itemIds.push(await PlacesUtils.promiseItemId(guid));
+    promiseMoveNotifications.push(PlacesTestUtils.waitForNotification(
+      "onItemMoved",
+      (itemId, parentId, oldIndex, newParentId, newIndex, itemType, itemGuid,
+       oldParentGuid, newParentGuid) =>
+        itemGuid == guid && newParentGuid == bookmarks[0].guid
+    ));
+  }
+
+  let library = await promiseLibrary("UnfiledBookmarks");
+
+  library.ContentTree.view.selectItems(itemIds);
+
+  await withBookmarksDialog(
+    false,
+    () => {
+      library.ContentTree.view._controller.doCommand("placesCmd_moveBookmarks");
+    },
+    async (dialogWin) => {
+      let tree = dialogWin.document.getElementById("foldersTree");
+
+      tree.selectItems([PlacesUtils.unfiledBookmarksFolderId], true);
+
+      tree.selectItems([folderId], true);
+
+      dialogWin.document.documentElement.acceptDialog();
+
+      info("Waiting for notifications of moves");
+      await Promise.all(promiseMoveNotifications);
+      Assert.ok(true, "should have completed all moves successfully");
+    },
+    null,
+    "chrome://browser/content/places/moveBookmarks.xul",
+    true
+  );
+
+  library.close();
+});

browser/components/places/tests/browser/head.js

@@ -277,14 +277,16 @@ function isToolbarVisible(aToolbar) {
  *        A function to be used to wait for pending work when the dialog is
  *        closing. It is passed the dialog window handle and should return a promise.
  */
-var withBookmarksDialog = async function(autoCancel, openFn, taskFn, closeFn) {
+var withBookmarksDialog = async function(autoCancel, openFn, taskFn, closeFn,
+                                         dialogUrl = "chrome://browser/content/places/bookmarkProperties",
+                                         skipOverlayWait = false) {
   let closed = false;
   let dialogPromise = new Promise(resolve => {
     Services.ww.registerNotification(function winObserver(subject, topic, data) {
       if (topic == "domwindowopened") {
         let win = subject.QueryInterface(Ci.nsIDOMWindow);
         win.addEventListener("load", function() {
-          ok(win.location.href.startsWith("chrome://browser/content/places/bookmarkProperties"),
+          ok(win.location.href.startsWith(dialogUrl),
              "The bookmark properties dialog is open");
           // This is needed for the overlay.
           waitForFocus(() => {
@@ -306,9 +308,11 @@ var withBookmarksDialog = async function(autoCancel, openFn, taskFn, closeFn) {
   let dialogWin = await dialogPromise;
 
   // Ensure overlay is loaded
-  info("waiting for the overlay to be loaded");
-  await waitForCondition(() => dialogWin.gEditItemOverlay.initialized,
-                         "EditItemOverlay should be initialized");
+  if (!skipOverlayWait) {
+    info("waiting for the overlay to be loaded");
+    await waitForCondition(() => dialogWin.gEditItemOverlay.initialized,
+                           "EditItemOverlay should be initialized");
+  }
 
   // Check the first textbox is focused.
   let doc = dialogWin.document;
@@ -329,10 +333,13 @@ var withBookmarksDialog = async function(autoCancel, openFn, taskFn, closeFn) {
   try {
     await taskFn(dialogWin);
   } finally {
+    if (!closed && !autoCancel) {
+      // Give the dialog a little time to close itself in the manually closing
+      // case.
+      await BrowserTestUtils.waitForCondition(() => closed,
+        "The test should have closed the dialog!");
+    }
     if (!closed) {
-      if (!autoCancel) {
-        ok(false, "The test should have closed the dialog!");
-      }
       info("withBookmarksDialog: canceling the dialog");
 
       doc.documentElement.cancelDialog();