910184 - Browser Debugger: Use separate server instance

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Change the Browser Debugger to launch a separate instance of the DebuggerServer, to enable things like debugging the tools server itself (actors, etc.).

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Use a separate server for Browser Debugger

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 796650 [details] [diff] [review]
Use a separate server for Browser Debugger

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

This looks like it should do the trick. What do you think about testing this? Let's consult with dcamp about it.

::: toolkit/devtools/server/main.js
@@ +9,5 @@
>   * Toolkit glue for the remote debugging protocol, loaded into the
>   * debugging global.
>   */
>  
> +const { Ci, Cc, CC, Cu, Cr, components: Components } = require("chrome");

It doesn't look like you're using lowercase components anywhere. Are you sure you need to destructure it at all?

@@ +27,5 @@
> +    let loader = Cc["@mozilla.org/moz/jssubscript-loader;1"]
> +      .getService(Ci.mozIJSSubScriptLoader);
> +    loader.loadSubScript(aURL, this);
> +  } catch(e) {
> +    dump("Error loading: " + aURL + ": " + e + " - " + e.stack + "\n");

Might want to also Cu.reportError.

::: toolkit/devtools/server/transport.js
@@ +191,5 @@
>  
>      dumpn("Got: " + packet);
>      let self = this;
>      Services.tm.currentThread.dispatch(makeInfallible(function() {
> +      if (self.hooks) {

Please add a comment explaining why self.hooks might be falsy.

Comment 3

[Victor Porof [:vporof][:vp]]

Dave said this should be ok if it's green on try. Do a full run to be sure.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Use a separate server for Browser Debugger (v2)

Carry over Victor's r+ from attachment 796650 [details] [diff] [review].

(In reply to Victor Porof [:vp] from comment #2)
> Comment on attachment 796650 [details] [diff] [review]
> Use a separate server for Browser Debugger
> 
> ::: toolkit/devtools/server/main.js
> @@ +9,5 @@
> >   * Toolkit glue for the remote debugging protocol, loaded into the
> >   * debugging global.
> >   */
> >  
> > +const { Ci, Cc, CC, Cu, Cr, components: Components } = require("chrome");
> 
> It doesn't look like you're using lowercase components anywhere. Are you
> sure you need to destructure it at all?

You are right that it is not used in this file, but it also controls whether Components is accessible to subsequent files pulled in by the loaded (since "server/main" is the main file of the loader in this case).  This could likely be improved in a subsequent change by updating many files to explicitly require it when necessary.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=583bbe47833a

Comment 6

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/f0ce14507a78

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f0ce14507a78