The script debugger is designed from the ground up to debug remote applications and mobile browsers in particular. One such case is the Boot to Gecko project (https://wiki.mozilla.org/B2G). The interesting twist in B2G is that the lines that separate chrome and content code are somewhat blurry (to me at least) and since our debugger cannot debug chrome code at the moment, it remains an open question how far we can go right now.

This patch seems to work for some use cases. The server socket port number should not be hard-coded of course, but I left that task for a future day. Another thing to investigate is if there is a better place to hook into the startup sequence than shell.js, but that seems fine for now. I made a screencast of a debugging session with it: https://vimeo.com/36966384 The main problem right now is that the debugger does not appear to receive onNewScript notifications for most scripts, which seems to be caused by bug 723563. I don't know if I can think of a workaround like in bug 697040, but getAllScripts shouldn't be too long now, anyway.

Until we get a proper UI in the debugger for connecting to a remote server, this one-liner should help testing the main patch.

(In reply to Panos Astithas [:past] from comment #1) > Created attachment 598249 [details] [diff] [review] > WIP > > This patch seems to work for some use cases. The server socket port number > should not be hard-coded of course, but I left that task for a future day. > Another thing to investigate is if there is a better place to hook into the > startup sequence than shell.js, but that seems fine for now. > > I made a screencast of a debugging session with it: > > https://vimeo.com/36966384 > > The main problem right now is that the debugger does not appear to receive > onNewScript notifications for most scripts, which seems to be caused by bug > 723563. I don't know if I can think of a workaround like in bug 697040, but > getAllScripts shouldn't be too long now, anyway. Well I did the same workaround as in bug 697040, but it's still not enough. The reason is that the root actor gets initialized once a client connects, since it needs a connection to attach its actor pools. If the client & server roles were reversed (in other words if the server initiated the connection), this would have worked.

Since bug 723563 now has a working patch, I've rebased this to work on top of it and removed the workaround.

This version is pretty much what we do in desktop and mobile Firefox, only simplified a bit further, since there are no location changes anticipated in B2G. Requesting review from Dave for the debugger bits and Vivien for the b2g bits. I'm not 100% sure who the module peers for b2g/ are, so Vivien if I need to get another review, let me know.

Comment on attachment 613283 [details] [diff] [review] Working patch >diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js > >+XPCOMUtils.defineLazyGetter(this, "DebuggerServer", function() { >+ Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); >+ return DebuggerServer; >+}); >+ nit: use single quotes to stay consistent with the rest of the file >+// Start the debugger server. >+function startDebugger() { >+ let port = Services.prefs.getIntPref("b2g.debugger.port") || 6000; >+ if (!DebuggerServer.initialized) { >+ DebuggerServer.init(); >+ DebuggerServer.addActors('chrome://browser/content/dbg-browser-actors.js'); >+ } >+ DebuggerServer.openListener(port, false); >+} >+ You can move this block of code at the end of the file, and then add the ContentStart event listener right after. Also you probably want to declare port just before your call to DebuggerServer.openListener and a line break before it. What is the second argument of openListener for? >@@ -425,16 +440,21 @@ Services.obs.addObserver(function onCons > serverSocket.asyncListen(listener); > })(); > > CustomEventManager = { > init: function custevt_init() { > window.addEventListener("ContentStart", (function(evt) { > content.addEventListener("mozContentEvent", this, false, true); > }).bind(this), false); >+ window.addEventListener("ContentStart", (function(evt) { >+ if (Services.prefs.getBoolPref("b2g.debugger.enabled")) { >+ startDebugger(); >+ } >+ }).bind(this), false); > }, As said above, move this code next to the startDebugger function and you can't remove the bind(this), it is unneeded here. I'm curious about b2g.debugger.port and b2g.debugger.enabled, is there any equivalent for Firefox desktop/mobile? It yes it could make sense to reuse them, if not, it could make sense to add them :) Not my part but I will just give some comment on the dbg-browser-actors file related to what is b2g specific (and maybe more). >+++ b/b2g/chrome/content/dbg-browser-actors.js >@@ -0,0 +1,284 @@ >+/* -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+"use strict"; nit: use single quotes to stay consistent with the rest of b2g/ >+/** >+ * B2G-specific actors. >+ */ >+ Seems obvious enough :) >+function createRootActor(aConnection) >+{ >+ return new DeviceRootActor(aConnection); >+} nit: b2g/ use bracket on the same line as the function and do not use 'a' argument prefix >+ >+/** >+ * Creates the root actor that client-server communications always start with. >+ * The root actor is responsible for the initial 'hello' packet and for >+ * responding to a 'listTabs' request that produces the list of currently open >+ * tabs. >+ * >+ * @param aConnection DebuggerServerConnection >+ * The conection to the client. >+ */ conection -> connection >+function DeviceRootActor(aConnection) >+{ nit: bracket on the same line and do not use 'connection' instead of 'aConnection' to stay consistent >+ this.conn = aConnection; >+ this._tabActors = new WeakMap(); >+ this._tabActorPool = null; >+ this._actorFactories = null; >+} There is a few calls to this.browser above, maybe you want to declare it here too? (this.browser = null;) Also this is unlikely that we change the browser window in b2g, so what about initializing this.browser here: this.browser = Services.wm.getMostRecentWindow('navigator:browser'); >+ >+DeviceRootActor.prototype = { >+ /** >+ * Return a 'hello' packet as specified by the Remote Debugging Protocol. >+ */ >+ sayHello: function DRA_sayHello() { >+ return { from: "root", >+ applicationType: "browser", >+ traits: [] }; return { 'from': 'root', 'applicationType': 'browser', traits: [] }; >+ }, >+ >+ /** >+ * Disconnects the actor from the browser window. >+ */ >+ disconnect: function DRA_disconnect() { >+ let actor = this._tabActors.get(this.browser); >+ if (actor) { >+ actor.exit(); >+ } >+ }, >+ >+ /** >+ * Handles the listTabs request. Builds a list of actors for the single >+ * tab (window) running in the process. The actors will survive >+ * until at least the next listTabs request. >+ */ >+ onListTabs: function DRA_onListTabs() { >+ let actorPool = new ActorPool(this.conn); >+ >+ this.browser = Services.wm.getMostRecentWindow("navigator:browser"); >+ >+ let actor = this._tabActors.get(this.browser); >+ if (!actor) { >+ actor = new DeviceTabActor(this.conn, this.browser); >+ actor.parentID = this.actorID; Where does this.actorID comes from? >+ this._tabActors.set(this.browser, actor); >+ } >+ actorPool.addActor(actor); nit: declare actorPool next above this call. >+ >+ // Now drop the old actorID -> actor map. Actors that still mattered were >+ // added to the new map, others will go away. >+ if (this._tabActorPool) { >+ this.conn.removeActorPool(this._tabActorPool); >+ } >+ this._tabActorPool = actorPool; >+ this.conn.addActorPool(this._tabActorPool); If the list of tabs never change, do we need to do that or can we keep the old tabActorPool and reuse it? >+ >+ return { "from": "root", >+ "selected": 0, >+ "tabs": [actor.grip()] }; return { 'from' : 'root', 'selected': 0, 'tabs': [actor.grip()] }; >+ } >+ >+} nit: }; >+ >+/** >+ * The request types this actor can handle. >+ */ >+DeviceRootActor.prototype.requestTypes = { >+ "listTabs": DeviceRootActor.prototype.onListTabs >+}; nit: single quotes >+ >+/** >+ * Creates a tab actor for handling requests to the single tab, like attaching >+ * and detaching. >+ * >+ * @param aConnection DebuggerServerConnection >+ * The conection to the client. conection -> connection >+ * @param aBrowser browser >+ * The browser instance that contains this tab. >+ */ >+function DeviceTabActor(aConnection, aBrowser) >+{ >+ this.conn = aConnection; >+ this._browser = aBrowser; >+} nit: b2g/ use bracket on the same line as the function and do not use 'a' argument prefix >+ >+DeviceTabActor.prototype = { >+ get browser() { return this._browser; }, Why do you need a getter here since you exposed directly the this._browser property? What about setting this.browser directly on DeviceTabActor and use that? >+ >+ get exited() { return !this.browser; }, >+ get attached() { return !!this._attached }, >+ >+ _tabPool: null, >+ get tabActorPool() { return this._tabPool; }, >+ >+ _contextPool: null, >+ get contextActorPool() { return this._contextPool; }, >+ I don't really like function write on the same line, I rather prefer: get exited() { return this.browser; }, otherwise if you really want it to be a one liner let's write: get exited() !!attached, >+ actorPrefix: "tab", >+ nit: single quotes >+ grip: function DTA_grip() { >+ dbg_assert(!this.exited, >+ "grip() shouldn't be called on exited browser actor."); >+ dbg_assert(this.actorID, >+ "tab should have an actorID."); >+ return { actor: this.actorID, >+ title: this.browser.contentTitle, >+ url: this.browser.document.documentURI } >+ }, return { 'actor': this.actorID, 'title': this.browser.contentTitle, 'url': this.browser.document.documentURI }; >+ >+ /** >+ * Called when the actor is removed from the connection. >+ */ >+ disconnect: function DTA_disconnect() { >+ this._detach(); >+ }, >+ >+ /** >+ * Called by the root actor when the underlying tab is closed. >+ */ >+ exit: function DTA_exit() { >+ if (this.exited) { >+ return; >+ } >+ >+ if (this.attached) { >+ this._detach(); >+ this.conn.send({ from: this.actorID, >+ type: "tabDetached" }); nit: this.conn.send({ 'from': this.actorID, 'type': 'tabDetached' }); >+ } >+ >+ this._browser = null; >+ }, >+ >+ /** >+ * Does the actual work of attching to a tab. >+ */ nit: attching -> attaching >+ _attach: function DTA_attach() { >+ if (this._attached) { >+ return; >+ } >+ >+ // Create a pool for tab-lifetime actors. >+ dbg_assert(!this._tabPool, "Shouldn't have a tab pool if we weren't attached."); nit: use single quotes >+ this._tabPool = new ActorPool(this.conn); >+ this.conn.addActorPool(this._tabPool); >+ >+ // ... and a pool for context-lifetime actors. >+ this._pushContext(); >+ >+ this._attached = true; >+ }, >+ >+ /** >+ * Creates a thread actor and a pool for context-lifetime actors. It then sets >+ * up the content window for debugging. >+ */ >+ _pushContext: function DTA_pushContext() { >+ dbg_assert(!this._contextPool, "Can't push multiple contexts"); >+ >+ this._contextPool = new ActorPool(this.conn); >+ this.conn.addActorPool(this._contextPool); >+ >+ this.threadActor = new ThreadActor(this); >+ this._addDebuggees(this.browser.content.wrappedJSObject); >+ this._contextPool.addActor(this.threadActor); >+ }, >+ >+ /** >+ * Add the provided window and all windows in its frame tree as debuggees. >+ */ >+ _addDebuggees: function BTA__addDebuggees(aWindow) { >+ this.threadActor.addDebuggee(aWindow); >+ let frames = aWindow.frames; >+ for (let i = 0; i < frames.length; i++) { >+ this._addDebuggees(frames[i]); >+ } nit: aWindow -> content Also sometimes you use BTA_ and sometimes DTA_ in the function name. >+ }, >+ >+ /** >+ * Exits the current thread actor and removes the context-lifetime actor pool. >+ * The content window is no longer being debugged after this call. >+ */ >+ _popContext: function DTA_popContext() { >+ dbg_assert(!!this._contextPool, "No context to pop."); nit: '' instead of "" >+ >+ this.conn.removeActorPool(this._contextPool); >+ this._contextPool = null; >+ this.threadActor.exit(); >+ this.threadActor = null; >+ }, >+ >+ /** >+ * Does the actual work of detaching from a tab. >+ */ >+ _detach: function DTA_detach() { >+ if (!this.attached) { >+ return; >+ } >+ >+ this._popContext(); >+ >+ // Shut down actors that belong to this tab's pool. >+ this.conn.removeActorPool(this._tabPool); >+ this._tabPool = null; >+ >+ this._attached = false; >+ }, >+ >+ // Protocol Request Handlers >+ >+ onAttach: function DTA_onAttach(aRequest) { >+ if (this.exited) { >+ return { type: "exited" }; >+ } >+ >+ this._attach(); >+ >+ return { type: "tabAttached", threadActor: this.threadActor.actorID }; >+ }, >+ >+ onDetach: function DTA_onDetach(aRequest) { >+ if (!this.attached) { >+ return { error: "wrongState" }; >+ } >+ >+ this._detach(); >+ >+ return { type: "detached" }; >+ }, >+ nit: '' instead of "" >+ /** >+ * Prepare to enter a nested event loop by disabling debuggee events. >+ */ >+ preNest: function DTA_preNest() { >+ let windowUtils = this.browser.content >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindowUtils); >+ windowUtils.suppressEventHandling(true); >+ windowUtils.suspendTimeouts(); >+ }, >+ >+ /** >+ * Prepare to exit a nested event loop by enabling debuggee events. >+ */ >+ postNest: function DTA_postNest(aNestData) { >+ let windowUtils = this.browser.content >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindowUtils); >+ windowUtils.resumeTimeouts(); >+ windowUtils.suppressEventHandling(false); >+ } >+ >+}; >+ >+/** >+ * The request types this actor can handle. >+ */ >+DeviceTabActor.prototype.requestTypes = { >+ "attach": DeviceTabActor.prototype.onAttach, >+ "detach": DeviceTabActor.prototype.onDetach >+}; nit: '' instead of ""

Whoah, I just finished converting all single quotes to double quotes. You guys are strict! (In reply to Vivien Nicolas (:vingtetun) from comment #6) > What is the second argument of openListener for? It's for specifying whether to listen on the loopback device or not. See: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-server.jsm#145 > I'm curious about b2g.debugger.port and b2g.debugger.enabled, is there any > equivalent for Firefox desktop/mobile? It yes it could make sense to reuse > them, if not, it could make sense to add them :) In mobile Firefox there are remote-debugger.enabled and remote-debugger.port. In desktop Firefox there is only devtools.debugger.enabled. The latter is actually in toolkit, so it could be readily reused. I have no preference either way. > >+ }, > >+ > >+ /** > >+ * Disconnects the actor from the browser window. > >+ */ > >+ disconnect: function DRA_disconnect() { > >+ let actor = this._tabActors.get(this.browser); > >+ if (actor) { > >+ actor.exit(); > >+ } > >+ }, > >+ > >+ /** > >+ * Handles the listTabs request. Builds a list of actors for the single > >+ * tab (window) running in the process. The actors will survive > >+ * until at least the next listTabs request. > >+ */ > >+ onListTabs: function DRA_onListTabs() { > >+ let actorPool = new ActorPool(this.conn); > >+ > >+ this.browser = Services.wm.getMostRecentWindow("navigator:browser"); > >+ > >+ let actor = this._tabActors.get(this.browser); > >+ if (!actor) { > >+ actor = new DeviceTabActor(this.conn, this.browser); > >+ actor.parentID = this.actorID; > > Where does this.actorID comes from? It is set on an actor the moment it is added to an actor pool: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/server/dbg-server.jsm#292 > >+ > >+ // Now drop the old actorID -> actor map. Actors that still mattered were > >+ // added to the new map, others will go away. > >+ if (this._tabActorPool) { > >+ this.conn.removeActorPool(this._tabActorPool); > >+ } > >+ this._tabActorPool = actorPool; > >+ this.conn.addActorPool(this._tabActorPool); > > If the list of tabs never change, do we need to do that or can we keep the > old tabActorPool and reuse it? We could drop many more things from this file, if the goal was to get a minimal implementation. Right now, my goal is to have a working implementation. Mid-term, I plan on refactoring the desktop, fennec and b2g browser actors to decrease code duplication and be more oo-friendly. Also, there are plans to refactor the server-side code to be more close to the low-level API, as well as plans to change the protocol (and thereby the actors) a bit to better accommodate chrome debugging. I would prefer to limit changes to this file to the absolute minimum, in order to help with these plans, if you don't mind. > >+ > >+DeviceTabActor.prototype = { > >+ get browser() { return this._browser; }, > > Why do you need a getter here since you exposed directly the this._browser > property? What about setting this.browser directly on DeviceTabActor and use > that? It's a minor thing, but this.browser is used from the script actors and this is how all other browser actors are structured. So I'd rather leave it as it is for now, for the reasons explained above. All other nits addressed!

(In reply to Panos Astithas [:past] from comment #7) > > I'm curious about b2g.debugger.port and b2g.debugger.enabled, is there any > > equivalent for Firefox desktop/mobile? It yes it could make sense to reuse > > them, if not, it could make sense to add them :) > > In mobile Firefox there are remote-debugger.enabled and > remote-debugger.port. In desktop Firefox there is only > devtools.debugger.enabled. The latter is actually in toolkit, so it could be > readily reused. I have no preference either way. devtools.debugger.port and devtools.debugger.enabled seems good to me. I think Firefox Mobile should use the same. > > >+ let actor = this._tabActors.get(this.browser); > > >+ if (!actor) { > > >+ actor = new DeviceTabActor(this.conn, this.browser); > > >+ actor.parentID = this.actorID; > > > > Where does this.actorID comes from? > > It is set on an actor the moment it is added to an actor pool: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/ > server/dbg-server.jsm#292 > Let's add a comment somewhere (near createRootActor?), it is very unclear that createRootActor() is called magically by the include .jsm that also set some public property of the resulting actor.

(In reply to Vivien Nicolas (:vingtetun) from comment #8) > (In reply to Panos Astithas [:past] from comment #7) > > > I'm curious about b2g.debugger.port and b2g.debugger.enabled, is there any > > > equivalent for Firefox desktop/mobile? It yes it could make sense to reuse > > > them, if not, it could make sense to add them :) > > > > In mobile Firefox there are remote-debugger.enabled and > > remote-debugger.port. In desktop Firefox there is only > > devtools.debugger.enabled. The latter is actually in toolkit, so it could be > > readily reused. I have no preference either way. > > devtools.debugger.port and devtools.debugger.enabled seems good to me. I > think Firefox Mobile should use the same. Done. Note that in order to have a working debugger the following preferences need to be set: user_pref("devtools.debugger.enabled", true); user_pref("devtools.debugger.port", 60000); user_pref("marionette.defaultPrefs.enabled", false); I'm not sure if you want to mention them in the wiki, or change the gaia Makefile to create them automatically. > > > >+ let actor = this._tabActors.get(this.browser); > > > >+ if (!actor) { > > > >+ actor = new DeviceTabActor(this.conn, this.browser); > > > >+ actor.parentID = this.actorID; > > > > > > Where does this.actorID comes from? > > > > It is set on an actor the moment it is added to an actor pool: > > > > http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/debugger/ > > server/dbg-server.jsm#292 > > > > Let's add a comment somewhere (near createRootActor?), it is very unclear > that createRootActor() is called magically by the include .jsm that also set > some public property of the resulting actor. I added two comments, one in createRootActor and the other right before this.actorID.

(In reply to Panos Astithas [:past] from comment #9) > Created attachment 613968 [details] [diff] [review] > Working patch v3 > > (In reply to Vivien Nicolas (:vingtetun) from comment #8) > > (In reply to Panos Astithas [:past] from comment #7) > > > > I'm curious about b2g.debugger.port and b2g.debugger.enabled, is there any > > > > equivalent for Firefox desktop/mobile? It yes it could make sense to reuse > > > > them, if not, it could make sense to add them :) > > > > > > In mobile Firefox there are remote-debugger.enabled and > > > remote-debugger.port. In desktop Firefox there is only > > > devtools.debugger.enabled. The latter is actually in toolkit, so it could be > > > readily reused. I have no preference either way. > > > > devtools.debugger.port and devtools.debugger.enabled seems good to me. I > > think Firefox Mobile should use the same. > > Done. Note that in order to have a working debugger the following > preferences need to be set: > > user_pref("devtools.debugger.enabled", true); > user_pref("devtools.debugger.port", 60000); > user_pref("marionette.defaultPrefs.enabled", false); > The wiki seems a good place to start. We can also add an option in the makefile later.