From c9f1c242db7a12b337ded17714b472df45c211d2 Mon Sep 17 00:00:00 2001 From: Mike Conley Date: Tue, 24 Nov 2015 18:41:32 -0500 Subject: [PATCH] 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 --- .../components/sessionstore/SessionStore.jsm | 21 ++++--- .../sessionstore/TabStateFlusher.jsm | 60 ++++++++++++++++--- .../test/browser_send_async_message_oom.js | 6 +- 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/browser/components/sessionstore/SessionStore.jsm b/browser/components/sessionstore/SessionStore.jsm index a194d0e425c4..600c733e48a5 100644 --- a/browser/components/sessionstore/SessionStore.jsm +++ b/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); diff --git a/browser/components/sessionstore/TabStateFlusher.jsm b/browser/components/sessionstore/TabStateFlusher.jsm index 9959ff7e15f8..6397efe9d8a6 100644 --- a/browser/components/sessionstore/TabStateFlusher.jsm +++ b/browser/components/sessionstore/TabStateFlusher.jsm @@ -37,9 +37,19 @@ this.TabStateFlusher = Object.freeze({ /** * Resolves the flush request with the given flush ID. + * + * @param 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 () + * 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 () + * 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 () + * 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. diff --git a/browser/components/sessionstore/test/browser_send_async_message_oom.js b/browser/components/sessionstore/test/browser_send_async_message_oom.js index 582d62ac65d5..6afd771db3f8 100644 --- a/browser/components/sessionstore/test/browser_send_async_message_oom.js +++ b/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;