1618695 - stop destroying TabDescriptor when navigating to another process

Status: RESOLVED FIXED

(DevTools :: Framework, task, P1)

Description

[Julian Descottes [:jdescottes]]

We want to keep a tab descriptor alive even if the associated target is destroyed (eg navigation to another process). This way a toolbox can be opened for a single tab descriptor which won't change during the lifecycle of the toolbox.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Remove exited event and property from tab descriptor

This is just a cleanup, exited is not used in the tab descriptor actor

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Stop watching for TabRemotenessChange in BrowserTabList

Depends on D73700

The TabDescriptorActor/Front is never destroyed, but we used to clean the map entry on TabRemotenessChange.
Removing this cleanup allows to reuse tab descriptors after a navigation.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Add test to check that TabDescriptor survives a process change

Depends on D73701

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Remove unused options argument for TabDescriptorFront

Depends on D73726

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Stop updating shouldCloseClient on the old local tab target

Depends on D73778

Flipping this boolean can create race conditions.
We set it to false and then wait for the "close" event fired by the target front
After that, in parallel:

• the target will finish initializing fronts (async)
  -> then we check shouldCloseClient
• the new target will be created (for target switching scenarios)
  -> then we flip back shouldCloseClient

We have no guarantee that we will check shouldCloseClient before it is flipped back to true.
Furthermore, this target is being destroyed, there is no reason to flip the boolean.
So removing this code seems like the best option.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Wait for target to be fully destroyed before switching targets

Depends on D73779
Reusing the same tab descriptors across process switches means that calling
getTarget on the tab descriptor must create a new target and not return the old one.

The caching mechanism on descriptor fronts relies on the fact that the TargetFront is still alive.
When TargetFront.actorID is null, the cache is invalidated and we fetch a new target.

However the current target switching mechanism relies on an event fired very early in the target
destruction sequence. This means that when we will call getTarget, the old target might still be
alive.

We also start waiting for an event in RDMs target switching, which was not waiting for anything so far

Comment 7

[Julian Descottes [:jdescottes]]

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed27985caecb80d6de1845c422b70988800c8aa5

Comment 8

[Chris Peterson [:cpeterson]]

Tracking Fission DevTools bugs for Fission Nightly (M6) milestone

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1618695 - Forward local tab instance to tab descriptor

Depends on D73777

The filter is a special argument that is closely linked to local-tab
It can only be set by rootFront getTab.

However if we reuse the same tab descriptor for several targets, we need
to make sure that the initial filter will be reused.

Usually the tab descriptor target is created via getTab and cached, but under
some circumstances, we can attempt to call getTarget on a tab descriptor from
another codepath first (eg reparent remote frame).

The filter becomes a property of the TabDescriptorFront, and getTarget takes
no argument, which is consistent with other target descriptors.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f18631370c2a
Remove exited event and property from tab descriptor r=ochameau
https://hg.mozilla.org/integration/autoland/rev/71f6fff3e824
Stop watching for TabRemotenessChange in BrowserTabList r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ebb8f925196f
Add test to check that TabDescriptor survives a process change r=ochameau
https://hg.mozilla.org/integration/autoland/rev/5ad220771a71
Remove unused options argument for TabDescriptorFront r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c2e6ab53a7eb
Forward local tab instance to tab descriptor r=ochameau
https://hg.mozilla.org/integration/autoland/rev/b5f8894ffd12
Stop updating shouldCloseClient on the old local tab target r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ffcffdd673b4
Wait for target to be fully destroyed before switching targets r=ochameau

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f18631370c2a
https://hg.mozilla.org/mozilla-central/rev/71f6fff3e824
https://hg.mozilla.org/mozilla-central/rev/ebb8f925196f
https://hg.mozilla.org/mozilla-central/rev/5ad220771a71
https://hg.mozilla.org/mozilla-central/rev/c2e6ab53a7eb
https://hg.mozilla.org/mozilla-central/rev/b5f8894ffd12
https://hg.mozilla.org/mozilla-central/rev/ffcffdd673b4