Closed Bug 1281726 Opened 8 years ago Closed 8 years ago

Test about:devtools-toolbox?target feature

Categories

(DevTools :: Framework, enhancement)

Product:
DevTools
Component:
Framework
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files, 4 obsolete files)

Emit TabActor.tabDetached whenever the actor is detached - v1
1.33 KB, patch
jryans
: review+
Details | Diff | Splinter Review
Support TabActor.getTab against mozbrowser iframes - v3
2.95 KB, patch
ochameau
: review+
Details | Diff | Splinter Review
the test - patch v3
3.68 KB, patch
ochameau
: review+
Details | Diff | Splinter Review 
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.
Blocks: 1233463
Severity: normal → enhancement
Attached patch the test - patch v1 (obsolete) — — Splinter Review 
Here is the test, but some fixes are required to make it pass.
Attachment #8768331 - Flags: review?(jryans)
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)
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)
(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)
(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.
Attached patch the test - patch v2 (obsolete) — — Splinter Review 
No more "new Promise()", all replaced with once().
Attachment #8769619 - Flags: review+
Attachment #8768331 - Attachment is obsolete: true
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+
(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.
Added a nsIDOMChromeWindow check.
Attachment #8770578 - Flags: review?(jryans)
Attachment #8768333 - Attachment is obsolete: true
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+
Fix browser_target_from_url.js to use a real invalid ID (1000)
instead of an existing DOMChromeWindowID (1)
Attachment #8770737 - Flags: review+
Attachment #8770578 - Attachment is obsolete: true
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+
Attachment #8769619 - Attachment is obsolete: true
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: