The DOM panel should use new TargetList API and properly support target switching.

See also bug 1565263.

Honza

Some pointers to the code base:

1. Here is the place where the panel evaluates window expression in the target and gets the root grip
   https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/devtools/client/dom/panel.js#187

2. Here is the place where the grip is sent to panel content frame
   https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/devtools/client/dom/panel.js#117

3. The content frame is rendering all the properties (using reps) and fetching new properties as the user is expanding objects
   The place where the props are fetched from the backend is here

https://searchfox.org/mozilla-central/rev/581466eef9269afb03d8a0dba2f50215f3a9026c/devtools/client/dom/panel.js#164-166

@Alex, can you please provide some instructions about how the TargetList API should be used here
(e.g. some pointers to existing code base for inspiration/examples)

This might be good-second-bug

Thanks!

Honza

See bug 1592763 comment 3 where I replied to similar question from the inspector perspective.
But I need some more time to look at DOM codebase to provide a meaningful comment, more specific to DOM panel.

Tracking dt-fission-m2 bugs for Fission Nightly (M6)

Here is a generic comment I posted on all bugs about migrating to TargetList:
Bug 1565263 landed the TargetList API which helps support switching to another target (as well as supporting the additional remote iframe target).
But we should followup in each tool in order to start using this new API.
The very first goal is to:

• stop memoizing the target/targetFront and may be also the target scoped front (inspector, console, thread, storage, ...). Instead we should use toolbox.targetList.targetFront in order to query for the current top level target front.
• support target switching by using TargetList.watchTargets. In a first iteration we would only care about the top level target. In order to do so, we can check for the isTopLevel argument being passed to the two callbacks register to watchTargets:

  this._toolbox.targetList.watchTargets([this._toolbox.targetList.TYPES.FRAME],
    ({ type, targetFront, isTopLevel }) => {
      if (isTopLevel) {
        // A new top level target is available
        // This will be fired on toolbox opening, for the first one,
        // And then, evertime we navigate to another process.
        // For now, you would need `devtools.target-switching.enabled` to be set to true
        // And navigate from any http website to about:robots, which loads into the parent process. Or enable Fission and navigate between two distinct top level domains.
      }
    },
    ({ type, targetFront, isTopLevel }) => {
      if (isTopLevel) {
        // A top level target has been destroyed
      }
    }

See bug 1565263 for how we migrated the console or bug 1578242 for the inspector.

To be more specific about the DOM Panel, we should review all usages of target from the panel module:
https://searchfox.org/mozilla-central/rev/c61720a7d0c094d772059f9d6a7844eb7619f107/devtools/client/dom/panel.js#218-220
We should either remove this target getter, or map it to this._toolbox.targetList.targetFront.
And as previous comment says, start using watchTargets. We should probably do share the same implementation between onTargetAvailable and onTabNavigated, in order to refresh the panel when we navigate to a new top level target.

First simple attempt at fixing this. The target getter in the DOM panel already
refers to the current toolbox top-level target. There doesn't seem to be a need
to change this as this is the only target the DOM panel cares about.
So, I'm only adding a listener for new top-level targets and refreshing the
panel when that happens, just like we do on navigation.
Am I missing something?

Backed out for perma fails on browser_dom_fission_target_switching.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=287586126&resultStatus=testfailed%2Cbusted%2Cexception&revision=d2ebc96ddb56f94a7b069b18e8c0e3f2e2f952d4

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=287586126&repo=autoland&lineNumber=9931

Tier 1 failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=287587387&repo=autoland&lineNumber=1921

Backout: https://hg.mozilla.org/integration/autoland/rev/0fa466366383c636ddd22156b5e9740c13563f16

While making the change in the previous patch (to make the dom panel use
the targetList API), and writing a test for it, I stumbled upon a weird
issue that I don't think we've encountered so far.

The console's actor method evaluateJSAsync does things a bit differently
than other methods. It spawns the work it needs to do, but does not wait
for it to be done, and immediately returns an ID to the client.
Later, when the work is done, it sends an event back to the client with
the response.
It's then up to the client to use the ID provided in the immediate response
and match it against the incoming event to verify that this is, indeed,
the right response.

In all cases we've seen so far, the event comes back after the initial
method response. This seems logical as evaluateJSAsync uses an
executeSoon helper to spawn the work needed in the next event loop.

Now, the case I have seen as witnessed by the test I added is that,
sometimes, the event actually comes back before the initial response.
Because both things go through the protocol.js message handling, and
because all of it is asynchronous, it may indeed happen. There's no
guaranty at the protocol level to avoid this.

So, my approach here is to simply avoid this to happen from the client
side. I don't think we should be doing a generic fix in protocol.js for
this, but instead clients should be prepared for these things to happen.

Latest try push for these 2 patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba4f459f71e2946409d2f766db712f626c9f205&group_state=expanded

https://hg.mozilla.org/mozilla-central/rev/a0c402418ef8
https://hg.mozilla.org/mozilla-central/rev/1d62d6dac1ee