1059308 - Show iframe selection button in the browser toolbox to allow inspecting other globals (windows, frames, popups, etc.)

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

It used to work while working on bug 977043, but addressing review comments and fixing test failures made it disappear from the browser toolbox.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I wonder how hard this is to fix...  The root actor (that Browser Toolbox uses) is not a tab actor. :/

It seems like the root actor has just enough methods for current functions to work without actually using the tab actor code.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Alex, would we need to make the root actor really be a tab actor I guess?  Or share the code somehow at least...  You were trying to tell it must be a tab actor, but it really doesn't seem that way to me right now...

Comment 3

[Alexandre Poirot [:ochameau]]

Ok ok it is not a tab actor, nor do we have any tab actor.
So it won't be easy. But I think we should be having a tab actor instead of all these hacks on the root actor!
May be it is somewhat easily doable do just spawn a tab actor and use it instead.

Comment 5

[Paul Rouget [:paul]]

Now that we have XBL inspection, this is the last piece that we need to make our devtools as good as DOMi.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Here is a wip patch that seems to work (Just played with console and inspector).

So the thing is as state in previous comment is that RootActor has to somehow expose TabActor
in order to support frame selection.
Then there is two options. Inherits from it or expose a new kind of tab actor.
I went for the second option as it sounds less hacky.
The root actor then becomes just an actor factory.
It is clearer when you think about the security restrictions on b2g,
where we have to prevent exposing chrome debugging.

Then the good things about that is that:
- "chrome" debugging is less special,
we go into the same codepath than tabs. (See the chrome:false argument for the target)
We can now attach and detach to the chrome actor.
- RootActor will stop duplicate many stuff from TabActor
(I have not cleaned it up in this patch, but we will be able to remove docShell, window, createStyleSheetActor, ...)
- once we support iframe switching, we can easily support other top level window.
I did that from ChromeTabActor, by watching for new top level window and registering their docshell.

The only drawback I see is about the lifetime of this actor.
It is related to the original top level window we attached to,
but we can easily ease that from TabActor.
Like for frames, we can switch to any still-alive document in the docshell list.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8538561 [details] [diff] [review]
patch v1

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

Cool!  It works nicely overall!

Various questions about the details.

I did notice that if I attach the Browser Toolbox, then start a Loop call (so there's a open, docked window showing my camera), and switch to the frame, the target browser appeared to hang and started printing many "stackwalker.cc:125: INFO: Couldn't load symbols for:" and "basic_code_modules.cc:88: INFO: No module at" messages.  Maybe with a debug build I would get something real.  Anyway, probably not directly related to your work.

::: browser/devtools/framework/toolbox-process-window.js
@@ +77,4 @@
>    let options = {
>      form: form,
>      client: gClient,
> +    chrome: chrome

I think the |chrome| key here is going to need a new name, as it's really complex now...

It's meaning seems hard to express in a single property... :/

It's pretty surprising to set |chrome: false| for a chrome window!

::: toolkit/devtools/server/actors/chrome.js
@@ +15,5 @@
> + *
> + * @param aConnection DebuggerServerConnection
> + *        The connection to the client.
> + */
> +function ChromeTabActor(aConnection)

I know it extends TabActor, but it's not really for a tab anymore, right?  So |ChromeWindowActor| maybe?

@@ +19,5 @@
> +function ChromeTabActor(aConnection)
> +{
> +  TabActor.call(this, aConnection);
> +
> +  let window = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);

What would this look like for multiple top-level windows?

Is there a new |ChromeTabActor| for each window?

@@ +27,5 @@
> +                 .getInterface(Ci.nsIDocShell)
> +  });
> +
> +  Services.ww.registerNotification(this);
> +}

The debugger has way less sources now.

Maybe you need to override |makeDebugger|[1]?

For example, if I open an in-tab toolbox in the target, I used to see the DevTools actors.  Many other things are missing too.

[1]: http://hg.mozilla.org/mozilla-central/annotate/9bb8b0b4daae/toolkit/devtools/server/actors/root.js#l110

::: toolkit/devtools/server/actors/root.js
@@ +491,5 @@
> +    // XXX: check for certified pref
> +
> +    // Create a ChromeTabActor for the current top level xul window
> +    let { ChromeTabActor } = require("devtools/server/actors/chrome");
> +    let actor = this.chromeActor = new ChromeTabActor(this.conn);

Maybe |this._chromeActor| so your memoized check above works?

@@ +508,4 @@
>  };
>  
>  RootActor.prototype.requestTypes = {
> +  "chromeActor": RootActor.prototype.onChromeActor,

Can't think of too many cases where "<FOO>Actor" is part of the actual server method, so this seems a bit odd to me.

The right answer will depend on what this would look like with multiple window support.  See other questions on this topic.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +2003,5 @@
>                                            Ci.nsIWebProgress.NOTIFY_STATE_WINDOW |
>                                            Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
>  
>      let handler = docShell.chromeEventHandler;
> +    if (!handler) {

Maybe it's better to test |isRootActor| here?

A similar thing happens elsewhere[1], maybe there should be a central method to get the right one.

[1]: http://hg.mozilla.org/mozilla-central/annotate/9bb8b0b4daae/toolkit/devtools/server/actors/highlighter.js#l264

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #7)
> Comment on attachment 8538561 [details] [diff] [review]
> I did notice that if I attach the Browser Toolbox, then start a Loop call
> (so there's a open, docked window showing my camera), and switch to the
> frame, the target browser appeared to hang and started printing many
> "stackwalker.cc:125: INFO: Couldn't load symbols for:" and
> "basic_code_modules.cc:88: INFO: No module at" messages.  Maybe with a debug
> build I would get something real.  Anyway, probably not directly related to
> your work.

I've seen it crashing firefox as well.

> 
> ::: browser/devtools/framework/toolbox-process-window.js
> @@ +77,4 @@
> >    let options = {
> >      form: form,
> >      client: gClient,
> > +    chrome: chrome
> 
> I think the |chrome| key here is going to need a new name, as it's really
> complex now...

Good luck with that. Grepping for chrome on mozilla-central is... hard :p

> 
> It's meaning seems hard to express in a single property... :/
> 
> It's pretty surprising to set |chrome: false| for a chrome window!

I can also manually attach before calling openToolbox,
but I think that's better like this, I prefer to have the same codepath than regular toolboxes.
I'm not sure we need anything special on client side, everything seems to be set on the server side.

> 
> ::: toolkit/devtools/server/actors/chrome.js
> @@ +15,5 @@
> > + *
> > + * @param aConnection DebuggerServerConnection
> > + *        The connection to the client.
> > + */
> > +function ChromeTabActor(aConnection)
> 
> I know it extends TabActor, but it's not really for a tab anymore, right? 
> So |ChromeWindowActor| maybe?

Yep, I would even go for ChromeActor as it isn't related to just one window.

> 
> @@ +19,5 @@
> > +function ChromeTabActor(aConnection)
> > +{
> > +  TabActor.call(this, aConnection);
> > +
> > +  let window = Services.wm.getMostRecentWindow(DebuggerServer.chromeWindowType);
> 
> What would this look like for multiple top-level windows?

That already works with multiple top level windows.
This one is just the default one.

> 
> Is there a new |ChromeTabActor| for each window?

No, just one actor, then we rely on frame switching to go to any other window/sub-window.

> 
> @@ +27,5 @@
> > +                 .getInterface(Ci.nsIDocShell)
> > +  });
> > +
> > +  Services.ww.registerNotification(this);
> > +}
> 
> The debugger has way less sources now.
> 
> Maybe you need to override |makeDebugger|[1]?

Oh yes, I forgot that!
One more thing to remove from RootActor.

> @@ +508,4 @@
> >  };
> >  
> >  RootActor.prototype.requestTypes = {
> > +  "chromeActor": RootActor.prototype.onChromeActor,
> 
> Can't think of too many cases where "<FOO>Actor" is part of the actual
> server method, so this seems a bit odd to me.
> 
> The right answer will depend on what this would look like with multiple
> window support.  See other questions on this topic.

If you follow my thoughts, you may imagine that I'm tempted to return such actor from listProcesses...
So that we get a more functional toolbox for content processes without having to handle any complex actor hierarchy, nor any complex UI to get an inspector working.
One codepath to rule that all...
There is just the filetime of the TabActor to tweak,
but as we inherit from it we should be able to customize its behavior.

> 
> ::: toolkit/devtools/server/actors/webbrowser.js
> @@ +2003,5 @@
> >                                            Ci.nsIWebProgress.NOTIFY_STATE_WINDOW |
> >                                            Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
> >  
> >      let handler = docShell.chromeEventHandler;
> > +    if (!handler) {
> 
> Maybe it's better to test |isRootActor| here?

Not really, that's more a platform issue where chromeEventHandler doesn't work for some docshell,
ideally it would always return something.

> 
> A similar thing happens elsewhere[1], maybe there should be a central method
> to get the right one.

But yes, we should definitely unify that!

Note that we also have this one:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#613
that shouldn't be necessary anymore (thanks to platform fix), for docshell in child processes.

> 
> [1]:
> http://hg.mozilla.org/mozilla-central/annotate/9bb8b0b4daae/toolkit/devtools/
> server/actors/highlighter.js#l264

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8538561 [details] [diff] [review]
patch v1

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

::: toolkit/devtools/server/actors/chrome.js
@@ +43,5 @@
> +    let docShell = aSubject.QueryInterface(Ci.nsIInterfaceRequestor)
> +                           .getInterface(Ci.nsIWebNavigation)
> +                           .QueryInterface(Ci.nsIDocShell);
> +    this._progressListener.watch(docShell);
> +    this._notifyDocShellsUpdate([docShell]);

If we are looking for new windows, then shouldn't we also enumerate all existing windows in the constructor, instead of just the most recent one?

::: toolkit/devtools/server/actors/root.js
@@ +483,5 @@
> +
> +  onChromeActor: function () {
> +    if (this._chromeActor) {
> +      return {
> +        form: this._chromeActor.form()

I think this should be:

chromeActor: this._chromeActor.form()

for consistency with other places. We generally use the "thing" as the key and the "thing's form" as the value.

@@ +491,5 @@
> +    // XXX: check for certified pref
> +
> +    // Create a ChromeTabActor for the current top level xul window
> +    let { ChromeTabActor } = require("devtools/server/actors/chrome");
> +    let actor = this.chromeActor = new ChromeTabActor(this.conn);

It would be a good idea to initialize this._chromeActor to null in the constructor to avoid hitting SpiderMonkey's slow path (dictionary mode).

::: toolkit/devtools/server/actors/webconsole.js
@@ +179,5 @@
>     */
>    _getWindowForBrowserConsole: function WCA__getWindowForBrowserConsole()
>    {
>      // Check if our last used chrome window is still live.
> +    // XXX: this cache mess up with frame switching...

Can we make the actor listen for a frame switching event (we emit one of those, right?) to fix this?

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

I've trimmed and condensed the discussion around multiple windows, because I think it is critical to ensure we are all on the same page here.

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] from comment #7)
> > I know it extends TabActor, but it's not really for a tab anymore, right? 
> > So |ChromeWindowActor| maybe?
> 
> Yep, I would even go for ChromeActor as it isn't related to just one window.
> 
> > What would this look like for multiple top-level windows?
> 
> That already works with multiple top level windows.
> This one is just the default one.
> 
> > Is there a new |ChromeTabActor| for each window?
> 
> No, just one actor, then we rely on frame switching to go to any other
> window/sub-window.
> 
> If you follow my thoughts, you may imagine that I'm tempted to return such
> actor from listProcesses...
> So that we get a more functional toolbox for content processes without
> having to handle any complex actor hierarchy, nor any complex UI to get an
> inspector working.
> One codepath to rule that all...
> There is just the filetime of the TabActor to tweak,
> but as we inherit from it we should be able to customize its behavior.

So the idea is that the client asks for the chromeActor (or picks the chromeActor from the listProcesses response) and then uses it to debug chrome windows and the privileged code that runs in them?

In that case a few questions come to mind:
- does that debugging cover all of our tools (e.g. inspector and debugger)? IOW do some tools need to use the chromeActor, while others an existing global-scoped actor? If not, I think we could then ditch openToolbox's 'chrome' parameter (and similar checks in other places).
- shouldn't the chromeActor then contain the other global-scoped actors, if we are to preserve the existing client code paths (and unify chrome and content debugging)?
- in a future state where Firefox uses multiple content processes, should we make attachProcess return the tab actors, like an implicit listTabs response?
- how do you imagine using the iframe switcher to tell iframes and chrome windows apart? Will we have 2 switchers, one for windows and one for iframes?

I'm not saying that all of the above needs to be handled in this bug necessarily, but I want to understand the big picture and make sure we know where we are going.

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 8538561 [details] [diff] [review]
> ::: toolkit/devtools/server/actors/chrome.js
> @@ +43,5 @@
> > +    let docShell = aSubject.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                           .getInterface(Ci.nsIWebNavigation)
> > +                           .QueryInterface(Ci.nsIDocShell);
> > +    this._progressListener.watch(docShell);
> > +    this._notifyDocShellsUpdate([docShell]);
> 
> If we are looking for new windows, then shouldn't we also enumerate all
> existing windows in the constructor, instead of just the most recent one?

Good point, I missed overloading TabActor.docShells for that!

> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +179,5 @@
> >     */
> >    _getWindowForBrowserConsole: function WCA__getWindowForBrowserConsole()
> >    {
> >      // Check if our last used chrome window is still live.
> > +    // XXX: this cache mess up with frame switching...
> 
> Can we make the actor listen for a frame switching event (we emit one of
> those, right?) to fix this?

Good idea!

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Addressing comments 7 and 9.
Also started cleaning up root.js and fixed the browser console!

TODO:
- figure out chromeActor vs listProcesses,
- see if we can get rid of the `chrome`  field in target (not convinced because of addons),
- add an helper to get chromeEventHandler.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c097459f2f57

On top of the interdiff, in this patch, I also introduce the helper for chromeEventHandler,
soo getDocShellChromeEventHandler in webbrowser.js.

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #10)
> (In reply to Alexandre Poirot [:ochameau] from comment #8)
> So the idea is that the client asks for the chromeActor (or picks the
> chromeActor from the listProcesses response) and then uses it to debug
> chrome windows and the privileged code that runs in them?

Yes. Any (top-level) (chrome) window. (i.e. from browser.xul, to content, but also sidebar and addons documents, as soon as they are living in the parent process).

> 
> In that case a few questions come to mind:
> - does that debugging cover all of our tools (e.g. inspector and debugger)?

Yes.
Most of the work here is related to TabActor (crafting a real TabActor for chrome documents),
so in term of code size and code paths, it is more about inspector and actors relying on working on a document (tab actor dependent actors).
But there is also the makeDebugger and isRootActor tricks that help
child actors to debug more than just the targeted document.

> IOW do some tools need to use the chromeActor, while others an existing
> global-scoped actor? If not, I think we could then ditch openToolbox's
> 'chrome' parameter (and similar checks in other places).

Yes all "browser/chrome" features should be moving to chromeActor (Browser toolbox, content toolbox and browser console [see hudservice.js modification]). But I don't know about addons yet. They do not have any document I think?
I haven't looked yet, but if I remember correctly, the `chrome` flag on target is just used to know if we have to attach to the actor or not (do attach/detach requests). If that's the case, I think we should remove this flag and make all actors used on toolboxes have attach/detach.

> - shouldn't the chromeActor then contain the other global-scoped actors, if
> we are to preserve the existing client code paths (and unify chrome and
> content debugging)?

TabActors (inspector, console, webgl, ...) will finally only be tab actors and just that (Still need to do it in this patch)!
They won't be returned from listTabs anymore.
The others, the really global one like device, prefs, webapps, can be kept as-is and returned in the listTabs response. I'm not sure they should become child of chromeActor. I think it makes sense for them to be related to the root actor. Also, I'm not sure any of these actors make sense in the child processes. So if we start using chromeActor in child processes it will be disturbing.

> - in a future state where Firefox uses multiple content processes, should we
> make attachProcess return the tab actors, like an implicit listTabs response?

Yes, I would like to make listProcesses/attachProcess to use ChromeActor.
We expose TabActor, and in their form() method we return the tab actor childs.
So there is no implicit listTabs in my mind. This actor is just able to debug any document (top-level, chrome, content). Also listTabs is kind of outdated if you think about frame switching feature. We could have only one content actor working for all tabs if we want, but I don't think there is much value in doing that yet.

> - how do you imagine using the iframe switcher to tell iframes and chrome
> windows apart? Will we have 2 switchers, one for windows and one for iframes?

I think we should just implement a tree view in the menu, it would make everything clearer!
If that's not enough, listFrames request can send more information, so that we can highlight the top level windows. In this menu you may end up having a quite large tree if tabs in in-process and you have a lot of them.

> 
> I'm not saying that all of the above needs to be handled in this bug
> necessarily, but I want to understand the big picture and make sure we know
> where we are going.

Sure. I think it is quite significant move, that's why I asked you both feedback before continuing on this patch!


New todo items:
- check cross version compatibility issues,
- cleanup addChildActors to make all tab actors only tab actors.

Comment 15

[Alexandre Poirot [:ochameau]]

Note that this patch depends on bug 1098391.

Comment 16

[Alexandre Poirot [:ochameau]]

Cross version compatibility:
- old version of firefox can't debug main process of more recent device, as we no longer exposes tab actors on root actor.
- new version of firefox can debug main process of old devices as we fallback from chromeActor to rootActor. But it looks like there is some issues with the console. It may be because of another patch in my branch... I'll verify this.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot [:ochameau] from comment #16)
> - new version of firefox can debug main process of old devices as we
> fallback from chromeActor to rootActor. But it looks like there is some
> issues with the console. It may be because of another patch in my branch...
> I'll verify this.

Console for pre-37 servers was recently broken, but fixed by bug 1116199, so you might be missing this patch.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

New patch that handles cross version compatibility (more recent client than server).
Removes the tab actors hooked on the root actor and cleaned up root.js.

todo: check for certified pref, verify console with cross-version.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb33105782aa

New patch that checks for certified pref.
I think we are ready for a first round of review.

I've let the "chromeActor" request on RootActor,
mainly because it makes it easy to pull "the main process chrome actor"
from toolbox-process-window.js, hudservice.js and app-manager.js.
But I'm fine getting rid of it and integrate within listProcesses/attachProcess.
We would have to call listProcess, filter out the main process one and call attachProcess.

About `chrome` attribute on target. I think it would be really better to clean that up
in a followup. We may be able to get rid of it, the main issue is the BrowserAddonActor
that relies on this flag. But it looks like it has an attach/detach method
so that I don't really get why it is using chrome:true...

Finally, about comment 16/17, it looks like cross version compatiblity was failing
because of bug 1116199. I updated gecko to today's master and recent firefox (with my patch)
works fine with older devices/simulators.

I flagged you both for review as I think this patch is significant enough
to receive feedback from more than just one person. But do not hesitate to agree on passing
the final review to just one of you ;)

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8545292 [details] [diff] [review]
patch v4

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

(In reply to Alexandre Poirot [:ochameau] from comment #19)
> I flagged you both for review

I thought git-bz would do it for me, but it obviously didn't...

There is some failure on try, but I would like to hear another round of feedback before addressing tests.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am out Friday, hoping to look at this on Monday.

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] from comment #21)
> I am out Friday, hoping to look at this on Monday.

I mean Tuesday since Monday is a US holiday... :)

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8545292 [details] [diff] [review]
patch v4

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

Since it still exists for now, you should also update the connect screen[1].

In general, please try turning on every tool that toolbox options allows in the Browser Toolbox and playing with them for a bit.

Some of these are okay to resolve in follow ups, but please at least file them if so.  Some may also have existing dupes.

My rough notes:

* Inspector
  1. Open a loop call
  2. Switch to the about:loopconversation frame
  3. I can't switch to any other frame now (if it's specific to loop only, let's at least file a bug)
* Debugger
  * Still shows all sources after switching frames (maybe okay, not sure)
* Style Editor
  * Edit some CSS, change apply correctly
  * Switch frames, and switch back
  * Edits still applied, but changes no longer there
* Shader Editor
  * Reload button seems to break browser, probably disable tool
* Canvas
  * Tool appears even though actor is not present (adjust tool filter probably)
* Performance
  * Fails because framerateActor is missing, used to work so should be fixed
* Timeline
  * Fails because timelineActor is missing
* Storage
  * Wants target.browser currently
* Web Audio
  * Reload button seems to break browser, probably disable tool
* Scratchpad
  * console.log does not log anymore

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/connect/connect.js

::: browser/devtools/framework/toolbox-process-window.js
@@ +58,5 @@
>  function setPrefDefaults() {
>    Services.prefs.setBoolPref("devtools.inspector.showUserAgentStyles", true);
>    Services.prefs.setBoolPref("devtools.profiler.ui.show-platform-data", true);
>    Services.prefs.setBoolPref("browser.devedition.theme.showCustomizeButton", false);
> +  Services.prefs.setBoolPref("browser.dom.window.dump.enabled", true);

Be sure to remove later.

@@ +77,4 @@
>    let options = {
>      form: form,
>      client: gClient,
> +    chrome: chrome

One side-effect of using chrome: false but while still operating in a chrome toolbox is that some tool filtering expressions[1] are incorrect, making them appear in the Browser Toolbox now.

I still feel that target.chrome == false is a bad idea.  It makes the "chrome" attribute meaningless on the target because it's basically a lie.

I think you'll need to visit each usage and find new ways to set things right in this new world.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=target.chrome&case=true&=mozilla-central&redirect=true

::: browser/devtools/webconsole/hudservice.js
@@ +188,5 @@
>          DebuggerServer.addBrowserActors();
>        }
>  
>        let client = new DebuggerClient(DebuggerServer.connectPipe());
> +      client.connect(() => {

See t-p-w about chrome: false.

::: browser/devtools/webide/modules/app-manager.js
@@ +221,5 @@
> +                   .then(aResponse => {
> +                     return devtools.TargetFactory.forRemoteTab({
> +                       form: aResponse.chromeActor,
> +                       client: this.connection.client,
> +                       chrome: false

See t-p-w about chrome: false.

@@ +402,5 @@
> +    // Fx <37 exposes chrome tab actors on RootActor
> +    // Fx >=37 exposes a dedicated actor, "chromeActor"
> +    return (this._listTabsResponse &&
> +            this._listTabsResponse.consoleActor) ||
> +           this.connection.client.mainRoot.traits.chromeActor;

This is not equivalent.  The old check was asking "do I have access to debug chrome", but the new trait only answers "does this server know about the new code path".

::: toolkit/devtools/server/actors/chrome.js
@@ +40,5 @@
> +exports.ChromeActor = ChromeActor;
> +
> +ChromeActor.prototype = Object.create(TabActor.prototype);
> +
> +ChromeActor.prototype.constructor = ChromeActor;

You could use the new Object.assign to add all these properties and methods from an object literal if you wish.

@@ +48,5 @@
> +/**
> + * Getter for the list of all docshell in this tabActor
> + * @return {Array}
> + */
> +Object.defineProperty(ChromeActor.prototype, "docShells", {

The |docShell| (singular) property is also used in parts of TabActor.  Do you need to define a version of it?

Various tools (such as storage) expect[1] a |browser| property to be defined.  If you can't define such a value, then you should adjust the tools to work around this (which could mean ensure the tool is disabled).

[1]: https://dxr.mozilla.org/mozilla-central/search?q=.browser%20path%3Aactors&case=true&=mozilla-central&redirect=true

@@ +49,5 @@
> + * Getter for the list of all docshell in this tabActor
> + * @return {Array}
> + */
> +Object.defineProperty(ChromeActor.prototype, "docShells", {
> +  get : function () {

Nit: get: func

@@ +53,5 @@
> +  get : function () {
> +    // Iterate over all top-level windows and all their docshells.
> +    let docShells = [];
> +    let e = Services.ww.getWindowEnumerator();
> +    while(e.hasMoreElements()) {

Nit: while (

@@ +58,5 @@
> +      let window = e.getNext();
> +      let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                           .getInterface(Ci.nsIWebNavigation)
> +                           .QueryInterface(Ci.nsIDocShell);
> +      let docShellsEnum = docShell.getDocShellEnumerator(

Maybe this inner enumeration can be shared with TabActor instead of duplicated here?

@@ +104,5 @@
> +
> +/**
> + * Prepare to enter a nested event loop by disabling debuggee events.
> + */
> +ChromeActor.prototype.preNest = function() {

I don't know this bits very well, but looks like you copied them correctly. :)

::: toolkit/devtools/server/actors/root.js
@@ -176,5 @@
>      };
>    },
>  
>    /**
> -   * This is true for the root actor only, used by some child actors

I am very happy to see this block of duplicated stuff go away!

@@ +361,5 @@
>        delete this._extraActors[aName];
>      }
> +   },
> +
> +  onChromeActor: function () {

I think it would make more sense to me for this new actor to accessed via list / attachProcesses, as we've discussed in comments.  

That seems more in line with our long-term strategy, and I don't want to have this here as a short-term hack, since then it becomes part of the RDP for all time.

@@ +362,5 @@
>      }
> +   },
> +
> +  onChromeActor: function () {
> +    if (Services.prefs.getBoolPref("devtools.debugger.forbid-certified-apps")) {

|addBrowserActor|'s restrictPrivileges argument was nice to avoid depending on a specific pref here.

Further, with this patch, we now have to set "devtools.debugger.forbid-certified-apps" even on Firefox, where we did not before.

Either create some code path for each embedding application to say whether chrome is allowed, or at least create a new pref that actually has the right meaning.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +39,5 @@
> +function getDocShellChromeEventHandler(docShell) {
> +  let handler = docShell.chromeEventHandler;
> +  if (!handler) {
> +    try {
> +      // toplevel xul window's docshell don't have chromeEventHandler attribute

Nit: don't -> doesn't

@@ +42,5 @@
> +    try {
> +      // toplevel xul window's docshell don't have chromeEventHandler attribute
> +      handler = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                        .getInterface(Ci.nsIDOMWindow);
> +    } catch(e) {}

Should something else happen in case of error?

Comment 24

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #23)
> @@ +48,5 @@
> > +/**
> > + * Getter for the list of all docshell in this tabActor
> > + * @return {Array}
> > + */
> > +Object.defineProperty(ChromeActor.prototype, "docShells", {
> 
> The |docShell| (singular) property is also used in parts of TabActor.  Do
> you need to define a version of it?
> 
> Various tools (such as storage) expect[1] a |browser| property to be
> defined.  If you can't define such a value, then you should adjust the tools
> to work around this (which could mean ensure the tool is disabled).
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/search?q=.
> browser%20path%3Aactors&case=true&=mozilla-central&redirect=true

There shouldn't be any use of browser... Any use is going to throw on b2g :/
We expose ContentActor which inherits from TabActor.
highlighter.js seems to not be using browser and gcli, I don't think we care for browser toolbox??
Also the issue with storage actor is that it still doesn't support frame switching (bug 1060925),
if it was refactored to support frame switching, it would be working on b2g + frames and it would stop using browser reference \o/

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: cleanup highlighter.js

Cleanup from unused browser tab actor attribute.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Alex, is it ok if I skip this review until you have posted an updated patch? Sorry for the delay, but I've been mired in reviews all week.

Comment 27

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Comment 28

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

(In reply to J. Ryan Stinnett [:jryans] from comment #23)
> Comment on attachment 8545292 [details] [diff] [review]
> * Inspector
>   1. Open a loop call
>   2. Switch to the about:loopconversation frame
>   3. I can't switch to any other frame now (if it's specific to loop only,
> let's at least file a bug)

Confirmed, there is some exception, but only console seems broken.
It should be fixed with the new patch.

> * Debugger
>   * Still shows all sources after switching frames (maybe okay, not sure)

We can tweak that by modifying the behavior of makeDebugger when switching frame.
The only issue I see if when should we do a unrestricted debugger?
When we are on the original top level window? (that may be closed at some point, then we won't be able to get such unrestricted debugger anymore)
Or when we are on any top level window? or just always, like now?
The other issue in limiting the view of the debugger is that if you want to set a breakpoint in a JSM while debugging loop of any non-top-level document, you would have to open two Browser toolbox (if that's possible??)
It may be better to have a debugger panel specific setting in menu like "god mode, see everything",
or otherwise only show document scripts (it would be specific to browser toolbox).

> * Style Editor
>   * Edit some CSS, change apply correctly
>   * Switch frames, and switch back
>   * Edits still applied, but changes no longer there

That's not specific to Browser toolbox, given that style editor just load registered CSS files content, it doesn't save previous modifications. It is like:
1/ open a regular toolbox against a website
2/ edit CSS
3/ close toolbox, reopen it
4/ css are still modified but Style editor doesn't have the modifications

> * Shader Editor
>   * Reload button seems to break browser, probably disable tool
  Disabled

> * Canvas
>   * Tool appears even though actor is not present (adjust tool filter
> probably)
  Disabled

> * Performance
>   * Fails because framerateActor is missing, used to work so should be fixed
  Fixed

> * Timeline
>   * Fails because timelineActor is missing
  Fixed

> * Storage
>   * Wants target.browser currently
  Disabled

> * Web Audio
>   * Reload button seems to break browser, probably disable tool
  Disabled

> * Scratchpad
>   * console.log does not log anymore
  I haven't tested with the last patch, but it works with the new one.


> 
> ::: browser/devtools/framework/toolbox-process-window.js
> @@ +58,5 @@
> >  function setPrefDefaults() {
> >    Services.prefs.setBoolPref("devtools.inspector.showUserAgentStyles", true);
> >    Services.prefs.setBoolPref("devtools.profiler.ui.show-platform-data", true);
> >    Services.prefs.setBoolPref("browser.devedition.theme.showCustomizeButton", false);
> > +  Services.prefs.setBoolPref("browser.dom.window.dump.enabled", true);
> 
> Be sure to remove later.

Actually, this pref really helps debugging browser toolbox issues.
Can't we keep it?


> @@ +402,5 @@
> > +    // Fx <37 exposes chrome tab actors on RootActor
> > +    // Fx >=37 exposes a dedicated actor, "chromeActor"
> > +    return (this._listTabsResponse &&
> > +            this._listTabsResponse.consoleActor) ||
> > +           this.connection.client.mainRoot.traits.chromeActor;
> 
> This is not equivalent.  The old check was asking "do I have access to debug
> chrome", but the new trait only answers "does this server know about the new
> code path".

Fixed by introducing allowProcessDebug trait which is *defined* if the feature is available (FX >=37)
and is *true* if it is allowed to call attachProcess. 

> @@ +48,5 @@
> > +/**
> > + * Getter for the list of all docshell in this tabActor
> > + * @return {Array}
> > + */
> > +Object.defineProperty(ChromeActor.prototype, "docShells", {
> 
> The |docShell| (singular) property is also used in parts of TabActor.  Do
> you need to define a version of it?

docShell is set in ChromeActor constructor (just set the default docshell),
then it is set by the iframe switching feature built-in into TabActor.

> @@ +58,5 @@
> > +      let window = e.getNext();
> > +      let docShell = window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                           .getInterface(Ci.nsIWebNavigation)
> > +                           .QueryInterface(Ci.nsIDocShell);
> > +      let docShellsEnum = docShell.getDocShellEnumerator(
> 
> Maybe this inner enumeration can be shared with TabActor instead of
> duplicated here?

I'm now sharing getChildDocShells method between these two modules.

> 
> @@ +104,5 @@
> > +
> > +/**
> > + * Prepare to enter a nested event loop by disabling debuggee events.
> > + */
> > +ChromeActor.prototype.preNest = function() {
> 
> I don't know this bits very well, but looks like you copied them correctly.
> :)

Me neither, last time I tried to remove them and it debugger was still working fine,
but as I have no idea what is this code about, I prefered to keep it...

> @@ +361,5 @@
> >        delete this._extraActors[aName];
> >      }
> > +   },
> > +
> > +  onChromeActor: function () {
> 
> I think it would make more sense to me for this new actor to accessed via
> list / attachProcesses, as we've discussed in comments.  
> 
> That seems more in line with our long-term strategy, and I don't want to
> have this here as a short-term hack, since then it becomes part of the RDP
> for all time.

Done, I'm using attachProcess now.

> 
> @@ +362,5 @@
> >      }
> > +   },
> > +
> > +  onChromeActor: function () {
> > +    if (Services.prefs.getBoolPref("devtools.debugger.forbid-certified-apps")) {
> 
> |addBrowserActor|'s restrictPrivileges argument was nice to avoid depending
> on a specific pref here.
> 
> Further, with this patch, we now have to set
> "devtools.debugger.forbid-certified-apps" even on Firefox, where we did not
> before.
> 
> Either create some code path for each embedding application to say whether
> chrome is allowed, or at least create a new pref that actually has the right
> meaning.
> 

Fixed with an attribute on DebuggerServer called `allowProcessDebug`.

> @@ +42,5 @@
> > +    try {
> > +      // toplevel xul window's docshell don't have chromeEventHandler attribute
> > +      handler = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                        .getInterface(Ci.nsIDOMWindow);
> > +    } catch(e) {}
> 
> Should something else happen in case of error?

No, it should cover all the cases, all non-top level documents should have a chromeEventHandler attribute.
Otherwise top level should be able to query to a window which is the chrome event handler.
I don't remember why I added try/catch here, but it can throw for xbl I think. If I remember correctly they do have a custom docshell without chromeEventHandler, nor querying to a window. But I'm not sure we do call this method with any XBL docshell?



TODO:
1/ connect screen
> Since it still exists for now, you should also update the connect screen.

2/ chrome flag
> One side-effect of using chrome: false but while still operating in a chrome
> toolbox is that some tool filtering expressions[1] are incorrect, making
> them appear in the Browser Toolbox now.
> 
> I still feel that target.chrome == false is a bad idea.  It makes the
> "chrome" attribute meaningless on the target because it's basically a lie.
> 
> I think you'll need to visit each usage and find new ways to set things
> right in this new world.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/search?q=target.
> chrome&case=true&=mozilla-central&redirect=true

Comment 29

[Alexandre Poirot [:ochameau]]

Attachment: cleanup highlighter.js

Just a simple cleanup to stop mentioning tabActor.browser
which shouldn't be used... (tabActor.chromeEventHandler is better)

Comment 30

[Alexandre Poirot [:ochameau]]

Attachment: cleanup highlighter.js

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8554622 [details] [diff] [review]
cleanup highlighter.js

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

Good catch. Thanks.

Comment 32

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8554618 [details] [diff] [review]
patch v5

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

I have a bunch of comments, but for my part it looks good! I didn't have much time to play with it, but luckily Ryan is very thorough in his testing, so we should be good there :)

One error I did spot was this, on shutdown:

console.error: 
  Message: TypeError: topWindow is null
  Stack:
    CH__sendMessageToTopWindow@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox-hosts.js:294:5
WH_destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox-hosts.js:324:7
Toolbox.prototype.destroyHost@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1589:12
Toolbox.prototype.destroy/this._destroyer<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:1663:19
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:870:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:749:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:691:37

I don't think I've seen it without this patch.

::: browser/devtools/framework/ToolboxProcess.jsm
@@ +136,5 @@
>  
>      if (!this.debuggerServer.initialized) {
>        this.debuggerServer.init();
>        this.debuggerServer.addBrowserActors();
> +      this.debuggerServer.allowProcessDebug = true;

So how does this work if I open a content toolbox first and then open a browser toolbox? Won't the first toolbox start a debugger server with allowProcessDebug == false, and this part skip initialization because a server is already started?

::: browser/devtools/webide/modules/app-manager.js
@@ +223,5 @@
> +          to: "root",
> +          type: "attachProcess",
> +          id: 0 // 0 for main process
> +        };
> +        return this.connection.client.request(packet)

Can you add a simple client.attachProcess(id) helper method in dbg-client.jsm to avoid the boilerplate?

@@ +430,5 @@
> +    // Fx <37 exposes chrome tab actors on RootActor
> +    // Fx >=37 exposes a dedicated actor via attachProcess request
> +    return (this._listTabsResponse &&
> +            this._listTabsResponse.consoleActor) ||
> +           this.connection.client.mainRoot.traits.allowProcessDebug;

Nitpicking, but if you swap the order of the || operands, it will be optimized for new code instead of old code.

::: toolkit/devtools/server/actors/chrome.js
@@ +12,5 @@
> +const makeDebugger = require("./utils/make-debugger");
> +
> +/**
> + * Creates a tab actor for handling requests to all the chrome content
> + * in the parent process. Most of the implementation comes from TabActor.

This is a bit spartan. Could you elaborate on the parent-child relationship between the ChromeActor and other formerly called global-scoped actors? A brief summary of how things are, compared to how they used to be, would be helpful.

@@ +53,5 @@
> +
> +ChromeActor.prototype.isRootActor = true;
> +
> +/**
> + * Getter for the list of all docshell in this tabActor

Typo: doschells

::: toolkit/devtools/server/actors/root.js
@@ +325,5 @@
> +    if (typeof(aRequest.id) != "number") {
> +      return { error: "wrongParameter",
> +               message: "attachProcess requires a valid `id` attribute." };
> +    }
> +    if (aRequest.id === 0) { // XXX based on onListProcesses implementation

Wouldn't a convention of "missing id === main process" be even cleaner? I'm thinking that client.attachProcess() would be easier to remember without the magic properties of id == 0.

@@ +341,5 @@
> +        return { error: "noProcess",
> +                 message: "There is no process with id '" + aRequest.id + "'." };
> +      }
> +      return DebuggerServer.connectToContent(this.conn, mm)
> +                           .then(form => ({ form: form }));

I believe you can now take advantage of ES6 cuteness if you prefer:

.then(form => ({ form }));

@@ -450,5 @@
> -   * @return StyleSheetActor actor
> -   *         The actor for this style sheet.
> -   *
> -   */
> -  createStyleSheetActor: function(styleSheet) {

Where did this go? Isn't it used any more?

@@ +385,5 @@
>     }
>  };
>  
>  RootActor.prototype.requestTypes = {
> +  "chromeActor": RootActor.prototype.onChromeActor,

Leftover?

::: toolkit/devtools/server/actors/webbrowser.js
@@ +658,5 @@
>    /**
>     * Getter for the list of all docshell in this tabActor
>     * @return {Array}
>     */
> +  get docShells() getChildDocShells(this.docShell),

Isn't this the soon-to-be-deleted expression closure syntax? Can we use a regular getter instead?

Comment 33

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8554618 [details] [diff] [review]
patch v5

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

I think resolving |target.chrome| will influence how we decide which tools are available, so I'd like to see that work before r+.

(In reply to Alexandre Poirot [:ochameau] from comment #28)
> Created attachment 8554618 [details] [diff] [review]
> patch v5
> 
> (In reply to J. Ryan Stinnett [:jryans] from comment #23)
> > Comment on attachment 8545292 [details] [diff] [review]
> > * Inspector
> >   1. Open a loop call
> >   2. Switch to the about:loopconversation frame
> >   3. I can't switch to any other frame now (if it's specific to loop only,
> > let's at least file a bug)
> 
> Confirmed, there is some exception, but only console seems broken.
> It should be fixed with the new patch.

For me, console and frame switching are still broken here in the new patch.

> > * Debugger
> >   * Still shows all sources after switching frames (maybe okay, not sure)
> 
> We can tweak that by modifying the behavior of makeDebugger when switching
> frame.
> The only issue I see if when should we do a unrestricted debugger?
> When we are on the original top level window? (that may be closed at some
> point, then we won't be able to get such unrestricted debugger anymore)
> Or when we are on any top level window? or just always, like now?
> The other issue in limiting the view of the debugger is that if you want to
> set a breakpoint in a JSM while debugging loop of any non-top-level
> document, you would have to open two Browser toolbox (if that's possible??)
> It may be better to have a debugger panel specific setting in menu like "god
> mode, see everything",
> or otherwise only show document scripts (it would be specific to browser
> toolbox).

Let's stick with the simple path of showing everything for now.

> > ::: browser/devtools/framework/toolbox-process-window.js
> > @@ +58,5 @@
> > >  function setPrefDefaults() {
> > >    Services.prefs.setBoolPref("devtools.inspector.showUserAgentStyles", true);
> > >    Services.prefs.setBoolPref("devtools.profiler.ui.show-platform-data", true);
> > >    Services.prefs.setBoolPref("browser.devedition.theme.showCustomizeButton", false);
> > > +  Services.prefs.setBoolPref("browser.dom.window.dump.enabled", true);
> > 
> > Be sure to remove later.
> 
> Actually, this pref really helps debugging browser toolbox issues.
> Can't we keep it?

Do you think it makes the toolbox performance worse?  That's my only worry with leaving it on.

> > @@ +42,5 @@
> > > +    try {
> > > +      // toplevel xul window's docshell don't have chromeEventHandler attribute
> > > +      handler = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > > +                        .getInterface(Ci.nsIDOMWindow);
> > > +    } catch(e) {}
> > 
> > Should something else happen in case of error?
> 
> No, it should cover all the cases, all non-top level documents should have a
> chromeEventHandler attribute.
> Otherwise top level should be able to query to a window which is the chrome
> event handler.
> I don't remember why I added try/catch here, but it can throw for xbl I
> think. If I remember correctly they do have a custom docshell without
> chromeEventHandler, nor querying to a window. But I'm not sure we do call
> this method with any XBL docshell?

I'm not sure, I don't really know much about XBL.

::: browser/devtools/framework/ToolboxProcess.jsm
@@ +136,5 @@
>  
>      if (!this.debuggerServer.initialized) {
>        this.debuggerServer.init();
>        this.debuggerServer.addBrowserActors();
> +      this.debuggerServer.allowProcessDebug = true;

I *think* we are safe here... this is a separate loader instance from the content toolbox, so there really are two |DebuggerServer|s that get initialized separately.

::: browser/devtools/webide/modules/app-manager.js
@@ +217,5 @@
>  
>    getTarget: function() {
>      if (this.selectedProject.type == "mainProcess") {
> +      // Fx >=37 exposes a ChromeActor to debug the main process
> +      if (this.connection.client.mainRoot.traits.allowProcessDebug) {

That's an impressive property chain! ;)

The traits are also on |client|, so it's enough to do:

this.connection.client.traits.allowProcessDebug

::: toolkit/devtools/server/actors/chrome.js
@@ +18,5 @@
> + * @param aConnection DebuggerServerConnection
> + *        The connection to the client.
> + */
> +function ChromeActor(aConnection)
> +{

Nit: Newer code typically put this on the previous line

@@ +75,5 @@
> +});
> +
> +ChromeActor._createExtraActors = function (factories, pool) {
> +  // Filters only supported actors in browser toolbox
> +  let supported = ["webconsoleActor", "inspectorActor", "styleSheetsActor",

I guess this is not what I was expecting for disabling the tools... :P

I think we should try to avoid maintaining yet another list of actors that get enabled in certain situations, as that's just like the situation we had before they were in one place, and we always had to update b2g, fennec, etc. separately for each new actor.

I was expecting that disabling tools would be done via the |isTargetSupported| checks in the front end.  But I think you'll need to first resolve |target.chrome|, which would then likely lead to you tweaking |isTargetSupported| expressions.

Do you really feel like another static list is a good way to handle it?

@@ +86,5 @@
> +  }
> +  return createExtraActors(chromeFactories, pool);
> +}
> +
> +ChromeActor.prototype.observe = function(aSubject, aTopic, aData) {

This seems to not be enough to notice all new frames.

STR:

1. Open Browser Toolbox
2. Open WebIDE
3. Connect to local runtime
4. Open toolbox in WebIDE
5. Look at Browser Toolbox frame list: there is no WebIDE toolbox
6. Close and reopen Browser Toolbox: now WebIDE toolbox is present and inspectable

@@ +97,5 @@
> +                           .getInterface(Ci.nsIWebNavigation)
> +                           .QueryInterface(Ci.nsIDocShell);
> +    this._progressListener.watch(docShell);
> +    this._notifyDocShellsUpdate([docShell]);
> +  } else if (aTopic == "domwindowclosed") {

We also don't clean up the frame list correctly.

STR:

1. Start Firefox with one regular browser window
2. Open Browser Toolbox
3. Open WebIDE
4. The WebIDE frames are correctly present in the frame list
5. Close WebIDE
6. The WebIDE frames are still there

::: toolkit/devtools/server/actors/root.js
@@ +159,5 @@
>      fetchPackagedApp: true,
> +    // Whether root actor exposes tab actors
> +    // if processActor is implemented, you should fetch a ChromeActor instance
> +    // to debug chrome and any non-content ressource via attachProcess request
> +    processActor: DebuggerServer.allowProcessDebug

I believe you want the trait to be named the same as the |DebuggerServer| property.

@@ +317,5 @@
>      return { processes: processes };
>    },
>  
>    onAttachProcess: function (aRequest) {
> +    if (!DebuggerServer.allowProcessDebug) {

How does this work for b2g?

@@ +325,5 @@
> +    if (typeof(aRequest.id) != "number") {
> +      return { error: "wrongParameter",
> +               message: "attachProcess requires a valid `id` attribute." };
> +    }
> +    if (aRequest.id === 0) { // XXX based on onListProcesses implementation

As it is today, |onListProcess| puts the main process as "0", so it seems reasonable to handle that number here.

Could we expose a const for the client to use at least? MAIN_PROCESS = 0 or something.

Also, it looks like the process message managers keep various flags interally, like MM_CHROME etc.  If those were exposed via IDL, we'd have a more useful process list.  Could you file a follow up to expose this in IDL?

@@ -450,5 @@
> -   * @return StyleSheetActor actor
> -   *         The actor for this style sheet.
> -   *
> -   */
> -  createStyleSheetActor: function(styleSheet) {

It's in the |TabActor|, which |ChromeActor| extends.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +2048,5 @@
>    },
>  
>    _getWindowsInDocShell: function(docShell) {
> +    return getChildDocShells(docShell).map(
> +      d => {

Nit: It's more typical to put this part up on the previous line

@@ +2051,5 @@
> +    return getChildDocShells(docShell).map(
> +      d => {
> +        return d.QueryInterface(Ci.nsIInterfaceRequestor)
> +                .getInterface(Ci.nsIDOMWindow);
> +      }

Nit: And this down in the next line

::: toolkit/devtools/server/main.js
@@ +164,5 @@
>  
>    /**
> +   * Allow debugging chrome of (parent or child) processes.
> +   */
> +  allowProcessDebug: false,

Maybe "allowChromeProcess"?  I think "chrome" should be in the name somewhere to clarify the security risk involved.

"Debug" seems redundant here.

I think it's great for security that we're defaulting this to false.  However, you now need to find all the cases where this used to be true and update them.  You've already done:

* Browser Toolbox

But there's also:

* B2G (when in full DevTools)
* Browser Content Toolbox
* Fennec

Comment 34

[Alexandre Poirot [:ochameau]]

(In reply to Panos Astithas [:past] from comment #32)
> Comment on attachment 8554618 [details] [diff] [review]
>
> One error I did spot was this, on shutdown:
> 
> console.error: 
>   Message: TypeError: topWindow is null
>   Stack:
>    
> CH__sendMessageToTopWindow@resource://gre/modules/commonjs/toolkit/loader.js
> -> resource:///modules/devtools/framework/toolbox-hosts.js:294:5
> 
> I don't think I've seen it without this patch.

May be related to bug 1117067? I don't see how this patch would affect panel iframe behavior,
but I'll see if I can reproduce this exception.

> ::: browser/devtools/framework/ToolboxProcess.jsm
> @@ +136,5 @@
> >  
> >      if (!this.debuggerServer.initialized) {
> >        this.debuggerServer.init();
> >        this.debuggerServer.addBrowserActors();
> > +      this.debuggerServer.allowProcessDebug = true;
> 
> So how does this work if I open a content toolbox first and then open a
> browser toolbox? Won't the first toolbox start a debugger server with
> allowProcessDebug == false, and this part skip initialization because a
> server is already started?

In this particular case, the browser toolbox, we spawn a new Loader, so that we are using a distinct DebuggerServer instance. This `if (!this.debuggerServer.initialized)` is not useful. I removed both if (also the `if (!this.loader)` as they are useless, we call this method only once and we have to setup everything.

But, this comment applies to the browser console usecase, in browser/devtools/webconsole/hudservice.js !
Good catch! In hudservice.js, I moved the allowProcessDebug out of this condition.

> 
> ::: browser/devtools/webide/modules/app-manager.js
> @@ +223,5 @@
> > +          to: "root",
> > +          type: "attachProcess",
> > +          id: 0 // 0 for main process
> > +        };
> > +        return this.connection.client.request(packet)
> 
> Can you add a simple client.attachProcess(id) helper method in
> dbg-client.jsm to avoid the boilerplate?

Ahah, I already introduced one when working on Content process toolbox.
I'll try to use my stuff !

> 
> ::: toolkit/devtools/server/actors/chrome.js
> @@ +12,5 @@
> > +const makeDebugger = require("./utils/make-debugger");
> > +
> > +/**
> > + * Creates a tab actor for handling requests to all the chrome content
> > + * in the parent process. Most of the implementation comes from TabActor.
> 
> This is a bit spartan. Could you elaborate on the parent-child relationship
> between the ChromeActor and other formerly called global-scoped actors? A
> brief summary of how things are, compared to how they used to be, would be
> helpful.

Comment tweaked!
I added a dedicated and separate comment about the history.
To avoid disturbing new contributors about the past.

> ::: toolkit/devtools/server/actors/root.js
> @@ +325,5 @@
> > +    if (typeof(aRequest.id) != "number") {
> > +      return { error: "wrongParameter",
> > +               message: "attachProcess requires a valid `id` attribute." };
> > +    }
> > +    if (aRequest.id === 0) { // XXX based on onListProcesses implementation
> 
> Wouldn't a convention of "missing id === main process" be even cleaner? I'm
> thinking that client.attachProcess() would be easier to remember without the
> magic properties of id == 0.

Yep, sounds good.

> 
> @@ -450,5 @@
> > -   * @return StyleSheetActor actor
> > -   *         The actor for this style sheet.
> > -   *
> > -   */
> > -  createStyleSheetActor: function(styleSheet) {
> 
> Where did this go? Isn't it used any more?

This was one of the many things purely copy-pasted from webbrowser.js:TabActor.

> 
> ::: toolkit/devtools/server/actors/webbrowser.js
> @@ +658,5 @@
> >    /**
> >     * Getter for the list of all docshell in this tabActor
> >     * @return {Array}
> >     */
> > +  get docShells() getChildDocShells(this.docShell),
> 
> Isn't this the soon-to-be-deleted expression closure syntax? Can we use a
> regular getter instead?

Hum, I have no idea, but I can change that. How looks like regular getter?
  get docShells() {
    return getChildDocShells(this.docShell);
  }
?
I switched to that by default.


(I'll attach new patch after also addressing jryans comments)

Comment 35

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #33)
> Comment on attachment 8554618 [details] [diff] [review]
> 
> For me, console and frame switching are still broken here in the new patch.

It works for me, but I have many patches in my queue, I'll retest with just this patch.

> > Actually, this pref really helps debugging browser toolbox issues.
> > Can't we keep it?
> 
> Do you think it makes the toolbox performance worse?  That's my only worry
> with leaving it on.

It can if there is many dumps and you have a console opened logging it.

> ::: browser/devtools/webide/modules/app-manager.js
> @@ +217,5 @@
> >  
> >    getTarget: function() {
> >      if (this.selectedProject.type == "mainProcess") {
> > +      // Fx >=37 exposes a ChromeActor to debug the main process
> > +      if (this.connection.client.mainRoot.traits.allowProcessDebug) {
> 
> That's an impressive property chain! ;)
> 
> The traits are also on |client|, so it's enough to do:
> 
> this.connection.client.traits.allowProcessDebug

I was trying to follow bug 878901, but I can do that.
It's not really clear what belongs to what until we really clean DebuggerClient from stuff exposed on mainRoot.

> @@ +75,5 @@
> > +});
> > +
> > +ChromeActor._createExtraActors = function (factories, pool) {
> > +  // Filters only supported actors in browser toolbox
> > +  let supported = ["webconsoleActor", "inspectorActor", "styleSheetsActor",
> 
> I guess this is not what I was expecting for disabling the tools... :P
> 
> I think we should try to avoid maintaining yet another list of actors that
> get enabled in certain situations, as that's just like the situation we had
> before they were in one place, and we always had to update b2g, fennec, etc.
> separately for each new actor.
> 
> I was expecting that disabling tools would be done via the
> |isTargetSupported| checks in the front end.  But I think you'll need to
> first resolve |target.chrome|, which would then likely lead to you tweaking
> |isTargetSupported| expressions.
> 
> Do you really feel like another static list is a good way to handle it?

I felt that it was something server should control as the actor themself were broken.
But may be I'm wrong, and in some cases, that's only a client issue...

> 
> @@ +86,5 @@
> > +  }
> > +  return createExtraActors(chromeFactories, pool);
> > +}
> > +
> > +ChromeActor.prototype.observe = function(aSubject, aTopic, aData) {
> 
> This seems to not be enough to notice all new frames.

WFM :/

> 
> @@ +97,5 @@
> > +                           .getInterface(Ci.nsIWebNavigation)
> > +                           .QueryInterface(Ci.nsIDocShell);
> > +    this._progressListener.watch(docShell);
> > +    this._notifyDocShellsUpdate([docShell]);
> > +  } else if (aTopic == "domwindowclosed") {
> 
> We also don't clean up the frame list correctly.
> 

Same, WFM

> ::: toolkit/devtools/server/actors/root.js
> @@ +159,5 @@
> >      fetchPackagedApp: true,
> > +    // Whether root actor exposes tab actors
> > +    // if processActor is implemented, you should fetch a ChromeActor instance
> > +    // to debug chrome and any non-content ressource via attachProcess request
> > +    processActor: DebuggerServer.allowProcessDebug
> 
> I believe you want the trait to be named the same as the |DebuggerServer|
> property.
> 
> @@ +317,5 @@
> >      return { processes: processes };
> >    },
> >  
> >    onAttachProcess: function (aRequest) {
> > +    if (!DebuggerServer.allowProcessDebug) {
> 
> How does this work for b2g?

I forgot to update b2g code to set allowProcessDebug.
Otherwise, I haven't tried it on child process, but you should be able to attach to child process.
A followup would be to spawn a ChromeActor instead of ChildProcessActor and end up merging child.js and content-server.jsm which contain a lot of duplicates!

> 
> @@ +325,5 @@
> > +    if (typeof(aRequest.id) != "number") {
> > +      return { error: "wrongParameter",
> > +               message: "attachProcess requires a valid `id` attribute." };
> > +    }
> > +    if (aRequest.id === 0) { // XXX based on onListProcesses implementation
> 
> As it is today, |onListProcess| puts the main process as "0", so it seems
> reasonable to handle that number here.
> 
> Could we expose a const for the client to use at least? MAIN_PROCESS = 0 or
> something.

I updated this code to follow Panos suggestion to connect to main process when there is no id given.
I think it is enough, no need to make a special MAIN_PROCESS value?

> 
> Also, it looks like the process message managers keep various flags
> interally, like MM_CHROME etc.  If those were exposed via IDL, we'd have a
> more useful process list.  Could you file a follow up to expose this in IDL?

I would like to wait for a real need before opening followups.
listProcess is really dumb now as there is no need for anything smarter (only used for content toolbox and connects to the first child process).
The only special case is main process but I think attachProcess() with no argument to connect to main solves it.
If we start using listProcess on b2g or fx desktop, we would have to figure out how to filter and sort the list of processes.

> ::: toolkit/devtools/server/main.js
> @@ +164,5 @@
> >  
> >    /**
> > +   * Allow debugging chrome of (parent or child) processes.
> > +   */
> > +  allowProcessDebug: false,
> 
> Maybe "allowChromeProcess"?  I think "chrome" should be in the name
> somewhere to clarify the security risk involved.
> 
> "Debug" seems redundant here.

WFM, then it will be even more important to followup in order to expose ChromeActor in child processes!
Renamed.

> I think it's great for security that we're defaulting this to false. 
> However, you now need to find all the cases where this used to be true and
> update them.  You've already done:
> 
> * Browser Toolbox
> 
> But there's also:
> 
> * B2G (when in full DevTools)
> * Browser Content Toolbox
> * Fennec

Fixed

Comment 36

[Alexandre Poirot [:ochameau]]

Oh, I was able to reproduce the missing toolbox document and the non-removal of some document,
there is many things broken with chrome document as they don't dispatch webnavigation-created/destroyed,
hopefully we have some equivalent chrome-webnavigation-created/destroyed.
It allows better tracking of document and no longer relies on domwindowopened/closed!

Comment 37

[Alexandre Poirot [:ochameau]]

I have a new patch that works very well, it is quite exciting to see the inspector switch between **ANY** document of the browser, flawlessly, without any exception in the console!

I still have to look at the chrome attribute in target...

Comment 38

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

New interdiff with modification addressing comment 32 and 33.

Also, I finally tweaked target.chrome and split it into two different target attributes:
- chrome: are we debugging non-content thing, where we expect a different toolbox
(no dev-edition promotion, different tools)
- isTabTarget: for addon actor and backward compatiblity with chrome debugging on RootActor,
these actors do not requires attach/detach requests and also have slightly different
behavior due to the miss of TabActor features.

Comment 39

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6ef36917ab4

Comment 41

[Alexandre Poirot [:ochameau]]

Attachment: patch v7

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66212ab9c2fb
The condition around target.chrome/target.isTabActor were wrong...
Here is new patch that should make try greener.

Comment 42

[:Gijs (out for now; he/him)]

Does this actually let me select a non-browser window global whose scripts I want to debug?

Comment 43

[:Gijs (out for now; he/him)]

(I'll just note that it's ironic to dupe a bug claiming it has a bad summary and then dupe it to a bug whose summary is clearly specific to "iframes" when the bug you're duping is about "windows", which need not be in iframes at all...)

Comment 44

[Alexandre Poirot [:ochameau]]

It allows you to select any document chrome or content living in the chrome process.
So that it can be iframe, windows, popups, ...
I duped to this bug as the work is being done over here.
Gijs, if you are unhappy with the dupe, please reopen with more details if you think this patch isn't going to address your issue!

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8559798 [details] [diff] [review]
patch v7

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

r+, great work!  There are a few rough edges left to fix, but I don't think anymore review is needed.

Yay, we're now tracking opening and closing frames correctly!

* Browser Content Toolbox menu item (for e10s) is broken
  * No toolbox opens, no errors logged either
* Switch from regular app to main process in existing simulator broken
  * See comment in webbrowser.js:992

::: browser/devtools/debugger/debugger-controller.js
@@ +205,5 @@
>      this.client = client;
>  
>      if (target.isAddon) {
>        yield this._startAddonDebugging(actor);
> +    } else if (target.chrome && !target.isTabActor) {

Does |target.chrome| matter here?  Isn't it only |!target.isTabActor| that matters?

This check is about what APIs to use when attaching only, right?

@@ +212,1 @@
>        yield this._startChromeDebugging(chromeDebugger);

It's sad how similar |_startChromeDebugging| and |_startDebuggingTab| are!  Someone should clean it up in the future!

::: browser/devtools/framework/connect/connect.js
@@ +228,5 @@
>  /**
>   * The user clicked on one of the buttons.
>   * Opens the toolbox.
>   */
> +function openToolbox(form, chrome=false, tool="webconsole", isTabActor) {

Yay, even more boolean params! :/ 

It would be nice if this took and options object instead, but I don't care as much here, since we want to remove this file anyway.

::: browser/devtools/framework/target.js
@@ +310,5 @@
>      return this._client;
>    },
>  
> +  // Tells us if we are debugging content document
> +  // or if we are debugging chrome stuff

Maybe clarify that the intended purpose is to limit which tools and features are available in chrome vs. not?

::: browser/devtools/framework/toolbox-process-window.js
@@ +67,5 @@
>  function onCloseCommand(event) {
>    window.close();
>  }
>  
> +function openToolbox(form, chrome, isTabActor) {

Please use an options object parameter here, so that it's clear at each call site what the boolean args mean.

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +212,5 @@
>      let target = this._target;
>      let { client, form } = target;
> +    // Some actors like AddonActor or RootActor for chrome debugging
> +    // do not support attach/detach and can be used directly
> +    if (target.chrome && !target.isTabActor) {

Does |target.chrome| matter here?  Isn't it only |!target.isTabActor| that matters?

This check is about what APIs to use when attaching only, right?

::: browser/devtools/performance/modules/front.js
@@ +102,5 @@
>    /**
>     * Initializes a connection to the profiler actor.
>     */
>    _connectProfilerActor: Task.async(function*() {
>      // Chrome debugging targets have already obtained a reference

Maybe this comment should be updated since you removed the block it was about...?

::: browser/devtools/profiler/utils/shared.js
@@ +82,5 @@
>  
>      // Local debugging needs to make the target remote.
>      yield this._target.makeRemote();
>  
>      // Chrome debugging targets have already obtained a reference

Maybe this comment should be updated since you removed the block it was about...?

::: browser/devtools/webconsole/hudservice.js
@@ +203,5 @@
>  
>      let target;
>      function getTarget(aConnection)
>      {
>        let options = {

Seems like this is redundant and you can just pass |aConnection| now.

::: browser/devtools/webide/modules/app-manager.js
@@ +219,5 @@
>      if (this.selectedProject.type == "mainProcess") {
> +      // Fx >=37 exposes a ChromeActor to debug the main process
> +      if (this.connection.client.mainRoot.traits.allowChromeProcess) {
> +        return this.connection.client.attachProcess()
> +            .then(aResponse => {

Nit: there's no great way to do it, but maybe align |.then| under |.connection|

::: toolkit/devtools/server/actors/chrome.js
@@ +17,5 @@
> + * RootActor.attachProcess request.
> + * ChromeActor exposes all tab actors via its form() request, like TabActor.
> + *
> + * History lecture:
> + * All tab actors were used to also be registered as global actors,

Nit: were used to also -> used to also

@@ +21,5 @@
> + * All tab actors were used to also be registered as global actors,
> + * so that the root actor was also exposing tab actors for the main process.
> + * Tab actors ended up having RootActor as parent actor,
> + * but more and more features of the tab actors were relying on TabActor.
> + * So we are now exposing tab actors for main process with a TabActor inherited

Maybe "we are now exposing a process actor that offers the same API as TabActor by inheriting its functionality"?

@@ +41,5 @@
> +  if (!window) {
> +    window = Services.wm.getMostRecentWindow(null);
> +  }
> +
> +  Object.defineProperty(this, "docShell", {

Why do this here instead of on the prototype?

@@ +102,5 @@
> +ChromeActor.prototype._attach = function() {
> +  TabActor.prototype._attach.call(this);
> +
> +  // Listen for any new/destroyed chrome docshell
> +  Services.obs.addObserver(this, "chrome-webnavigation-create", false);

Probably should remove these somewhere on detach.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +988,5 @@
>    _docShellsToWindows: function (docshells) {
>      return docshells.map(docShell => {
> +      let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIWebProgress);
> +      let window = webProgress.DOMWindow;

If I connect to simulator 3.0 and:

* Open toolbox for Clock app
* Choose Main Process from project menu

the main process toolbox fails to open, and I see the error:

Handler function threw an exception: [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIWebProgress.DOMWindow]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js :: TabActor.prototype._docShellsToWindows/< :: line 992"  data: no]
Stack: TabActor.prototype._docShellsToWindows/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:992:10
TabActor.prototype._docShellsToWindows@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:989:0
TabActor.prototype._notifyDocShellsUpdate@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:1015:18
TabActor.prototype._onDocShellCreated/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:967:6
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:13
Line: 992, column: 0

If I then try to re-open the main process toolbox via the wrench, it opens successfully.

Maybe this is unrelated to your work...?  Perhaps WebIDE is being crazy?  It seems worse than what happens without this patch, though.

Comment 46

[Philipp Kewisch [:Fallen][📱📆]]

I'm having trouble using the connect page to connect to a Thunderbird target. I'm using the v6 try builds (since they are the last that build mac).

Depending on Firefox's mood I'm getting one or more of the following errors even before the connection is established:

Timestamp: 2/11/15, 12:22:21 AM
Error: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/security/socket.js :: <TOP_LEVEL> :: line 12"  data: no]
Source File: chrome://browser/content/devtools/connect.js
Line: 45

Timestamp: 2/11/15, 12:42:36 AM
Error: DebuggerSocket is not defined
Source File: resource://gre/modules/devtools/dbg-client.jsm
Line: 381

I tried to use WebIDE which at least connects, but only tabs are shown. I don't see a way to select a chrome window or even the main process from WebIDE. I've set DebuggerServer.allowChromeProcess = true when initializing the Thunderbird Debugger Server and also imported the toolkit bits from the patch.

Comment 47

[Alexandre Poirot [:ochameau]]

(In reply to Philipp Kewisch [:Fallen] from comment #46)
> Timestamp: 2/11/15, 12:22:21 AM
> Error: [Exception... "Component returned failure code: 0x80570016
> (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult:
> "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame ::
> resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/security/socket.js :: <TOP_LEVEL> :: line
> 12"  data: no]
> Source File: chrome://browser/content/devtools/connect.js
> Line: 45

That looks like the exception happenning if you open the connect page in OOP tab.
Try opening it from non-e10s window.

> I tried to use WebIDE which at least connects, but only tabs are shown. I
> don't see a way to select a chrome window or even the main process from
> WebIDE. I've set DebuggerServer.allowChromeProcess = true when initializing
> the Thunderbird Debugger Server and also imported the toolkit bits from the
> patch.

Ideally you would be using WebIDE to connect to thunderbird,
we would like to get rid of the connect page that is poorly maintained.
See bug 1111748. If you apply this patch to firefox and set DebuggerServer.allowChromeProcess = true,
you should see the "Main process" item in the app list (In "open app" left menu).
Also it should appear today if you use firefox and thunderbird as-is. If not, could you please fill a bug blocking bug 1111748 in order to ensure that webide works against TB. Thanks!

Comment 48

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Alexandre Poirot [:ochameau] from comment #47)
> That looks like the exception happenning if you open the connect page in OOP
> tab.
> Try opening it from non-e10s window.
Oh duh. I have e10s disabled on my main profile but was using a secondary profile where I forgot about this. This does allow me to connect. Thanks for the hint!

> > I tried to use WebIDE which at least connects, but only tabs are shown. I
> > don't see a way to select a chrome window or even the main process from
> > WebIDE. I've set DebuggerServer.allowChromeProcess = true when initializing
> > the Thunderbird Debugger Server and also imported the toolkit bits from the
> > patch.
Great, I got this to work, both in the connect page and in WebIDE. Without the patch on the server side, the main process showed up. When I applied the patch server side it first didn't work, but it was because I set DebuggerServer.allowChromeProcess = true after calling init() and addBrowserActors(). This fails because the trait is set from the value of DebuggerServer.allowChromeProcess when the file is loaded. Do you think you could make the trait a getter that returns the current value, or at least add a comment to the trait that it needs to be set before the file is loaded?

Thank you so much for working on this feature, it makes chrome debugging so much nicer. Now that we have this, you might be able to get rid of DebuggerServer.chromeWindowType completely?

Comment 49

[Philipp Kewisch [:Fallen][📱📆]]

One more, I'm getting this on disconnect on the server side:

Handler function DebuggerTransport.prototype.onInputStreamReady threw an exception: TypeError: this._progressListener is null
Stack: ChromeActor.prototype._detach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/chrome.js:136:5
BTA_disconnect@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:808:5
AP_cleanup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/common.js:284:7
DSC_onClosed/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1542:40
DSC_onClosed@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1542:5
DebuggerTransport.prototype.close@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:203:7
DebuggerTransport.prototype.onInputStreamReady<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:346:7
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
Line: 136, column: 4

Comment 50

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Philipp Kewisch [:Fallen] from comment #49)
> One more, I'm getting this on disconnect on the server side:
> 
> Handler function DebuggerTransport.prototype.onInputStreamReady threw an
> exception: TypeError: this._progressListener is null
> Stack:
> ChromeActor.prototype._detach@resource://gre/modules/commonjs/toolkit/loader.
> js -> resource://gre/modules/devtools/server/actors/chrome.js:136:5
> BTA_disconnect@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/webbrowser.js:808:5
> AP_cleanup@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/actors/common.js:284:7
> DSC_onClosed/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:1542:40
> DSC_onClosed@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/server/main.js:1542:5
> DebuggerTransport.prototype.close@resource://gre/modules/commonjs/toolkit/
> loader.js -> resource://gre/modules/devtools/transport/transport.js:203:7
> DebuggerTransport.prototype.onInputStreamReady<@resource://gre/modules/
> commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/transport/transport.js:346:7
> makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://gre/modules/devtools/DevToolsUtils.js:82:14
> Line: 136, column: 4

I also saw this issue in my own testing, but forget to report it before.

Comment 51

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to J. Ryan Stinnett [:jryans] from comment #45)
> If I connect to simulator 3.0 and:
> 
> * Open toolbox for Clock app
> * Choose Main Process from project menu
> 
> the main process toolbox fails to open, and I see the error:

More specific steps:

1. Disable "remember last project" & "reconnect to prev. runtime" (easier to reproduce with this off)
2. Open a fresh WebIDE window
3. Connect to your favorite simulator, let's go with 2.2
  * No toolbox opens automatically (as expected)
4. Choose the Clock runtime app
  * Toolbox opens automatically (as expected)
5. Choose Main Process
  * No toolbox, operation timed out error

Comment 52

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

Fixed comments positively except the following ones:

(In reply to J. Ryan Stinnett [:jryans] from comment #45)
> Comment on attachment 8559798 [details] [diff] [review]
>
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +205,5 @@
> >      this.client = client;
> >  
> >      if (target.isAddon) {
> >        yield this._startAddonDebugging(actor);
> > +    } else if (target.chrome && !target.isTabActor) {
> 
> Does |target.chrome| matter here?  Isn't it only |!target.isTabActor| that
> matters?
> 
> This check is about what APIs to use when attaching only, right?

Yes, it should be only about having to call attach/detach.
But I can't remember why I had to keep checking for target.chrome... may be some funky test setup.
I've let it as-is until I'm able to reproduce the issue I've seen with just checking isTabActor.

> 
> ::: browser/devtools/framework/connect/connect.js
> @@ +228,5 @@
> >  /**
> >   * The user clicked on one of the buttons.
> >   * Opens the toolbox.
> >   */
> > +function openToolbox(form, chrome=false, tool="webconsole", isTabActor) {
> 
> Yay, even more boolean params! :/ 
> 
> It would be nice if this took and options object instead, but I don't care
> as much here, since we want to remove this file anyway.

I've let it as-is, I so want to get rid of the connect page.
I keep hearing people using it :/

> 
> @@ +41,5 @@
> > +  if (!window) {
> > +    window = Services.wm.getMostRecentWindow(null);
> > +  }
> > +
> > +  Object.defineProperty(this, "docShell", {
> 
> Why do this here instead of on the prototype?
> 

It sounds easier like this, but I moved it on prototype with a lazy getter.




In this patch, I also did a getter for allowProcessDebugging trait, suggested by philipp in comment 48.
And I tweaked webbrowser.js in order to allow closing the original top level window.
(without that the browser toolbox was lost when closing it)

Comment 53

[Alexandre Poirot [:ochameau]]

Attachment: Switch to the next available top level window when the current one is destroyed

I'm going to split the patch is multiple meaninful pieces.
The previous interdiff contains all these pieces, I'm just spliting.

This one is about the fix to support closing the original top level window
the browser toolbox connected to.

Comment 54

[Alexandre Poirot [:ochameau]]

Attachment: Add Target.isTabActor to tell if the remote tab actor supports attach/detach requests

You already r+ with comments this patch, but I'm r? again for sanity checks.
I've let the target.chrome checks, but I'll look into this.

Comment 55

[Alexandre Poirot [:ochameau]]

Attachment: patch v8

And the remaining diff, more focused on ChromeActor.

Comment 56

[Alexandre Poirot [:ochameau]]

I'm still unable to reproduce the issue with the main process toolbox in WebIDE...
Could it be a Mac-specific issue?
Could you give it another try with this new patch, it could potentialy fix various exception.

Comment 57

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=547a01f633f9
Try is still not green...

Comment 58

[Alexandre Poirot [:ochameau]]

Attachment: new interdiff

Here is some more modification to hopefully get a fully green try.
Browser scratchpad was broken.
xpcshell support also, as it is quite special and there is no document available.
But most of this patch is about converting tests using tab actors directly on root actor to chrome actor.

I'll shortly dispatch these modification to existing attachments.

Comment 59

[Alexandre Poirot [:ochameau]]

Attachment: patch v9

Comment 60

[Alexandre Poirot [:ochameau]]

Attachment: fix tests

Yet another patch, dedicated to fix tests.

Comment 61

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8517c2c5153

Comment 62

[Alexandre Poirot [:ochameau]]

Attachment: interdiff 3

Fix mochitest chrome:
- local runtime was broken in WebIDE (missing allowChromeProcess)
- toolboxPromise in webIDE isn't always a Promise.jsm one and only expose then (and not catch).
[I don't get why this patch changed that...]

Comment 63

[Alexandre Poirot [:ochameau]]

Attachment: patch v10

Comment 64

[Alexandre Poirot [:ochameau]]

Attachment: interdiff 4

Fixed webconsole test using chrome actor

Comment 65

[Alexandre Poirot [:ochameau]]

Attachment: fix tests v2

Updated the patch with interdiff 4, to fix console tests.

Comment 66

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60a607475a6a

Comment 67

[Alexandre Poirot [:ochameau]]

Try is green!

Comment 68

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8565053 [details] [diff] [review]
interdiff

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

::: toolkit/devtools/server/actors/root.js
@@ +159,5 @@
>      // to debug chrome and any non-content ressource via attachProcess request
>      // if allocChromeProcess is defined, but not true, it means that root actor
>      // no longer expose tab actors, but also that attachProcess forbids
>      // exposing actors for security reasons
> +    get allowChromeProcess() DebuggerServer.allowChromeProcess,

This is a Mozilla-specific getter syntax, use:

get allowChromeProcess() {
  return DebuggerServer.allowChromeProcess;
}

Comment 69

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8565122 [details] [diff] [review]
new interdiff

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

::: toolkit/devtools/server/actors/webbrowser.js
@@ +787,3 @@
>      };
>  
> +    if (this.window) {

Is this for xpcshell too?  Add a comment about why this case happens.

Comment 70

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8565383 [details] [diff] [review]
interdiff 3

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

::: browser/devtools/webide/content/webide.js
@@ +944,5 @@
>      // Only have a live toolbox if |this.toolboxPromise| exists
>      if (this.toolboxPromise) {
>        let toolboxPromise = this.toolboxPromise;
>        this.toolboxPromise = null;
> +      toolboxPromise.then(toolbox => {

We really do want to return the promise though, right?

@@ +949,2 @@
>          return toolbox.destroy();
> +      }).then(null, console.error)

|catch| should be safe to use now, I had to change the head.js for tests.  Have you fully rebased?

Comment 71

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8565061 [details] [diff] [review]
Add Target.isTabActor to tell if the remote tab actor supports attach/detach requests

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

::: browser/devtools/debugger/debugger-controller.js
@@ +205,5 @@
>      this.client = client;
>  
>      if (target.isAddon) {
>        yield this._startAddonDebugging(actor);
> +    } else if (target.chrome && !target.isTabActor) {

I still want to know if you can check only |!target.isTabActor| here.  That's all you need right?  Or is it something to do with backward compatibility with servers before this change?

If true, you should rename |_startChromeDebugging|, since it's not really about that anymore.

::: browser/devtools/netmonitor/netmonitor-controller.js
@@ +212,5 @@
>      let target = this._target;
>      let { client, form } = target;
> +    // Some actors like AddonActor or RootActor for chrome debugging
> +    // do not support attach/detach and can be used directly
> +    if (target.chrome && !target.isTabActor) {

Again, I want to know why.

Comment 72

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8565385 [details] [diff] [review]
patch v10

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

webide.js seems wrong, but maybe I am confused.

::: browser/devtools/webide/content/webide.js
@@ +939,5 @@
>      // Only have a live toolbox if |this.toolboxPromise| exists
>      if (this.toolboxPromise) {
>        let toolboxPromise = this.toolboxPromise;
>        this.toolboxPromise = null;
> +      toolboxPromise.then(toolbox => {

See comments on interdiff.

::: toolkit/devtools/server/actors/webconsole.js
@@ +1503,5 @@
>      // This method is called after this.window is changed,
>      // so we register new listener on this new window
>      this.onStartListeners({listeners: listeners});
> +
> +    // Also reset the cached top level chrome window being targetted

Nit: targeted

Comment 73

[Alexandre Poirot [:ochameau]]

Attachment: interdiff

(In reply to J. Ryan Stinnett [:jryans] from comment #69)
> Comment on attachment 8565122 [details] [diff] [review]
> ::: toolkit/devtools/server/actors/webbrowser.js
> @@ +787,3 @@
> >      };
> >  
> > +    if (this.window) {
> 
> Is this for xpcshell too?  Add a comment about why this case happens.

yes, comment added.


(In reply to J. Ryan Stinnett [:jryans] from comment #70)
> Comment on attachment 8565383 [details] [diff] [review]
> ::: browser/devtools/webide/content/webide.js
> @@ +944,5 @@
> >      // Only have a live toolbox if |this.toolboxPromise| exists
> >      if (this.toolboxPromise) {
> >        let toolboxPromise = this.toolboxPromise;
> >        this.toolboxPromise = null;
> > +      toolboxPromise.then(toolbox => {
> 
> We really do want to return the promise though, right?

Oops, yes, fixed.

> 
> @@ +949,2 @@
> >          return toolbox.destroy();
> > +      }).then(null, console.error)
> 
> |catch| should be safe to use now, I had to change the head.js for tests. 
> Have you fully rebased?

Yes, and it still fails, so I kept the catch -> then(null, console.error) modification.
I don't want to figure this out in this bug. It can come from many places, I see the deprecated promises being used in dbg-client.jsm and app manager :s

(In reply to J. Ryan Stinnett [:jryans] from comment #71)
> Comment on attachment 8565061 [details] [diff] [review]
> Add Target.isTabActor to tell if the remote tab actor supports attach/detach
> requests
> 
> Review of attachment 8565061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +205,5 @@
> >      this.client = client;
> >  
> >      if (target.isAddon) {
> >        yield this._startAddonDebugging(actor);
> > +    } else if (target.chrome && !target.isTabActor) {
> 
> I still want to know if you can check only |!target.isTabActor| here. 
> That's all you need right?  Or is it something to do with backward
> compatibility with servers before this change?
> 

Thanks for reminding me that story again! It appears that it was a bug in this patch...
In target.js I was only setting isTabActor for remote target, so that it was broken for non-OOP tabs!!!
Now that I set it correctly for all targets, tests pass again without the target.chrome checks in debugger and netmon.

> If true, you should rename |_startChromeDebugging|, since it's not really
> about that anymore.

I really miss naming ideas. The difference between both is that the "chrome" one connects to the root actor and uses client.attachThread whereas the "tab" one connects to the tab actors and uses target.activeTab.attachThread (with different arguments :s).
For netmon, both chrome and tab are calling client.attachConsole but with different actorID.

Comment 74

[Alexandre Poirot [:ochameau]]

Attachment: Add Target.isTabActor to tell if the remote tab actor supports attach/detach requests

Comment 75

[Alexandre Poirot [:ochameau]]

Attachment: patch v11

Comment 76

[Alexandre Poirot [:ochameau]]

Comment on attachment 8568602 [details] [diff] [review]
patch v11

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7affaa358f5

Comment 77

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8568602 [details] [diff] [review]
patch v11

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

Great work! :D

Comment 78

[Alexandre Poirot [:ochameau]]

Attachment: cleanup highlighter.js r=pbrosset

Rebased patch, just to be sure it will apply correctly in case I fixed some conflicts locally over time...

Comment 79

[Alexandre Poirot [:ochameau]]

Attachment: Switch to the next available top level window when the current one is destroyed.

Comment 80

[Alexandre Poirot [:ochameau]]

Attachment: Add Target.isTabActor to tell if the remote tab actor supports attach/detach requests

Comment 81

[Alexandre Poirot [:ochameau]]

Attachment: patch v11

Comment 82

[Alexandre Poirot [:ochameau]]

Attachment: Fix tests to support chrome actor

Comment 83

[Alexandre Poirot [:ochameau]]

And another try, just to be sure:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b7dc5284c9

Comment 84

[Alexandre Poirot [:ochameau]]

Attachment: Fix docshell property

One last minute change, I reverted back ChromeActor.docShell to original proposal.
Setting it on the prototype doesn't work.

Comment 85

[Alexandre Poirot [:ochameau]]

And a new try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d34223d41c96

Comment 86

[Alexandre Poirot [:ochameau]]

Comment on attachment 8569806 [details] [diff] [review]
cleanup highlighter.js r=pbrosset

Carrying r+ after rebase.

Comment 87

[Alexandre Poirot [:ochameau]]

Attachment: patch v12

Merged the two patches and should fix the xpcshell error.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b5e4bd33503

Comment 88

[Alexandre Poirot [:ochameau]]

Comment on attachment 8570162 [details] [diff] [review]
patch v12

Try is green, carrying over r+.

Comment 89

[Alexandre Poirot [:ochameau]]

Checkin needed for all the patches.

Comment 90

[Carsten Book [:Tomcat]]

patch v12 failed to apply:

renamed 1059308 -> Bug-1059308---Make-frame-selection-button-to-work-.patch
applying Bug-1059308---Make-frame-selection-button-to-work-.patch
patching file browser/devtools/webconsole/webconsole.js
Hunk #1 FAILED at 431
1 out of 2 hunks FAILED -- saving rejects to file browser/devtools/webconsole/webconsole.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug-1059308---Make-frame-selection-button-to-work-.patch

could you take a look, thanks!

Comment 91

[Alexandre Poirot [:ochameau]]

Attachment: patch v13

Rebased against current master.

Comment 92

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/77dbb9002dbf
https://hg.mozilla.org/integration/fx-team/rev/97707eff3199
https://hg.mozilla.org/integration/fx-team/rev/4f1e44bd6204
https://hg.mozilla.org/integration/fx-team/rev/4696a52daeed
https://hg.mozilla.org/integration/fx-team/rev/2433abf44e53

Comment 93

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=bc64e4250685 seems one of this changes caused :

https://treeherder.mozilla.org/logviewer.html#?job_id=2141984&repo=fx-team

Comment 94

[Alexandre Poirot [:ochameau]]

Attachment: patch v14

Fix a typo in TabActor._detach (processListener instead of progressListener :/)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca5bb48656a

Comment 95

[Alexandre Poirot [:ochameau]]

Comment on attachment 8572594 [details] [diff] [review]
patch v14

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

Try is green!

Comment 96

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/ec47d96bca03
https://hg.mozilla.org/integration/fx-team/rev/227dfa987953
https://hg.mozilla.org/integration/fx-team/rev/d155ba13541c
https://hg.mozilla.org/integration/fx-team/rev/7635da795416
https://hg.mozilla.org/integration/fx-team/rev/7697ad4919e7

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec47d96bca03
https://hg.mozilla.org/mozilla-central/rev/227dfa987953
https://hg.mozilla.org/mozilla-central/rev/d155ba13541c
https://hg.mozilla.org/mozilla-central/rev/7635da795416
https://hg.mozilla.org/mozilla-central/rev/7697ad4919e7

Comment 98

[Will Bamberg [:wbamberg]]

I've added a section to the Browser toolbox page on this: /en-US/docs/Tools/Browser_Toolbox#iframe_selection

Please let me know if it works for you!

Comment 99

[Alexandre Poirot [:ochameau]]

Just two comments:

1) target specific iframes -> target specific documents

For the browser toolbox, we don't list only frames of the topmost window,
but all the top level windows and their iframes. So it isn't just iframes, but all the documents of the main process.

2) The screenshot. It may help to take a screenshot of the whole toolbox, or at least show the button on which this menu appear. That, to help figure out on which button to click to get this feature.

Thanks Will!

Comment 100

[Will Bamberg [:wbamberg]]

Thanks Alex.

I've rewritten that section and changed the title. I've also refreshed the other screenshots in that page, as they were pretty old. I'm going to mark this as dev-doc-complete now, but please do let me know if there's anything else you need there.