Bug 1240913 - Destroy RDM early on quit. r=ochameau
Watch for the tab's `beforeunload` and destroy RDM when it happens. This
resolves issues with session restore getting confused about the state of content
managed by RDM.
MozReview-Commit-ID: DXs87Xm11JR
--- a/devtools/client/responsive.html/manager.js
+++ b/devtools/client/responsive.html/manager.js
@@ -77,23 +77,25 @@ const ResponsiveUIManager = exports.Resp
/**
* Closes the responsive UI, if not already closed.
*
* @param window
* The main browser chrome window.
* @param tab
* The browser tab.
+ * @param object
+ * Close options, which currently includes a `reason` string.
* @return Promise
* Resolved (with no value) when closing is complete.
*/
- closeIfNeeded: Task.async(function* (window, tab) {
+ closeIfNeeded: Task.async(function* (window, tab, options) {
if (this.isActiveForTab(tab)) {
let ui = this.activeTabs.get(tab);
- let destroyed = yield ui.destroy();
+ let destroyed = yield ui.destroy(options);
if (!destroyed) {
// Already in the process of destroying, abort.
return;
}
this.activeTabs.delete(tab);
if (!this.activeTabs.size) {
off(events.activate, "data", onActivate);
off(events.close, "data", onClose);
@@ -212,16 +214,19 @@ ResponsiveUI.prototype = {
* to ensure all in-page state is preserved, just like when you move a tab to
* a new window.
*
* For more details, see /devtools/docs/responsive-design-mode.md.
*/
init: Task.async(function* () {
let ui = this;
+ // Watch for the tab's beforeunload to catch app shutdown early
+ this.tab.linkedBrowser.addEventListener("beforeunload", this, true);
+
// Swap page content from the current tab into a viewport within RDM
this.swap = swapToInnerBrowser({
tab: this.tab,
containerURL: TOOL_URL,
getInnerBrowser: Task.async(function* (containerBrowser) {
let toolWindow = ui.toolWindow = containerBrowser.contentWindow;
toolWindow.addEventListener("message", ui);
yield message.request(toolWindow, "init");
@@ -231,72 +236,109 @@ ResponsiveUI.prototype = {
toolWindow.document.querySelector("iframe.browser");
return toolViewportContentBrowser;
})
});
yield this.swap.start();
// Notify the inner browser to start the frame script
yield message.request(this.toolWindow, "start-frame-script");
-
- // TODO: Session restore continues to store the tool UI as the page's URL.
- // Most likely related to browser UI's inability to show correct location.
}),
/**
* Close RDM and restore page content back into a regular tab.
*
+ * @param object
+ * Destroy options, which currently includes a `reason` string.
* @return boolean
* Whether this call is actually destroying. False means destruction
* was already in progress.
*/
- destroy: Task.async(function* () {
+ destroy: Task.async(function* (options) {
if (this.destroying) {
return false;
}
this.destroying = true;
+ // If our tab is about to be unloaded, there's not enough time to exit
+ // gracefully, but that shouldn't be a problem since the tab will go away.
+ // So, skip any yielding when we're about to unload.
+ let isBeforeUnload = options && options.reason == "beforeunload";
+
// Ensure init has finished before starting destroy
- yield this.inited;
+ if (!isBeforeUnload) {
+ yield this.inited;
+ }
+
+ this.tab.linkedBrowser.removeEventListener("beforeunload", this, true);
+ this.toolWindow.removeEventListener("message", this);
// Notify the inner browser to stop the frame script
- yield message.request(this.toolWindow, "stop-frame-script");
+ if (!isBeforeUnload) {
+ yield message.request(this.toolWindow, "stop-frame-script");
+ }
// Destroy local state
let swap = this.swap;
this.browserWindow = null;
this.tab = null;
this.inited = null;
this.toolWindow = null;
this.swap = null;
// Undo the swap and return the content back to a normal tab
swap.stop();
return true;
}),
handleEvent(event) {
- let { tab, window, toolWindow } = this;
+ let { browserWindow, tab } = this;
+
+ switch (event.type) {
+ case "message":
+ this.handleMessage(event);
+ break;
+ case "beforeunload":
+ // Ignore the event for locations like about:blank
+ if (event.target.location != TOOL_URL) {
+ return;
+ }
+ // We want to ensure we close RDM before browser window close when
+ // quitting. Session restore starts its final session data flush when
+ // it gets the `quit-application-granted` notification. If we were to
+ // wait for browser windows to close before destroying (which will undo
+ // the content swap), session restore gets wedged in a confused state
+ // since we're moving browsers at the same time that it is blocking
+ // shutdown on messages from each browser. So instead, we destroy
+ // earlier based on the tab's `beforeunload` event.
+ ResponsiveUIManager.closeIfNeeded(browserWindow, tab, {
+ reason: event.type,
+ });
+ break;
+ }
+ },
+
+ handleMessage(event) {
+ let { browserWindow, tab } = this;
if (event.origin !== "chrome://devtools") {
return;
}
switch (event.data.type) {
case "content-resize":
let { width, height } = event.data;
this.emit("content-resize", {
width,
height,
});
break;
case "exit":
- toolWindow.removeEventListener(event.type, this);
- ResponsiveUIManager.closeIfNeeded(window, tab);
+ ResponsiveUIManager.closeIfNeeded(browserWindow, tab);
break;
}
},
/**
* Helper for tests. Assumes a single viewport for now.
*/
getViewportSize() {
--- a/devtools/docs/responsive-design-mode.md
+++ b/devtools/docs/responsive-design-mode.md
@@ -54,10 +54,13 @@ 5. Swap the content into the original br
When restarting Firefox and restoring a user's browsing session, we must
correctly restore the tab history. If the RDM tool was opened when the session
was captured, then it would be acceptable to either:
* A: Restore the tab content without any RDM tool displayed **OR**
* B: Restore the RDM tool the tab content inside, just as before the restart
-Option A (no RDM after session restore) seems more in line with how the rest of
-DevTools currently functions after restore.
+We currently follow path A (no RDM after session restore), which seems more in
+line with how the rest of DevTools currently functions after restore. To do so,
+we watch for `beforeunload` events on the tab at shutdown and quickly exit RDM
+so that session restore records only the original page content during its final
+write at shutdown.