From 40e37fad19f57ba9f77eb272d60fd0239f8c137b Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Sat, 18 Apr 2015 09:39:07 +0200 Subject: [PATCH] Bug 1145049 - Prevent caching tab actors in child processes. r=jryans --- toolkit/devtools/server/actors/childtab.js | 35 ++---- .../server/actors/director-registry.js | 4 +- toolkit/devtools/server/actors/webapps.js | 27 ++-- toolkit/devtools/server/actors/webbrowser.js | 47 ++++--- toolkit/devtools/server/child.js | 17 +-- .../server/docs/lazy-actor-modules.md | 10 +- toolkit/devtools/server/main.js | 119 +++++++++--------- 7 files changed, 126 insertions(+), 133 deletions(-) diff --git a/toolkit/devtools/server/actors/childtab.js b/toolkit/devtools/server/actors/childtab.js index f50eea78726..cd8a6f689cc 100644 --- a/toolkit/devtools/server/actors/childtab.js +++ b/toolkit/devtools/server/actors/childtab.js @@ -21,10 +21,14 @@ let { TabActor } = require("devtools/server/actors/webbrowser"); * The conection to the client. * @param chromeGlobal * The content script global holding |content| and |docShell| properties for a tab. + * @param prefix + * the prefix used in protocol to create IDs for each actor. + * Used as ID identifying this particular TabActor from the parent process. */ -function ContentActor(connection, chromeGlobal) +function ContentActor(connection, chromeGlobal, prefix) { this._chromeGlobal = chromeGlobal; + this._prefix = prefix; TabActor.call(this, connection, chromeGlobal); this.traits.reconfigure = false; this._sendForm = this._sendForm.bind(this); @@ -49,32 +53,11 @@ Object.defineProperty(ContentActor.prototype, "title", { }); ContentActor.prototype.exit = function() { - this._chromeGlobal.removeMessageListener("debug:form", this._sendForm); - this._sendForm = null; - TabActor.prototype.exit.call(this); -}; - -// Override form just to rename this._tabActorPool to this._tabActorPool2 -// in order to prevent it to be cleaned on detach. -// We have to keep tab actors alive as we keep the ContentActor -// alive after detach and reuse it for multiple debug sessions. -ContentActor.prototype.form = function () { - let response = { - "actor": this.actorID, - "title": this.title, - "url": this.url - }; - - // Walk over tab actors added by extensions and add them to a new ActorPool. - let actorPool = new ActorPool(this.conn); - this._createExtraActors(DebuggerServer.tabActorFactories, actorPool); - if (!actorPool.isEmpty()) { - this._tabActorPool2 = actorPool; - this.conn.addActorPool(this._tabActorPool2); + if (this._sendForm) { + this._chromeGlobal.removeMessageListener("debug:form", this._sendForm); + this._sendForm = null; } - - this._appendExtraActors(response); - return response; + return TabActor.prototype.exit.call(this); }; /** diff --git a/toolkit/devtools/server/actors/director-registry.js b/toolkit/devtools/server/actors/director-registry.js index 51cf1accea3..84b3cb38bf8 100644 --- a/toolkit/devtools/server/actors/director-registry.js +++ b/toolkit/devtools/server/actors/director-registry.js @@ -111,7 +111,7 @@ const DirectorRegistry = exports.DirectorRegistry = { let gTrackedMessageManager = new Set(); -exports.setupParentProcess = function setupParentProcess({mm, childID}) { +exports.setupParentProcess = function setupParentProcess({mm, prefix}) { // prevents multiple subscriptions on the same messagemanager if (gTrackedMessageManager.has(mm)) { return; @@ -121,7 +121,7 @@ exports.setupParentProcess = function setupParentProcess({mm, childID}) { // listen for director-script requests from the child process mm.addMessageListener("debug:director-registry-request", handleChildRequest); - DebuggerServer.once("disconnected-from-child:" + childID, handleMessageManagerDisconnected); + DebuggerServer.once("disconnected-from-child:" + prefix, handleMessageManagerDisconnected); /* parent process helpers */ diff --git a/toolkit/devtools/server/actors/webapps.js b/toolkit/devtools/server/actors/webapps.js index c14daf6c126..9c394b82b10 100644 --- a/toolkit/devtools/server/actors/webapps.js +++ b/toolkit/devtools/server/actors/webapps.js @@ -213,9 +213,9 @@ function WebappsActor(aConnection) { Cu.import("resource://gre/modules/AppsUtils.jsm"); Cu.import("resource://gre/modules/FileUtils.jsm"); - // Keep reference of already created app actors. - // key: app frame message manager, value: ContentActor's grip() value - this._appActorsMap = new Map(); + // Keep reference of already connected app processes. + // values: app frame message manager + this._connectedApps = new Set(); this.conn = aConnection; this._uploads = []; @@ -960,24 +960,33 @@ WebappsActor.prototype = { // Only create a new actor, if we haven't already // instanciated one for this connection. - let map = this._appActorsMap; + let set = this._connectedApps; let mm = appFrame.QueryInterface(Ci.nsIFrameLoaderOwner) .frameLoader .messageManager; - let actor = map.get(mm); - if (!actor) { + if (!set.has(mm)) { let onConnect = actor => { - map.set(mm, actor); + set.add(mm); return { actor: actor }; }; let onDisconnect = mm => { - map.delete(mm); + set.delete(mm); }; return DebuggerServer.connectToChild(this.conn, appFrame, onDisconnect) .then(onConnect); } - return { actor: actor }; + // We have to update the form as it may have changed + // if we detached the TabActor + let deferred = promise.defer(); + let onFormUpdate = msg => { + mm.removeMessageListener("debug:form", onFormUpdate); + deferred.resolve({ actor: msg.json }); + }; + mm.addMessageListener("debug:form", onFormUpdate); + mm.sendAsyncMessage("debug:form"); + + return deferred.promise; }, watchApps: function () { diff --git a/toolkit/devtools/server/actors/webbrowser.js b/toolkit/devtools/server/actors/webbrowser.js index 6d1b76a5418..8b3adc97873 100644 --- a/toolkit/devtools/server/actors/webbrowser.js +++ b/toolkit/devtools/server/actors/webbrowser.js @@ -876,10 +876,7 @@ TabActor.prototype = { * Called when the actor is removed from the connection. */ disconnect: function BTA_disconnect() { - this._detach(); - this._extraActors = null; - this._styleSheetActors.clear(); - this._exited = true; + this.exit(); }, /** @@ -901,6 +898,14 @@ TabActor.prototype = { type: "tabDetached" }); } + Object.defineProperty(this, "docShell", { + value: null, + configurable: true + }); + + this._extraActors = null; + this._styleSheetActors.clear(); + this._exited = true; }, @@ -1221,11 +1226,6 @@ TabActor.prototype = { this._tabActorPool = null; } - Object.defineProperty(this, "docShell", { - value: null, - configurable: true - }); - this._attached = false; return true; }, @@ -1822,7 +1822,10 @@ function RemoteBrowserTabActor(aConnection, aBrowser) RemoteBrowserTabActor.prototype = { connect: function() { - let connect = DebuggerServer.connectToChild(this._conn, this._browser); + let onDestroy = () => { + this._form = null; + }; + let connect = DebuggerServer.connectToChild(this._conn, this._browser, onDestroy); return connect.then(form => { this._form = form; return this; @@ -1835,15 +1838,21 @@ RemoteBrowserTabActor.prototype = { }, update: function() { - let deferred = promise.defer(); - let onFormUpdate = msg => { - this._mm.removeMessageListener("debug:form", onFormUpdate); - this._form = msg.json; - deferred.resolve(this); - }; - this._mm.addMessageListener("debug:form", onFormUpdate); - this._mm.sendAsyncMessage("debug:form"); - return deferred.promise; + // If the child happens to be crashed/close/detach, it won't have _form set, + // so only request form update if some code is still listening on the other side. + if (this._form) { + let deferred = promise.defer(); + let onFormUpdate = msg => { + this._mm.removeMessageListener("debug:form", onFormUpdate); + this._form = msg.json; + deferred.resolve(this); + }; + this._mm.addMessageListener("debug:form", onFormUpdate); + this._mm.sendAsyncMessage("debug:form"); + return deferred.promise; + } else { + return this.connect(); + } }, form: function() { diff --git a/toolkit/devtools/server/child.js b/toolkit/devtools/server/child.js index 2d350168d4c..88cce054c06 100644 --- a/toolkit/devtools/server/child.js +++ b/toolkit/devtools/server/child.js @@ -17,10 +17,6 @@ let chromeGlobal = this; const { dumpn } = DevToolsUtils; const { DebuggerServer, ActorPool } = Cu.import("resource://gre/modules/devtools/dbg-server.jsm", {}); - if (!DebuggerServer.childID) { - DebuggerServer.childID = 1; - } - if (!DebuggerServer.initialized) { DebuggerServer.init(); @@ -44,17 +40,16 @@ let chromeGlobal = this; let mm = msg.target; let prefix = msg.data.prefix; - let id = DebuggerServer.childID++; let conn = DebuggerServer.connectToParent(prefix, mm); - connections.set(id, conn); + connections.set(prefix, conn); - let actor = new DebuggerServer.ContentActor(conn, chromeGlobal); + let actor = new DebuggerServer.ContentActor(conn, chromeGlobal, prefix); let actorPool = new ActorPool(conn); actorPool.addActor(actor); conn.addActorPool(actorPool); - sendAsyncMessage("debug:actor", {actor: actor.form(), childID: id}); + sendAsyncMessage("debug:actor", {actor: actor.form(), prefix: prefix}); }); addMessageListener("debug:connect", onConnect); @@ -95,11 +90,11 @@ let chromeGlobal = this; // Call DebuggerServerConnection.close to destroy all child actors // (It should end up calling DebuggerServerConnection.onClosed // that would actually cleanup all actor pools) - let childID = msg.data.childID; - let conn = connections.get(childID); + let prefix = msg.data.prefix; + let conn = connections.get(prefix); if (conn) { conn.close(); - connections.delete(childID); + connections.delete(prefix); } }); addMessageListener("debug:disconnect", onDisconnect); diff --git a/toolkit/devtools/server/docs/lazy-actor-modules.md b/toolkit/devtools/server/docs/lazy-actor-modules.md index e8d8410a60b..52fbd75e717 100644 --- a/toolkit/devtools/server/docs/lazy-actor-modules.md +++ b/toolkit/devtools/server/docs/lazy-actor-modules.md @@ -76,7 +76,7 @@ connected to the child process as parameter, e.g. in the **director-registry**: let gTrackedMessageManager = new Set(); -exports.setupParentProcess = function setupParentProcess({ mm, childID }) { +exports.setupParentProcess = function setupParentProcess({ mm, prefix }) { if (gTrackedMessageManager.has(mm)) { return; } gTrackedMessageManager.add(mm); @@ -84,7 +84,7 @@ exports.setupParentProcess = function setupParentProcess({ mm, childID }) { mm.addMessageListener("debug:director-registry-request", handleChildRequest); // time to unsubscribe from the disconnected message manager - DebuggerServer.once("disconnected-from-child:" + childID, handleMessageManagerDisconnected); + DebuggerServer.once("disconnected-from-child:" + prefix, handleMessageManagerDisconnected); function handleMessageManagerDisconnected(evt, { mm: disconnected_mm }) { ... @@ -109,8 +109,8 @@ In the child process: In the parent process: - The DebuggerServer receives the DebuggerServer.setupInParent request - it tries to load the required module -- it tries to call the **mod[setupParent]** method with the frame message manager and the childID - in the json parameter **{ mm, childID }** +- it tries to call the **mod[setupParent]** method with the frame message manager and the prefix + in the json parameter **{ mm, prefix }** - the module setupParent helper use the mm to subscribe the messagemanager events - the module setupParent helper use the DebuggerServer object to subscribe *once* the - **"disconnected-from-child:CHILDID"** event (needed to unsubscribe the messagemanager events) + **"disconnected-from-child:PREFIX"** event (needed to unsubscribe the messagemanager events) diff --git a/toolkit/devtools/server/main.js b/toolkit/devtools/server/main.js index 609d2b56665..7ed006ee26a 100644 --- a/toolkit/devtools/server/main.js +++ b/toolkit/devtools/server/main.js @@ -809,13 +809,15 @@ var DebuggerServer = { * The debugger server connection to use. * @param nsIDOMElement aFrame * The browser element that holds the child process. - * @param function [aOnDisconnect] - * Optional function to invoke when the child is disconnected. + * @param function [aOnDestroy] + * Optional function to invoke when the child process closes + * or the connection shuts down. (Need to forget about the + * related TabActor) * @return object * A promise object that is resolved once the connection is * established. */ - connectToChild: function(aConnection, aFrame, aOnDisconnect) { + connectToChild: function(aConnection, aFrame, aOnDestroy) { let deferred = defer(); let mm = aFrame.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader @@ -824,7 +826,6 @@ var DebuggerServer = { let actor, childTransport; let prefix = aConnection.allocID("child"); - let childID = null; let netMonitor = null; // provides hook to actor modules that need to exchange messages @@ -841,7 +842,7 @@ var DebuggerServer = { return false; } - m[setupParent]({ mm: mm, childID: childID }); + m[setupParent]({ mm: mm, prefix: prefix }); return true; } catch(e) { @@ -855,10 +856,11 @@ var DebuggerServer = { mm.addMessageListener("debug:setup-in-parent", onSetupInParent); let onActorCreated = DevToolsUtils.makeInfallible(function (msg) { + if (msg.json.prefix != prefix) { + return; + } mm.removeMessageListener("debug:actor", onActorCreated); - childID = msg.json.childID; - // Pipe Debugger message from/to parent/child via the message manager childTransport = new ChildDebuggerTransport(mm, prefix); childTransport.hooks = { @@ -882,70 +884,65 @@ var DebuggerServer = { }).bind(this); mm.addMessageListener("debug:actor", onActorCreated); - let onMessageManagerClose = DevToolsUtils.makeInfallible(function (subject, topic, data) { - if (subject == mm) { - Services.obs.removeObserver(onMessageManagerClose, topic); + let destroy = DevToolsUtils.makeInfallible(function () { + // provides hook to actor modules that need to exchange messages + // between e10s parent and child processes + DebuggerServer.emit("disconnected-from-child:" + prefix, { mm: mm, prefix: prefix }); - // provides hook to actor modules that need to exchange messages - // between e10s parent and child processes - this.emit("disconnected-from-child:" + childID, { mm: mm, childID: childID }); - - mm.removeMessageListener("debug:setup-in-parent", onSetupInParent); - - if (childTransport) { - // If we have a child transport, the actor has already - // been created. We need to stop using this message manager. - childTransport.close(); - childTransport = null; - aConnection.cancelForwarding(prefix); - - // ... and notify the child process to clean the tab actors. - mm.sendAsyncMessage("debug:disconnect", { childID: childID }); - } else { - // Otherwise, the app has been closed before the actor - // had a chance to be created, so we are not able to create - // the actor. - deferred.resolve(null); - } - if (actor) { - // The ContentActor within the child process doesn't necessary - // have to time to uninitialize itself when the app is closed/killed. - // So ensure telling the client that the related actor is detached. - aConnection.send({ from: actor.actor, type: "tabDetached" }); - actor = null; - } - - if (netMonitor) { - netMonitor.destroy(); - netMonitor = null; - } - - if (aOnDisconnect) { - aOnDisconnect(mm); - } - } - }).bind(this); - Services.obs.addObserver(onMessageManagerClose, - "message-manager-close", false); - - events.once(aConnection, "closed", () => { if (childTransport) { - // When the client disconnects, we have to unplug the dedicated - // ChildDebuggerTransport... + // If we have a child transport, the actor has already + // been created. We need to stop using this message manager. childTransport.close(); childTransport = null; aConnection.cancelForwarding(prefix); // ... and notify the child process to clean the tab actors. - mm.sendAsyncMessage("debug:disconnect", { childID: childID }); - - if (netMonitor) { - netMonitor.destroy(); - netMonitor = null; - } + mm.sendAsyncMessage("debug:disconnect", { prefix: prefix }); + } else { + // Otherwise, the app has been closed before the actor + // had a chance to be created, so we are not able to create + // the actor. + deferred.resolve(null); } + if (actor) { + // The ContentActor within the child process doesn't necessary + // have time to uninitialize itself when the app is closed/killed. + // So ensure telling the client that the related actor is detached. + aConnection.send({ from: actor.actor, type: "tabDetached" }); + actor = null; + } + + if (netMonitor) { + netMonitor.destroy(); + netMonitor = null; + } + + if (aOnDestroy) { + aOnDestroy(mm); + } + + // Cleanup all listeners + Services.obs.removeObserver(onMessageManagerClose, "message-manager-close"); + mm.removeMessageListener("debug:setup-in-parent", onSetupInParent); + if (!actor) { + mm.removeMessageListener("debug:actor", onActorCreated); + } + events.off(aConnection, "closed", destroy); }); + // Listen for app process exit + let onMessageManagerClose = function (subject, topic, data) { + if (subject == mm) { + destroy(); + } + }; + Services.obs.addObserver(onMessageManagerClose, + "message-manager-close", false); + + // Listen for connection close to cleanup things + // when user unplug the device or we lose the connection somehow. + events.on(aConnection, "closed", destroy); + mm.sendAsyncMessage("debug:connect", { prefix: prefix }); return deferred.promise;