core(scripts): narrow to only listen to parsed events

[brendankenny]

The gatherer only wants network requests from the main session, and it ends up narrowing down to only handling Debugger.scriptParsed events. Rather than listening to the firehose of all events and narrowing to both requirements, it can instead listen just for Debugger.scriptParsed directly on the main session.

As the only other consumer of session.addProtocolMessageListener() besides network-monitor, removing this also makes the refactors for #14078 a bit simpler.

[connorjclark]

My plan for enabling further instrumentation of workers scripts, OOPIF scripts, etc (anything not from main session) was to simply remove the sessionId check here. Is there still a reasonable approach to doing the same with this refactor?

[brendankenny]

I forgot to mention in the initial comment, defaultSession only has the flattened protocol messages in legacy mode, so to get cross-session scriptParsed events in FR, this gatherer would need to do something like make a networkMonitor to get all the events (like devtools-log does). That should indeed be more straightforward after the refactor.

[connorjclark]

I forgot to mention in the initial comment, defaultSession only has the flattened protocol messages in legacy mode

Oh, then how does the legacy smoke test for oopif-scripts pass right now?

lighthouse/lighthouse-cli/test/smokehouse/test-definitions/oopif-scripts.js

Line 117 in 3f8d598

url: 'http://localhost:10200/simple-script.js',

If legacy defaultSession has the flattened protocol, I'd expect the gatherer to get Scripts for :10503 (and from the workers).

Ah, because we need to send Debugger.enable to all the sessions... then they'll come through in the gatherer. We could do that in the legacy driver _handleTargetAttached (easy, and we're gonna remove all that soon anyhow), can't think of a simple way to do it in the Scripts gatherer itself.

so to get cross-session scriptParsed events in FR, this gatherer would need to do something like make a networkMonitor to get all the events (like devtools-log does). That should indeed be more straightforward after the refactor.

Sounds good.

[paulirish]

My plan for enabling further instrumentation of workers scripts, OOPIF scripts, etc (anything not from main session) was to simply remove the sessionId check here. Is there still a reasonable approach to doing the same with this refactor?

On this topic…

@connorjclark I was pitching brendan on changing the if (record.sessionId) check that we have in a few places to be much more explicit… something like if (record.isFromOOPIF).

We have actually changed code to maintain these questionable conditionals.. but there's really no justification, from a CDP level, that the presence of a sessionId == not the "root" session.

[connorjclark]

Right, it's not the same. See the deleted comment in this PR.

Would we need to cross-validate it with target information, to know they are from 1) not the main frame session (this also will continue to use the mainSessionId hack for devtools) and 2) are from an iframe (today we fail this part)? the sessionId check was (apparently) a simple close-enough solution which lost us scripts from workers, and I'm not sure we ever realized that :)

But also, if we are gonna start considering OOPIFs and don't want to make any distinction I don't think the change is worth the effort. Though I suppose we could want OOPIFs filtered in some audits but not others.