Open Bug 942756 Opened 11 years ago Updated 3 years ago

Unify the different ways we start the debugger server

Categories

(DevTools :: General, defect, P3)

x86
All
defect

Tracking

(Not tracked)

People

(Reporter: paul, Unassigned)

References

(Blocks 4 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(10 files, 103 obsolete files)

52.76 KB, patch
paul
: review+
Details | Diff | Splinter Review
10.35 KB, patch
paul
: review+
Details | Diff | Splinter Review
12.76 KB, patch
paul
: review+
Details | Diff | Splinter Review
39.44 KB, patch
paul
: review+
Details | Diff | Splinter Review
1.59 KB, patch
paul
: review+
Details | Diff | Splinter Review
8.45 KB, patch
paul
: review+
Details | Diff | Splinter Review
3.23 KB, patch
paul
: review+
Details | Diff | Splinter Review
19.84 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
janx
: review+
janx
: checkin+
Details | Review
126.29 KB, patch
Details | Diff | Splinter Review
This generic way of starting the debugger would be used for the "enable remote debugging" feature. This feature is accessible from the debugging checkbox in Gaia and Fennec. Also from a GCLI command (Firefox Desktop), and a nsICommandLineHandler (where could define the port and the unixsocket path) (for all the platforms).

Initializing the server is not hard, but registering the relevant actors and setting the chromeWindowType is not easy. Depending on the app the context, there are several ways to start the debugger:

http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#1048
B2G on device
Use unix socket

http://mxr.mozilla.org/mozilla-central/source/webapprt/RemoteDebugger.jsm#19
WebappRT
port

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7296
Android
port

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser-ui.js#200
Metro UI
port

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/hudservice.js#200
Browser Console
Pipe

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/scratchpad/scratchpad.js#1869
Scratchpad
pipe

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/BuiltinCommands.jsm#1907
GCLI
port

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/DebuggerProcess.jsm#66
Browser debugger
port

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/frameworkw/target.js#269
target.makeRemote()
pipe

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/child.js#5
webapp child process

-----


What would be the right approach here?
Summary: Unify the different way we start the debugger server → Unify the different ways we start the debugger server
Panos, what do you think?
Flags: needinfo?(past)
I'm not sure what the goal is here. Indeed there are multiple ways to start the debugger server, but they came out of specific reqthat needed to be addressed.
Flags: needinfo?(past)
Please ignore the previous incomplete comment.

I'm not sure what the goal is here. Indeed there are multiple ways to start the debugger server, but they came out of specific requirements that needed to be addressed.

The first difference is in the transport being used. In most cases a socket is required, but occasionally we optimize for speed by using a pipe (e.g. browser console or scratchpad). We also discovered that a UNIX domain socket provides some tangible benefits compared to a TCP socket, but unfortunately Windows doesn't support UNIX domain sockets, so we limit those to Linux for now.

Some tools are in a transitional state and will switch to something different soon, like Android or browser debugger, which will be folded into a remote chrome toolbox.

If you can enumerate the actual problems we are trying to solve here, I might be able to provide a more thoughtful perspective.
I also think that b2g should now be able to simply use DebuggerServer.addBrowserActors(), as the security issues that required the different behavior are now dealt with the UNIX domain socket.
(In reply to Panos Astithas [:past] from comment #3)
> If you can enumerate the actual problems we are trying to solve here, I
> might be able to provide a more thoughtful perspective.


I want to be able to start a gecko-based app in a remote-debug mode.

How do we implement a command line option that would start the debugger:

./xxx --start-debugger-server                # listen to default port
./xxx --start-debugger-server 4242           # listen to 4242
./xxx --start-debugger-server /tmp/aSocket   # use unix socket

Where xxx can be B2G, Firefox Desktop, Thunderbird, … ?
Pipes are not applicable in this case, so I think it should be pretty simple since we are overloading openListener() for every option. Just some sanity checking should be all that is needed.

The fully parameterized case should look like this:

if (!DebuggerServer.initialized) {
  DebuggerServer.init(customPermissionCallback);
  DebuggerServer.addBrowserActors(windowType);
  DebuggerServer.addActors(productSpecificActorPath);
}
DebuggerServer.openListener(portOrPath);

We could have a DebuggerServer.start() method or similar that every product could override with their own version, so your code would always call that one. I don't think it would make sense in every case though. Your command line option wouldn't apply to Fennec for example.
(In reply to Paul Rouget [:paul] from comment #5)
> (In reply to Panos Astithas [:past] from comment #3)
> > If you can enumerate the actual problems we are trying to solve here, I
> > might be able to provide a more thoughtful perspective.
> 
> 
> I want to be able to start a gecko-based app in a remote-debug mode.

For Thunderbird we auto-start the remote server if a pref is set. Using mozprofile/mozrunner you could set things up accordingly. Maybe some of this code could be generalized.

Thunderbird uses a module and extra callback to init the actors:
http://mxr.mozilla.org/comm-central/source/mail/components/devtools/modules/RemoteDebuggerServer.jsm
http://mxr.mozilla.org/comm-central/source/mail/components/devtools/content/dbg-messenger-overlay.js#16

On a related note, see bug 933720 which is the counterpart, starting Firefox with the remote debugger as a client.
Assignee: nobody → paul
Attached patch WIP (only B2G) (obsolete) — Splinter Review
Attached patch WIP (Firefox) (obsolete) — Splinter Review
Not to self: update doc here once this is done: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging
I've spent the week hacking on this. This is my plan:



We have many products that have their own way to start the debugger server: Firefox Desktop, Firefox Metro, Thunderbird, WebappRT, Firefox Android and Firefox OS.

The differences between these products are:
- actors added
- way to start the debugger from the UI (if any)
- behavior on errors and on success (dump and popup)
- meaning of the "enable-remote" pref
- on-connection prompt

The new behavior will be:

for any app, using this command line option:

> -start-debugger-server [<port or path>]

will start the debugger automatically. If a number is passed, we'll use a TCP port. If a string is passed, we'll use a unix domain socket. If nothing is passed, we'll look first at the "unix-domain-socket" pref, if unset, we'll look at the "remote-port" pref.

For everything **except Firefox Desktop non-Metro**, if the command line option is not used, when the product starts, we check if the pref "remote-enable" is "true". If so, we will start the debugger server (checking "unix-domain-socket" first, then "remote-port"). Changing this pref in "about:config" will automatically stop/start the debugger server.

Using "-start-debugger-server" command line option will not turn the "remote-enable" pref on.

Once the debugger has started, the product will notify the user via an alert notification (I believe it's too easy to forget that the server is running). 

About the implementation:

Because there are many product-specific code (which actors to add, should we listen to the pref, how do we notify the user, how do we show the prompt), we can't gather all the debugger-related code in /toolkit/.

But, I also think it's important to isolate this code from the product code. For example, in B2G, you'll find debugger code in shell.js and settings.js. You will also find code in android/browser.js, metro/browser-ui.js.

So each product will be in charge of registering one xpcom component named: "DebuggerServerController" implementing an interface named "nsIDebuggerServerController". This component will expose 5 methods:
- void start(portOrPath)
- void stop()
- void notifyUserStart(portOrPath)
- void notifyUserStop()
- boolean controlledByPref() // see bellow

I have a working patch here for metro/b2g/android, and the impact on the product code is nice:
> b/b2g/chrome/content/settings.js                                        |  208 -----
> b/b2g/chrome/content/shell.js                                           |   85 --
> b/browser/metro/base/content/browser-ui.js                              |   51 -
> b/mobile/android/chrome/content/browser.js                              |  116 ---

0 references to the debugger in any of these files.

Implementing this component is not required, the debugger is still functional if such controller is not present.

About the pref "enable-remote" in Firefox Desktop non-Metro:

Sadly, the "enable-remote" pref on Firefox Desktop non-Metro has a different meaning than the other products. In other products it means "start/stop debugger server", in FxDesktop it mean "we're allowed to connect to a remote gecko". We can't change this behavior as many people have probably used this pref in FxDesktop to connect to another Firefox. So we don't want in Firefox Desktop to be able to start the server via this pref. Which is ok because: most of the scenarios involve mobile firefox being the server and desktop firefox being the client. If we want to start the server on Firefox Desktop non-metro, we can:
- use the command line
- use gcli (which will use the controller to start the debugger)
- do it automatically when we start the chrome toolbox

Also, we can't change the name of this pref because there's many docs out there referencing this pref.
Attachment #8345846 - Attachment is obsolete: true
Attachment #8345855 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
Panos, can I ask you to comment on comment 11?
Flags: needinfo?(past)
I haven't looked at your WIP in detail yet, but I agree in general with this plan. I think that the LOC savings that you mention would tilt the other way overall if you included the new component implementation, since AFAIK you can't easily reuse code that way. But maybe the maintainability is improved if the debugger code is isolated in a separate file, so I'm not against it.

For Firefox desktop I think we should just add a "Start debugger server" checkbox in the options panel, like every other product. The other additional methods are all fine, but there is no reason to not have the same UI for starting a server on desktop IMO. The only reason we didn't do this on day 1 was some security concerns over exposing an unauthenticated remote debugging protocol to end users, which I think we can all agree is now moot, as it has been exposed in every other product.

In the same vein I believe that we should remove the "is remote debugging allowed" checks from the devtools UI (the additional meaning of remote-enabled on desktop) and just enable those tools by default. Our newest remote debugging tool, the App Manager, doesn't respect that pref and the sky hasn't fallen yet. If we do that, then the new "Start debugger server" checkbox could just toggle the existing remote-enabled pref (after a migration*) and we basically should forget about those prefs at all. No longer mention them anywhere. At that point all of our products will have adequate UI for enabling remote debugging and users should be using that exclusively.

* Migration would be required for the cases where users already have the pref enabled in the Firefox desktop profiles in order to debug their phones. When Firefox 29 comes out they shouldn't be surprised by a debugger server that starts automatically without their consent. This migration code should be pretty simple I think. Pseudocode:

if (!Prefs.migrated) {
  if (Prefs.remote-enabled) {
    Prefs.remote-enabled = false;
  }
  Prefs.migrated = true;
}

Not sure when it would be safe to remove that code path though. We could also just use a different pref name and avoid the migration effort altogether if we stop talking about prefs. Nobody but us will care and that's what we have UIs for!
Flags: needinfo?(past)
Attached file Patch V1 (obsolete) —
Panos, for this first review, I think it will be easier to use a github PR. Hope that works for you.

After your first pass, I will split this patch in product-specific patches and ask others for reviews.

What's left:
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- make sure I didn't break Adb
Attachment #8347284 - Attachment is obsolete: true
Attachment #8348029 - Flags: feedback?(past)
Comment on attachment 8348029 [details] [review]
Patch V1

Using components provides a better separation of concerns, but it turns out to increase the code size as I anticipated :-) I have left a number of comments on GitHub, but in general I think this is a good solution, thanks!
Attachment #8348029 - Flags: feedback?(past) → feedback+
Blocks: dbg-remote
What's left:
- address Panos comments
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb
Blocks: 958451
Blocks: 958498
Panos, you say that you want to keep remote-enable.

That makes the command line and gcli implementation is a little tricky, because they are one-shot startups, and starting the debugger just for one session is not easy as we will rely on a pref (persistent across sessions).

If the user starts Firefox with the --start-debugger-server command, we will then need to set the pref, start the debugger, and then reset the pref. At this point, we don't listen to the remote-enable pref yet (it happens later), so the debugger won't get shutdown when we reset the pref.

But for gcli ("listen 6000"), it's tricky. We set the pref, start the debugger, and…
- either reset the pref, but then the debugger will be kill immediately
- don't reset the pref, but then when the browser restarts, the debugger will starts

I see different ways to work around that, but it implies doing some ugly hacks (reset the pref on firefox shutdown, add extra preferences for one-shot debugger startups, …).

Maybe we can just remove the "listen" command and ask the user to use --start-debugger-server or the checkbox in the toolbox (which is permanent too, but it's a checkbox, the user would expect that, and would know how to disable it again).

What do you think?
Flags: needinfo?(past)
I think I figured out a clean way to work around that…
Flags: needinfo?(past)
No I didn't.
Flags: needinfo?(past)
What if I introduce a "preventAutoShutdown" property to the controller?

For "listen 6000", I would do:

dbg.controller.preventAutoShutdown = true;
setBool("remote-enabled", true);
dbg.controller.start();
setBool("remote-enabled", false);

But that feels wonky to me…
Is checking the pref in openListener that useful? Because if I can call openListener, I can also change the pref. That only makes things harder for us, and doesn't actually offer any security benefits.
I always thought that the "listen" command should be accompanied by a "listen --stop" (or something like that), which would kill the server (I might have filed a bug about that). But more generally, it feels like starting the server from a GCLI command is the same as flipping a switch (UI or pref). If we combine that with a UI indicator of some sort (maybe a notification at the bottom of the window like sync?) that the server is enabled, then that should be OK I think. Hadn't we discussed something like that?

About removing the pref security check completely, it's not me that you have to convince, but our security folks, like Mark and Paul :-)
Flags: needinfo?(past)
TODO:
- address Panos comments
- revisit changes from bug 952214
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb
(this is not B2G specific, and "app" here doesn't refer to Firefox OS apps)

I need a security input about the check on remote-enabled in openListener().

This is the whole story:

We want to refactor how the debugger starts and unify the code across all our apps (Firefox Desktop, B2G, Fennec, Thunderbird, Metro and WebRT), from a user point of view and from a code point of view.

The 2 options to start a debugger server would be:

* starting the app with command line option: --start-debugger-server XXX where XXX is a number (4242) or a path (/tmp/unixSocket) or nothing (then we use "unix-domain-socket" pref, if null, we use "remote-port");

* switching the pref "remote-enabled". If the app starts and "remote-enabled" is true, we start the debugger, or, while the app is running, if the pref is switched, we either start or stop the debugger server depending on the value of the pref;

It's up to the app to expose a UI mechanism to turn on and off the "remote-enabled" pref.

This refactoring also exposes a new feature: when the debugger starts or stops, we notify the user (with a system notification).

Beside the command line option (--start-debugger-server) there is nothing new for Metro, Fennec, WebRT, Thunderbird and B2G. They were all already looking at the "remote-enabled" pref to start/stop the debugger. It changes something for Firefox Desktop though: in Firefox Desktop, this pref had an extra meaning: allowing connecting the devtools to a remote debugger (we'll drop that, it's not useful anymore).

First question: is this worflow ok?

Now, the tricky part: the pref is persistent. If the pref is turned on, then, if the app restarts, we will start the debugger automatically. This is what we want, and this is how it works today for B2G, Thunderbird, Metro Fennec and WebRT. And we want that for Firefox Desktop too and I don't think it's a problem. However, we also want to be able to start the debugger server **for the current session only**. As in, we start the debugger, and on restart, the debugger doesn't start. This is what we want when the user uses the --start-debugger-server command line option. We start the debugger only for the session. If the user restart Firefox without this option, we won't start the debugger. That means that when we use --start-debugger-server, we don't want to set the "remote-enabled" pref (which is persistent). But, in our code, we only allow starting the debugger if the pref "remote-enabled" is on (in /toolkit/devtools/server/main.js, function "openListener"). I want to remove this check to be able to implement the --start-debugger-server command line option. This pref also prevents the user to start the debugger for only one session via GCLI ("listen 6000"). A work around could be to set the pref to true first, start the debugger, then turn it off. But turning the pref off would kill the debugger. So I want to remove the "remote-enabled" check in "openListener" to allow a non-pref-based debugger startup. I also believe that this check is now useless. I don't see how this check could prevent anything.

Second question: can I remove the check of the "remote-enabled" pref in the openListener() function?
Flags: needinfo?(mgoodwin)
(In reply to Paul Rouget [:paul] from comment #27)
> First question: is this worflow ok?

Yes, this workflow is OK.

> Second question: can I remove the check of the "remote-enabled" pref in the
> openListener() function?

Yes, I'm OK with this too.

The concern I have is for users who already have 'remote-enabled' set on desktop Firefox as this will change meaning.  If someone has this set to so they can remotely debug from the profile in question, they then find themselves in a situation where the browser (without any interaction on their part) is listening for debugger connections. This wouldn't be good so we should address this if we can.

Can we turn off this pref when a profile is first used with the new version? That way we can have some certainty (aside from people who switch back and forth between versions on the same profile) that the pref being set means they actually intend to receive debugger connections.
Flags: needinfo?(mgoodwin)
(In reply to Mark Goodwin [:mgoodwin] from comment #28)
> (In reply to Paul Rouget [:paul] from comment #27)
> > First question: is this worflow ok?
> 
> Yes, this workflow is OK.
> 
> > Second question: can I remove the check of the "remote-enabled" pref in the
> > openListener() function?
> 
> Yes, I'm OK with this too.

Excellent! Thank you.

> The concern I have is for users who already have 'remote-enabled' set on
> desktop Firefox as this will change meaning.  If someone has this set to so
> they can remotely debug from the profile in question, they then find
> themselves in a situation where the browser (without any interaction on
> their part) is listening for debugger connections. This wouldn't be good so
> we should address this if we can.
> 
> Can we turn off this pref when a profile is first used with the new version?
> That way we can have some certainty (aside from people who switch back and
> forth between versions on the same profile) that the pref being set means
> they actually intend to receive debugger connections.

This is plan. We're resetting the pref on Desktop on first use. See https://github.com/paulrouget/gecko-dev/commit/28c87acf7d53f5423220d4c9cb7b4fe0b118cc5b#diff-902228b290ad904d1b3cc5d33acdef11R34
(In reply to Paul Rouget [:paul] from comment #29)
> This is plan. We're resetting the pref on Desktop on first use.

Awesome. Thanks.
Attachment #8348029 - Attachment is obsolete: true
Attached patch G v1 - B2G Debugger controller (obsolete) — Splinter Review
Attached patch L v1 - More prefs changes (obsolete) — Splinter Review
Todo:
- update browser/installer/package-manifest.in
- revisit changes from bug 952214
- add tests
- make sure all existing tests pass (https://tbpl.mozilla.org/?tree=Try&rev=a4db4b179dca)
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb
Todo:
- update browser/installer/package-manifest.in
- revisit changes from bug 952214
- add tests
- make sure all existing tests pass (https://tbpl.mozilla.org/?tree=Try&rev=a4db4b179dca)
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb
- make sure Gaia reports the right debugging status in settings (settings in sync with pref)
Todo:
- update browser/installer/package-manifest.in
- revisit changes from bug 952214
- add tests
- make sure all existing tests pass (https://tbpl.mozilla.org/?tree=Try&rev=a4db4b179dca)
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb (and it gets killed on lock)
- make sure Gaia reports the right debugging status in settings (settings in sync with pref)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- keep ADB code in shell.js
- make sure the listen command still works with not argument
Todo:
- update browser/installer/package-manifest.in
- revisit changes from bug 952214
- add tests
- make sure all existing tests pass (https://tbpl.mozilla.org/?tree=Try&rev=a4db4b179dca)
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb (and it gets killed on lock)
- make sure Gaia reports the right debugging status in settings (settings in sync with pref)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- keep ADB code in shell.js
- make sure the listen command still works with not argument
- missing removeObserver of prefs and settings
Todo:
- revisit changes from bug 952214
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb (and it gets killed on lock)
- make sure Gaia reports the right debugging status in settings (settings in sync with pref)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- make sure the listen command still works with not argument
- missing removeObserver of prefs and settings
- dont use remote-enable in CommandLineHandler.js
Todo:
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb (and it gets killed on lock)
- make sure Gaia reports the right debugging status in settings (settings in sync with pref)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- make sure the listen command still works with not argument
- missing removeObserver of prefs and settings
- dont use remote-enable in CommandLineHandler.js
- Use: Services.obs.notifyObservers(null, 'debugger-server-started', null);
Todo:
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb (and it gets killed on lock)
- make sure Gaia reports the right debugging status in settings (settings in sync with pref)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- missing removeObserver of prefs and settings
Depends on: 928981
Attachment #8359191 - Attachment is obsolete: true
Attachment #8359190 - Attachment is obsolete: true
Attachment #8359189 - Attachment is obsolete: true
Attachment #8359188 - Attachment is obsolete: true
Attachment #8359187 - Attachment is obsolete: true
Attachment #8359186 - Attachment is obsolete: true
Attachment #8359185 - Attachment is obsolete: true
Attachment #8359184 - Attachment is obsolete: true
Attachment #8359183 - Attachment is obsolete: true
Attachment #8359182 - Attachment is obsolete: true
Attachment #8359181 - Attachment is obsolete: true
Attachment #8359179 - Attachment is obsolete: true
Attached patch 6.diff - B2G Debugger controller (obsolete) — Splinter Review
Todo:
- add tests
- make sure all existing tests pass
- test on Android (didn't even build yet)
- test webrt
- make sure I didn't break Adb (and it gets killed on lock)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- missing removeObserver of prefs and settings
Failures on B2G. Reference with patch not applied: https://tbpl.mozilla.org/?tree=Try&rev=610285fdcd80
Attachment #8361782 - Attachment is obsolete: true
Attachment #8361783 - Attachment is obsolete: true
Attachment #8361784 - Attachment is obsolete: true
Attachment #8361785 - Attachment is obsolete: true
Attachment #8361787 - Attachment is obsolete: true
Attachment #8361788 - Attachment is obsolete: true
Attachment #8361790 - Attachment is obsolete: true
Attachment #8361791 - Attachment is obsolete: true
Attachment #8361792 - Attachment is obsolete: true
Attached patch 6.diff - B2G Debugger controller (obsolete) — Splinter Review
Attached patch 10.diff - fix tests (obsolete) — Splinter Review
Comment on attachment 8364945 [details] [diff] [review]
5.diff - Firefox Desktop Debugger controller

Matt, this bug is about simplifying and unifying how we start the debugger server. This affects metro. Can you look at the changes in /browser/devtools/components/DebuggerServerController.js and /browser/metro/base/content/browser-ui.js and tell me if you're ok with these changes. I haven't tested if it works correctly with our Metro UI. Can you check for me? Look at the first half of comment 27 to understand the new behavior.
Attachment #8364945 - Flags: feedback?(mbrubeck)
Comment on attachment 8364947 [details] [diff] [review]
7.diff - Fennec Debugger controller

Mark, this bug is about simplifying and unifying how we start the debugger server. This affects Fennec. Can you look at the changes in /mobile/android/chrome/content/browser.js and /mobile/android/components/DebuggerServerController.js and tell me if you're ok with these changes. I haven't tested if it works correctly on Android. Can you check for me? Look at the first half of comment 27 to understand the new behavior.
Attachment #8364947 - Flags: feedback?(mark.finkle)
Comment on attachment 8364942 [details] [diff] [review]
2.diff - DevToolsAppStartup component

Philipp, this bug is about simplifying and unifying how we start the debugger server (now at the toolkit level). This affects Thunderbird. I haven't changed Thunderbird code, but I guess it's just about removing all the code that starts/stops the debugger server, and implement a controller component as I do in patch 5 6 and 7. Can you confirm that this new behavior works for you? And can you make sure Thunderbird code gets updated once this lands?
Attachment #8364942 - Flags: feedback?(philipp)
Todo:
- add tests
- make sure all existing tests pass
- get f+ from Android, Metro, Thunderbird, WebRT and B2G people
- ADB is broken (adbController is not reachable from the js xpcom component)
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- missing removeObserver of prefs and settings
Attached patch 11.diff - test command line (obsolete) — Splinter Review
Comment on attachment 8365037 [details] [diff] [review]
11.diff - test command line

Alex, is that ugly?
Attachment #8365037 - Flags: feedback?(poirot.alex)
Comment on attachment 8364945 [details] [diff] [review]
5.diff - Firefox Desktop Debugger controller

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

Unfortunately Metro does not load modules from /browser/devtools, so we need to put the shared code in /toolkit or duplicate it in /browser/metro.
Attachment #8364945 - Flags: feedback?(mbrubeck) → feedback-
Comment on attachment 8365037 [details] [diff] [review]
11.diff - test command line

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

Better ask feedback to try/tbpl. The test is reasonable only if it is stable green on test slaves
and easily runnable on each developer environment.
Attachment #8365037 - Flags: feedback?(poirot.alex)
Blocks: 928981
No longer depends on: 928981
Comment on attachment 8364942 [details] [diff] [review]
2.diff - DevToolsAppStartup component

I haven't actually tested the patches in this bug, but from looking at the code I've created the needed Thunderbird controller. It looks like it will work out fine, so f+ from me.
Attachment #8364942 - Flags: feedback?(philipp) → feedback+
Ok, I have tested the code for Thunderbird/XULRunner now and it works fine, with one exception. I can no longer detect the chromeWindowType required by addBrowserActors(), as the debugger server is now started before the main window opens.

The Thunderbird code to retrieve the chrome window type was previously checking open windows to see if the hostname part of the uri is the same as the basename of the file and getting the windowtype attribute from it (i.e chrome://messenger/content/messenger.xul). I have done this because the code is shared with the extension intended for xulrunner and friends, where the main window url is obviously not mail:3pane.

I cannot get the window this way now. What I can do is hardcode it for Thunderbird and add a window watcher observer that waits until the window with browser.chromeURL is opened for xulrunner, but this means I need to set chromeWindowType later on. I think this is ok, but if you see any way to optimize it let me know.
Attached patch All patched folded (obsolete) — Splinter Review
(In reply to Philipp Kewisch [:Fallen] from comment #86)
> Ok, I have tested the code for Thunderbird/XULRunner now and it works fine,
> with one exception. I can no longer detect the chromeWindowType required by
> addBrowserActors(), as the debugger server is now started before the main
> window opens.
> 
> The Thunderbird code to retrieve the chrome window type was previously
> checking open windows to see if the hostname part of the uri is the same as
> the basename of the file and getting the windowtype attribute from it (i.e
> chrome://messenger/content/messenger.xul). I have done this because the code
> is shared with the extension intended for xulrunner and friends, where the
> main window url is obviously not mail:3pane.
> 
> I cannot get the window this way now. What I can do is hardcode it for
> Thunderbird and add a window watcher observer that waits until the window
> with browser.chromeURL is opened for xulrunner, but this means I need to set
> chromeWindowType later on. I think this is ok, but if you see any way to
> optimize it let me know.

chromeWindowType is only needed when the debugger needs to start. So you can always set it dynamically. It's only a problem if you start from the command line, right? In this case, you'll have to hardcode the window. Or maybe there's a way to know ahead of time the window type that will be loaded? Or maybe we can wait until the first window is loaded to start the debugger.
Todo:
- get f+ from Android, Metro, Thunderbird, WebRT and B2G people
- test that the check on Cr.NS_ERROR_NOT_AVAILABLE works
- missing removeObserver of prefs and settings
Attachment #8364941 - Attachment is obsolete: true
Attachment #8364942 - Attachment is obsolete: true
Attachment #8364943 - Attachment is obsolete: true
Attachment #8364944 - Attachment is obsolete: true
Attachment #8364945 - Attachment is obsolete: true
Attachment #8364946 - Attachment is obsolete: true
Attached patch 7.diff - B2G Debugger controller (obsolete) — Splinter Review
Attachment #8364947 - Attachment is obsolete: true
Attachment #8364947 - Flags: feedback?(mark.finkle)
Attachment #8364948 - Attachment is obsolete: true
Attachment #8364949 - Attachment is obsolete: true
Attachment #8364950 - Attachment is obsolete: true
Attachment #8365037 - Attachment is obsolete: true
Attached patch 12.diff - fix tests (obsolete) — Splinter Review
Attached patch 13.diff - Add test (obsolete) — Splinter Review
(In reply to Paul Rouget [:paul] from comment #88)

> chromeWindowType is only needed when the debugger needs to start. So you can
> always set it dynamically. It's only a problem if you start from the command
> line, right? In this case, you'll have to hardcode the window. Or maybe
> there's a way to know ahead of time the window type that will be loaded? Or
> maybe we can wait until the first window is loaded to start the debugger.

I've managed to postpone detecting the window type by overwriting the DebuggerServer.chromeWindowType getter in my root actor file. This works quite well, so f=me for Thunderbird.
Comment on attachment 8376213 [details] [diff] [review]
All patched folded

I notice you were worried about remotely debugging a Firefox instance because they had enabled the preference for other reasons. Does Firefox not prompt you for an incoming connection?

>+    if (!portOrPath) {
>+      // If the "devtools.debugger.unix-domain-socket" pref is set, we use a unix socket.
>+      // If not, we use a regular TCP socket.
>+      try {
>+        portOrPath = Services.prefs.getCharPref("devtools.debugger.unix-domain-socket");
>+      } catch (e) {
>+        portOrPath = Services.prefs.getIntPref("devtools.debugger.remote-port");
>+      }
>+    }
>+
>+    try {
>+      DebuggerServer.openListener(portOrPath);
You seem to have triplicated this code. Is there any reason not to move the portOrPath pref lookup into openListener itself?
(In reply to neil@parkwaycc.co.uk from comment #105)
> Comment on attachment 8376213 [details] [diff] [review]
> All patched folded
> 
> I notice you were worried about remotely debugging a Firefox instance
> because they had enabled the preference for other reasons. Does Firefox not
> prompt you for an incoming connection?

It does. But we don't want to keep the debugger running (listening) for no reason.

> >+    if (!portOrPath) {
> >+      // If the "devtools.debugger.unix-domain-socket" pref is set, we use a unix socket.
> >+      // If not, we use a regular TCP socket.
> >+      try {
> >+        portOrPath = Services.prefs.getCharPref("devtools.debugger.unix-domain-socket");
> >+      } catch (e) {
> >+        portOrPath = Services.prefs.getIntPref("devtools.debugger.remote-port");
> >+      }
> >+    }
> >+
> >+    try {
> >+      DebuggerServer.openListener(portOrPath);
> You seem to have triplicated this code. Is there any reason not to move the
> portOrPath pref lookup into openListener itself?

I'm not sure yet. Maybe.
(In reply to Paul Rouget [:paul] from comment #108)
> https://tbpl.mozilla.org/?tree=Try&rev=0262d1188a41

Meh. Crash everywhere.
Sorry for the tbpl spam, getting closer:

https://tbpl.mozilla.org/?tree=Try&rev=76b7ad223ea5
Blocks: 973530
Attachment #8376256 - Attachment is obsolete: true
Attachment #8376254 - Attachment is obsolete: true
Attachment #8376252 - Attachment is obsolete: true
Attachment #8376251 - Attachment is obsolete: true
Attachment #8376250 - Attachment is obsolete: true
Attachment #8376249 - Attachment is obsolete: true
Attachment #8376247 - Attachment is obsolete: true
Attachment #8376246 - Attachment is obsolete: true
Attachment #8376240 - Attachment is obsolete: true
Attachment #8376245 - Attachment is obsolete: true
Attachment #8376238 - Attachment is obsolete: true
Attachment #8376244 - Attachment is obsolete: true
Attachment #8376242 - Attachment is obsolete: true
Attachment #8376213 - Attachment is obsolete: true
Attachment #8382874 - Flags: review?(poirot.alex)
Attachment #8382875 - Flags: review?(poirot.alex)
Attachment #8382876 - Flags: review?(poirot.alex)
Attachment #8382877 - Flags: review?(poirot.alex)
Attachment #8382878 - Flags: review?(poirot.alex)
Attached patch 6.diff - B2G Debugger controller (obsolete) — Splinter Review
Attachment #8382879 - Flags: feedback?(poirot.alex)
Attachment #8382880 - Flags: feedback?(poirot.alex)
Attachment #8382881 - Flags: review?(poirot.alex)
Attachment #8382882 - Flags: review?(poirot.alex)
Attached patch 10.diff - fix tests (obsolete) — Splinter Review
Attached patch 11.diff - add test (obsolete) — Splinter Review
Attachment #8382887 - Flags: review?(mbrubeck)
Attachment #8382887 - Flags: review?(mbrubeck)
Attachment #8382878 - Attachment is obsolete: true
Attachment #8382878 - Flags: review?(poirot.alex)
Attachment #8382892 - Flags: review?(poirot.alex)
Attachment #8382887 - Attachment is obsolete: true
Attachment #8382893 - Flags: review?(mbrubeck)
Should fix the browser mochitests: https://tbpl.mozilla.org/?tree=Try&rev=8c25bebf79df
Comment on attachment 8382875 [details] [diff] [review]
2.diff - DevToolsAppStartup component

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

May need another look depending on last comment.

::: toolkit/devtools/DevToolsAppStartup.js
@@ +5,5 @@
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +

It would be very pollite to drop a short description of this module.
To highlight the two main goals:
 - Handle command line argument to start a debugger server on a given tcp port or unix socket file,
 - automatically start and stop the debugger depending on devtools.debugger.remote-enabled pref value.

@@ +17,5 @@
> +  } catch(e) {
> +    try {
> +      this.dbgPortOrPath = Services.prefs.getIntPref("devtools.debugger.remote-port");
> +    } catch(e) {}
> +  }

What about making dbgPortOrPath a getter, or listen for these two prefs?
That would allow changing the path or port without rebooting.

@@ +39,5 @@
> +      if (this.dbgPortOrPath) {
> +        str += " (default: " + this.dbgPortOrPath + ")\n";
> +      } else {
> +        str += "\n";
> +      }

nit:
  if (this.dbgPortOrPath) {
    str += " (default: " + this.dbgPortOrPath + ")";
  }
  str += "\n";

@@ +74,5 @@
> +    // Time to start the debugger if needed and observe the remote-enable
> +    // pref.
> +
> +    if (startDebuggerServerBecauseCmdLine ||
> +        Services.prefs.getBoolPref('devtools.debugger.remote-enabled')) {

nit: It would be really nice of you if you define devtools.debugger.remote-enabled only once in a const or something.
nit2: use " for strings.

@@ +76,5 @@
> +
> +    if (startDebuggerServerBecauseCmdLine ||
> +        Services.prefs.getBoolPref('devtools.debugger.remote-enabled')) {
> +      if (this.dbgPortOrPath) {
> +        DebuggerServer.controller.start(this.dbgPortOrPath);

By default, that won't start because of this code in DebuggerServer.openListener:
openListener: function DS_openListener(aPortOrPath) {
    if (!Services.prefs.getBoolPref("devtools.debugger.remote-enabled")) {
      return false;
    }

Is it expected?
If yes, that would be worth not trying to even start and dump an error message.
Attachment #8382875 - Flags: review?(poirot.alex)
Attachment #8382874 - Flags: review?(poirot.alex) → review+
Attachment #8382876 - Flags: review?(poirot.alex) → review+
Attachment #8382877 - Flags: review?(poirot.alex) → review+
Addressed comments.
Attachment #8382875 - Attachment is obsolete: true
Attachment #8383023 - Flags: review?(poirot.alex)
Comment on attachment 8382879 [details] [diff] [review]
6.diff - B2G Debugger controller

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

I wish I could test adb behavior... but it looks like there is no way to update /default.prop file on unagi to prevent adb to be always on.

f+ as it looks good codewise, with the setRemoteDebuggerState call and debugger.remote-mode pref comments being adressed.

::: b2g/components/DebuggerServerController.js
@@ +65,5 @@
> +    }
> +
> +    if (topic != "mozsettings-changed") {
> +      return;
> +    }

nit: most observe method uses switch case and do not make such special case for the last case.
Could you make it dumb but easily readable with something like this?
switch(topic) {
  case "xpcom-shutdown":
    ...
  case ...
    ...
  case "mozsettings-changed":
    let {key, value} = JSON.parse(data);
    switch (key) {
      case "lockscreen.locked":
         ..
      case ..
         ..
    }
}

@@ +82,5 @@
> +
> +    // Keep the old setting to not break people that won't have updated
> +    // gaia and gecko.
> +    if (key == "devtools.debugger.remote-enabled") {
> +      Services.prefs.setBoolPref('devtools.debugger.remote-enabled', value);

nit: here, and in many other places in this file, try to stick with same quotes for all string, " seems to be the one used in gecko code.

@@ +141,5 @@
> +        portOrPath = Services.prefs.getCharPref("devtools.debugger.unix-domain-socket");
> +      } catch (e) {
> +        portOrPath = Services.prefs.getIntPref("devtools.debugger.remote-port");
> +      }
> +    }

This code is duplicated between b2g and browser, and it is also being fetched from DevToolsAppStartup.js.
Shouldn't we just ensure that DevToolsAppStartup.js watch these prefs and consider nsIDebuggerController is always called with a value,
if not DevToolsAppStartup.js would throw.

@@ +158,5 @@
> +  stop: function() {
> +    DebuggerServer.destroy();
> +    if (this.adbController) {
> +      this.adbController.setRemoteDebuggerState(false);
> +    }

I don't think it is any useful to call this function from here, as well as from start().
remoteDebuggerEnabled() should be called whenever we enable ADB, it isn't related to devtools remote debugger.
(the name of this function is misleading...)

@@ +163,5 @@
> +  },
> +
> +  _onDebuggerStarted: function(portOrPath) {
> +    dump('Devtools debugger server started: ' + portOrPath + '\n');
> +    Settings.createLock().set("debugger.remote-mode", "adb-devtools", null);

That's suspicious. By doing that, we would toggle the setting and the pref if we start it from the command line or programatically.
Is it any useful/important?

@@ +175,5 @@
> +      } else {
> +        detail = l10n.formatStringFromName("debuggerStartedAlert.detailPath", [portOrPath], 1);
> +      }
> +      Alerts.showAlertNotification(null, title, detail, false, "", function(){});
> +    }

I don't see the value of these notifications?
It looks like the goal was to have parity with android behavior, but that's not exactly it.
Android only shows a _silent_ and _transient_ notification when a device is plugged to a computer in dev mode.
It is displayed until we unplug the phone.
Here we display it whenever the debugger mode is on/off, with a sound and it seems to stay for ever.
It feel especially weird when you receive this notification when toggling devtools in the settings app.

Also it doesn't always work and I see following exception:
  [JavaScript Error: "currentListener.observer.observe is not a function" {file: "chrome://b2g/content/shell.js" line: 902}]

Can we keep that for a followup?
Attachment #8382879 - Flags: feedback?(poirot.alex) → feedback+
Comment on attachment 8382882 [details] [diff] [review]
9.diff - introduce DebuggerServer.isSocketConnected()

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

Unless I missunderstand this code, this looks really wrong and I'm unable to test the actual behavior of this...

::: b2g/chrome/content/adbController.js
@@ +183,5 @@
>      }
>  
>      // Check if we have a remote debugging session going on. If so, we won't
>      // disable adb even if the screen is locked.
> +    let isDebugging = this.adbOnly || DebuggerServer.isSocketConnected();

I think you should get rid of adbOnly here and in b2g/components/DebuggerServerController.js
Here we are going to prevent adb disabling when the lockscreen turns one while being in adb-only mode!
The purpose of isDebugging is only to prevent shutting down the debugger server if we have a toolbox still connected.
The following code in DebuggerServerController will ensure shutting down the server when the client disconnects:
  DebuggerServer.onConnectionChange = function(what) {
    this.adbController.updateState();
  }
Attachment #8382882 - Flags: review?(poirot.alex) → review-
Attachment #8383023 - Flags: review?(poirot.alex) → review+
Comment on attachment 8382893 [details] [diff] [review]
12.diff - Metro Debugger controller

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

::: browser/metro/components/DebuggerServerController.js
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* 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"; would be nice. :)
Attachment #8382893 - Flags: review?(mbrubeck) → review+
Comment on attachment 8382880 [details] [diff] [review]
7.diff - Make adbController standalone

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

After applying all these patches, I still get some adbController. But I already mentioned some of them should be removed in comment 129.
The only one left should only be:
  if (this.adbController) {
    DebuggerServer.onConnectionChange = function(what) {
      this.adbController.updateState();
    }
  }
which looks ok.

Also we should merge various of these patches before landing as they depend on each other.
Thanks for splitting them up, it was really helpfull for the review!
Attachment #8382880 - Flags: feedback?(poirot.alex) → feedback+
Comment on attachment 8382881 [details] [diff] [review]
8.diff - repurpose the meaning of the remote-enabled pref

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

nit: it might be usefull to tweak the pref comment over here:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js#532
At the end, the original comment describe more the new behavior than the previous one...
But it can be useful to state that it will open/close a port on your firefox.

::: browser/devtools/framework/gDevTools.jsm
@@ +415,1 @@
>                          Services.prefs.getBoolPref("devtools.debugger.chrome-enabled");

Don't you miss a `&&` here or splinter is showing me weird stuff?
Attachment #8382881 - Flags: review?(poirot.alex) → review+
Attachment #8382884 - Flags: review?(poirot.alex)
Comment on attachment 8382892 [details] [diff] [review]
5.diff - Firefox Desktop Debugger controller

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

Please, do not harm the Browser toolbox!

::: browser/devtools/components/DebuggerServerController.js
@@ +15,5 @@
> +
> +
> +XPCOMUtils.defineLazyModuleGetter(this,
> +    "DebuggerServer",
> +    "resource://gre/modules/devtools/dbg-server.jsm");

There is a overall issue with modules that ends up breaking the browser debugger.
For the browser debugger, we have to load and instanciate a brand new RemoteDebugger instance
to prevent any conflict with an already running one.

So here, if you open firefox, enable the remote pref, the browser toolbox will fail to load.

That's because, right here, you always fetch the same DebuggerServer singleton, the one firefox uses for the remote pref.
You have to be carefull when using xpcom and jsm. Ideally we would always be using SDK modules so that we can easily instanciate and segregate duplicate such instances.

You can work around that by making DebuggerServerController instanciable and pass it the debugger server instance in init().
It may be usefull to make DebuggerServer a class instead of singleton to prevent such issue.

@@ +109,5 @@
> +      detail = l10n.formatStringFromName("debuggerStartedAlert.detailPort", [portOrPath], 1);
> +    } else {
> +      detail = l10n.formatStringFromName("debuggerStartedAlert.detailPath", [portOrPath], 1);
> +    }
> +    Alerts.showAlertNotification(null, title, detail, false, "", function(){});

As for b2g patch, I'm not sure there is much value in these notifications.
Attachment #8382892 - Flags: review?(poirot.alex) → review-
(In reply to Alexandre Poirot (:ochameau) from comment #129)
> @@ +163,5 @@
> > +  },
> > +
> > +  _onDebuggerStarted: function(portOrPath) {
> > +    dump('Devtools debugger server started: ' + portOrPath + '\n');
> > +    Settings.createLock().set("debugger.remote-mode", "adb-devtools", null);
> 
> That's suspicious. By doing that, we would toggle the setting and the pref
> if we start it from the command line or programatically.
> Is it any useful/important?

Without that, it would be impossible to stop the debugger. Also, I'll remove the call to Alert, so that will be the only indicator that the debugger server is running. The side effect is that it will set the pref, so restarting B2G will start the debugger server. But is that a bad thing?
(In reply to Alexandre Poirot (:ochameau) from comment #132)
> Comment on attachment 8382880 [details] [diff] [review]
> 7.diff - Make adbController standalone
> 
> Review of attachment 8382880 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After applying all these patches, I still get some adbController. But I
> already mentioned some of them should be removed in comment 129.
> The only one left should only be:
>   if (this.adbController) {
>     DebuggerServer.onConnectionChange = function(what) {
>       this.adbController.updateState();
>     }
>   }
> which looks ok.

Yeah, I forgot to remove these parts. I also think we should remove this call. Simply because adbController is not accessible from the component. Can't we call udpateState from adbController.js on debugger-server-start/stopped notifications?
(In reply to Alexandre Poirot (:ochameau) from comment #134)
> @@ +109,5 @@
> > +      detail = l10n.formatStringFromName("debuggerStartedAlert.detailPort", [portOrPath], 1);
> > +    } else {
> > +      detail = l10n.formatStringFromName("debuggerStartedAlert.detailPath", [portOrPath], 1);
> > +    }
> > +    Alerts.showAlertNotification(null, title, detail, false, "", function(){});
> 
> As for b2g patch, I'm not sure there is much value in these notifications.

I'd like to keep this notification on Desktop. It's the only feedback we get (unlike on B2G, where we see the debugger option). And because we're changing the meaning of the remote-enable pref, I think it's important to tell the user something is happening (even though we migrate the pref).
(In reply to Paul Rouget [:paul] from comment #137)
> (In reply to Alexandre Poirot (:ochameau) from comment #132)
> > Comment on attachment 8382880 [details] [diff] [review]
> > 7.diff - Make adbController standalone
> > 
> > Review of attachment 8382880 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > After applying all these patches, I still get some adbController. But I
> > already mentioned some of them should be removed in comment 129.
> > The only one left should only be:
> >   if (this.adbController) {
> >     DebuggerServer.onConnectionChange = function(what) {
> >       this.adbController.updateState();
> >     }
> >   }
> > which looks ok.
> 
> Yeah, I forgot to remove these parts. I also think we should remove this
> call. Simply because adbController is not accessible from the component.
> Can't we call udpateState from adbController.js on
> debugger-server-start/stopped notifications?

This won't work. We need a new notification. I'll introduce "debugger-connection-changed".
Attachment #8383023 - Attachment is obsolete: true
Attachment #8382893 - Attachment is obsolete: true
Attachment #8382892 - Attachment is obsolete: true
Attachment #8382888 - Attachment is obsolete: true
Attachment #8382885 - Attachment is obsolete: true
Attachment #8382884 - Attachment is obsolete: true
Attachment #8382884 - Flags: review?(poirot.alex)
Attachment #8382882 - Attachment is obsolete: true
Attachment #8382877 - Attachment is obsolete: true
Attachment #8382881 - Attachment is obsolete: true
Attachment #8382874 - Attachment is obsolete: true
Attachment #8382880 - Attachment is obsolete: true
Attachment #8382879 - Attachment is obsolete: true
Attachment #8382876 - Attachment is obsolete: true
Attached patch Part 1: devtools & browser (obsolete) — Splinter Review
Addressed Alex' comments.
Attachment #8383745 - Flags: review?(poirot.alex)
Attached patch Part 2: b2g (obsolete) — Splinter Review
Attached patch Part 3: new test (obsolete) — Splinter Review
Attached patch Part 4: metro (obsolete) — Splinter Review
Carrying mbrubeck's r+.
Attachment #8383748 - Flags: review+
Attached patch Part 5: Fennec (obsolete) — Splinter Review
Alex: I changed the way adbController works. Removed all reference from other part of the code. I had to introduce a debugger-connection-changed notification. I kept the notifications on Desktop and removed the one in B2G. I fixed the browser toolbox (we don't use a singleton anymore).

Sorry, I had to fold the patches, it was getting too hard to maintain a clean set of commits.
Comment on attachment 8383746 [details] [diff] [review]
Part 2: b2g

Fabrice, this patch refactor the way we start the server debugger. Code is now isolated into a js component. adbController has also been moved in its own file and works in a self-contained way.

shell.js and settings.js are a little bit cleaner now.

Please pay some special attention at the adbController code. I didn't just move the code, I also made changes.
Attachment #8383746 - Flags: review?(fabrice)
Matt, I didn't actually test the Metro code. Can you make sure it's functional?
Flags: needinfo?(mbrubeck)
Attachment #8383749 - Flags: review?(mark.finkle)
Comment on attachment 8383748 [details] [diff] [review]
Part 4: metro

I think this is the wrong patch.
Comment on attachment 8382893 [details] [diff] [review]
12.diff - Metro Debugger controller

I tested with this patch, and after adding "this.wrappedJSObject = this;" to the controller constructor, it seems to work fine.
Flags: needinfo?(mbrubeck)
(In reply to Matt Brubeck (:mbrubeck) from comment #148)
> I think this is the wrong patch.

Oops, sorry.

> I tested with this patch, and after adding "this.wrappedJSObject = this;" to
> the controller constructor, it seems to work fine.

Alright. Thank you.
Attached patch Part 2: b2g (obsolete) — Splinter Review
See comment 146
Attachment #8383746 - Attachment is obsolete: true
Attachment #8383746 - Flags: review?(fabrice)
Attachment #8384471 - Flags: review?(fabrice)
Attached patch Part 4: metro (obsolete) — Splinter Review
Attachment #8384472 - Flags: review+
Attachment #8383748 - Attachment is obsolete: true
Comment on attachment 8383745 [details] [diff] [review]
Part 1: devtools & browser

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

::: browser/devtools/components/DebuggerServerController.js
@@ +36,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDebuggerServerController, Ci.nsIObserver]),
> +
> +  get debugger() {
> +    if (!this._debugger) {
> +      this._debugger = DebuggerServer;

That looks like a footgun aimed at your feet.
It will silently use the singleton server if something goes wrong with setting the custom server.
DebuggerServerController requires an explicit debuggerserver instance.

Also this code in main.js looks hackish:
  this._controller.wrappedJSObject.debugger = this;

What about adding the `init(in jsval server);` method to the idl and ensure calling it from main.js?

If you do so, do not forget to remove
  XPCOMUtils.defineLazyModuleGetter(this,
    "DebuggerServer",
    "resource://gre/modules/devtools/dbg-server.jsm");
to prevent any miscomprehension.
(In reply to Paul Rouget [:paul] from comment #136)
> (In reply to Alexandre Poirot (:ochameau) from comment #129)
> > @@ +163,5 @@
> > > +  },
> > > +
> > > +  _onDebuggerStarted: function(portOrPath) {
> > > +    dump('Devtools debugger server started: ' + portOrPath + '\n');
> > > +    Settings.createLock().set("debugger.remote-mode", "adb-devtools", null);
> > 
> > That's suspicious. By doing that, we would toggle the setting and the pref
> > if we start it from the command line or programatically.
> > Is it any useful/important?
> 
> Without that, it would be impossible to stop the debugger. Also, I'll remove
> the call to Alert, so that will be the only indicator that the debugger
> server is running. The side effect is that it will set the pref, so
> restarting B2G will start the debugger server. But is that a bad thing?

Just to ensure I don't miss something... This is only for the cases where we start the debuger server fromm the command line or somehow somewhere programatically?
(In reply to Alexandre Poirot (:ochameau) from comment #154)
> (In reply to Paul Rouget [:paul] from comment #136)
> > (In reply to Alexandre Poirot (:ochameau) from comment #129)
> > > @@ +163,5 @@
> > > > +  },
> > > > +
> > > > +  _onDebuggerStarted: function(portOrPath) {
> > > > +    dump('Devtools debugger server started: ' + portOrPath + '\n');
> > > > +    Settings.createLock().set("debugger.remote-mode", "adb-devtools", null);
> > > 
> > > That's suspicious. By doing that, we would toggle the setting and the pref
> > > if we start it from the command line or programatically.
> > > Is it any useful/important?
> > 
> > Without that, it would be impossible to stop the debugger. Also, I'll remove
> > the call to Alert, so that will be the only indicator that the debugger
> > server is running. The side effect is that it will set the pref, so
> > restarting B2G will start the debugger server. But is that a bad thing?
> 
> Just to ensure I don't miss something... This is only for the cases where we
> start the debuger server fromm the command line or somehow somewhere
> programatically?

Yes. We want to setting to reflect the debugger server status.
Attached patch add debuggerController.init (obsolete) — Splinter Review
Alex, I will merge this patch with the previous patches. Is that what you had in mind?
Comment on attachment 8383745 [details] [diff] [review]
Part 1: devtools & browser

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

::: browser/devtools/components/DebuggerServerController.js
@@ +36,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDebuggerServerController, Ci.nsIObserver]),
> +
> +  get debugger() {
> +    if (!this._debugger) {
> +      this._debugger = DebuggerServer;

(Note that, if you don't stop uing wrappedJSObject, `this._controller.wrappedJSObject.debugger = this;` throws on b2g as wrappedJSObject isn't defined on b2g controler)

@@ +91,5 @@
> +
> +    try {
> +      this.debugger.openListener(pathOrPort);
> +    } catch (e if e.result != Cr.NS_ERROR_NOT_AVAILABLE) {
> +      dump('Unable to start debugger server (' + pathOrPort + '): ' + e + '\n');

nit: strings, quotes, simple, double, choose :)

::: toolkit/devtools/server/main.js
@@ +289,5 @@
>      this._allowConnection = null;
>      this._transportInitialized = false;
>      this._initialized = false;
>  
> +    Services.obs.notifyObservers(null, 'debugger-connection-changed', "closed");

^

@@ +433,5 @@
>        dumpn("Could not start debugging listener on '" + aPortOrPath + "': " + e);
>        throw Cr.NS_ERROR_NOT_AVAILABLE;
>      }
>      this._socketConnections++;
> +    Services.obs.notifyObservers(null, 'debugger-server-started', aPortOrPath);

^

@@ +455,5 @@
>      if (--this._socketConnections == 0 || aForce) {
>        this._listener.close();
>        this._listener = null;
>        this._socketConnections = 0;
> +      Services.obs.notifyObservers(null, 'debugger-server-stopped', null);

^

@@ +581,5 @@
>        aTransport.send(conn.rootActor.sayHello());
>      }
>      aTransport.ready();
>  
> +    Services.obs.notifyObservers(null, 'debugger-connection-changed', "opened");

^

@@ +590,5 @@
>     * Remove the connection from the debugging server.
>     */
>    _connectionClosed: function DS_connectionClosed(aConnection) {
>      delete this._connections[aConnection.prefix];
> +    Services.obs.notifyObservers(null, 'debugger-connection-changed', "closed");

^
Comment on attachment 8383745 [details] [diff] [review]
Part 1: devtools & browser

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

Tested firefox (command line [various options], chrome debugging, multiple debugger server instances, pref toggling)
       b2g (various settings, debug environment)

And everything works fine. The only thing I can't test is adb controler and full adb disabling as I can't update the bootimg of my phone
to prevent it from ALWAYS starting adb via android properties set in /default.prop (fabrice, if you have any idea how to fix that on a unagi...)
Attachment #8383745 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot (:ochameau) from comment #158)
> Comment on attachment 8383745 [details] [diff] [review]
> Part 1: devtools & browser
> 
> Review of attachment 8383745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tested firefox (command line [various options], chrome debugging, multiple
> debugger server instances, pref toggling)
>        b2g (various settings, debug environment)
> 
> And everything works fine. The only thing I can't test is adb controler and
> full adb disabling as I can't update the bootimg of my phone
> to prevent it from ALWAYS starting adb via android properties set in
> /default.prop (fabrice, if you have any idea how to fix that on a unagi...)

Did you disable marionnette?
Comment on attachment 8383749 [details] [diff] [review]
Part 5: Fennec

>diff --git a/mobile/android/components/DebuggerServerController.js b/mobile/android/components/DebuggerServerController.js

>+  _onDebuggerStarted: function(portOrPath) {
>+    if (!Services.prefs.getBoolPref("devtools.debugger.show-server-notifications"))
>+      return;
>+    let title = l10n.GetStringFromName("debuggerStartedAlert.title");
>+    let port = Number(portOrPath);
>+    let detail;
>+    if (port) {
>+      detail = l10n.formatStringFromName("debuggerStartedAlert.detailPort", [portOrPath], 1);
>+    } else {
>+      detail = l10n.formatStringFromName("debuggerStartedAlert.detailPath", [portOrPath], 1);
>+    }
>+    Alerts.showAlertNotification(null, title, detail, false, "", function(){});
>+  },
>+
>+  _onDebuggerStopped: function() {
>+    if (!Services.prefs.getBoolPref("devtools.debugger.show-server-notifications"))
>+      return;
>+    let title = l10n.GetStringFromName("debuggerStopped.title");
>+    Alerts.showAlertNotification(null, title, null, false, "", function(){});
>+  },
>+};

Seems good overall. The alert stuff is new, but I see it's controlled via a pref. Let's see how it feels.

I want Myk to see this change too, just to make sure debugging WebApps will still work.
Attachment #8383749 - Flags: review?(mark.finkle)
Attachment #8383749 - Flags: review+
Attachment #8383749 - Flags: feedback?(myk)
Attached patch Part 1: devtools & browser (obsolete) — Splinter Review
Addressed Alex' comments. And s/"/'/.
Attachment #8383745 - Attachment is obsolete: true
Attachment #8384708 - Flags: review+
Attached patch Part 2: b2g (obsolete) — Splinter Review
Addressed Alex' comments. And s/"/'/.

Fabrice, see comment 146.
Attachment #8384471 - Attachment is obsolete: true
Attachment #8384471 - Flags: review?(fabrice)
Attachment #8384710 - Flags: review?(fabrice)
Attached patch Part 4: metro (obsolete) — Splinter Review
Addressed Alex' comments. And s/"/'/.
Attachment #8384472 - Attachment is obsolete: true
Attachment #8384711 - Flags: review+
Attached patch Part 4: metro (obsolete) — Splinter Review
Attachment #8384711 - Attachment is obsolete: true
Attachment #8384712 - Flags: review+
Attached patch Part 5: Fennec (obsolete) — Splinter Review
Addressed Alex' comments. And s/'/"/.
Attachment #8383749 - Attachment is obsolete: true
Attachment #8383749 - Flags: feedback?(myk)
Attachment #8384713 - Flags: review+
Attachment #8384713 - Flags: feedback?(myk)
Attachment #8384573 - Attachment is obsolete: true
fyi, bug 958043 is about to land and will most likely need a merge on shell.js.
Comment on attachment 8384713 [details] [diff] [review]
Part 5: Fennec

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

This seems reasonable to me.

Note that mobile/android/chrome/content/WebAppRT.js, which implements the web runtime for running webapps in their own processes (and with their own profiles), also starts the debugger server if the webapp is in Android's "debug" mode, regardless of the value of the remote-enabled pref.

But it currently does so by setting the remote-enabled pref, which always seemed kinda hacky (especially since it persists across sessions, even though "debug" mode could change in a new session if the user updates the app with a non-debug version).

Perhaps WebappRT should switch to calling nsIDebuggerServerController.start directly?

Also note that WebappRT picks a random free port.  It'd be nice if the ability to do that was built into the controller.

::: mobile/android/components/DebuggerServerController.js
@@ +9,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this,
> +    "Alerts",
> +    "@mozilla.org/alerts-service;1", "nsIAlertsService");

Nit: Fennec hackers tend to prefer long lines to aggressive wrapping (up to a point, of course), so I would put a statement like this all on one line.
Attachment #8384713 - Flags: feedback?(myk) → feedback+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #167)
> Note that mobile/android/chrome/content/WebAppRT.js, which implements the
> web runtime for running webapps in their own processes (and with their own
> profiles), also starts the debugger server if the webapp is in Android's
> "debug" mode, regardless of the value of the remote-enabled pref.
> 
> But it currently does so by setting the remote-enabled pref, which always
> seemed kinda hacky (especially since it persists across sessions, even
> though "debug" mode could change in a new session if the user updates the
> app with a non-debug version).

Setting the pref is now a "clean" way to start the server.
 
> Perhaps WebappRT should switch to calling nsIDebuggerServerController.start
> directly?

Switching the pref does that for you.

> Also note that WebappRT picks a random free port.  It'd be nice if the
> ability to do that was built into the controller.

We have the code in the connection API. It's pretty easy to use that in the controller.
Followup.
These patches have badly bitrotted while I was away. Rebasing…
Attached patch Part 1: devtools & browser (obsolete) — Splinter Review
Attachment #8384708 - Attachment is obsolete: true
Attachment #8398477 - Flags: review+
Attached patch Part 2: b2g (obsolete) — Splinter Review
Attachment #8384710 - Attachment is obsolete: true
Attachment #8384710 - Flags: review?(fabrice)
Attachment #8398478 - Flags: review?(fabrice)
Attached patch Part 3: metro (obsolete) — Splinter Review
Attachment #8383747 - Attachment is obsolete: true
Attachment #8384712 - Attachment is obsolete: true
Attachment #8398479 - Flags: review+
Attached patch Part 4: Fennec (obsolete) — Splinter Review
Attachment #8384713 - Attachment is obsolete: true
Attachment #8398480 - Flags: review+
This really needs to land. It's blocking many bugs and bitrots frequently.
 
The rebasing was pretty rough. Code in shell.js has changed quite a bit. Alex, I won't be against a quick re-review of part2.

Fabrice, can you please look at this soon? Check comment 146 for more details.
Flags: needinfo?(poirot.alex)
Comment on attachment 8398478 [details] [diff] [review]
Part 2: b2g

Damn, a .orig has been added to the patch. Updating…
Attachment #8398478 - Attachment is obsolete: true
Attachment #8398478 - Flags: review?(fabrice)
Attached patch Part 1: devtools & browser (obsolete) — Splinter Review
Attachment #8398477 - Attachment is obsolete: true
Attachment #8398493 - Flags: review+
Attached patch Part 2: b2g (obsolete) — Splinter Review
Attachment #8398494 - Flags: review?(fabrice)
Attached patch Part 3: metro (obsolete) — Splinter Review
Attachment #8398479 - Attachment is obsolete: true
Attachment #8398495 - Flags: review+
Attached patch Part 4: Fennec (obsolete) — Splinter Review
Attachment #8398480 - Attachment is obsolete: true
Attachment #8398496 - Flags: review+
Attachment #8398495 - Attachment description: 3.diff → Part 3: metro
(In reply to Paul Rouget [:paul] from comment #168)
> Setting the pref is now a "clean" way to start the server.

The primary problem with the pref is that it persists across sessions.  The Android runtime wants to enable debugging for the given session only.  Does the pref enable it to do so (without saving the original value of the pref on startup, observing manual changes to make sure it doesn't override them, and restoring the original value on shutdown if no manual changes have been made)?
(In reply to Myk Melez [:myk] [@mykmelez] from comment #181)
> (In reply to Paul Rouget [:paul] from comment #168)
> > Setting the pref is now a "clean" way to start the server.
> 
> The primary problem with the pref is that it persists across sessions.  The
> Android runtime wants to enable debugging for the given session only.  Does
> the pref enable it to do so (without saving the original value of the pref
> on startup, observing manual changes to make sure it doesn't override them,
> and restoring the original value on shutdown if no manual changes have been
> made)?

So in this case, you indeed need to use nsIDebuggerServerController.start or use the command line (./fennec -start-debugger-server 6000).
Paul, I'm away next week so if you are really in a hurry you'll have to find another reviewer. Please make sure that this doesn't break the "adb only"/"adb and devtools" if you haven't tested that yet.
Comment on attachment 8398494 [details] [diff] [review]
Part 2: b2g

Rebasing needed. Working on it.
Attachment #8398494 - Flags: review?(fabrice)
Attachment #8398493 - Attachment is obsolete: true
Attachment #8398494 - Attachment is obsolete: true
Attachment #8398495 - Attachment is obsolete: true
Attachment #8398496 - Attachment is obsolete: true
Attached patch Part 1: devtools & browser (obsolete) — Splinter Review
Attachment #8404025 - Flags: review+
Attached patch Part 2: b2g (obsolete) — Splinter Review
Attachment #8404026 - Flags: review?(fabrice)
Attached patch Part 3: metro (obsolete) — Splinter Review
Attachment #8404028 - Flags: review+
Attached patch Part 4: Fennec (obsolete) — Splinter Review
Attachment #8404029 - Flags: review+
Fabrice, I rebased the patch queue. I didn't really take the time to re-check if I didn't break anything, but it should be ok. I had a hard time testing ADB life time (adb kept starting no matter what, I had to manually disable marionnette), but I think it works as expected now. But I'd feel more comfortable if you could test that on your side too. Thanks.
Attachment #8404026 - Attachment is patch: true
Comment on attachment 8404026 [details] [diff] [review]
Part 2: b2g

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

That looks very good overall, but I want to have a second look at the changes to DebuggerServerController.js

::: b2g/components/DebuggerServerController.js
@@ +59,5 @@
> +            // Keep the old setting to not break people that won't have updated
> +            // gaia and gecko.
> +            Services.prefs.setBoolPref("devtools.debugger.remote-enabled", value);
> +            Services.prefs.savePrefFile(null);
> +            break;

at this point in time, everyone should have updated properly. I would just remove all support for devtools.debugger.remote-enabled

@@ +171,5 @@
> +  _promptAnswer: false,
> +  _listenerAttached: false,
> +
> +  getShell: function() {
> +    let browser = Services.wm.getMostRecentWindow("navigator:browser");

We changed that recently, see bug 963239 for the correct way to do it.

::: b2g/components/moz.build
@@ +29,5 @@
>      ]
>  
>  EXTRA_PP_COMPONENTS += [
>      'B2GComponents.manifest',
> +    'DebuggerServerController.js',

this component does not need to be pre-processed. move it to EXTRA_COMPONENTS
Attachment #8404026 - Flags: review?(fabrice) → review-
Thank you Fabrice. I need to rebase the patches (debugger events changed), and I will address your comments.
Blocks: 998328
Attachment #8404025 - Attachment is obsolete: true
Attachment #8409036 - Flags: review+
Attachment #8404026 - Attachment is obsolete: true
Attachment #8409038 - Flags: review?(fabrice)
Attachment #8404028 - Attachment is obsolete: true
Attachment #8404029 - Attachment is obsolete: true
Attachment #8409041 - Flags: review+
Comment on attachment 8409038 [details] [diff] [review]
Unify debugger server startup: b2g code. r=fabrice r=ochameau

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

r=me - sorry for the overall lag!

::: b2g/components/DebuggerServerController.js
@@ +18,5 @@
> +
> +
> +XPCOMUtils.defineLazyGetter(this,
> +    "l10n",
> +    () => Services.strings.createBundle("chrome://global/locale/devtools/debugger.properties"));

That looks unused (which is good, we don't want gecko l10n in b2g/ in general!)
Attachment #8409038 - Flags: review?(fabrice) → review+
Addressed Fabrice's comment.
Attachment #8409038 - Attachment is obsolete: true
Attachment #8410245 - Flags: review+
You should drop this code in the b2g patch:
  http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/desktop.js#49
Flags: needinfo?(poirot.alex)
Attachment #8410396 - Attachment is obsolete: true
Attachment #8411354 - Flags: review?(dtownsend+bugmail)
Attachment #8410393 - Flags: review?(fabrice)
Attachment #8411354 - Flags: review?(dtownsend+bugmail) → review+
Attachment #8410393 - Flags: review?(fabrice) → review+
Attachment #8411354 - Attachment is obsolete: true
Attachment #8411456 - Flags: review+
Green.
Keywords: checkin-needed
dev-doc-needed:

We changed the behavior of the devtools.debugger.remote-enable pref on Firefox Desktop (didn't change for fennec, b2g and other products). In Firefox Desktop, like in any other product now, toggling this pref will stop/start the debugger server. Before, in Firefox Desktop, it was allowing connection **to** a remote server.

(FYI: we reset the pref to false the first time this code runs)

Also, a new command line option has been introduced:

> -start-debugger-server [<port or unix domain socket path>]

For example:

> ./firefox -start-debugger-server

will start the debugger server on port 6000 in Firefox Desktop. 6000 because it's the default value (see devtools.debugger.remote-port in about:config)

> ./b2g -start-debugger-server

will start the debugger server using a unix socket (/data/local/debugger-socket) in B2G. "/data/local/debugger-socket/" because remote-port is not set in about:config but devtools.debugger.unix-domain-socket is)

> ./anyProduct -start-debugger-server XXX

will start the debugger server port XXX (if XXX is a number) or unix socket XXX (if XXX is a string)
Keywords: dev-doc-needed
Attachment #8411960 - Flags: review?(poirot.alex)
Latest patch fixes the test failure.
Attachment #8411960 - Flags: review?(poirot.alex) → review?(fabrice)
Comment on attachment 8411960 [details] [diff] [review]
Unify debugger server startup: fix HUD r=fabrice

s/r=ochameau/r=fabrice

I'll fix the commit message later.
Attachment #8411960 - Attachment description: Unify debugger server startup: fix HUD r=ochameau → Unify debugger server startup: fix HUD r=fabrice
Attachment #8411960 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
Optimizer noticed that main.js gets loaded at startup no mater what. It's easy to fix. I'll add a fix to this patch queue.
Flags: needinfo?(paul)
Messed up with gaia.json. Re-pushing: https://tbpl.mozilla.org/?tree=Try&rev=ba2a0f0ee93e
Comment on attachment 8415154 [details] [diff] [review]
Unify debugger server startup: don't load dbg-server.jsm at startup. r=ochameau

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

That's weird.  DevToolsAppStartup.hasController depends on DebuggerServer internals. (the fact that Debugger.controller will be defined if this xpcom exists)
I would rather make DebuggerServer.controller return null instead of throwing.
Comment on attachment 8415154 [details] [diff] [review]
Unify debugger server startup: don't load dbg-server.jsm at startup. r=ochameau

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

discussed offline. So if that's to prevent loading DebuggerServer, that makes more sense and justify such practice.
Can you please add a comment in hasController?
Attachment #8415154 - Flags: review?(poirot.alex) → review+
load debugger server in executeScript:

https://tbpl.mozilla.org/?tree=Try&rev=fe33ae605a3a
Running into a systematic crash with test_webapps_actor.html
Attachment #8409039 - Attachment is obsolete: true
Attachment #8409041 - Attachment is obsolete: true
Attachment #8410245 - Attachment is obsolete: true
Attachment #8410393 - Attachment is obsolete: true
Attachment #8411456 - Attachment is obsolete: true
Attachment #8411960 - Attachment is obsolete: true
Attachment #8415154 - Attachment is obsolete: true
Attachment #8409036 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=73df31305358

The last patch changes the way the controller is linked to the debugger server. Because we want the controller to be loaded at startup (to add app-specific listeners) but not the debugger server, I needed to make the controller an independent object. Now it's a service, that user the global debugger server. It can be used as an single instance too, and can be linked to a custom debugger server (see IDL changes).

Waiting for a green try before asking for a review.
Attachment #8415793 - Flags: review?(poirot.alex)
Comment on attachment 8416361 [details] [review]
Unify debugger server startup: dbg-server.jsm should be imported in tests. r=ochameau

Alex, can you review this or should I ask Vivien?
Attachment #8416361 - Attachment is patch: true
Attachment #8416361 - Attachment mime type: text/x-github-pull-request → text/plain
Attachment #8416361 - Flags: review?(poirot.alex)
Attachment #8416361 - Attachment is patch: false
Attachment #8416361 - Attachment mime type: text/plain → text/x-github-pull-request
Comment on attachment 8416361 [details] [review]
Unify debugger server startup: dbg-server.jsm should be imported in tests. r=ochameau

Stealing the review from Alex because the change looks harmless. With it, the tests still pass (on Travis and locally) even without the rest of the patch applied, so I think it'd be a good idea to land this as soon as possible to prevent transient breakage when the other patches start landing.
Attachment #8416361 - Flags: review?(poirot.alex) → review+
Comment on attachment 8416361 [details] [review]
Unify debugger server startup: dbg-server.jsm should be imported in tests. r=ochameau

Merged.
Attachment #8416361 - Flags: checkin+
Comment on attachment 8415793 [details] [diff] [review]
Unify debugger server startup: unlink debugger server and controller, make controller a service. r=ochameau

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

Do you have a git branch of this patch queue somewhere or a squashed patch that I can easily apply?

::: browser/devtools/framework/ToolboxProcess.jsm
@@ +127,5 @@
>        this.debuggerServer.on("connectionchange", this.emit.bind(this));
>      }
>  
> +    let controller = Cc["@mozilla.org/devtools/DebuggerServerController;1"]
> +                       .createInstance(Ci.nsIDebuggerServer);

If I follow the code correctly, I'm expecting this code to register listeners for debugger-server-started/stopped twice and then end up with duplicated notifications (one per controler instance).
It highlights the two indendant features of controler:
1/ start/stop a server 
2/ react to start/stop of a server
With the current implementation, only the first feature needs a reference to a DebuggerServer.

Also, the current patch queue introduce a controler attribute on DebuggerServer, but it is never used from DebugerServer/main.js. It is always used from external code like that: DebuggerServer.controler.start/stop (stop in only called from DevtoolsAppStartup.js).
In this patch, you are rewriting all these call sites in order to create a controller or get the service one, and call its start method directly. I think that's a good move, but that highlight the fact that controller owns a debuggerserver and not the opposite. (with this patch queue both own each other)
Given all that, I think we should:
- stop setting the controller attribute on DebuggerServer.
- instanciate only one controller as a service
- make it very explicit where and when we load dbg-server.jsm
  (As it instanciates a new DebuggerServer instance)
  (i.e. in you new code, avoid using defineLazyModuleGetter and do an explicit Cu.import at the precise place where we expect to instanciate a new server)
- accept a debugger server argument in controler.start/stop. If that can help we can make it optional and instanciate the shared one from dbg-server.jsm if none is given. But tbh, as it is a platform-independant behavior, I would rather keep this argument mandatory and just pass the debugger server we already have defined in each call site.

(BTW, speaking about controler.stop there is something weird. b2g and firefox call closeListener whereas all other platform are calling destroy. Destroy cleanup everything, whereas closeListener just shut down the server socket.)
Attachment #8415793 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot (:ochameau) from comment #235)
> Do you have a git branch of this patch queue somewhere or a squashed patch
> that I can easily apply?

To easily apply this patch queue, run this in your gecko-dev master branch:

> git checkout HEAD@{"2014-05-01 10:22 CEST"}
> git bz apply 942756

and then "y" to everything. For your convenience, I've done this, and hosted the patch queue at https://github.com/jankeromnes/gecko-dev/tree/unify
(In reply to Alexandre Poirot (:ochameau) from comment #235)
> Also, the current patch queue introduce a controler attribute on
> DebuggerServer

It has been removed in my latest patch. There's no controller property
in DebuggerServer.

> but it is never used from DebugerServer/main.js. It is
> always used from external code like that:
> DebuggerServer.controler.start/stop (stop in only called from
> DevtoolsAppStartup.js).

Not anymore.

> In this patch, you are rewriting all these call sites in order to create a
> controller or get the service one, and call its start method directly. I
> think that's a good move, but that highlight the fact that controller owns a
> debuggerserver and not the opposite. (with this patch queue both own each
> other)

No, debuggerServer doesn't own the controller anymore.

> Given all that, I think we should:
> - stop setting the controller attribute on DebuggerServer.
> - instanciate only one controller as a service

Done already.

> (BTW, speaking about controler.stop there is something weird. b2g and
> firefox call closeListener whereas all other platform are calling destroy.
> Destroy cleanup everything, whereas closeListener just shut down the server
> socket.)

There was a good reason to do that. But I can't remember what it was. I'll look at this.
Attachment #8418627 - Attachment is obsolete: true
Comment on attachment 8415793 [details] [diff] [review]
Unify debugger server startup: unlink debugger server and controller, make controller a service. r=ochameau

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

::: browser/devtools/framework/ToolboxProcess.jsm
@@ +127,5 @@
>        this.debuggerServer.on("connectionchange", this.emit.bind(this));
>      }
>  
> +    let controller = Cc["@mozilla.org/devtools/DebuggerServerController;1"]
> +                       .createInstance(Ci.nsIDebuggerServer);

There is a typo nsIDebuggerServer -> nsIDebuggerServerController.
(which breaks Browser toolbox)

Also sorry to insist, but here, by using createInstance instead of getService, you do create more than one controller instance.
It results in multiple alerts on browser debugger start/stop.
So yes, a part of my previous comment was wrong, there is no more DebuggerServer.controller with all patches applied.
But the rest is still valid.

Let me be more concrete about what I'm suggesting here:
let controller = Cc["@mozilla.org/devtools/DebuggerServerController;1"]
		.getService(Ci.nsIDebuggerServerController);
controller.start(Prefs.chromeDebuggingPort, this.debuggerServer);
Which needs modification to the idl:
  void start(in string portOrPath, [optional] in jsval server);
  void stop([optional] in jsval server);
And to the actual implementation:
  start: function (pathOrPath, debuggerServer) {
    if (!debuggerServer) {
      debuggerServer = this.globalDebuggerServer;
    }
    ...
  }
  // Same thing for stop

(You may also make this argument be first and mandatory and pass a debugger from call site)

::: browser/metro/components/DebuggerServerController.js
@@ +37,5 @@
> +  },
> +
> +  get debugger() {
> +    if (!this._debugger) {
> +      this._debugger = DebuggerServer;

Here and in other controllers, It would be clearer and more explicit to load the jsm from here instead of relying on lazy getter:
  if (!this._debugger) {
    this._debugger = Cu.import(..., {}).DebuggerServer;
  }
  return this._debugger;
(sounds like something that can be done in followup if that can help landing this patch)
No longer blocks: 916777
What's the story on this bug?  I was about to file a bug to have --start-debugger-server in the remote debugging server extension Philipp maintains, but I see it moves to toolkit in the combined patch - which is several months old now.
(In reply to Panos Astithas [:past] from comment #242)
> That part landed in bug 1119894.

Not according to mxr.mozilla.org/mozilla-central/search?string=start-debugger-server .  It's still in browser/, not toolkit/ .

So, again, what's the story on this bug?
(In reply to Alex Vincent [:WeirdAl] from comment #243)
> (In reply to Panos Astithas [:past] from comment #242)
> > That part landed in bug 1119894.
> 
> Not according to
> mxr.mozilla.org/mozilla-central/search?string=start-debugger-server .  It's
> still in browser/, not toolkit/ .
> 
> So, again, what's the story on this bug?

This work has essentially been abandoned for the moment.  I would not expect it to move forward soon.  This bug tried to change many things at once.  It would likely be much easier to focus on a specific point like the one you are mentioning in a new bug.
I gave up a long time ago. Sorry.
Assignee: paul → nobody
Product: Firefox → DevTools
Severity: normal → S3
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.