1401343 - Provide ability to run privileged code in a remote tab/content scope

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Matthew N. [:MattN]]

Example: I want to call input.getAutocompleteInfo()[1] on an <input> that is in a tab which is remote. getAutocompleteInfo is ChromeOnly so can only be called from privileged scopes, so I can't call it from the Web Console for the tab.

Prior to e10s I could simply call the method on the <input> from the Browser Toolbox or Browser Console as the <input> would exist in the same tree as browser.xul. With e10s I can't get a reference to the input from the parent process (at least not without a CPOW).

E10S has regressed my ability to debug content pages issues when it comes to using privileged APIs, not just ChromeOnly WebIDL but also passing DOM nodes to a JSM in the content process for debugging/iterating on some code.

Workaround:
1) Open the Browser Content Toolbox
2) Switch to the debugger tab
3) Find your favourite frame script that listens for an event you can get to fire in the context of the page you want to debug and set a breakpoint in that event listener
4) Trigger that event in the content page.
5) Now that the breakpoint is hit, you now have priveleged access to the event target which gives you access to the window and document of the tab you want to debug. You can now get a reference to the input and run your privileged code. e.g. 
> event.target.ownerDocument.querySelectorAll("#myinput").getAutocompleteInfo()

It would be so much nicer if the sandbox of the Browser Content Toolbox had a getter for the currently selected tab's scope or a way to enumerate all the content scopes in the process.

Ideally the solution would also handle multiple content processes with e10s-multi.

[1] https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/dom/webidl/HTMLInputElement.webidl#184-187

Comment 1

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

Bug 1346316 attempts to improve this somewhat by at least giving Browser Content Toolbox a useful global scope to start from.  I should probably try to finish that...

Still probably not as easy as the pre-e10s world, though.

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1401343 - Expose a 'tabs' getter in the child-process actor.

Review commit: https://reviewboard.mozilla.org/r/183728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183728/

Comment 3

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

Comment on attachment 8912406 [details]
Bug 1401343 - Expose a 'tabs' getter in the child-process actor.

https://reviewboard.mozilla.org/r/183728/#review188934

Thanks, I think this seems reasonable to me!  I am curious what :ochameau thinks, so I'll request his review as well.

::: devtools/server/actors/child-process.js:43
(Diff revision 1)
> +      return windows;
> +    }
> +  };
> +
>    // Scope into which the webconsole executes:
> -  // An empty sandbox with chrome privileges
> +  // An sandbox with chrome privileges with a `contentWindows` getter.

Nit: An -> A

Comment 4

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #0)
> It would be so much nicer if the sandbox of the Browser Content Toolbox had
> a getter for the currently selected tab's scope

So this is bug 1346316.

> or a way to enumerate all the content scopes in the process.

This is what I attached in comment 2 by exposing a `contentWindows` getter in the Sandbox.

Ideally the frame picker UI would list all windows in the process and it seems like the Tab actor code that handles this would be very similar to what would be needed for the child-process actor. Do you think it would make sense to share the relevant Tab actor code with the child-process actor to reduce duplication?

I think just exposing the getter would be great in the short term and I could file another bug for the frame (window/docshell) picker. If you don't want to expose the getter then we can use this bug for the picker.

What do you think?

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8912406 [details]
Bug 1401343 - Expose a 'tabs' getter in the child-process actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183728/diff/1-2/

Comment 6

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

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> (In reply to Matthew N. [:MattN] from comment #0)
> > It would be so much nicer if the sandbox of the Browser Content Toolbox had
> > a getter for the currently selected tab's scope
> 
> So this is bug 1346316.
> 
> > or a way to enumerate all the content scopes in the process.
> 
> This is what I attached in comment 2 by exposing a `contentWindows` getter
> in the Sandbox.
> 
> Ideally the frame picker UI would list all windows in the process and it
> seems like the Tab actor code that handles this would be very similar to
> what would be needed for the child-process actor. Do you think it would make
> sense to share the relevant Tab actor code with the child-process actor to
> reduce duplication?
> 
> I think just exposing the getter would be great in the short term and I
> could file another bug for the frame (window/docshell) picker. If you don't
> want to expose the getter then we can use this bug for the picker.
> 
> What do you think?

Right, I think it's fine to proceed with this quick fix personally, since it solves a real need.  We can always change to something more advanced later, even if it causes some kind of breaking change in the properties exposed by the BCT, since it's relatively easy to communicate this to the set of browser devs.

However, I am curious if :ochameau feels strongly one way or the other.

Comment 7

[Alexandre Poirot [:ochameau]]

I think we should make one bug duplicate of the other as we end up having the exact same discussion between here and bug 1346316!

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> (In reply to Matthew N. [:MattN] from comment #0)
> > or a way to enumerate all the content scopes in the process.
> 
> This is what I attached in comment 2 by exposing a `contentWindows` getter
> in the Sandbox.

This is really close to what I suggested doing in bug 1346316 comment 6.
Except that I also imagined having access to the TabChildGlobal, which contains frame script global.
(unless they now execute in their own private scope?)
It will allow you to get access to: addMessageListener, sendAsyncMessage, content and docShell.

Having said that, we can start with just `contentWindows` and have something else for TabChild global object?

> Ideally the frame picker UI would list all windows in the process and it
> seems like the Tab actor code that handles this would be very similar to
> what would be needed for the child-process actor. Do you think it would make
> sense to share the relevant Tab actor code with the child-process actor to
> reduce duplication?

As said in bug 1346316, I originaly introduced ChildProcessActor without any convinction.
To me, Browser Content Toolbox should be using a TabActor, like the Browser Toolbox.
We may have to still have a dedicated class inherited from TabActor in order to have TabChild global as global instead of document global. Otherwise you will most likely still have content privileges in the console...

So, yes, it would be great to have the frame picker in Browser Content Toolbox.
To do that, make ChildProcessActor inherit from TabActor.
At the end, ChildProcessActor may just be ChromeActor, or inherit from it.
I don't know exactly what necessary tweaks would be required, but at least changing this to fetch a TabChild global:
  http://searchfox.org/mozilla-central/source/devtools/server/actors/chrome.js#45-62

> I think just exposing the getter would be great in the short term and I
> could file another bug for the frame (window/docshell) picker. If you don't
> want to expose the getter then we can use this bug for the picker.

+1 switching to TabActor may not be trivial...

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8912406 [details]
Bug 1401343 - Expose a 'tabs' getter in the child-process actor.

https://reviewboard.mozilla.org/r/183728/#review189192

::: devtools/server/actors/child-process.js:36
(Diff revision 2)
> +  let sandboxPrototype = {
> +    get contentWindows() {
> +      let windows = [];
> +      let windowEnumerator = Services.ww.getWindowEnumerator();
> +      while (windowEnumerator.hasMoreElements()) {
> +        windows.push(windowEnumerator.getNext());

I'm wondering if we should QueryInterface to something? Sometimes when using Enumerators, you end up with only a nsISupports interface object if noone ever QIed this object.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8912406 [details]
Bug 1401343 - Expose a 'tabs' getter in the child-process actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183728/diff/2-3/

Comment 10

[Matthew N. [:MattN]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> I think we should make one bug duplicate of the other as we end up having
> the exact same discussion between here and bug 1346316!
> 
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #4)
> > (In reply to Matthew N. [:MattN] from comment #0)
> > > or a way to enumerate all the content scopes in the process.
> > 
> > This is what I attached in comment 2 by exposing a `contentWindows` getter
> > in the Sandbox.
> 
> This is really close to what I suggested doing in bug 1346316 comment 6.
> Except that I also imagined having access to the TabChildGlobal, which
> contains frame script global.
> (unless they now execute in their own private scope?)
> It will allow you to get access to: addMessageListener, sendAsyncMessage,
> content and docShell.

I had looked at that bug but didn't read your comment… interesting that we both came up with a similar solution.

I like your TabChildGlobal idea better so I switched to using it.

jryans, are you also fine with the TabChildGlobal approach?

Comment 11

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

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #10)
> I had looked at that bug but didn't read your comment… interesting that we
> both came up with a similar solution.
> 
> I like your TabChildGlobal idea better so I switched to using it.
> 
> jryans, are you also fine with the TabChildGlobal approach?

Yes, works for me!  We can always tweak down the road.

Comment 12

[Matthew N. [:MattN]]

I'll land it after a try push then. Thanks for the quick reviews everyone!

It sounds like bug 1346316 can/will cover the frame picker UI so I won't file a new bug for it.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8912406 [details]
Bug 1401343 - Expose a 'tabs' getter in the child-process actor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183728/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1b4bde4dadd
Expose a 'tabs' getter in the child-process actor. r=jryans,ochameau

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f1b4bde4dadd

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something you wanted to nominate for Beta approval in the interests of the DevEdition users or can this ride the 58 train?

Comment 17

[Matthew N. [:MattN]]

It's trivial but I don't see a real need to bother with the approval process.

Comment 18

[Matthew N. [:MattN]]

I updated docs at https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Debugging_frame_scripts