1565263 - Support process switching without closing the toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

This most likely depends on bug 1565200.

For now, we badly handle once process switching scenario, which isn't exactly the one to be coming due to fission.
When you open a page in the parent process (like about:sessionrestore), open the DevTools and navigate to a Web page (loaded in a content process), the toolbox closes and reopens.

The goal here would be to implement a subset of the work needed for Fission.
The main work of fission would be to support the remoted frames.
But there is also the case where the tab will use distinct processes when navigating from domain A to domain B. And we really don't want to see the toolbox blink like it does today.
Supporting the process switching doesn't require as extensive work as the remoted frames support.
Support of remoted frames means that the toolbox has to support more than one target at a time.
Support of process switching means that the target should be able to support more than one target, but always one at a time.

Here is what typically happens when a tab changes its process:

• The docshell of the first document is destroy and the BrowsingContextTargetActor self-destroy itself, here,
• The actor emits a tabDetached event here,
• In case the actor doesn't have time to emit this event, there is also some code in the parent process to ensure firing this event, here (We might send duplicated event?)
• The target listen for this event in order to self-destroy itself, from here.
• The target emit a close event, which is listened by the toolbox in order to self-destroy, from here

I imagine that we will have two possibly independent tasks here:

• The first goal would be to somehow stop self-destroying everything. The toolbox, when we are in process of a process switch, shouldn't destroy itself and instead wait for the new target and attach to it.
• Then, in all our codebase, we should review that nothing cleans up on target close event. And we should make it so that everything is able to hear about the new target front and re-attach to it. Before or during that operation it should also unregister/cleanup regarding the previous target that has been destroyed. This is where target.onFront and a possible onTarget helper function may help implementing that.

The toolbox and panel codebase should be more "reactive" than "demanding" regarding targets and fronts. The onSomething helpers are there to abstract the fact that:

• the "something" can change over time. The passed callback will be called each time there is a new one. That's the process switching scenario.
• the "something" can actually be instantiated multiple times at the same time. That's the remoted frames scenarios.

We should look into all the places where we register front listeners or call initializing methods on the fronts. There, we should be using onFront callbacks rather registering against one front, once for all, via getFront.

let front = target.getFront("inspector");
front.on("foo", this.onPanelDoFoo);
front.startListeningOrDoSomething();

should become:

target.onFront("inspector", front => {
  front.on("foo", this.onPanelDoFoo);
  front.startListeningOrDoSomething();
});

I imagine similar pattern should be used for targets as well and a combined helper onAllTargetsFronts might also help in order to avoid too many callbacks in callbacks!

Comment 1

[Alexandre Poirot [:ochameau]]

I started looking into this. The first insight is that we should avoid at all cost memoizing the target anywhere: Toolbox, Panel, but all all inner class of panels.
The second insight is that we should identify and isolate the code relating to "attaching" to a new target or a new target scoped front.
We should typically have two functions, dedicated to that in each panel. One to "attach" and another one to "detach".
These funtions would typically register a few RDP event listeners and call some initialization method on the target scoped actors. These methods are often method to start listening and emitting these RDP events.
One issue that came up is that the "detach" function was doing more cleanup than just unregistering the fronts. It was also destroying some UI component which is critical for the panel. So we should be careful to really focus on only cleaning things about the fronts, and only that.

Comment 2

[Patrick Brosset <:pbro>]

I wasn't sure what to classify this bug as. If Fission wasn't coming, I would say this is an enhancement. Merely improving the navigation from parent process to content process. And therefore this would be rather low priority.
Now, because Fission might cause a navigation web domain A to domain B (in the same tab) to use a different process, then I would say this is a defect because it negatively impact something that works fine today. And so the priority is higher.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1565263 - Support target switching for the console.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1565263 - Print method name when RDP can't be sent because the front is destroyed.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1565263 - Move DebuggerClient.close call from TargetMixin.destroy up to Toolbox.destroy.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1565263 - Make TargetMixin.destroy more synchronous.

Protocol.js's Front and Pool's destroy are not expected to be async.
But TargetMixin.destroy is. It makes DebuggerClient.close do not wait
for all Target fronts destroys correctly. The client close method calls
the cleanup method of all the pools. Top level fronts are pools.
Target fronts are still self managed and so are pools.
And so, when we close the toolbox, the target destroy is still pending
after toolbox.destroy is resolved.

Comment 7

[Alexandre Poirot [:ochameau]]

One cross platform try to see if I'd be able to land:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b1865198c904d99d91f665f95013673dd9250e7

Comment 8

[Phabricator Automation]

Comment on attachment 9092347 [details]
Bug 1565263 - Print method name when RDP can't be sent because the front is destroyed.

Revision D45667 was moved to bug 1585986. Setting attachment 9092347 [details] to obsolete.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1565263 - Use Target.shouldCloseClient as only flag to check if a Target should close its Client.

This will later allow dynamically change this value for local tabs,
during Fission processes switches.

Comment 10

[Alexandre Poirot [:ochameau]]

Looks like I should finally be able to land this patch queue:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=270297073&revision=2cf84b0a200f37a60c28e1b4e7e1e93453dd74bb

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/85e51a860dae
Use Target.shouldCloseClient as only flag to check if a Target should close its Client. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/263886b0a46b
Support target switching for the console. r=nchevobbe,yulia,jdescottes
https://hg.mozilla.org/integration/autoland/rev/b5e0f6f76dd6
Make TargetMixin.destroy more synchronous. r=jdescottes

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/85e51a860dae
https://hg.mozilla.org/mozilla-central/rev/263886b0a46b
https://hg.mozilla.org/mozilla-central/rev/b5e0f6f76dd6