Bug 1240912 - Toolbox remains connected across frame swaps. r=ochameau

It is now possible keep the toolbox open while toggling RDM open and closed.
The toolbox will follow the frame as it moves and update the message manager it
uses internally with each move.

MozReview-Commit-ID: DvLzCmOXfnb

devtools/client/framework/toolbox-init.js

@@ -74,7 +74,7 @@ if (url.search.length > 1) {
       window.removeEventListener("unload", onUnload);
       toolbox.destroy();
     }
-    window.addEventListener("unload", onUnload, true);
+    window.addEventListener("unload", onUnload);
     toolbox.on("destroy", function () {
       window.removeEventListener("unload", onUnload);
     });

devtools/client/responsive.html/browser/swap.js

@@ -35,6 +35,20 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) {
   let innerBrowser;
   let tunnel;
 
+  // Dispatch a custom event each time the _viewport content_ is swapped from one browser
+  // to another.  DevTools server code uses this to follow the content if there is an
+  // active DevTools connection.  While browser.xml does dispatch it's own SwapDocShells
+  // event, this one is easier for DevTools to follow because it's only emitted once per
+  // transition, instead of twice like SwapDocShells.
+  let dispatchDevToolsBrowserSwap = (from, to) => {
+    let CustomEvent = tab.ownerDocument.defaultView.CustomEvent;
+    let event = new CustomEvent("DevTools:BrowserSwap", {
+      detail: to,
+      bubbles: true,
+    });
+    from.dispatchEvent(event);
+  };
+
   return {
 
     start: Task.async(function* () {
@@ -72,6 +86,7 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) {
       // 4. Swap tab content from the regular browser tab to the browser within
       //    the viewport in the tool UI, preserving all state via
       //    `gBrowser._swapBrowserDocShells`.
+      dispatchDevToolsBrowserSwap(tab.linkedBrowser, innerBrowser);
       gBrowser._swapBrowserDocShells(tab, innerBrowser);
 
       // 5. Force the original browser tab to be non-remote since the tool UI
@@ -115,6 +130,7 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) {
       // 4. Swap tab content from the browser within the viewport in the tool UI
       //    to the regular browser tab, preserving all state via
       //    `gBrowser._swapBrowserDocShells`.
+      dispatchDevToolsBrowserSwap(innerBrowser, contentBrowser);
       gBrowser._swapBrowserDocShells(contentTab, innerBrowser);
       innerBrowser = null;
 
@@ -126,6 +142,7 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) {
       // 6. Swap the content into the original browser tab and close the
       //    temporary tab used to hold the content via
       //    `swapBrowsersAndCloseOther`.
+      dispatchDevToolsBrowserSwap(contentBrowser, tab.linkedBrowser);
       gBrowser.swapBrowsersAndCloseOther(tab, contentTab);
       gBrowser = null;
 

devtools/client/responsive.html/test/browser/browser.ini

@@ -28,10 +28,10 @@ support-files =
 [browser_page_state.js]
 [browser_permission_doorhanger.js]
 [browser_resize_cmd.js]
-skip-if = true # GCLI target confused after swap, will fix in bug 1240912
 [browser_screenshot_button.js]
 [browser_shutdown_close_sync.js]
 [browser_toolbox_computed_view.js]
 [browser_toolbox_rule_view.js]
+[browser_toolbox_swap_browsers.js]
 [browser_touch_simulation.js]
 [browser_viewport_basics.js]

devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js

@@ -0,0 +1,82 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Verify that toolbox remains open when opening and closing RDM.
+
+const TEST_URL = "http://example.com/";
+
+function getServerConnections(browser) {
+  ok(browser.isRemoteBrowser, "Content browser is remote");
+  return ContentTask.spawn(browser, {}, function* () {
+    const Cu = Components.utils;
+    const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
+    const { DebuggerServer } = require("devtools/server/main");
+    if (!DebuggerServer._connections) {
+      return 0;
+    }
+    return Object.getOwnPropertyNames(DebuggerServer._connections);
+  });
+}
+
+let checkServerConnectionCount = Task.async(function* (browser, expected) {
+  let conns = yield getServerConnections(browser);
+  is(conns.length || 0, expected, "Server connection count");
+});
+
+let checkToolbox = Task.async(function* (tab, location) {
+  let target = TargetFactory.forTab(tab);
+  ok(!!gDevTools.getToolbox(target), `Toolbox exists ${location}`);
+});
+
+add_task(function* () {
+  let tab = yield addTab(TEST_URL);
+
+  // Open toolbox outside RDM
+  {
+    // 0: No DevTools connections yet
+    yield checkServerConnectionCount(tab.linkedBrowser, 0);
+    let { toolbox } = yield openInspector();
+    // 2: One for each tab (starting tab plus the one we opened).  Only one truly needed,
+    //    but calling listTabs will create one for each tab.  `registerTestActor` calls
+    //    this, triggering the extra tab's actor to be made.
+    yield checkServerConnectionCount(tab.linkedBrowser, 2);
+    yield checkToolbox(tab, "outside RDM");
+    let { ui } = yield openRDM(tab);
+    // 3: RDM UI uses an extra connection
+    yield checkServerConnectionCount(ui.getViewportBrowser(), 3);
+    yield checkToolbox(tab, "after opening RDM");
+    yield closeRDM(tab);
+    // 2: RDM UI closed, back to one for each tab
+    yield checkServerConnectionCount(tab.linkedBrowser, 2);
+    yield checkToolbox(tab, tab.linkedBrowser, "after closing RDM");
+    yield toolbox.destroy();
+    // 0: All DevTools usage closed
+    yield checkServerConnectionCount(tab.linkedBrowser, 0);
+  }
+
+  // Open toolbox inside RDM
+  {
+    // 0: No DevTools connections yet
+    yield checkServerConnectionCount(tab.linkedBrowser, 0);
+    let { ui } = yield openRDM(tab);
+    // 1: RDM UI uses an extra connection
+    yield checkServerConnectionCount(ui.getViewportBrowser(), 1);
+    let { toolbox } = yield openInspector();
+    // 3: One for each tab (starting tab plus the one we opened).  Only one truly needed,
+    //    but calling listTabs will create one for each tab.  `registerTestActor` calls
+    //    this, triggering the extra tab's actor to be made.
+    yield checkServerConnectionCount(ui.getViewportBrowser(), 3);
+    yield checkToolbox(tab, ui.getViewportBrowser(), "inside RDM");
+    yield closeRDM(tab);
+    // 2: RDM UI closed, back to one for each tab
+    yield checkServerConnectionCount(tab.linkedBrowser, 2);
+    yield checkToolbox(tab, tab.linkedBrowser, "after closing RDM");
+    yield toolbox.destroy();
+    // 0: All DevTools usage closed
+    yield checkServerConnectionCount(tab.linkedBrowser, 0);
+  }
+
+  yield removeTab(tab);
+});

devtools/server/main.js

@@ -1005,7 +1005,24 @@ var DebuggerServer = {
     // or else fallback to asking the frameLoader itself.
     let mm = frame.messageManager || frame.frameLoader.messageManager;
     mm.loadFrameScript("resource://devtools/server/child.js", false);
-    this._childMessageManagers.add(mm);
+
+    let trackMessageManager = () => {
+      frame.addEventListener("DevTools:BrowserSwap", onBrowserSwap);
+      mm.addMessageListener("debug:setup-in-parent", onSetupInParent);
+      if (!actor) {
+        mm.addMessageListener("debug:actor", onActorCreated);
+      }
+      DebuggerServer._childMessageManagers.add(mm);
+    };
+
+    let untrackMessageManager = () => {
+      frame.removeEventListener("DevTools:BrowserSwap", onBrowserSwap);
+      mm.removeMessageListener("debug:setup-in-parent", onSetupInParent);
+      if (!actor) {
+        mm.removeMessageListener("debug:actor", onActorCreated);
+      }
+      DebuggerServer._childMessageManagers.delete(mm);
+    };
 
     let actor, childTransport;
     let prefix = connection.allocID("child");
@@ -1044,7 +1061,6 @@ var DebuggerServer = {
         return false;
       }
     };
-    mm.addMessageListener("debug:setup-in-parent", onSetupInParent);
 
     let onActorCreated = DevToolsUtils.makeInfallible(function (msg) {
       if (msg.json.prefix != prefix) {
@@ -1069,11 +1085,25 @@ var DebuggerServer = {
       let { NetworkMonitorManager } = require("devtools/shared/webconsole/network-monitor");
       netMonitor = new NetworkMonitorManager(frame, actor.actor);
 
-      events.emit(DebuggerServer, "new-child-process", { mm });
-
       deferred.resolve(actor);
     }).bind(this);
-    mm.addMessageListener("debug:actor", onActorCreated);
+
+    // Listen for browser frame swap
+    let onBrowserSwap = ({ detail: newFrame }) => {
+      // Remove listeners from old frame and mm
+      untrackMessageManager();
+      // Update frame and mm to point to the new browser frame
+      frame = newFrame;
+      // Get messageManager from XUL browser (which might be a specialized tunnel for RDM)
+      // or else fallback to asking the frameLoader itself.
+      mm = frame.messageManager || frame.frameLoader.messageManager;
+      // Add listeners to new frame and mm
+      trackMessageManager();
+
+      if (childTransport) {
+        childTransport.swapBrowser(mm);
+      }
+    };
 
     let destroy = DevToolsUtils.makeInfallible(function () {
       // provides hook to actor modules that need to exchange messages
@@ -1119,16 +1149,14 @@ var DebuggerServer = {
       }
 
       // Cleanup all listeners
+      untrackMessageManager();
       Services.obs.removeObserver(onMessageManagerClose, "message-manager-close");
-      mm.removeMessageListener("debug:setup-in-parent", onSetupInParent);
-      if (!actor) {
-        mm.removeMessageListener("debug:actor", onActorCreated);
-      }
       events.off(connection, "closed", destroy);
-
-      DebuggerServer._childMessageManagers.delete(mm);
     });
 
+    // Listen for various messages and frame events
+    trackMessageManager();
+
     // Listen for app process exit
     let onMessageManagerClose = function (subject, topic, data) {
       if (subject == mm) {

devtools/shared/transport/transport.js

@@ -699,11 +699,11 @@
   exports.LocalDebuggerTransport = LocalDebuggerTransport;
 
   /**
-   * A transport for the debugging protocol that uses nsIMessageSenders to
+   * A transport for the debugging protocol that uses nsIMessageManagers to
    * exchange packets with servers running in child processes.
    *
-   * In the parent process, |sender| should be the nsIMessageSender for the
-   * child process. In a child process, |sender| should be the child process
+   * In the parent process, |mm| should be the nsIMessageSender for the
+   * child process. In a child process, |mm| should be the child process
    * message manager, which sends packets to the parent.
    *
    * |prefix| is a string included in the message names, to distinguish
@@ -712,10 +712,10 @@
    * This transport exchanges messages named 'debug:<prefix>:packet', where
    * <prefix> is |prefix|, whose data is the protocol packet.
    */
-  function ChildDebuggerTransport(sender, prefix) {
+  function ChildDebuggerTransport(mm, prefix) {
     EventEmitter.decorate(this);
 
-    this._sender = sender;
+    this._mm = mm;
     this._messageName = "debug:" + prefix + ":packet";
   }
 
@@ -729,13 +729,13 @@
 
     hooks: null,
 
-    ready: function () {
-      this._sender.addMessageListener(this._messageName, this);
+    _addListener() {
+      this._mm.addMessageListener(this._messageName, this);
     },
 
-    close: function () {
+    _removeListener() {
       try {
-        this._sender.removeMessageListener(this._messageName, this);
+        this._mm.removeMessageListener(this._messageName, this);
       } catch (e) {
         if (e.result != Cr.NS_ERROR_NULL_POINTER) {
           throw e;
@@ -744,6 +744,14 @@
         // this point with a dead messageManager which only throws errors but does not
         // seem to indicate in any other way that it is dead.
       }
+    },
+
+    ready: function () {
+      this._addListener();
+    },
+
+    close: function () {
+      this._removeListener();
       this.emit("close");
       this.hooks.onClosed();
     },
@@ -756,7 +764,7 @@
     send: function (packet) {
       this.emit("send", packet);
       try {
-        this._sender.sendAsyncMessage(this._messageName, packet);
+        this._mm.sendAsyncMessage(this._messageName, packet);
       } catch (e) {
         if (e.result != Cr.NS_ERROR_NULL_POINTER) {
           throw e;
@@ -769,7 +777,13 @@
 
     startBulkSend: function () {
       throw new Error("Can't send bulk data to child processes.");
-    }
+    },
+
+    swapBrowser(mm) {
+      this._removeListener();
+      this._mm = mm;
+      this._addListener();
+    },
   };
 
   exports.ChildDebuggerTransport = ChildDebuggerTransport;