Closed Bug 910184 Opened 7 years ago Closed 7 years ago

Browser Debugger: Use separate server instance

Categories

(DevTools :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(1 file, 1 obsolete file)

Change the Browser Debugger to launch a separate instance of the DebuggerServer, to enable things like debugging the tools server itself (actors, etc.).
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.
Attachment #796650 - Flags: review?(vporof) → review+
Dave said this should be ok if it's green on try. Do a full run to be sure.
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.
Attachment #796650 - Attachment is obsolete: true
Attachment #796724 - Flags: review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f0ce14507a78
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.