From 673a539ce88cb4abde3472c11f5dfbc045ca5e39 Mon Sep 17 00:00:00 2001 From: Mike Conley Date: Fri, 13 Nov 2015 14:46:37 -0500 Subject: [PATCH] Bug 1177310 - Don't flush windows synchronously on application shutdown. r=billm,feedback=ttaubert Instead of synchronously flushing the windows, we use AsyncShutdown to wait for all window flushes to finish. --HG-- extra : commitid : 1dDdTLBcZrt extra : rebase_source : ea10bed3dc4920e769e73a6469510d4f303b88dc --- browser/components/sessionstore/RunState.jsm | 19 +++-- .../components/sessionstore/SessionStore.jsm | 83 +++++++++++++++---- .../sessionstore/test/browser_broadcast.js | 29 ------- 3 files changed, 79 insertions(+), 52 deletions(-) diff --git a/browser/components/sessionstore/RunState.jsm b/browser/components/sessionstore/RunState.jsm index f7fa0fbc0d34..f8a0fb3bb617 100644 --- a/browser/components/sessionstore/RunState.jsm +++ b/browser/components/sessionstore/RunState.jsm @@ -19,14 +19,6 @@ const STATE_CLOSED = 4; // We're initially stopped. var state = STATE_STOPPED; -function observer(subj, topic) { - Services.obs.removeObserver(observer, topic); - state = STATE_QUITTING; -} - -// Listen for when the application is quitting. -Services.obs.addObserver(observer, "quit-application-granted", false); - /** * This module keeps track of SessionStore's current run state. We will * always start out at STATE_STOPPED. After the sessionw as read from disk and @@ -95,5 +87,14 @@ this.RunState = Object.freeze({ if (this.isClosing) { state = STATE_CLOSED; } - } + }, + + // Switch the run state to STATE_QUITTING. This should be called once we're + // certain that the browser is going away and before we start collecting the + // final window states to save in the session file. + setQuitting() { + if (this.isRunning) { + state = STATE_QUITTING; + } + }, }); diff --git a/browser/components/sessionstore/SessionStore.jsm b/browser/components/sessionstore/SessionStore.jsm index 600c733e48a5..fe234b03bb73 100644 --- a/browser/components/sessionstore/SessionStore.jsm +++ b/browser/components/sessionstore/SessionStore.jsm @@ -31,7 +31,7 @@ const MAX_CONCURRENT_TAB_RESTORES = 3; // global notifications observed const OBSERVING = [ "browser-window-before-show", "domwindowclosed", - "quit-application-requested", "browser-lastwindow-close-granted", + "quit-application-granted", "browser-lastwindow-close-granted", "quit-application", "browser:purge-session-history", "browser:purge-domain-data", "idle-daily", @@ -180,6 +180,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "Utils", "resource:///modules/sessionstore/Utils.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "ViewSourceBrowser", "resource://gre/modules/ViewSourceBrowser.jsm"); +XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown", + "resource://gre/modules/AsyncShutdown.jsm"); /** * |true| if we are in debug mode, |false| otherwise. @@ -627,8 +629,8 @@ var SessionStoreInternal = { case "domwindowclosed": // catch closed windows this.onClose(aSubject); break; - case "quit-application-requested": - this.onQuitApplicationRequested(); + case "quit-application-granted": + this.onQuitApplicationGranted(); break; case "browser-lastwindow-close-granted": this.onLastWindowCloseGranted(); @@ -1402,23 +1404,76 @@ var SessionStoreInternal = { }, /** - * On quit application requested + * On quit application granted */ - onQuitApplicationRequested: function ssi_onQuitApplicationRequested() { - // get a current snapshot of all windows - this._forEachBrowserWindow(function(aWindow) { - // Flush all data queued in the content script to not lose it when - // shutting down. - TabState.flushWindow(aWindow); - this._collectWindowData(aWindow); + onQuitApplicationGranted: function ssi_onQuitApplicationGranted() { + // Collect an initial snapshot of window data before we do the flush + this._forEachBrowserWindow((win) => { + this._collectWindowData(win); }); - // we must cache this because _getMostRecentBrowserWindow will always - // return null by the time quit-application occurs + + // Now add an AsyncShutdown blocker that'll spin the event loop + // until the windows have all been flushed. + + // This progress object will track the state of async window flushing + // and will help us debug things that go wrong with our AsyncShutdown + // blocker. + let progress = { total: -1, current: -1 }; + + // We're going down! Switch state so that we treat closing windows and + // tabs correctly. + RunState.setQuitting(); + + AsyncShutdown.quitApplicationGranted.addBlocker( + "SessionStore: flushing all windows", + this.flushAllWindowsAsync(progress), + () => progress); + }, + + /** + * An async Task that iterates all open browser windows and flushes + * any outstanding messages from their tabs. This will also close + * all of the currently open windows while we wait for the flushes + * to complete. + * + * @param progress (Object) + * Optional progress object that will be updated as async + * window flushing progresses. flushAllWindowsSync will + * write to the following properties: + * + * total (int): + * The total number of windows to be flushed. + * current (int): + * The current window that we're waiting for a flush on. + * + * @return Promise + */ + flushAllWindowsAsync: Task.async(function*(progress={}) { + let windowPromises = []; + // We collect flush promises and close each window immediately so that + // the user can't start changing any window state while we're waiting + // for the flushes to finish. + this._forEachBrowserWindow((win) => { + windowPromises.push(TabStateFlusher.flushWindow(win)); + win.close(); + }); + + progress.total = windowPromises.length; + + // We'll iterate through the Promise array, yielding each one, so as to + // provide useful progress information to AsyncShutdown. + for (let i = 0; i < windowPromises.length; ++i) { + progress.current = i; + yield windowPromises[i]; + }; + + // We must cache this because _getMostRecentBrowserWindow will always + // return null by the time quit-application occurs. var activeWindow = this._getMostRecentBrowserWindow(); if (activeWindow) this.activeWindowSSiCache = activeWindow.__SSi || ""; DirtyWindows.clear(); - }, + }), /** * On last browser window close diff --git a/browser/components/sessionstore/test/browser_broadcast.js b/browser/components/sessionstore/test/browser_broadcast.js index fc89cf757cb1..49dddc3a66cc 100644 --- a/browser/components/sessionstore/test/browser_broadcast.js +++ b/browser/components/sessionstore/test/browser_broadcast.js @@ -21,29 +21,6 @@ add_task(function flush_on_tabclose() { "sessionStorage data has been flushed on TabClose"); }); -/** - * This test ensures we won't lose tab data queued in the content script when - * the application tries to quit. - */ -add_task(function flush_on_quit_requested() { - let tab = yield createTabWithStorageData(["http://example.com"]); - let browser = tab.linkedBrowser; - - yield modifySessionStorage(browser, {test: "on-quit-requested"}); - - // Note that sending quit-application-requested should not interfere with - // other tests and code. We're just notifying about a shutdown request but - // we will not send quit-application-granted. Observers will thus assume - // that some other observer has canceled the request. - sendQuitApplicationRequested(); - - let {storage} = JSON.parse(ss.getTabState(tab)); - is(storage["http://example.com"].test, "on-quit-requested", - "sessionStorage data has been flushed when a quit is requested"); - - gBrowser.removeTab(tab); -}); - /** * This test ensures we won't lose tab data queued in the content script when * duplicating a tab. @@ -152,9 +129,3 @@ function createTabWithStorageData(urls, win = window) { throw new Task.Result(tab); }); } - -function sendQuitApplicationRequested() { - let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"] - .createInstance(Ci.nsISupportsPRBool); - Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null); -}