diff --git a/devtools/client/framework/toolbox-init.js b/devtools/client/framework/toolbox-init.js index 95eee3075507..2b776dc1796b 100644 --- a/devtools/client/framework/toolbox-init.js +++ b/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); }); diff --git a/devtools/client/responsive.html/browser/swap.js b/devtools/client/responsive.html/browser/swap.js index 53f7d4ae8ad8..deb673bb7c5e 100644 --- a/devtools/client/responsive.html/browser/swap.js +++ b/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; diff --git a/devtools/client/responsive.html/test/browser/browser.ini b/devtools/client/responsive.html/test/browser/browser.ini index de84aad5510f..3ad1f94a8d64 100644 --- a/devtools/client/responsive.html/test/browser/browser.ini +++ b/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] diff --git a/devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js b/devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js new file mode 100644 index 000000000000..8834f8331f31 --- /dev/null +++ b/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); +}); diff --git a/devtools/server/main.js b/devtools/server/main.js index af3417a378ac..65e437978e2c 100644 --- a/devtools/server/main.js +++ b/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) { diff --git a/devtools/shared/transport/transport.js b/devtools/shared/transport/transport.js index c0fbb1e4c7b5..e83bf8de66e5 100644 --- a/devtools/shared/transport/transport.js +++ b/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::packet', where * 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;