Closed Bug 1411565 Opened 4 years ago Closed 4 years ago

Allow about debugging to connect to a remote instance of Firefox with URL parameters

Categories

(DevTools :: about:debugging, enhancement, P3)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

This bug is about adding basic support for :

about:debugging?host=localhost&port=6080 

which should open about:debugging tab allowing to debug a remote instance of Firefox listening on localhost:6080.

This is a requirement for Bug 1243329, but we should be able to land this feature on its own with no UI changes.
This first patch allows to connect to a remote debugger server by manually specifying port & host as URL parameters.

about:debugging will list all the tabs/addons/workers for this debugger server, and they can all be debugged.

A quick list of what I would like to address in follow ups:
- addon debugging & multi e10s preferences should be read and set on the remote instance
- support other connection types (remote devices)
- add a connect UI
- show a status bar when connected to a remote instance
- show remote instance details (similar WebIDE feature)
- allow to view/edit preferences on the remote instance (similar WebIDE feature)
- list and debug processes (in the tab category?) (similar Connect page feature)
- handle connection errors by showing an error template
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8921964 [details]
Bug 1411565 - about:debugging connect to remote runtime using url parameters;

rebase seems to have broken a few things. Clearing r?
Attachment #8921964 - Flags: review?(poirot.alex)
Alex: In the first patch, I reused code from the existing connect page to show the addon debugging toolbox. But I later realized that it could be cleaner to have the same experience when doing local and remote addon debugging, so I added two changesets that aim at reusing the BrowserToolboxProcess for remote addon debugging as well as for local debugging.

If you're ok with this approach I can fold all of that in my first changeset.

Anyway, please have a look at all 3 changesets and let me know what you think.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #9)
> Alex: In the first patch, I reused code from the existing connect page to
> show the addon debugging toolbox. But I later realized that it could be
> cleaner to have the same experience when doing local and remote addon
> debugging, so I added two changesets that aim at reusing the
> BrowserToolboxProcess for remote addon debugging as well as for local
> debugging.
> 
> If you're ok with this approach I can fold all of that in my first changeset.
> 
> Anyway, please have a look at all 3 changesets and let me know what you
> think.

Codewise, it looks cool. It is handy to reuse the same codepath for local and remote.
But you don't have to run the toolbox in another process for remote debugging (addons).
From user perspective, when debugging remote things, only the addons will run
a toolbox in another process. They may not see the difference visually,
but in term of opening time, they will. Also in term of addons and preferences,
not everything is shared.

So I would say, keep running only what really requires running in the browser toolbox process.
Comment on attachment 8921964 [details]
Bug 1411565 - about:debugging connect to remote runtime using url parameters;

https://reviewboard.mozilla.org/r/192930/#review207398

Thanks for looking into this, it looks like a very good first step!

::: devtools/client/aboutdebugging/components/tabs/Target.js:41
(Diff revision 4)
>    debug() {
> -    let { target } = this.props;
> -    window.open("about:devtools-toolbox?type=tab&id=" + target.outerWindowID);
> +    let { target, connect } = this.props;
> +    let url = "about:devtools-toolbox?type=tab&id=" + target.outerWindowID;
> +    if (connect.type == "REMOTE") {
> +      let {host, port} = connect.params;
> +      url += `&host=${host}&port=${port}`;

You should use `encodeURIComponent` for `host` and `port`

::: devtools/client/aboutdebugging/initializer.js:66
(Diff revision 4)
> -    DebuggerServer.registerAllActors();
>  
> -    this.client = new DebuggerClient(DebuggerServer.connectPipe());
> +    if (client !== null) {
> +      this.client = client;
> +      await this.client.connect();
> +    }

Can `client` be null?
Looking at `createClient`, it doesn't seem to be possible.
Will anything in about:debugging work if there is no client? Shouldn't we display an error instead?

::: devtools/client/aboutdebugging/modules/connect.js:1
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

You should share this code with this existing one:
https://searchfox.org/mozilla-central/source/devtools/client/framework/target-from-url.js#125-142
May be create `devtools/client/framework/client-from-url.js`?

Because `target-from-url` is going to be used for about:devtools-toolbox urls and should support the exact same attributes as about:debugging for defining the client.
Attachment #8921964 - Flags: review?(poirot.alex)
Attachment #8930154 - Attachment is obsolete: true
Attachment #8930155 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #10)
> 
> Codewise, it looks cool. It is handy to reuse the same codepath for local
> and remote.
> But you don't have to run the toolbox in another process for remote
> debugging (addons).
> From user perspective, when debugging remote things, only the addons will run
> a toolbox in another process. They may not see the difference visually,
> but in term of opening time, they will. Also in term of addons and
> preferences,
> not everything is shared.
> 
> So I would say, keep running only what really requires running in the
> browser toolbox process.

Thanks for the feedback! I removed it for now. 

 (In reply to Alexandre Poirot [:ochameau] from comment #11)
> Comment on attachment 8921964 [details]
> Bug 1411565 - about:debugging connect to remote runtime using url parameters;
> 
> https://reviewboard.mozilla.org/r/192930/#review207398
> 
> Thanks for looking into this, it looks like a very good first step!
> 
> ::: devtools/client/aboutdebugging/components/tabs/Target.js:41
> (Diff revision 4)
> >    debug() {
> > -    let { target } = this.props;
> > -    window.open("about:devtools-toolbox?type=tab&id=" + target.outerWindowID);
> > +    let { target, connect } = this.props;
> > +    let url = "about:devtools-toolbox?type=tab&id=" + target.outerWindowID;
> > +    if (connect.type == "REMOTE") {
> > +      let {host, port} = connect.params;
> > +      url += `&host=${host}&port=${port}`;
> 
> You should use `encodeURIComponent` for `host` and `port`
> 
> ::: devtools/client/aboutdebugging/initializer.js:66
> (Diff revision 4)
> > -    DebuggerServer.registerAllActors();
> >  
> > -    this.client = new DebuggerClient(DebuggerServer.connectPipe());
> > +    if (client !== null) {
> > +      this.client = client;
> > +      await this.client.connect();
> > +    }
> 
> Can `client` be null?
> Looking at `createClient`, it doesn't seem to be possible.
> Will anything in about:debugging work if there is no client? Shouldn't we
> display an error instead?
> 

Not at the moment, so I removed it. I would like to handle connection errors and timeouts by showing an "error" page, but I will do that in another bug.

> ::: devtools/client/aboutdebugging/modules/connect.js:1
> (Diff revision 4)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> You should share this code with this existing one:
> https://searchfox.org/mozilla-central/source/devtools/client/framework/
> target-from-url.js#125-142
> May be create `devtools/client/framework/client-from-url.js`?
> 
> Because `target-from-url` is going to be used for about:devtools-toolbox
> urls and should support the exact same attributes as about:debugging for
> defining the client.

Good point, just exposed a clientFromURL helper in target-from-url. We might want to rename/repurpose the file later (url-connect.js?)
Comment on attachment 8921964 [details]
Bug 1411565 - about:debugging connect to remote runtime using url parameters;

https://reviewboard.mozilla.org/r/192930/#review209410

Thanks, it works great overall.

You may hide the load temporary add-on if connected to a remote target as it doesn't work:
  https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/components/addons/Controls.js#112-115

When debugging an add-on, the console is broken when enterring a text with this exception:
onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'autocomplete: TypeError: argument is not a global object\nStack: WCA_onAutocomplete@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:1003:21\nonPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1743:15\nDebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:483:11\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nLine: 1003, column: 21"}
Stack: onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/debugger-client.js:856:9
_onJSONObjectReady/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:483:11
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
Line: 856, column: 9

I'm not sure it is related to your patch? Was it working with the browser toolbox patch?

::: commit-message-4985a:17
(Diff revision 5)
> +targets.
> +
> +Known limitations:
> +- preferences are read from the local Firefox instance (multiprocess, addon
> +  debugging etc...). At the moment the remote instance must be manually
> +  correctly configured

nit: I'm not sure this is correct sentence:
"must be manually correctly configured".

::: devtools/client/aboutdebugging/modules/addon.js:69
(Diff revision 5)
> +  };
> +
> +  let target = await TargetFactory.forRemoteTab(options);
> +
> +  let hostType = Toolbox.HostType.WINDOW;
> +  remoteAddonToolbox = await gDevTools.showToolbox(target, "webconsole", hostType);

You may pass `null` instead of `"webconsole"` to let the current default panel being displayed.

::: devtools/client/aboutdebugging/modules/connect.js:68
(Diff revision 5)
> + */
> +exports.createClient = async function () {
> +  let href = window.location.href;
> +  let url = new window.URL(href.replace("about:", "http://"));
> +
> +  let connect = createDescriptorFromURL(url);

This looks redundant with existing attributes that we have access to.
- connect.type ~ target.isRemote
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#349
- connect.params ~ client._transport.connectionSettings
  https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_target_from_url.js#117-120
  This isn't easy to access, but if that helps, we could put an helper getter on `client`.

Not a big deal to duplicate this, but just in case you keep adding things in this "connect" object.

::: devtools/client/framework/target-from-url.js:14
(Diff revision 5)
> -const { Task } = require("devtools/shared/task");
>  
>  /**
>   * Construct a Target for a given URL object having various query parameters:
>   *
> - * host:
> + * - host, port & ws: See the documentation for createClientFromURL

s/createClientFromURL/clientFromURL/

::: devtools/client/framework/target-from-url.js:125
(Diff revision 5)
> + * host:
> + *    {String} The hostname or IP address to connect to.
> + * port:
> + *    {Number} The TCP port to connect to, to use with `host` argument.
> + * ws:
> + *    {Boolean} If true, connect via websocket instread of regular TCP connection.

s/instread/instead/
Attachment #8921964 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Comment on attachment 8921964 [details]
> Bug 1411565 - about:debugging connect to remote runtime using url parameters;
> 
> https://reviewboard.mozilla.org/r/192930/#review209410
> 
> Thanks, it works great overall.
> 
> You may hide the load temporary add-on if connected to a remote target as it
> doesn't work:
>  
> https://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/
> components/addons/Controls.js#112-115
> 
> When debugging an add-on, the console is broken when enterring a text with
> this exception:
> onPacket threw an exception: Error: Server did not specify an actor,
> dropping packet: {"error":"unknownError","message":"error occurred while
> processing 'autocomplete: TypeError: argument is not a global object\nStack:
> WCA_onAutocomplete@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/server/actors/webconsole.js:1003:21\nonPacket@resource://
> gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/server/main.js:1743:15\nDebuggerTransport.prototype.
> _onJSONObjectReady/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/shared/transport/transport.js:483:11\nexports.
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nexports.
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nLine: 1003,
> column: 21"}
> Stack: onPacket@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/client/debugger-client.js:856:9
> _onJSONObjectReady/<@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/transport/transport.js:483:11
> exports.makeInfallible/<@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
> exports.makeInfallible/<@resource://devtools/shared/base-loader.js ->
> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
> Line: 856, column: 9
> 
> I'm not sure it is related to your patch? Was it working with the browser
> toolbox patch?

Ah, good catch. It's not working with the browser toolbox approach either. It's also not working when using the current connect screen. I will investigate that before landing. Which also brings another question: how will we test all of this?
(In reply to Julian Descottes [:jdescottes][:julian] from comment #15)
> Ah, good catch. It's not working with the browser toolbox approach either.
> It's also not working when using the current connect screen. I will
> investigate that before landing.

Feel free to proceed with this patch as it feel really unrelated.
Also, may be :rpl know something?

> Which also brings another question: how will we test all of this?

Ideally, it would be really great to be able to run most/all about:debugging tests against a fake remote target.
Instead of connecting via a connectPipe, we can start a debugger server and connect to it via a tcp port.
I think that would be a great start. It would cover this add-ons breakage, while it would not catch all the issues regarding our direct usage of prefs/AddonManager.

You can also look into ToolboxProcess and try launching firefox against an empty profile, but that sounds like a lot of work.
I cannot figure out why this is failing, so yes I will proceed without this and log a follow up. Here's a short summary of what I found so far:
- the webconsole actor cannot find a proper global object against which evaluate the code
- in a regular addon debugging, the webconsole actor is actually an AddonConsoleActor, but in remote debugging it's a basic WebConsoleActor

I need more time to investigate this, will do so in a fallow up.
Comment on attachment 8921964 [details]
Bug 1411565 - about:debugging connect to remote runtime using url parameters;

https://reviewboard.mozilla.org/r/192930/#review209410

> This looks redundant with existing attributes that we have access to.
> - connect.type ~ target.isRemote
>   https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#349
> - connect.params ~ client._transport.connectionSettings
>   https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_target_from_url.js#117-120
>   This isn't easy to access, but if that helps, we could put an helper getter on `client`.
> 
> Not a big deal to duplicate this, but just in case you keep adding things in this "connect" object.

Let's keep this connect object for now, and review again once we have more connect features in about:debugging.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95b95506cb1b
about:debugging connect to remote runtime using url parameters;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/95b95506cb1b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.