890348 - Start the devtools debugger during application startup and register observers for preference changes.

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Comment 1

[Philip Chee]

Attachment: Patch v1.0 WIP

Start the Debugger server on Suite startup and shut it down on Suite exit.
Based on MetroFox Bug 888079.

Also listens for these preferences:
"devtools.debugger.remote-enabled"
"devtools.debugger.remote-port";

Comment 2

[[e-mail]]

Comment on attachment 771400 [details] [diff] [review]
Patch v1.0 WIP

>+      case DEBUGGER_REMOTE_ENABLED:
This doesn't work, the topic is always nsPref:changed here.

>+        this.changeDebugPort(Services.prefs.getIntPref(aData));
Not sure it's worthwhile pushing the debug port from here, just let startDebugServer get the port all the time.

>+    // Start the debugger now so we can use it on the startup code as well.
>+    if (Services.prefs.getBoolPref(DEBUGGER_REMOTE_ENABLED))
>+      this.startDebugServer();
>+    Services.prefs.addObserver(DEBUGGER_REMOTE_ENABLED, this, true);
>+    Services.prefs.addObserver(DEBUGGER_REMOTE_PORT, this, true);
Not sure that you can actually get to debug startup, since the default value of the pref is false (user prefs haven't been loaded yet), and you still need to connect the remote debugger ;-)

>+  stopDebugServer: function()
>+  {
>+    DebuggerServer.destroy();
Not closeListener()?

>+  changeDebugPort:function(aPort)
Nit: space after :

>+    if (DebuggerServer.initialized) {
>+      this.stopDebugServer();
>+      this.startDebugServer(aPort);
>+    }
Alternatively you could just close and reopen the listener in the observer. (You already know that you don't need to init the debug server.)

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Better fix.

>>+      case DEBUGGER_REMOTE_ENABLED:
> This doesn't work, the topic is always nsPref:changed here.
Oops! Fixed.

>>+        this.changeDebugPort(Services.prefs.getIntPref(aData));
> Not sure it's worthwhile pushing the debug port from here, just let startDebugServer get the port all the time.
Fixed.

>>+    // Start the debugger now so we can use it on the startup code as well.
> Not sure that you can actually get to debug startup, since the default value of 
> the pref is false (user prefs haven't been loaded yet), and you still need to 
> connect the remote debugger ;-)
I've moved the startup to _onProfileStartup().

>>+  stopDebugServer: function()
>>+  {
>>+    DebuggerServer.destroy();
> Not closeListener()?
I've split the logic in to two pieces:

1. In _onProfileShutdown() I call dbgShutdown() which calls DebuggerServer.destroy() to do an orderly shutdown of the debugger.

2. When the debugger port changes I close the listener and then reopen the listener on the new port.

>>+  changeDebugPort:function(aPort)
> Nit: space after :
Fixed,

>>+    if (DebuggerServer.initialized) {
>>+      this.stopDebugServer();
>>+      this.startDebugServer(aPort);
>>+    }
> Alternatively you could just close and reopen the listener in the observer.
> (You already know that you don't need to init the debug server.)
Fixed. I'm now using a helper method dbgRestart() to do this.

Comment 4

[[e-mail]]

Comment on attachment 771638 [details] [diff] [review]
Patch v1.1 Better fix.

>+Components.utils.import("resource://gre/modules/devtools/dbg-server.jsm");
[We should make this lazy so we only load the debugger server if we're actually going to start it.]

>+    if (this.dbgIsEnabled)
>+      this.dbgStart();
Actually this is too late because the pref observer would have already started it by now (loading user preferences notifies all the relevant observers.)

On the other hand, it might make sense to move this back into app startup, so that (as a developer) you could change the default value in all.js and that might let you debug the profile manager. (But I would have to try to see whether that even works.)

>+    this.dbgShutdown();
Only Metro goes in for the full shutdown; Android and b2g close the listener if you flip the pref; Firefox doesn't even bother to close its listener ;-)

>+    var port = aPort || Services.prefs.getIntPref(DEBUGGER_REMOTE_PORT);
You never pass the port in...

>+  dbgRestart: function(aPort)
You never pass the port in...

>+    if (DebuggerServer.initialized) {
You only call this if the debugger is already running...

Comment 5

[[e-mail]]

I can't find anyone who can definitively answer my shutdown question...

Comment 6

[[e-mail]]

(In reply to comment #4)
> On the other hand, it might make sense to move this back into app startup,
> so that (as a developer) you could change the default value in all.js and
> that might let you debug the profile manager. (But I would have to try to
> see whether that even works.)

It doesn't, so you should be good to go with the pref listener added during app startup.

(In reply to comment #5)
> I can't find anyone who can definitively answer my shutdown question...

I managed to crash intermittently on shutdown even with the debugger destroyed, so destroying it isn't actually useful.

Comment 7

[Philip Chee]

Attachment: Patch v1.2 fix more nits.

>>+Components.utils.import("resource://gre/modules/devtools/dbg-server.jsm");
> [We should make this lazy so we only load the debugger server if we're actually 
> going to start it.]
Now using XPCOMUtils.defineLazyModuleGetter()

>>+    if (this.dbgIsEnabled)
>>+      this.dbgStart();
> Actually this is too late because the pref observer would have already started 
> it by now (loading user preferences notifies all the relevant observers.)

> On the other hand, it might make sense to move this back into app startup, so 
> that (as a developer) you could change the default value in all.js and that 
> might let you debug the profile manager. (But I would have to try to see 
> whether that even works.)
As discussed over IRC app startup will load the preferences which will in turn trigger the pref observer. So we are good to go without explicitly starting the debugServer.

>>+    this.dbgShutdown();
> Only Metro goes in for the full shutdown; Android and b2g close the listener if 
> you flip the pref; Firefox doesn't even bother to close its listener ;-)
Fixed. Removed shutdown code.

> You never pass the port in...
> You never pass the port in...
Fixed.

>>+    if (DebuggerServer.initialized) {
> You only call this if the debugger is already running...
Removed "initialized" check.

> I managed to crash intermittently on shutdown even with the debugger
> destroyed, so destroying it isn't actually useful.
Removed shutdown code.

Comment 8

[[e-mail]]

Comment on attachment 775150 [details] [diff] [review]
Patch v1.2 fix more nits.

A couple of nits that I noticed:

>+  dbgStop: function()
>+  {
>+    if (DebuggerServer.initialized)
The debugger server should already have been initialised when it was previously started...

>+  /**
>+   * If the server is not on, port changes have nothing to effect.
>+   * The new value will be picked up if the server is started.
>+   */
This comment looks like it's in the wrong place. Also effect is a noun, you're thinking of the verb affect.

Comment 9

[Philip Chee]

>>+  dbgStop: function()
>>+  {
>>+    if (DebuggerServer.initialized)
> The debugger server should already have been initialised when it was previously started...
I think it's possible that on startup the debugger may not have been started.

>>+  /**
>>+   * If the server is not on, port changes have nothing to effect.
>>+   * The new value will be picked up if the server is started.
>>+   */
> This comment looks like it's in the wrong place. Also effect is a noun, you're thinking of the verb affect.
I've moved this to the observe section.

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/3bc81f0c4a47