There should be only one target manager

[connorjclark]

In FR, each NetworkMonitor creates its own target manager. The target manager does things to targets as they attach. It may be bad to do this stuff multiple times when one would suffice.

Instead, we could add to Driver a getTargetManagerForSession(session) method that would return a cached target manager.

[paulirish]

My understanding of our CDP target/session management

… but primarily scoped to the 3 files below. (There's plenty more.)

driver (legacy)

• on 'Page.frameNavigated' => for root frame, we do setAutoAttach (??? do we need to do this each time the frame is navigated? feels like its a target-level thing which would survives navigations)

• on 'Target.attachedToTarget' => handleTargetAttached():

  • if non-iframe (SW, worker, etc), just runIfWaitingForDebugger and move on
  • if iframe: Network.enable, runIfWaitingForDebugger and setAutoAttach

network-monitor (FR and legacy)

• what uses it in legacy nav mode?
  • pulled into legacy via driver/navigation/waitForCondition type stuff. a new NM created for each gotoURL
  • also pulled in when fullpagescreenshot needs to know if the network is idle.
• receives a defaultSession. (?? seemingly doesnt setup its _onProtocolMessage for the defaultSession?)
• also instantiates a new TargetManager
• with the targetManager up, NM provides TM.addTargetAttachedListener with NM._onTargetAttached..
  • when this is called it does 2 things 1) it issues Network.enable and 2) will register its _onProtocolMessage with the session, which is mostly there to inform the networkRECORDer.

target-manger (FR and legacy (modulo #14079))

• on Page.frameNavigated => _onFrameNavigated:
  • if non-iframe (SW, worker, etc), just setAutoAttach;
  • if iframe, do nothing.
  • (! note how over here the event intiating is Page.frameNavigated rather than driver's 'Target.attachedToTarget'… ?!) ... I don't see why this isn't based on the Target.attachedToTarget event instead.
• _onSessionAttached is called MOSTLY because we're creating targetManagers all the time. so that fn call has little do to with an actual session being attached to a new target (which only happens when the CDPSession/connection emits a sessionattached event)

Note: In FR the connection-level sessionattached dude is responsible for cluing in targetmanager to the actual new targets. In legacy, the stuff in driver does this.

Also.. while we're on the topic https://github.com/aslushnikov/getting-started-with-cdp#targets--sessions is a great resource.

and after connor's https://github.com/GoogleChrome/lighthouse/pull/14079 …

• for FR, same behavior across the above
• but in legacy, the targetmanager does nothing. also the PR fixes the weirdly missing _onProtocolMessage setup for the defaultSession.

Questions

1. Why are we creating new sessions when we are taking a screenshot!? I think it's a fresh execution of _onSessionAttached with a session that's not entirely new....
2. In FR, Is there 1 just 'connection' and multiple sessions routing through it?
3. AFAIK, in devtools/pptr/etc, targets and sessions are 1:1. I recall brendan saying patrick wanted a new session per gatherer (per target) or something? either way. we are minting TM sessions based off navigations (not targets). Also we dont create sessions for the subtargets (oopifs). TM just calls any targetAttached listeners outside the class (as it doesnt have any of its own).
4. I'm seeing a bunch of executions of the targetManager _onSessionAttached finally clause that seem to come out of nowhere and dont have a valid session. wtf
5. is there a reason driver's setAutoAttach is based off frameNavigated instead of Target.attachedToTarget ? feels weird.
6. we found Target.autoAttachRelated last time. It won't work for non-FR, but worth looking at for FR. Seems like it might automate some of this.
7. why doesn't the network-monitor call session.addProtocolMessageListener(this._onProtocolMessage); for its defaultSession? seems like an omission

Proposals

1. targetautoattach and handling new targets should be initalized at the session level. creating a target manager because one of many network monitors makes no sense.
2. the networkmonitor shouldn't be emitting 'protocolmessage' events. the session should. (and the devtoolslog gatherer should listen to it from there instead).
3. we have defaultSession (which seems to be the root page session). plenty of places we call something session but its still referring to that root session.
4. we probably should not drop our defaultSession on pagenavigation. the target is still the same.
5. delete FR session's callbackMap?
6. We've got a real awkward mix of classes that extend EventEmitter and classes with manual callback registering listeners. Probably everything should be using EventEmitter.
7. network-monitor (and similar modules) shouldn't be concerned with "targets" at all, but only sessions.
8. rename defaultSession to rootSession

[adamraine]

Why are we creating new sessions when we are taking a screenshot!? I think it's a fresh execution of _onSessionAttached with a session that's not entirely new....

A page could render a new iframe after we expand the viewport, and we collect network information from that frame when waiting for network quiet. It's probably might be fine if the network analysis in the screenshot just ignores newly created targets though.

In FR, Is there 1 just 'connection' and multiple sessions routing through it?

The implementation in FR comes from Puppeteer, but it does look like a single connection and multiple sessions based on the source code.

AFAIK, in devtools/pptr/etc, targets and sessions are 1:1. I recall brendan saying patrick wanted a new session per gatherer (per target) or something?

Pretty sure targets and sessions are not 1:1. In Puppeteer, you can create multiple sessions off of the same target (example). Creating a new session for each gatherer is theoretically useful (https://github.com/GoogleChrome/lighthouse/pull/13752), but it hasn't become a necessity yet.

either way. we are minting TM sessions based off navigations (not targets). Also we dont create sessions for the subtargets (oopifs). TM just calls any targetAttached listeners outside the class (as it doesnt have any of its own).

What to you mean? TM will create new sessions on the Puppeteer connection 'sessionattached' event, which is emitted on Target.attachedToTarget.

I'm seeing a bunch of executions of the targetManager _onSessionAttached finally clause that seem to come out of nowhere and dont have a valid session. wtf

What's invalid, is the session undefined?

is there a reason driver's setAutoAttach is based off frameNavigated instead of Target.attachedToTarget ? feels weird.

setAutoAttach is called on frame navigations and Target.attachedToTarget.

Also this: https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/driver/target-manager.js#L35-L37

we found Target.autoAttachRelated last time. It won't work for non-FR, but worth looking at for FR. Seems like it might automate some of this.

👀 yeah looks useful

why doesn't the network-monitor call session.addProtocolMessageListener(this._onProtocolMessage); for its defaultSession? seems like an omission

I think it does but it's super confusing. This call triggers the network monitor to add the listener to the default session.

[paulirish]

Why are we creating new sessions when we are taking a screenshot!? I think it's a fresh execution of _onSessionAttached with a session that's not entirely new....

A page could render a new iframe after we expand the viewport, and we collect network information from that frame when waiting for network quiet. It's probably might be fine if the network analysis in the screenshot just ignores newly created targets though.

This is true. But I can almost guarantee you this usecase isn't the impetus of this setup. I'm more just pointing out that the layering and semantics here are really silly. When we do the fullpagescreenshot, all our protocol/target handling stuff should be already set up and fine. It's totally reasonable for us to wait for network quiet, but I can't justify why we're issuing new Target domain CDP work as a direct result of FPSS asking if the network is quiet.

AFAIK, in devtools/pptr/etc, targets and sessions are 1:1. I recall brendan saying patrick wanted a new session per gatherer (per target) or something?

Pretty sure targets and sessions are not 1:1. In Puppeteer, you can create multiple sessions off of the same target (example). Creating a new session for each gatherer is theoretically useful (#13752), but it hasn't become a necessity yet.

Yeah this looks like the implementation I was thinking about. I don't believe the "local session" idea is in-practice or even considered by CDT frontend, pptr, or playwright. And while state can be per-session, I don't think this rule is consistently applied to be relied on. In other words, I surmise the local session approach would create more problems than it solves. But I'd want to consult more with caseq and the chrome-debugging-protocol mailing list.

either way. we are minting TM sessions based off navigations (not targets). Also we dont create sessions for the subtargets (oopifs). TM just calls any targetAttached listeners outside the class (as it doesnt have any of its own).

What to you mean? TM will create new sessions on the Puppeteer connection 'sessionattached' event, which is emitted on Target.attachedToTarget.

True. I did realize this detail later, but forgot to edit the text. The session isn't brand new but we're running the onSessionAttached handler AGAIN, despite that function serving as a "onBrandNewSessionAttached" handler as opposed to a "onNewTargetManagerInstanceForEachExistingSession" which is why it's most-often run.

I'm seeing a bunch of executions of the targetManager _onSessionAttached finally clause that seem to come out of nowhere and dont have a valid session. wtf

What's invalid, is the session undefined?

There's no targetInfo associated with the sessions. It's quite odd.

is there a reason driver's setAutoAttach is based off frameNavigated instead of Target.attachedToTarget ? feels weird.

setAutoAttach is called on frame navigations and Target.attachedToTarget.

FWIW https://github.com/GoogleChrome/lighthouse/pull/12421 is where patrick switched driver's setAutoAttach to be done based on frameNavigated rather than attachedToTarget. In it he said "Also moves a few of the setup protocol commands that weren't really a part of navigation but just were just a convenient spot to put a few environmental fixes." but I can't make much sense of it. No other CDP client implementations go this route, so I'd want to document it if we have a good reason.

(My goal here is for a CDP target management system that's both correct and understandable.)

Also this: https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/driver/target-manager.js#L35-L37

Yeah that does seem like the potential justification. I'd like to dig into this a bit more since no other CDP clients have the same workaround.

why doesn't the network-monitor call session.addProtocolMessageListener(this._onProtocolMessage); for its defaultSession? seems like an omission

I think it does but it's super confusing. This call triggers the network monitor to add the listener to the default session.

True... Weird. I traced the execution between all these onXXX and listener calling and couldn't match that up. But yeah I'm seeing it now.

[brendankenny]

a few things that came up today:

• having a standalone TargetManager is great. The legacy auto attach logic is diffuse and appears general (e.g. we allow sendCommand to arbitrary session ids), but is actually used only for listening to iframe network requests. The new TargetManager is nice in that it admits it only does that one thing and does it mostly in one place, so it makes a bit of sense to be managed by network-monitor.

  But: if we want a single targetManager, it makes sense to invert control and have it as an easily available tool at the driver/connection/defaultSession/wherever level. For instance, I recently wanted to write a gatherer that did a context.evaluate() to inject a performance.measure() into every iframe. That would have to all be manually done in lighthouse right now, even though we autoattach to all targets and check for iframe targetInfos and everything :) There's also no need to be as general as pptr (providing an API to find iframes and get a handle to them), because, again, we're already auto-attaching to them.

• networkMonitoring should similarly be persistent while gathering, and it should be easily accessible from anywhere. waitFor and fullPageScreenshot can be clients of that, rather than driving it. Lighthouse is already getting all the CDP events, whether it's being recorded or not, so there's no reason you shouldn't be able to just subscribe to network-critical-idle and be notified at any time, instead of having to fire up a networkMonitor of your own.

[connorjclark]

• there should just be one NetworkMonitor
• https://github.com/GoogleChrome/lighthouse/issues/14157 is related

but that's not a P1 anymore, all P1 stuff done

[brendankenny]

• consider tracking execution contexts per target in the target manager so it's easy to iterate over them

AFAIK there's no easy way to enumerate available execution contexts. Instead, you need to track Runtime.executionContextCreated and Runtime.executionContextDestroyed/Runtime.executionContextsCleared events and keep your own list of active context IDs. The targetManager would be a handy place for this to be done instead of at a per-gatherer level as needed.

One place where this is important is content scripts from extensions, which always run in an isolated context. The script itself will be picked up by the Scripts artifact, but any execution-context-dependent data about what that script is doing will be hidden away in the isolated context unless a gatherer explicitly looks in there. One example: bfcache failures due to extension content scripts in #14078