Bug 1240913 - Destroy RDM early on quit. r=ochameau draft
authorJ. Ryan Stinnett <jryans@gmail.com>
Wed, 11 May 2016 16:22:22 -0500
changeset 367558 2320a0bf6fca4a1bfb15601a38cea5c4a9fb334c
parent 367557 aba8c8b9e83f12dc2773df1feacf8ad37cabf8ba
child 521039 d6aae5e1a8e3d6ee58c48e9eee45cb0277cda1f1
push id18271
push userbmo:jryans@gmail.com
push dateMon, 16 May 2016 21:10:48 +0000
reviewersochameau
bugs1240913
milestone49.0a1
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
devtools/client/responsive.html/manager.js
devtools/docs/responsive-design-mode.md
--- 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.