Closed Bug 1392744 Opened 7 years ago Closed 7 years ago

Support opening a Browser Toolbox for more than one running Firefox instance

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

STR:

./mach run --profile /tmp/profile1
Open Browser Toolbox
./mach run --profile /tmp/profile2 --setpref devtools.debugger.chrome-debugging-port=6081
Open Browser Toolbox

Expected:
The second Browser Toolbox targets the second process

Actual:
The second Browser Toolbox targets the first process
Comment on attachment 8899954 [details]
Bug 1392744 - Pass the port along to the browser toolbox chrome uri;

https://reviewboard.mozilla.org/r/171282/#review176448

Nice, seems good to improve this.  I think we can make it even better! :)

It also seems related to bug 1204576, which wants a related feature for multiple versions to run BT at once.

::: devtools/client/framework/ToolboxProcess.jsm:150
(Diff revision 1)
> -    let chromeDebuggingPort =
> +    let {chromeDebuggingPort,chromeDebuggingWebSocket} = this._getConnectionInfo();
> -      Services.prefs.getIntPref("devtools.debugger.chrome-debugging-port");
> -    let chromeDebuggingWebSocket =
> -      Services.prefs.getBoolPref("devtools.debugger.chrome-debugging-websocket");
>      let listener = this.debuggerServer.createListener();
>      listener.portOrPath = chromeDebuggingPort;

If you set `portOrPath` to `-1`, it will look for a random open port.

Then check `listener.port` after calling `open()` to read the port you actually got and pass it to the client.

This seems like it will handle the multiple profile case nicely without manual port selection.  (Potentially we don't even need the `chrome-debugging-port` pref at all after that?  Maybe there's some use case where you want to force a known port, like debugging the connection.)
Attachment #8899954 - Flags: review?(jryans) → review-
Priority: -- → P3
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Comment on attachment 8899954 [details]
> Bug 1392744 - Pass the port along to the browser toolbox chrome uri;
> 
> https://reviewboard.mozilla.org/r/171282/#review176448
> 
> Nice, seems good to improve this.  I think we can make it even better! :)
> 
> It also seems related to bug 1204576, which wants a related feature for
> multiple versions to run BT at once.
> 
> ::: devtools/client/framework/ToolboxProcess.jsm:150
> (Diff revision 1)
> > -    let chromeDebuggingPort =
> > +    let {chromeDebuggingPort,chromeDebuggingWebSocket} = this._getConnectionInfo();
> > -      Services.prefs.getIntPref("devtools.debugger.chrome-debugging-port");
> > -    let chromeDebuggingWebSocket =
> > -      Services.prefs.getBoolPref("devtools.debugger.chrome-debugging-websocket");
> >      let listener = this.debuggerServer.createListener();
> >      listener.portOrPath = chromeDebuggingPort;
> 
> If you set `portOrPath` to `-1`, it will look for a random open port.
> 
> Then check `listener.port` after calling `open()` to read the port you
> actually got and pass it to the client.
> 
> This seems like it will handle the multiple profile case nicely without
> manual port selection.  (Potentially we don't even need the
> `chrome-debugging-port` pref at all after that?  Maybe there's some use case
> where you want to force a known port, like debugging the connection.)

I wasn't aware of that option, I'll have a look.  This does appear to be the only use of the port pref as well - in other cases 
like via CLI or GCLI you specify a port (unsure where the default port when just running `listen` in GCLI is coming from, though).
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Summary: Support opening a Browser Toolbox for more than one running Firefox instance, assuming they have different devtools.debugger.chrome-debugging-port prefs → Support opening a Browser Toolbox for more than one running Firefox instance
Comment on attachment 8899954 [details]
Bug 1392744 - Pass the port along to the browser toolbox chrome uri;

https://reviewboard.mozilla.org/r/171282/#review177476

Great, this seems reasonable to me! :)

::: devtools/client/framework/toolbox-process-window.js:52
(Diff revision 2)
> -    port: Prefs.chromeDebuggingPort,
> +    port: params.get("port"),
>      webSocket: Prefs.chromeDebuggingWebSocket,
>    });
>    gClient = new DebuggerClient(transport);
>    yield gClient.connect();
>    let addonID = getParameterByName("addonID");

Does the `addonID` still retrieve successfully?  I think the add-on debugging in about:debugging tests this path...?

Maybe change it to use `params.get` and remove `getParameterByName`?  (Not sure if `params.get` does the extra decoding step...)
Attachment #8899954 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Comment on attachment 8899954 [details]
> Bug 1392744 - Pass the port along to the browser toolbox chrome uri;
> 
> https://reviewboard.mozilla.org/r/171282/#review177476
> 
> Great, this seems reasonable to me! :)
> 
> ::: devtools/client/framework/toolbox-process-window.js:52
> (Diff revision 2)
> > -    port: Prefs.chromeDebuggingPort,
> > +    port: params.get("port"),
> >      webSocket: Prefs.chromeDebuggingWebSocket,
> >    });
> >    gClient = new DebuggerClient(transport);
> >    yield gClient.connect();
> >    let addonID = getParameterByName("addonID");
> 
> Does the `addonID` still retrieve successfully?  I think the add-on
> debugging in about:debugging tests this path...?
> 
> Maybe change it to use `params.get` and remove `getParameterByName`?  (Not
> sure if `params.get` does the extra decoding step...)

Addon debugging seems to work fine using params.get.  I'll go with the change and see what try thinks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c587c79c4252f72d0a0c50191e62fc4138ddd2e
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30182d13fb9d
Pass the port along to the browser toolbox chrome uri;r=jryans
https://hg.mozilla.org/mozilla-central/rev/30182d13fb9d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1397295
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: