When support for child process debugging landed in bug 817580, the devtools.debugger.enable-content-actors pref was introduced to disable child process debugging on devices, as the risk from a rogue TCP connection to a public IP was deemed too high. The decision was to enable child process debugging on devices only when remote protocol connections would take place over a UNIX domain socket (bug 817580 comment 51). This has been implemented for quite some time now in bug 832000, so I think it's time we removed the old pref handling code from b2g. CCing Jonas and Paul to make sure there are no disagreements before we do this.

I take this bug as an opportunity to clean things about actors on b2g: - remove enable-content-actors pref, - remove useless ContentTabActor in b2g parent process as we do not expose browser tab actor on b2g yet. It will most likely be very different to implement as tabs are OOP, - stop loading webbrowser.js on b2g parent process as we do not use BrowserTabActor there, - tweaks comments. (if you see something else that is unclear, let's continue clarifying things) Part of These cleanups highlight what the RootActor actually does on b2g. I was tempted to put, in shell.js or dbg-browser-actors.createRootActor, an explicit filter of DebuggerServer.globalActorFactories, something like DebuggerServer.globalActorFactories = { webappsActor: DebuggerServer.WebappsActor, deviceActor: DebuggerServer.DeviceActor }; That clearly goes against the automatic registering of actors, but would make it clear and obvious what is actually exposed. After some thoughts, the comment in shell.js may be enough.

Comment on attachment 8359799 [details] [diff] [review] patch v1 Review of attachment 8359799 [details] [diff] [review]: ----------------------------------------------------------------- There are still remnants of the pref in modules/libpref/src/init/all.js and in tests (toolkit/devtools/apps/tests/unit/head_apps.js and toolkit/devtools/apps/tests/unit/tail_apps.js). I'm also not fond of duplicating hasNativeConsoleAPI, how about doing it teh other way around and making the tab actor use the console actor implementation? I can't think of a scenario where we want to have the tab actor loaded, but not the console actor. Can you think of any? I haven't tried the patch yet, but can we still connect to the main b2g process via the Connect menu item and get a working console (with certified app debugging enabled of course)? ::: b2g/chrome/content/dbg-browser-actors.js @@ +14,5 @@ > > /** > * Construct a root actor appropriate for use in a server running in B2G. The > * returned root actor: > * - respects the factories registered with DebuggerServer.addGlobalActor, Nit: a bullet list with a single item looks funny. Better consolidate the bullet in the sentence above: "The returned root actor respects..." @@ +23,5 @@ > function createRootActor(connection) > { > let parameters = { > + // We do not exposed browser tab actors yet, > + // But we still have to define tabList.getList(), Nits: s/exposed/expose/, s/But/but/ ::: b2g/chrome/content/shell.js @@ +1072,5 @@ > DebuggerServer.init(this.prompt.bind(this)); > DebuggerServer.chromeWindowType = "navigator:browser"; > + > + // /!\ Be carufull when adding new actor, especially global actor. > + // any new global actor will be exposed and returned by the root actor. Nit: a few typos above. ::: toolkit/devtools/server/main.js @@ +394,1 @@ > if (!("ContentAppActor" in DebuggerServer)) { s/DebuggerServer/this/ for consistency?

My understanding here is pretty limited. But if we can verify that the connection is always coming from a different machine and not from the device itself, then I think we are good here. But I wasn't part of the original discussions to disable connecting to child processes.

Unix domain socket connections by definition come from the same machine, but since there is no web-exposed API to connect to a Unix domain socket, we can be sure that any such connections will come from adb forwarding (or other native or chrome-privileged code, of which we don't have any).

Do we only allow attaching a debugger through adb? I.e. there is no way to use TCP sockets to connect to a device?

(In reply to Jonas Sicking (:sicking) from comment #5) > Do we only allow attaching a debugger through adb? I.e. there is no way to > use TCP sockets to connect to a device? You are correct, we don't currently listen on TCP sockets. If someone set the pref "devtools.debugger.unix-domain-socket" to a number, we'd listen on that TCP port, but that is not possible to do on a production build (as I understand it).

I tried to address all your comments. You can find an interdiff in the previous attachment. (it miss the toolkit/devtools/server/main.js modification that I included in this patch)

Also I would like to have your feedback on such move. I'm still stressed with the way actor are registered. I'd be more confident if we have an explicit whitelist somewhere of what actors we are exposing.

And I forgot to mention that these patches doesn't break the ability to debug shell.html via connect menu. The code I removed from dbg-browser-actor.js was a big piece of dead code.

Comment on attachment 8369587 [details] [diff] [review] patch v2 Review of attachment 8369587 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/apps/tests/unit/head_apps.js @@ -83,5 @@ > // as only launchable apps are returned > Components.utils.import('resource://gre/modules/Webapps.jsm'); > DOMApplicationRegistry.allAppsLaunchable = true; > - > - originalPrefValue = Services.prefs.getBoolPref("devtools.debugger.enable-content-actors"); There is also the declaration of originalPrefValue earlier in the file that needs to go. ::: toolkit/devtools/server/actors/webbrowser.js @@ +920,5 @@ > * @return boolean > * True if the window.console object is native, or false otherwise. > */ > hasNativeConsoleAPI: function BTA_hasNativeConsoleAPI(aWindow) { > + // Do not expose WebConsoleActor function directy as it is always Typo: directly

Comment on attachment 8369591 [details] [diff] [review] Move RootActor to shell.js and use whitelist for global actors Review of attachment 8369591 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I'm OK with this change. ::: b2g/chrome/content/shell.js @@ +1129,5 @@ > + let root = new DebuggerServer.RootActor(connection, parameters); > + root.applicationType = "operating-system"; > + return root; > + } > + DebuggerServer.createRootActor = createRootActor; Nit: might want to assign directly using a function expression, but that's just my preferred style.

Here is a merge of the two previous patch, with all review comments being adressed. https://tbpl.mozilla.org/?tree=Try&rev=6083a1912d75

Comment on attachment 8384869 [details] [diff] [review] final patch Review of attachment 8384869 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.