1699120 - Use a new DevToolsClient for all about:debugging toolboxes

Status: NEW

(DevTools :: about:debugging, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

There is many ways to formulate this bug:

• Use a new DevToolsClient for all about:debugging toolboxes
• Each/all Toolboxes should have their own dedicated DevToolsClient
• Toolbox (, Descriptor?) and DevToolsClient all follow the same lifecycle

The motivations are about:

• getting rid of the detach request being done while closing the toolbox, from the TargetMixin:
  https://searchfox.org/mozilla-central/rev/1a47a74bd5ba89f2474aa27c40bd478e853f3276/devtools/client/fronts/targets/target-mixin.js#659-665
  This request will most likely fail and is a source of intermittents in tests.
  This is one of the many reasons why destroy codepath is async, while it would be best to have it be synchronous. That, because there is no guarantee the requests will complete during toolbox shutdown.
• stop sharing the same DevToolsClient for more than one toolbox.
  We do that only for about:debugging and that is a source of issues that are specific to remote debugging. We end up having uncovered regression because we don't have automated test for remote debugging. Various of these regressions wouldn't exists, or, would be catched by regular local tab tests if we were using the same one toolbox = one client pattern.

Comment 1

[Julian Descottes [:jdescottes]]

Overview of the current about:debugging implementation

tldr

For remote debugging, clients are connected when you click on "connect" next to a remote device on the about:debugging UI. This client is then "cached" and reused when we open about:devtools-toolbox tabs from about:debugging. This was done to avoid creating additional connections between debugger and debuggee, and also avoid showing the connection prompts every time the user would open an about:devtools-toolbox tab.

Detailed walkthrough

A "client" is created for a "runtime" via the runtimes.connectRuntime action: https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/aboutdebugging/src/actions/runtimes.js#84

This action uses an internal helper from about:debugging to create the client for a given runtime, RuntimeClientFactory::createClientForRuntime: https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/aboutdebugging/src/modules/runtime-client-factory.js#43

This module will mostly look at the provided runtime object, and will simply create a client with the appropriate transport layer and it will attempt to connect. Note: the runtime object is about:debugging specific, but it's just a collection of metadata which describe the runtime we want to connect to.

If the connection is successful the runtimes.connectRuntime action will do a few additional checks on the connected runtime (mostly checking prefs). And then it will dispatch CONNECT_RUNTIME_SUCCESS, meaning the connection was OK.

It's in the reducer of CONNECT_RUNTIME_SUCCESS that we will "store" the client for future usage, via the RemoteClientManager: https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/aboutdebugging/src/reducers/runtimes-state.js#111

This RemoteClientManager is mostly a map between a runtime id and a client. There's a bit of logic to translate the runtime id into a "remoteId", which is nothing more than a URL safe version of the id.

When a user clicks on "Inspect" for one of the targets available in a remote runtime, we retrieve the "remoteId" for the corresponding runtime and pass it as a URL parameter for the about:devtools-toolbox tab we are opening: https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/aboutdebugging/src/actions/debug-targets.js#79

Finally, when the about:devtools-toolbox tab is opened, it will use descriptor-from-url in order to build the Descriptor that will be used for this toolbox. And if it spots a "remoteId" URL parameter, instead of spawning a local client, it will query the RemoteClientManager to retrieve the corresponding, already connected, client: https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/framework/descriptor-from-url.js#158

The fact that we have a remoteId parameter will also implicitly mean that we are using a "cached client" and therefore destroying the target/descriptor for the tab should not lead to destroy the client: https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/framework/descriptor-from-url.js#48

A last item as a flashback, when we talked about RuntimeClientFactory::createClientForRuntime which is used to create the client, there is a check made there in case we already have a client in the RemoteClientManager. If we have one, we return it immediately instead of creating a new one. https://searchfox.org/mozilla-central/rev/6309f663e7396e957138704f7ae7254c92f52f43/devtools/client/aboutdebugging/src/modules/runtime-client-factory.js#49

Suggestions

Our end goal here is that about:devtools-toolbox should never reuse an existing client, and should always create a new one. First about:devtools-toolbox will need to have all the information needed to build a client. And I think in order to do that we can leverage most of what already exists.

The RemoteClientManager can be evolved into RemoteRuntimeInfoManager/Provider. Instead of returning clients it's just a store between an abstract "remoteId" and ... maybe metadata which is enough to create a new client.

The RuntimeClientFactory, which is currently an about:debugging specific module could be made more generic and it would be used both from about:debugging to create a client (we still need one to list targets, open the profiler etc...) but also from about:devtools-toolbox (precisely from descriptor-from-url).

Comment 2

[Julian Descottes [:jdescottes]]

To be clear, this will lead to behavior change for remote debugging users. They will get more connection prompts than before. We can address that differently. We had previously planned to disable connection prompts temporarily while about:debugging was connected, which is an option. I think it needs to be handled carefully and be reviewed by security peers if we do that though.

So my suggestion from a product perspective here is to accept the behavior regression, maybe communicate about it, but not try to address it in this bug. We will file follow ups. Aligning about:debugging and remote-debugging to use the same connection patterns as the local toolbox is way more important than the behavior regression.