bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Remove the devtools.debugger.enable-content-actors pref

VERIFIED FIXED in Firefox 30

Status

DevTools
Debugger
P3
normal
VERIFIED FIXED
5 years ago
a month ago

People

(Reporter: past, Assigned: ochameau)

Tracking

unspecified
Firefox 30
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Priority: -- → P3
(Assignee)

Comment 1

5 years ago
Created attachment 8359799 [details] [diff] [review]
patch v1

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.
(Assignee)

Updated

5 years ago
Attachment #8359799 - Flags: review?(past)
(Reporter)

Comment 2

5 years ago
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?
Attachment #8359799 - Flags: review?(past)
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.
(Reporter)

Comment 4

5 years ago
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).
(Assignee)

Updated

5 years ago
Assignee: nobody → poirot.alex
(Assignee)

Comment 8

5 years ago
Created attachment 8369587 [details] [diff] [review]
patch v2

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)
Attachment #8359799 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 8369591 [details] [diff] [review]
Move RootActor to shell.js and use whitelist for global actors

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.
Attachment #8369591 - Flags: feedback?(past)
(Assignee)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
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
Attachment #8369587 - Flags: review+
(Reporter)

Comment 12

5 years ago
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.
Attachment #8369591 - Flags: feedback?(past) → feedback+
(Assignee)

Comment 13

4 years ago
Created attachment 8384869 [details] [diff] [review]
final patch

Here is a merge of the two previous patch, with all review comments being adressed.

https://tbpl.mozilla.org/?tree=Try&rev=6083a1912d75
Attachment #8369582 - Attachment is obsolete: true
Attachment #8369587 - Attachment is obsolete: true
Attachment #8369591 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8384869 - Flags: review?(past)
(Reporter)

Comment 14

4 years ago
Comment on attachment 8384869 [details] [diff] [review]
final patch

Review of attachment 8384869 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8384869 - Flags: review?(past) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7a629e8da204
Flags: in-testsuite-
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7a629e8da204
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30

Updated

4 years ago
Status: RESOLVED → VERIFIED

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.