1441711 - Connect Console to multiple processes

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

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

With Site Isolation, a page's frames can be spread across multiple processes.  In addition, there are workers, shared workers, service workers, etc. that may also run in separate processes from the page.

We should move to a model where the Console is connecting to all of these targets of interest in each of their respective processes to retrieve messages, listen for new messages, etc.

The exact design and APIs for this in DevTools are still TBD, but we can use this bug to collect conversations around this topic.

Comment 1

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

I'll use this as a testing ground for Site Isolation API design.

Comment 2

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

Attachment: Bug 1441711 - Console: Use resources for Browser Console / Toolbox.

For the Browser Console or Toolbox, we want to also include console messages
from other processes. If the resource actor is available, use this to make
additional connections to the other processes. With this approach, we are
connecting to a separate console actor for each process, so DevTools code is in
control of whether messages need to flow across processes.

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

Comment 3

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

Comment on attachment 8993197 [details]
Bug 1441711 - Console: Use resources for Browser Console / Toolbox.

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

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8993197 [details]
Bug 1441711 - Console: Use resources for Browser Console / Toolbox.

https://reviewboard.mozilla.org/r/258006/#review264966

I'm concerned about the switch from one proxy to many of them, but I imagine Nicolas would confirm if there is any issue here.
Otherwise I would have seen the resource actor being used one level up so that it manages all the targets, including the top level one. But I'll do more comments about that in bug 1471754.

::: devtools/client/webconsole/webconsole-connection-proxy.js:226
(Diff revision 2)
> +      for (const target of targets) {
> +        this._addChildConnection(target);
> +      }
> +      resources.processes.listen("added", ({ target }) => {
> +        this._addChildConnection(target);
> +      });

I'm wondering if on the long term, we would like to have the resources front used one level higher.
So that all the targets are fetched from it, including the target for the top-level document.
I think it would be beneficial as all the targets would go through the same codepath instead of having one dedicated for the top-level one.

More concretely, my suggestion here is:
instead of creating proxies from within proxy, we would create all the proxies from an upper layer, outside of Proxy class.

Now, that may not fit the needs of all tools, but for  the current purpose of the console, I believe it would be easier to understand and maintain.

::: devtools/client/webconsole/webconsole-connection-proxy.js:231
(Diff revision 2)
> +      });
> +    }
> +  },
> +
> +  _addChildConnection(target) {
> +    const child = new WebConsoleConnectionProxy(this.webConsoleFrame, target);

I'm wondering if spawing more than one WebConsoleConnectionProxy is that easy.
The frame currently expect to be bound to only one:
  https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-frame.js#217
You do forward disconnect, so we are good for shutdown, but what about all the other proxy methods called from the webconsole codebase.
Like releaseActor:
  https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-frame.js#320
Are we going to correctly free objects from frames?
Or here, are we still going to fetch the string correctly?
  https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-output-wrapper.js#103
More generally, we may have to look into all the callsites:
https://searchfox.org/mozilla-central/search?q=proxy.&case=true&regexp=false&path=devtools%2Fclient%2Fwebconsole

Also, I'm wondering what happens regarding network requests and this cryptic code:
https://searchfox.org/mozilla-central/source/devtools/client/webconsole/enhancers/net-provider.js#55-60
We may have to refactor this to be done by the proxy.

May be doing what you do here is the right thing, but may be proxy should be a thing that proxies to multiple fronts, while a significant part of the existing class, that looks really like a front/client, should be moved to devtools/shared/{client/fronts}?
connect, _attachConsole, _onAttachConsole, _onCachedMessages and disconnect should be in a client or front class rather than here.

Comment 5

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

Comment on attachment 8993197 [details]
Bug 1441711 - Console: Use resources for Browser Console / Toolbox.

https://reviewboard.mozilla.org/r/258006/#review264966

> I'm wondering if on the long term, we would like to have the resources front used one level higher.
> So that all the targets are fetched from it, including the target for the top-level document.
> I think it would be beneficial as all the targets would go through the same codepath instead of having one dedicated for the top-level one.
> 
> More concretely, my suggestion here is:
> instead of creating proxies from within proxy, we would create all the proxies from an upper layer, outside of Proxy class.
> 
> Now, that may not fit the needs of all tools, but for  the current purpose of the console, I believe it would be easier to understand and maintain.

Yes, using resources as the first interaction with the protocol to also retrieve the primary target does sound like a cleaner approach overall, and that's what we might do if we were starting fresh.

For the moment, I haven't done that because it was easier to take the incremental step of using resources just for the extra targets.  It think this could be a good approach to start with, and then we could later refactor the startup phase of the toolbox to also get the primary target from resources.

For the proxies here in the Console, either design seems workable to me, so it's up to what seems best for Console maintenance I guess.  By nesting the child proxies inside a primary one, we don't need to alter the calling code that uses the proxy.  We can also go the other way to make it more explicit that there are many proxies, so all callers would be changed to treat them as an array.  I think your suggestion of creating them all at the same layer does seem like a better design that is easier to follow for the future.

> I'm wondering if spawing more than one WebConsoleConnectionProxy is that easy.
> The frame currently expect to be bound to only one:
>   https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-frame.js#217
> You do forward disconnect, so we are good for shutdown, but what about all the other proxy methods called from the webconsole codebase.
> Like releaseActor:
>   https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-frame.js#320
> Are we going to correctly free objects from frames?
> Or here, are we still going to fetch the string correctly?
>   https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-output-wrapper.js#103
> More generally, we may have to look into all the callsites:
> https://searchfox.org/mozilla-central/search?q=proxy.&case=true&regexp=false&path=devtools%2Fclient%2Fwebconsole
> 
> Also, I'm wondering what happens regarding network requests and this cryptic code:
> https://searchfox.org/mozilla-central/source/devtools/client/webconsole/enhancers/net-provider.js#55-60
> We may have to refactor this to be done by the proxy.
> 
> May be doing what you do here is the right thing, but may be proxy should be a thing that proxies to multiple fronts, while a significant part of the existing class, that looks really like a front/client, should be moved to devtools/shared/{client/fronts}?
> connect, _attachConsole, _onAttachConsole, _onCachedMessages and disconnect should be in a client or front class rather than here.

These are all great points!  I think it's mostly up to what design :nchevobbe and others on Console would prefer.

I definitely agree that there are various other methods and APIs I skipped over, like `releaseActor`, etc.  I agree we should likely look into all callsites that accesss the proxy.

Studying all of those callsites might clarify which design is best:

* A primary proxy with nested child proxies (this patch)
* A list of proxies that callers loop over
* A single proxy object that contains a list of fronts for primary and additional connections

Comment 6

[Alexandre Poirot [:ochameau]]

About proxies, in bug 1444132, we will most likely also spawn multiple netmonitor actors, directly controlled by the client.
The more I think about that, the more I see opportunities to have some helper to easily manage multiple fronts of the same kind at once. For example, for the network monitor actor, we would want to call setPreference on all the actors at once (it may be similar to releaseActor) and listen for networkEvent from all of them (it should be similar to attachConsole here).

Otherwise, some prior refactoring may help a lot here:
* releaseActor looks like something useless that would be better handled with pools on the server side.
  Or if that's still useful, at least convert it to "release" and use protocol.js way of doing.
  Then we would call .release() on the fronts instead of passing by the WebConsoleClient.
* we still use old clients for long string, instead of LongStringFront:
  https://searchfox.org/mozilla-central/source/devtools/client/webconsole/webconsole-output-wrapper.js#102-103
  it would be easier by switching to use LongStringFront as we wouldn't have to call getString on WebConsoleClient:
    https://searchfox.org/mozilla-central/source/devtools/shared/webconsole/client.js#742-766
  and instead call .string() on the longStringFront instances:
    https://searchfox.org/mozilla-central/source/devtools/shared/fronts/string.js#29-43
  (instead of passing around "grips", we would be passing LongStringFront instances)

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1441711 - Console: Use resources for Browser Console / Toolbox. r=ochameau,nchevobbe

For the Browser Console or Toolbox, we want to also include console messages
from other processes. If the resource actor is available, use this to make
additional connections to the other processes. With this approach, we are
connecting to a separate console actor for each process, so DevTools code is in
control of whether messages need to flow across processes.

Depends On D4339

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1441711 - Console: Use resources for Browser Console / Toolbox.

For the Browser Console or Toolbox, we want to also include console messages
from other processes. If the resource actor is available, use this to make
additional connections to the other processes. With this approach, we are
connecting to a separate console actor for each process, so DevTools code is in
control of whether messages need to flow across processes.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 9034709 [details]
Bug 1441711 - Console: Use resources for Browser Console / Toolbox.

Here is a new version of this patch that uses existing actors to connect the child processes.
It should help defining a new API which would help listening and connecting to multiple targets.

Comment 10

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1441711 - Console: Use basic resource api for Browser Console / Toolbox. r=ochameau,nchevobbe

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1441711 - Create multiple proxies when in Fission Browser Console. r=ochameau,yulia.

When opening the Browser Console, if the browser toolbox fission pref
is enabled, we create a WebConsoleConnectionProxy for each process.
This allow us to not start the ContentProcessMessages listener (that
uses ContentProcessMessagesForward), but directly connect to the content
process, to get fully inspectable console message, from the Browser Console.
This changes our assumption in the console codebase that we only have
a single proxy we can use, so we create a getProxy function in the
WebConsoleUI that will be used to retrieve the adequate proxy reference
given some parameters.
For now, if we have multiple proxies, we simply return the first one.
This is by no mean the final function, but just a ugly workaround so
we can land this experimental multi-proxy browser console. We'll fix
broken features once this landed, step by step until we're at parity
with the regular browser console.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d00cecfbe540
Create multiple proxies when in Fission Browser Console. r=ochameau.

Comment 13

[Razvan Maries]

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