core(target-manager): only listen to LH sessions

[adamraine]

oopif-requests has been failing a lot recently, I believe it has something to do with the upgrade to Puppeteer 16. It was failing how it usually does, with missing network requests.

I was not able to reproduce the smoke failure locally. However, I could find unsolicited network responses when I narrowed the focus of my testing to just network manager:

import puppeteer from 'puppeteer';
import {TargetManager} from '../../lighthouse/core/gather/driver/target-manager.js';

const requestedUrls = new Set();
const unsolicitedResponses = [];

const browser = await puppeteer.launch({
  headless: false,
  defaultViewport: puppeteer.devices['Moto G4'].viewport,
  ignoreDefaultArgs: ['--enable-automation'],
  executablePath: process.env.CHROME_PATH,
});
const page = await browser.newPage();
const session = await page.target().createCDPSession();

const targetManager = new TargetManager(session);
await targetManager.enable();
targetManager.on('protocolevent', e => {
  if (e.method === 'Network.responseReceived') {
    const url = e.params.response.url;
    if (!requestedUrls.has(url)) {
      unsolicitedResponses.push(url);
    }
  } else if (e.method === 'Network.requestWillBeSent') {
    requestedUrls.add(e.params.request.url);
  }
});

await page.goto('http://localhost:10200/oopif-requests.html', {
  waitUntil: ['networkidle0'],
});

console.log(JSON.stringify(unsolicitedResponses, null, 2));
console.log('Requested #', requestedUrls.size);

await page.close();
await browser.close();

I believe that the issues were caused by sessions created by Puppeteer being emitted into our session handler. TBH I didn't figure out the exact details for why this was a problem.

However, restricting our session handlers to handle only the sessions that we create seems to have fixed the issue. That is what is being done in this PR. We should run CI a few times as per usual.

[adamraine]

Hmm seems to have made the problem worse actually

[adamraine]

Looks like the latest puppeteer was necessary for this

[adamraine]

Seems like this has fixed or at least reduced the likelihood of an oopif-requests failure.

However, the Puppeteer upgrade seems to have dramatically increased the likelihood of a lantern-idle-callback-short failure #14271.

I'll keep this open but we should resolve the lantern-idle-callback-short situation before merging.

[adamraine]

And it suggests that with the pptr version bump to 17, it's not reliably emitted there, but instead within child connections.

It is reliably emitted, but on rootConnection we will emit sessionattached events for any session. This includes sessions automatically created by Puppeteer.

In addition to the sessions automatically created by Puppeteer, there are sessions we create when we call Target.setAutoAttach. These sessions will be emitted on the sessionattached events for the rootConnection as well as sessionattached events for the parent session.

This PR changes things so that we only listen to sessionattached on LH specific sessions so we aren't working with the sessions automatically created by Puppeteer.

[brendankenny]

However, I could find unsolicited network responses when I narrowed the focus of my testing to just network manager

FWIW unsolicitedResponses is always empty for me, whether on pptr 16 or 17 (10x runs each). Is that example very flaky for you?

[adamraine]

Is that example very flaky for you?

It was somewhat flaky but I don't think I ever saw unsolicitedResponses completely empty.

[brendankenny]

from some of the discussion from last week:

• What sessions are being attached that we don't want to listen to?
• The oopif-requests failures in CI seem to all be from missing requests (from the ones I've seen at least, e.g. this one), but this change is to eliminate unexpected extra requests?
• What kind of thing are you seeing in unsolicitedResponses? I'm still always getting empty results (being able to repro would also help answer the first question)

[adamraine]

TBH I didn't figure out the exact details for why this was a problem.

I have a more concrete explanation now.

What sessions are being attached that we don't want to listen to?

Puppeteer automatically creates its own sessions and emits them via connection.on('sessionattached') regardless of what Lighthouse does. These sessions are usable, but Puppeteer controls when Runtime.runIfWaitingForDebugger is called. We want to work with sessions we create instead, that way Puppeteer doesn't prematurely resume the targets.

We are still creating our own sessions, but they are ignored if a Puppeteer created session on the same target has already been handled:

https://github.com/GoogleChrome/lighthouse/blob/5e6ee16a519616ef40b2a3987869b79338fa1758/core/gather/driver/target-manager.js#L109

BTW this return ^ statement isn't the end of the function because it is wrapped in a try ... finally, so the session is always resumed even though we ignore it for target management:

https://github.com/GoogleChrome/lighthouse/blob/5e6ee16a519616ef40b2a3987869b79338fa1758/core/gather/driver/target-manager.js#L142-L145

The oopif-requests failures in CI seem to all be from missing requests (from the ones I've seen at least, e.g. this one), but this change is to eliminate unexpected extra requests?

This change isn't designed to eliminate extra requests. It's to prevent us from using sessions that Puppeteer has already resumed before we call Network.enable.

The missing requests can come from missing Network.requestWillBeSent events which I observed in the DevTools log of CI failure artifacts.

What kind of thing are you seeing in unsolicitedResponses? I'm still always getting empty results (being able to repro would also help answer the first question)

Requests that had a Network.responseReceived event but no Network.requestWillBeSent event. This is a sign that Network.enable was not called in time. This will show up as a missing request in our network requests.

[adamraine]

https://github.com/GoogleChrome/lighthouse/actions/runs/3108583849/jobs/5038074267

Possibly introduced by Puppeteer 18.03 -> 18.05 or something in DT or maybe its just more flakiness.

In any case, this PR alone probably shouldn't close #14271