Test about:devtools-toolbox?target feature

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Developer Tools: Framework
--
enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

a year ago
So far, no test is covering this codepath.
It allows loading devtools within an iframe and abitrary debug any iframe.

 <body>
   <iframe id="devtools" />
   <iframe id="debuggee" src="mozilla.org" />
 </body>

 devtools = document.getElementById("devtools");
 devtools.target = document.getElementById("debuggee");
 devtools.setAttribute("src", "about:devtools-toolbox?target");

This will load devtools debugging the mozilla.org frame.
(Assignee)

Updated

a year ago
Blocks: 1233463
(Assignee)

Comment 1

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a6563ba24c3
Severity: normal → enhancement
(Assignee)

Comment 2

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6237379cbe91
(Assignee)

Comment 3

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=798819784a44
(Assignee)

Comment 4

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe977b85cb0e
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d223a350b068
(Assignee)

Comment 6

a year ago
Created attachment 8768331 [details] [diff] [review]
the test - patch v1

Here is the test, but some fixes are required to make it pass.
Attachment #8768331 - Flags: review?(jryans)
(Assignee)

Comment 7

a year ago
Created attachment 8768333 [details] [diff] [review]
Support TabActor.getTab against mozbrowser iframes - v1

Like this. Currently getTab can only target <xul:browser> frames
that we retrieve from gBrowser. This is unecessary limiting our capabilities here.

In this patch, I can't use getOuterWindowWithId for tabs running in the content process
as this method only works for document from the same process.
Attachment #8768333 - Flags: review?(jryans)
(Assignee)

Comment 8

a year ago
Created attachment 8768334 [details] [diff] [review]
Emit TabActor.tabDetached whenever the actor is detached - v1

We do not call tabDetached even when detach is called, which is unfortunate...
It is only emitted if exit() is called, which appear to not be the case here.
Attachment #8768334 - Flags: review?(jryans)
Comment on attachment 8768331 [details] [diff] [review]
the test - patch v1

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

::: devtools/client/framework/test/browser_toolbox_target.js
@@ +11,5 @@
> +  // iframe loads the document to debug
> +  let iframe = document.createElement("iframe");
> +  document.documentElement.appendChild(iframe);
> +
> +  yield new Promise(done => {

Can you use a "once" helper or some kind on document loaded helper here?

@@ +32,5 @@
> +  toolboxIframe.target = iframe;
> +
> +  let onToolboxReady = gDevTools.once("toolbox-ready");
> +
> +  yield new Promise(done => {

Can you use a "once" helper or some kind on document loaded helper here?

@@ +52,5 @@
> +  let onToolboxDestroyed = gDevTools.once("toolbox-destroyed");
> +  let onTabActorDetached = new Promise(done => {
> +    let client = toolbox.target.client;
> +    client.addListener("tabDetached", function onDetached() {
> +      dump("received tabdetached\n");

info or remove
Attachment #8768331 - Flags: review?(jryans) → review+
Comment on attachment 8768334 [details] [diff] [review]
Emit TabActor.tabDetached whenever the actor is detached - v1

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

I think this makes sense... it's interesting that the onDetach path also sends its own "detached" message (separate from "tabDetached") as well.
Attachment #8768334 - Flags: review?(jryans) → review+
Comment on attachment 8768333 [details] [diff] [review]
Support TabActor.getTab against mozbrowser iframes - v1

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

Hmm, how does this compare to the WindowActor I still need to land from Positron (bug 1268134)?  Would that work for your case?
Attachment #8768333 - Flags: review?(jryans)
(Assignee)

Comment 12

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Comment on attachment 8768333 [details] [diff] [review]
> Support TabActor.getTab against mozbrowser iframes - v1
>
> Hmm, how does this compare to the WindowActor I still need to land from
> Positron (bug 1268134)?  Would that work for your case?

I must admit the differences are suble if you look at the implementation.

I'm querying tab actors for "browser elements". It is not necessarely a xul:browser referenced by gBrowser internals, but any xul:browser or html:iframe. Now how do we draw the line between a tab and a window is tenuous.

But I think I'll hit a roadblock if I start using WindowActor once I want to connect to <iframe remote>, where, it is hard to retrieve a outerWindowID for them. (xul:browser uses frame scripts to retrieve that asynchronously from the content process)
(Assignee)

Comment 13

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> Comment on attachment 8768334 [details] [diff] [review]
> Emit TabActor.tabDetached whenever the actor is detached - v1
> 
> Review of attachment 8768334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this makes sense... it's interesting that the onDetach path also
> sends its own "detached" message (separate from "tabDetached") as well.

"detached" is the response to the TabActor.detach request,
whereas "tabDetached" is an event that anyone can listen from.
(Assignee)

Comment 14

a year ago
Created attachment 8769619 [details] [diff] [review]
the test - patch v2

No more "new Promise()", all replaced with once().
Attachment #8769619 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8768331 - Attachment is obsolete: true
(Assignee)

Comment 15

a year ago
See comment 12. I wish I could close this bug before ptos ;)
Flags: needinfo?(jryans)
Comment on attachment 8768333 [details] [diff] [review]
Support TabActor.getTab against mozbrowser iframes - v1

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

I think I am okay with this in concept...  as you said, the line between tab vs. window is pretty blurry.

However, I want to be sure we don't inadvertently expose access to chrome windows here.  (I hope you'll agree that `getTab` is meant to be for content windows only.)

The case I am worried about is something like: about:devtools-toolbox?type=tab&id=3
(where the ID is the outerWindowID for some chrome window like the main browser window)

The current version does block it sort of, since the containerElement is null.  Could you add a stronger check to ensure the window from `Services.wm.getOuterWindowWithId(outerWindowID)` is not a ChromeWindow?
Attachment #8768333 - Flags: feedback+
Flags: needinfo?(jryans)
(Assignee)

Comment 17

a year ago
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
>
> However, I want to be sure we don't inadvertently expose access to chrome
> windows here.  (I hope you'll agree that `getTab` is meant to be for content
> windows only.)

Not necessarely only content as we can debug about:addons and such, which are somewhat chrome, but yes not top level windows.
We can restrict to stuff living within a frame which shouldn't be nsIDOMChromeWindow.
(Assignee)

Comment 18

a year ago
Created attachment 8770578 [details] [diff] [review]
Support TabActor.getTab against mozbrowser iframes - v2

Added a nsIDOMChromeWindow check.
Attachment #8770578 - Flags: review?(jryans)
(Assignee)

Updated

a year ago
Attachment #8768333 - Attachment is obsolete: true
(Assignee)

Comment 19

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f655eb3cee55
Comment on attachment 8770578 [details] [diff] [review]
Support TabActor.getTab against mozbrowser iframes - v2

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

Thanks, looks good to me!
Attachment #8770578 - Flags: review?(jryans) → review+
(Assignee)

Comment 21

a year ago
Created attachment 8770737 [details] [diff] [review]
Support TabActor.getTab against mozbrowser iframes - v3

Fix browser_target_from_url.js to use a real invalid ID (1000)
instead of an existing DOMChromeWindowID (1)
Attachment #8770737 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8770578 - Attachment is obsolete: true
(Assignee)

Comment 22

a year ago
Created attachment 8770738 [details] [diff] [review]
the test - patch v3

Switch to use <xul:browser type="content"> instead of <xul:iframe>
that to ensure the document we load, a data: URI is not a DOMChromeWindow.

I tried using a <html:iframe mozbrowser> but it doesn't work because of a _browserElementParents object being leaked
to the test globals. I'll try to followup on this. Also we can't target arbitrary remote browsers,
my other patches only address non-remote browser usages.
Attachment #8770738 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8769619 - Attachment is obsolete: true
(Assignee)

Comment 23

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=274ace0a7faf
(Assignee)

Comment 24

a year ago
https://hg.mozilla.org/integration/fx-team/rev/e8071e322140e6ec07a1dc2b2f4ae121b9ae74c4
Bug 1281726 - Support TabActor.getTab against mozbrowser iframes. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/867ead052c7457d5f5504b7f344fd9c5b1bcca53
Bug 1281726 - Emit TabActor.tabDetached whenever the actor is detached. r=jryans

https://hg.mozilla.org/integration/fx-team/rev/7345b89fb14fb8a05513b4dced4b6c6796267dda
Bug 1281726 - Test about:devtools-toolbox?target r=jryans

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8071e322140
https://hg.mozilla.org/mozilla-central/rev/867ead052c74
https://hg.mozilla.org/mozilla-central/rev/7345b89fb14f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.