Bug 1397387 - Move async actions out of the opening/closing cycles of the bookmarks dialog to ensure they finish. r=mak

MozReview-Commit-ID: 9H5hPfTNv0S

--HG--
extra : rebase_source : 7012972cb2e4af14500ca7a3376effdb7b1fd7e0

browser/components/places/PlacesUIUtils.jsm

@@ -23,6 +23,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "RecentWindow",
                                   "resource:///modules/RecentWindow.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
+                                  "resource://gre/modules/PromiseUtils.jsm");
 
 // PlacesUtils exposes multiple symbols, so we can't use defineLazyModuleGetter.
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
@@ -635,8 +637,7 @@ this.PlacesUIUtils = {
    * @see documentation at the top of bookmarkProperties.js
    * @return true if any transaction has been performed, false otherwise.
    */
-  showBookmarkDialog:
-  function PUIU_showBookmarkDialog(aInfo, aParentWindow) {
+  showBookmarkDialog(aInfo, aParentWindow) {
     // Preserve size attributes differently based on the fact the dialog has
     // a folder picker or not, since it needs more horizontal space than the
     // other controls.
@@ -648,8 +649,33 @@ this.PlacesUIUtils = {
                     "chrome://browser/content/places/bookmarkProperties.xul";
 
     let features = "centerscreen,chrome,modal,resizable=yes";
+
+    let topUndoEntry;
+    let batchBlockingDeferred;
+
+    if (this.useAsyncTransactions) {
+      // Set the transaction manager into batching mode.
+      topUndoEntry = PlacesTransactions.topUndoEntry;
+      batchBlockingDeferred = PromiseUtils.defer();
+      PlacesTransactions.batch(async () => {
+        await batchBlockingDeferred.promise;
+      });
+    }
+
     aParentWindow.openDialog(dialogURL, "", features, aInfo);
-    return ("performed" in aInfo && aInfo.performed);
+
+    let performed = ("performed" in aInfo && aInfo.performed);
+
+    if (this.useAsyncTransactions) {
+      batchBlockingDeferred.resolve();
+
+      if (!performed &&
+          topUndoEntry != PlacesTransactions.topUndoEntry) {
+        PlacesTransactions.undo().catch(Components.utils.reportError);
+      }
+    }
+
+    return performed;
   },
 
   _getTopBrowserWin: function PUIU__getTopBrowserWin() {

browser/components/places/content/bookmarkProperties.js

@@ -62,8 +62,6 @@
 Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
                                   "resource://gre/modules/PrivateBrowsingUtils.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
-                                  "resource://gre/modules/PromiseUtils.jsm");
 
 const BOOKMARK_ITEM = 0;
 const BOOKMARK_FOLDER = 1;
@@ -381,13 +379,7 @@ var BookmarkPropertiesPanel = {
   _beginBatch() {
     if (this._batching)
       return;
-    if (PlacesUIUtils.useAsyncTransactions) {
-      this._topUndoEntry = PlacesTransactions.topUndoEntry;
-      this._batchBlockingDeferred = PromiseUtils.defer();
-      PlacesTransactions.batch(async () => {
-        await this._batchBlockingDeferred.promise;
-      });
-    } else {
+    if (!PlacesUIUtils.useAsyncTransactions) {
       PlacesUtils.transactionManager.beginBatch(null);
     }
     this._batching = true;
@@ -395,18 +387,12 @@ var BookmarkPropertiesPanel = {
 
   _endBatch() {
     if (!this._batching)
-      return false;
+      return;
 
-    if (PlacesUIUtils.useAsyncTransactions) {
-      this._batchBlockingDeferred.resolve();
-      this._batchBlockingDeferred = null;
-    } else {
+    if (!PlacesUIUtils.useAsyncTransactions) {
       PlacesUtils.transactionManager.endBatch(false);
     }
     this._batching = false;
-    let changed = this._topUndoEntry != PlacesTransactions.topUndoEntry;
-    delete this._topUndoEntry;
-    return changed;
   },
 
   // nsISupports
@@ -450,12 +436,8 @@ var BookmarkPropertiesPanel = {
     // changes done as part of Undo may change the panel contents and by
     // that force it to commit more transactions.
     gEditItemOverlay.uninitPanel(true);
-    let changed = this._endBatch();
-    if (PlacesUIUtils.useAsyncTransactions) {
-      if (changed) {
-        PlacesTransactions.undo().catch(Components.utils.reportError);
-      }
-    } else {
+    this._endBatch();
+    if (!PlacesUIUtils.useAsyncTransactions) {
       PlacesUtils.transactionManager.undoTransaction();
     }
     window.arguments[0].performed = false;

browser/components/places/tests/browser/browser_bookmarkProperties_cancel.js

@@ -68,14 +68,14 @@ add_task(async function test_cancel_with_no_changes() {
       }
     );
 
-    Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called");
-
     // Check the bookmark is still removed.
     Assert.ok(!(await PlacesUtils.bookmarks.fetch(bookmarks[0].guid)),
       "The originally removed bookmark should not exist.");
 
     Assert.ok(await PlacesUtils.bookmarks.fetch(bookmarks[1].guid),
       "The second bookmark should still exist");
+
+    Assert.ok(PlacesTransactions.undo.notCalled, "undo should not have been called");
   });
 });
 
@@ -112,7 +112,7 @@ add_task(async function test_cancel_with_changes() {
       }
     );
 
-    Assert.ok(PlacesTransactions.undo.calledOnce,
+    await BrowserTestUtils.waitForCondition(() => PlacesTransactions.undo.calledOnce,
       "undo should have been called once.");
   });
 });