Last Comment Bug 881235 - Add actors and initial UI for remote debugging
: Add actors and initial UI for remote debugging
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 24.0
Assigned To: Philipp Kewisch [:Fallen]
:
Mentors:
http://kewisch.wordpress.com/2013/06/...
Depends on:
Blocks: tb-debugger
  Show dependency treegraph
 
Reported: 2013-06-10 06:15 PDT by Philipp Kewisch [:Fallen]
Modified: 2013-06-25 05:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix - v1 (9.97 KB, patch)
2013-06-10 06:15 PDT, Philipp Kewisch [:Fallen]
mconley: feedback+
Details | Diff | Review

Description Philipp Kewisch [:Fallen] 2013-06-10 06:15:57 PDT
Created attachment 760361 [details] [diff] [review]
Fix - v1

We want to allow remote connections from the devtools debugger. This bug will take care of some simple UI to allow the connection and the actors needed for the debugger.

It turns out this is much easier than I though, and since the devtools team has taken care of bug 870081 this is also a lot less code than it was before.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2013-06-11 18:28:21 PDT
Comment on attachment 760361 [details] [diff] [review]
Fix - v1

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

This is remarkably straight-forward. Thanks Philipp! Just a few suggestions.

::: mail/components/debugger/content/dbg-mail-actors.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Throughout this file, please use 2 space indentation.

@@ +4,5 @@
> +
> +Components.utils.import("resource://gre/modules/iteratorUtils.jsm");
> +
> +// This is needed until bug 880930 is fixed.
> +var windowMediator = Components.classes["@mozilla.org/appshell/window-mediator;1"]

Is it possible to use Services.wm instead?

@@ +54,5 @@
> +
> +  iterator: function() {
> +    // For now we just need to make sure this function is a generator. Actually
> +    // returning a list of tabs will be done in a future bug.
> +    for (let dummy in []) {

nit - this is fine for now, but for...in on Arrays doesn't work. for...of is the new sexy. :)

::: mail/components/debugger/content/dbg-messenger-overlay.js
@@ +7,5 @@
> +/**
> + * Start the devtools debugger server and open a listener to contact it.
> + */
> +function startDebugger() {
> +    Components.utils.reportError("starting debugger")

I do believe we have a console logging service now that we can use instead - Console.jsm: https://mxr.mozilla.org/mozilla-central/search?string=Console.jsm

I like logging, but I do believe Console.jsm is preferable to reportError.

@@ +10,5 @@
> +function startDebugger() {
> +    Components.utils.reportError("starting debugger")
> +    if (!DebuggerServer.initialized) {
> +        // Initialize the debugger, set the accept callback to always allow.
> +        DebuggerServer.init(function() true);

Always allow? So we don't ask the user if they'd like a remote connection?

@@ +27,5 @@
> +    let port = Services.prefs.getIntPref('devtools.debugger.remote-port') || 6000;
> +    try {
> +        DebuggerServer.openListener(port);
> +    } catch (e) {
> +        dump('Unable to start debugger server: ' + e + '\n');

console.error would be good here, instead of a dump. I even think Console.jsm has some facilities for nice printing of exceptions.

@@ +38,5 @@
> +function stopDebugger() {
> +    try {
> +        DebuggerServer.closeListener(true);
> +    } catch (e) {
> +        dump('Unable to stop debugger server: ' + e + '\n');

Console.jsm instead of dump please

::: mail/components/debugger/jar.mn
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +messenger.jar:
> +% overlay chrome://messenger/content/mailWindowOverlay.xul chrome://messenger/content/debugger/dbg-messenger-overlay.xul
> +    content/messenger/debugger/dbg-messenger-overlay.xul   (content/dbg-messenger-overlay.xul)

nit - please line these up with 2 space indentation.
Comment 2 Philipp Kewisch [:Fallen] 2013-06-12 02:34:02 PDT
(In reply to Mike Conley (:mconley) from comment #1)
> Throughout this file, please use 2 space indentation.
Ok, sure thing!

> 
> @@ +4,5 @@
> > +
> > +Components.utils.import("resource://gre/modules/iteratorUtils.jsm");
> > +
> > +// This is needed until bug 880930 is fixed.
> > +var windowMediator = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> 
> Is it possible to use Services.wm instead?
I'm not entirely sure, I'll give it a try. The root actor also has a global windowMediator they are trying to get rid of, I could give it a try with Services.wm.


> > +    for (let dummy in []) {
> nit - this is fine for now, but for...in on Arrays doesn't work. for...of is
> the new sexy. :)
This will go very soon (i.e by next week), therefore I'd suggest to just leave it there. I have a patch in the works for tabs support that actually fills the iterator with real content - it uses for...of :)


> > +function startDebugger() {
> > +    Components.utils.reportError("starting debugger")
> 
> I do believe we have a console logging service now that we can use instead -
> Console.jsm:
> https://mxr.mozilla.org/mozilla-central/search?string=Console.jsm
> 
> I like logging, but I do believe Console.jsm is preferable to reportError.
This is actually extra debugging I missed to remove. I personally don't think its needed.

> 
> @@ +10,5 @@
> > +function startDebugger() {
> > +    Components.utils.reportError("starting debugger")
> > +    if (!DebuggerServer.initialized) {
> > +        // Initialize the debugger, set the accept callback to always allow.
> > +        DebuggerServer.init(function() true);
> 
> Always allow? So we don't ask the user if they'd like a remote connection?
By default, devtools.debugger.force-local is set to true, so only connections from localhost are allowed. Then there is the added safety that if the user has the "allow remote debugging" option unchecked in the tools menu, there will be no connections. Therefore I think its fine to always allow, otherwise you will end up having to accept the connection each time.

What I could do is check for devtools.debugger.force-local and if its false, then have the default prompt show up.


> 
> @@ +27,5 @@
> > +    let port = Services.prefs.getIntPref('devtools.debugger.remote-port') || 6000;
> > +    try {
> > +        DebuggerServer.openListener(port);
> > +    } catch (e) {
> > +        dump('Unable to start debugger server: ' + e + '\n');
> 
> console.error would be good here, instead of a dump. I even think
> Console.jsm has some facilities for nice printing of exceptions.
Ok, will fix here and elsewhere.

> > +% overlay chrome://messenger/content/mailWindowOverlay.xul chrome://messenger/content/debugger/dbg-messenger-overlay.xul
> > +    content/messenger/debugger/dbg-messenger-overlay.xul   (content/dbg-messenger-overlay.xul)
> 
> nit - please line these up with 2 space indentation.

Oops, yes, will fix.
Comment 3 Philipp Kewisch [:Fallen] 2013-06-12 08:35:25 PDT
Pushed to comm-central changeset 188ea3049daf
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2013-06-12 09:00:53 PDT
Philipp's comm-central changeset 188ea3049daf was r=me over IRC.
Comment 5 rsx11m 2013-06-13 05:37:21 PDT
Philip, Mike: I saw this landing but can't quite figure out what this is about. There is https://wiki.mozilla.org/Remote_Debugging_Protocol with a technical description, and I read through the meta bug, but I'm still puzzled if remote is really remote (in the sense of someone logging into your machine in order to debug your installation, like external assistance) of if it's meant in the sense of remote from another process (but still on the same machine).

A pointer to a more conceptual than technical description would be helpful so that the scope and possible implications of this effort are clear. Thanks!
Comment 6 Philipp Kewisch [:Fallen] 2013-06-13 05:50:32 PDT
Sure thing, I will put up a blog post on this very soon and link it here. The short version is, this will allow you to enable the remote debugging capabilities of Firefox, connect to a running Thunderbird and then debug Thunderbird code from within Firefox.

The only "remote" aspect of this is that a socket connection is used. By default, the connection is limited to localhost connections (devtools.debugger.force-local). In theory, with that pref set to false, you could debug a remote Thunderbird (after confirmation) on your local network, or even on the internet if the port is exposed.

This might sound like a security risk, but since the default is local connections only and if devtools.debugger.force-local is set to false you are asked before the remote debugger can connect, it should be fine.
Comment 7 rsx11m 2013-06-13 05:55:35 PDT
Ok, so that's like a debugger attaching to a local process with the potential of being used beyond the loopback. And yes, this may trigger concerns about security or privacy, thus addressing those in a blog post would be a good idea.
Comment 9 rsx11m 2013-06-14 07:15:16 PDT
Thanks, this makes it quite clear now how it works and what the safeguards are.

Note You need to log in before you can comment on or make changes to this bug.