Support remote targets via about:devtools-toolbox urls

RESOLVED FIXED in Firefox 52

Status

P3
enhancement
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
Bug 1233463 introduced loading toolbox in a tab, but so far it only allows debugging local targets. We should allow debugging remote ones.
We need to come up with a new set of query parameters to define a target to debug.
(Assignee)

Updated

2 years ago
Depends on: 1243452
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
Note that it depends on bug 1243452.
That's because devtools/client/framework/test/browser_target_from_url.js has to wait for client.close()
correctly. It wasn't as it was assuming close returns a promise...
(Assignee)

Comment 3

2 years ago
Also, I pushed a clean branch to mozreview, you should see both bug patches in that branch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc675735ec18&selectedJob=26768735

Note that I also patch devtools/client/framework/test/browser_toolbox_sidebar*.js.
This is really weird. It seems to be more related to bug 1243452, which I depend on, but I verified and these tests pass if I only apply bug 1243452.
I had to ensure destroying the toolbox before finishing the tests otherwise it was trigerring a crash in String c++ API...

Comment 8

2 years ago
mozreview-review
Comment on attachment 8786978 [details]
Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters.

https://reviewboard.mozilla.org/r/75820/#review74562

Looks like a resonable step forward.  We might change up URL args later, but we should be free to do so for now at least (no one's depending on our URLs yet...).

I think the test to try a little harder to check that it's correct.

::: devtools/client/framework/test/browser_target_from_url.js:106
(Diff revision 2)
> +  info("Test remote process via TCP Connection");
> +
> +  let server = yield setupDebuggerServer(false);
> +
> +  let { port } = server.listener;
> +  let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port));

Shouldn't you check somehow that the host, port, and ws were actually used?

::: devtools/client/framework/test/browser_target_from_url.js:109
(Diff revision 2)
> +
> +  let { port } = server.listener;
> +  let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port));
> +  let topWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +  is(target.url, topWindow.location.href);
> +  is(target.isLocalTab, false);

These 4 lines are repeated a lot, maybe make a helper?
Attachment #8786978 - Flags: review?(jryans) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8787384 [details]
Bug 1299503 - Cleanup browser_toolbox_sidebar toolboxes at test end to prevent crashes.

https://reviewboard.mozilla.org/r/76164/#review74564
Attachment #8787384 - Flags: review?(jryans) → review+
(Assignee)

Comment 10

2 years ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> ::: devtools/client/framework/test/browser_target_from_url.js:106
> (Diff revision 2)
> > +  info("Test remote process via TCP Connection");
> > +
> > +  let server = yield setupDebuggerServer(false);
> > +
> > +  let { port } = server.listener;
> > +  let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port));
> 
> Shouldn't you check somehow that the host, port, and ws were actually used?
> 

I tried, but there is nothing on target.client/target.client._transport that reflect any information about the underlying connection we are using :/
It may help another work if we put some information about the client connection somewhere on client or transport. Should I do that?
Severity: normal → enhancement
Priority: -- → P3
(Assignee)

Comment 11

2 years ago
See comment 10.
Flags: needinfo?(jryans)
(In reply to Alexandre Poirot [:ochameau] from comment #10)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> > ::: devtools/client/framework/test/browser_target_from_url.js:106
> > (Diff revision 2)
> > > +  info("Test remote process via TCP Connection");
> > > +
> > > +  let server = yield setupDebuggerServer(false);
> > > +
> > > +  let { port } = server.listener;
> > > +  let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port));
> > 
> > Shouldn't you check somehow that the host, port, and ws were actually used?
> > 
> 
> I tried, but there is nothing on target.client/target.client._transport that
> reflect any information about the underlying connection we are using :/
> It may help another work if we put some information about the client
> connection somewhere on client or transport. Should I do that?

Ah, I should have remembered that!  I am not sure what the most natural place to put it is...  Maybe `_getTransport` in security/socket.js should copy the host, port, etc. onto the transport before returning it?
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8786978 [details]
Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters.

https://reviewboard.mozilla.org/r/75820/#review74562

> Shouldn't you check somehow that the host, port, and ws were actually used?

Just pushed a new changeset with such checks. I tweaked socket.js in order to do that and added some tests against this new code in dbg_socket.js xpcshell test.

> These 4 lines are repeated a lot, maybe make a helper?

I already had one so I used it!
(Assignee)

Comment 18

2 years ago
Comment on attachment 8786978 [details]
Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters.

Reasking for review as I modified socket.js and dbg_socket.js.
Attachment #8786978 - Flags: review+ → review?(jryans)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8786978 [details]
Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters.

https://reviewboard.mozilla.org/r/75820/#review82018

Thanks, seems like a good test improvement!

::: devtools/client/framework/target-from-url.js:106
(Diff revisions 2 - 4)
>  });
>  
>  function* createClient(params) {
>    let host = params.get("host");
>    let port = params.get("port");
> -  let webSocket = params.get("ws");
> +  let webSocket = Boolean(params.get("ws"));

Hmm, I think `!!params.get("ws")` would be more idiomatic and avoids creating a Boolean object.
Attachment #8786978 - Flags: review?(jryans) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f16f77530146
Support connecting to remote targets via about:devtools-toolbox query parameters. r=jryans
https://hg.mozilla.org/integration/autoland/rev/39e0e69febc5
Cleanup browser_toolbox_sidebar toolboxes at test end to prevent crashes. r=jryans

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f16f77530146
https://hg.mozilla.org/mozilla-central/rev/39e0e69febc5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.