Bug 1177310 - TabStateFlusher Promises should always resolve. r=Yoric

They'll always resolve, but might receive a negative success state
and a message. We're doing this so that we can maintain a Set of
in-flight flushes that we can call Promise.all on (which will
fast-fail if any Promise rejects, or will just never resolve if
one or more of the Promises never resolve).

--HG--
extra : commitid : AkIM83IZCxK
extra : rebase_source : 4fb8eb059589299c87fcbd3c4e519a81a8b93def

browser/components/sessionstore/SessionStore.jsm

@@ -837,6 +837,7 @@ var SessionStoreInternal = {
         break;
       case "SessionStore:error":
         this.reportInternalError(data);
+        TabStateFlusher.resolveAll(browser, false, "Received error from the content process");
         break;
       default:
         throw new Error(`received unknown message '${aMessage.name}'`);
@@ -1210,6 +1211,10 @@ var SessionStoreInternal = {
 
     var tabbrowser = aWindow.gBrowser;
 
+    // The tabbrowser binding will go away once the window is closed,
+    // so we'll hold a reference to the browsers in the closure here.
+    let browsers = tabbrowser.browsers;
+
     TAB_EVENTS.forEach(function(aEvent) {
       tabbrowser.tabContainer.removeEventListener(aEvent, this, true);
     }, this);
@@ -1281,10 +1286,6 @@ var SessionStoreInternal = {
         this.maybeSaveClosedWindow(winData, isLastWindow);
       }
 
-      // The tabbrowser binding will go away once the window is closed,
-      // so we'll hold a reference to the browsers in the closure here.
-      let browsers = tabbrowser.browsers;
-
       TabStateFlusher.flushWindow(aWindow).then(() => {
         // At this point, aWindow is closed! You should probably not try to
         // access any DOM elements from aWindow within this callback unless
@@ -1310,13 +1311,13 @@ var SessionStoreInternal = {
 
         // Update the tabs data now that we've got the most
         // recent information.
-        this.cleanUpWindow(aWindow, winData);
+        this.cleanUpWindow(aWindow, winData, browsers);
 
         // save the state without this window to disk
         this.saveStateDelayed();
       });
     } else {
-      this.cleanUpWindow(aWindow, winData);
+      this.cleanUpWindow(aWindow, winData, browsers);
     }
 
     for (let i = 0; i < tabbrowser.tabs.length; i++) {
@@ -1336,7 +1337,13 @@ var SessionStoreInternal = {
    *        DyingWindowCache in case anybody is still holding a
    *        reference to it.
    */
-  cleanUpWindow(aWindow, winData) {
+  cleanUpWindow(aWindow, winData, browsers) {
+    // Any leftover TabStateFlusher Promises need to be resolved now,
+    // since we're about to remove the message listeners.
+    for (let browser of browsers) {
+      TabStateFlusher.resolveAll(browser);
+    }
+
     // Cache the window state until it is completely gone.
     DyingWindowCache.set(aWindow, winData);
 

browser/components/sessionstore/TabStateFlusher.jsm

@@ -37,9 +37,19 @@ this.TabStateFlusher = Object.freeze({
 
   /**
    * Resolves the flush request with the given flush ID.
+   *
+   * @param browser (<xul:browser>)
+   *        The browser for which the flush is being resolved.
+   * @param flushID (int)
+   *        The ID of the flush that was sent to the browser.
+   * @param success (bool, optional)
+   *        Whether or not the flush succeeded.
+   * @param message (string, optional)
+   *        An error message that will be sent to the Console in the
+   *        event that a flush failed.
    */
-  resolve(browser, flushID) {
-    TabStateFlusherInternal.resolve(browser, flushID);
+  resolve(browser, flushID, success=true, message="") {
+    TabStateFlusherInternal.resolve(browser, flushID, success, message);
   },
 
   /**
@@ -47,9 +57,17 @@ this.TabStateFlusher = Object.freeze({
    * used when the content process crashed or the final update message was
    * seen. In those cases we can't guarantee to ever hear back from the frame
    * script so we just resolve all requests instead of discarding them.
+   *
+   * @param browser (<xul:browser>)
+   *        The browser for which all flushes are being resolved.
+   * @param success (bool, optional)
+   *        Whether or not the flushes succeeded.
+   * @param message (string, optional)
+   *        An error message that will be sent to the Console in the
+   *        event that the flushes failed.
    */
-  resolveAll(browser) {
-    TabStateFlusherInternal.resolveAll(browser);
+  resolveAll(browser, success=true, message="") {
+    TabStateFlusherInternal.resolveAll(browser, success, message);
   }
 });
 
@@ -95,8 +113,18 @@ var TabStateFlusherInternal = {
 
   /**
    * Resolves the flush request with the given flush ID.
+   *
+   * @param browser (<xul:browser>)
+   *        The browser for which the flush is being resolved.
+   * @param flushID (int)
+   *        The ID of the flush that was sent to the browser.
+   * @param success (bool, optional)
+   *        Whether or not the flush succeeded.
+   * @param message (string, optional)
+   *        An error message that will be sent to the Console in the
+   *        event that a flush failed.
    */
-  resolve(browser, flushID) {
+  resolve(browser, flushID, success=true, message="") {
     // Nothing to do if there are no pending flushes for the given browser.
     if (!this._requests.has(browser.permanentKey)) {
       return;
@@ -108,10 +136,14 @@ var TabStateFlusherInternal = {
       return;
     }
 
+    if (!success) {
+      Cu.reportError("Failed to flush browser: " + message);
+    }
+
     // Resolve the request with the given id.
     let resolve = perBrowserRequests.get(flushID);
     perBrowserRequests.delete(flushID);
-    resolve();
+    resolve(success);
   },
 
   /**
@@ -119,8 +151,16 @@ var TabStateFlusherInternal = {
    * used when the content process crashed or the final update message was
    * seen. In those cases we can't guarantee to ever hear back from the frame
    * script so we just resolve all requests instead of discarding them.
+   *
+   * @param browser (<xul:browser>)
+   *        The browser for which all flushes are being resolved.
+   * @param success (bool, optional)
+   *        Whether or not the flushes succeeded.
+   * @param message (string, optional)
+   *        An error message that will be sent to the Console in the
+   *        event that the flushes failed.
    */
-  resolveAll(browser) {
+  resolveAll(browser, success=true, message="") {
     // Nothing to do if there are no pending flushes for the given browser.
     if (!this._requests.has(browser.permanentKey)) {
       return;
@@ -129,9 +169,13 @@ var TabStateFlusherInternal = {
     // Retrieve active requests for given browser.
     let perBrowserRequests = this._requests.get(browser.permanentKey);
 
+    if (!success) {
+      Cu.reportError("Failed to flush browser: " + message);
+    }
+
     // Resolve all requests.
     for (let resolve of perBrowserRequests.values()) {
-      resolve();
+      resolve(success);
     }
 
     // Clear active requests.

browser/components/sessionstore/test/browser_send_async_message_oom.js

@@ -51,7 +51,11 @@ add_task(function*() {
 
   // Attempt to flush. This should fail.
   let promiseFlushed = TabStateFlusher.flush(browser);
-  promiseFlushed.then(() => {throw new Error("Flush should have failed")});
+  promiseFlushed.then((success) => {
+    if (success) {
+      throw new Error("Flush should have failed")
+    }
+  });
 
   // The frame script should report an error.
   yield promiseReported;