1603182 - Make the (old) performance panel support target switching

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

First, we can probably drop gTarget, which doesn't seem to be used:
https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/client/performance/panel.js#40
https://searchfox.org/mozilla-central/search?q=gTarget&case=true&regexp=false&path=devtools%2Fclient%2Fperformance

Then, we should use targetList.watchTargets in order to fetch a new performance front for each new top level target:
https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/client/performance/panel.js#32-36
We may actually do it from PerformanceController as it seems to be the only component listening to RDP events and doing RDP requests:
https://searchfox.org/mozilla-central/search?q=front&case=true&regexp=false&path=devtools%2Fclient%2Fperformance%2F
So we should review this code in order to listen for RDP events (this.fronts.on) on each top level target notified by TargetList.watchTargets, and also ensure that RDP requests are done against the last/current top level target.

Note that there might be some stuff to review also in the Toolbox code:
https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/client/framework/toolbox.js#3800-3823
This seems to be fine, as we reset this from the switchToTarget method:
https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/client/framework/toolbox.js#509

Comment 2

[Chris Peterson [:cpeterson]]

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

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: prototype.mp4

Hello Julien!
I'm tackling to address target-switching for performance panel now.
And, I have one question about behavior after target-switching.
As the performance front also will be disconnected, how should we make it behave?

It seems we can do 3 ideas below:

1. after target-switching, stop current recording.
2. after target-switching, stop current recording then start next recording as a new session.
3. even when target-switching, the recording is continued as same as the new performance panel.

I think 3 is ideal, but the implementation might become a bit complex.
The simplest is No1, and the video I attached is No.2 choice.

What do you think??

Comment 4

[Julien Wajsberg [:julienw]]

3 is ideal, as you said, but I believe this adds only a small value to other use cases. Moreover we're actively working on replacing this panel, so I'd advise against doing something too complex for our immediate needs.

The main use case for this panel is to record the performance on a specific website.
One important case to keep in mind is analyzing the performance of a website loading. That's why I think 1 isn't good enough.
So I think 2 is the right choice here. But we need to take great care that when restarting we don't miss the start of a website loading... If we can't have it this way then maybe 3 would be needed sadly...

Hope that answers your questions, please ask if you need anything! Our team is in the matrix room "Firefox Profiler" if you want to chat.

Comment 5

[Daisuke Akatsuka (:daisuke)]

Thank you very much for replying, Julien!
Okay, so, please let me upload patches for No.2 at first.
I suppose we would miss a bit at the beginning of the loading, but I want you to check whether this is enough or not.
(And if we should see the performance completely like N0.3, it might need to change the actor as well..)

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603182: Remove unused variables.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603182: Address target-switching for performance panel.

Depends on D68536

Comment 8

[Daisuke Akatsuka (:daisuke)]

This is the patch for no.2.
https://phabricator.services.mozilla.com/D68537
Couldn't you check the behavior??

Comment 9

[Julien Wajsberg [:julienw]]

Thanks, I'll have a look next week!

Comment 10

[Julien Wajsberg [:julienw]]

I tried your patch, but I don't see the behavior for now. Do I need to enable something in my firefox build, so that firefox is in "fission mode"? Sorry if that should be obvious :-)

Also I see a lot of JavaScript error: resource://devtools/server/actors/targets/browsing-context.js, line 1761: TypeError: can't access dead object in the console but this is happening in master too, so not your patch I believe.

Comment 11

[Daisuke Akatsuka (:daisuke)]

Hello, thank you very much for trying this!
STRs that hits the target-switching is below:

1. turn on devtools.target-switching.enabled pref
2. open normal page (e.g. http://example.com)
3. open a page running on the parent process (e.g. about:robots)
   While navigating from 2 to 3, the target-switching happens.

And I could not see such an error in my environment at least but

TypeError: this._currentBufferStatus is undefined: getBufferUsag
eForRecording@resource://devtools/client/fronts/performance.js:9
7:9

happens now.

Comment 12

[Chris Peterson [:cpeterson]]

Tentatively moving P1 Fission M6 bugs to M6a.

Comment 13

[Julien Wajsberg [:julienw]]

Oh I see, the switch only happens when moving from a context to another context. When reloading the same page it doesn't switch, contrary to what I understood before.
Then the new behavior is strictly better than the previous behavior, so I'm fine with it!

I see errors in the console, like:
console.error: (new Error("Can not send request 'release' because front 'obj' is already destroyed.", "resource://devtools/shared/protocol/Front/FrontClassWithSpec.js", 29))
or
console.warn: "Error while detaching the browsing context target front:" (new Error("Protocol error (noSuchActor): No such actor for ID: server0.conn0.child30839/frameTarget1 from: server0.conn0.child30839/frameTarget1", "resource://devtools/shared/protocol/Front.js", 319))

I believe there's some work to do around the destroy operation.

Comment 14

[Daisuke Akatsuka (:daisuke)]

Thank you very much for confirming and kind feedback!
Okay, I will refine the code and get rid of the errors.

Comment 15

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603182: Add a test for target-switching of performance panel.

Depends on D68537

Comment 16

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603182: Get rid of gFront from window.

Depends on D69488

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e09656d7e72
Remove unused variables. r=julienw
https://hg.mozilla.org/integration/autoland/rev/e4261f9f8087
Address target-switching for performance panel. r=julienw,ochameau
https://hg.mozilla.org/integration/autoland/rev/0ffce188e5b8
Add a test for target-switching of performance panel. r=julienw
https://hg.mozilla.org/integration/autoland/rev/aa0bb0e058f0
Get rid of gFront from window. r=julienw

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5e09656d7e72
https://hg.mozilla.org/mozilla-central/rev/e4261f9f8087
https://hg.mozilla.org/mozilla-central/rev/0ffce188e5b8
https://hg.mozilla.org/mozilla-central/rev/aa0bb0e058f0