Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose other sessions from connection #6863

Merged
merged 2 commits into from May 7, 2021

Conversation

patrickhulce
Copy link
Contributor

@patrickhulce patrickhulce commented Feb 10, 2021

This PR is aimed at providing mechanisms to allow puppeteer consumers to workaround OOPIF issues (see #2548). With this PR any consumer with a handle to a CDPSession should be able to find and listen to events from subtargets.

cc @mathiasbynens @jschfflr FYI

Example Script
const puppeteer = require('.');

async function go() {
  const browser = await puppeteer.launch({
    headless: false,
  });

  const page = await browser.newPage();

  const session = await page.target().createCDPSession();
  await session.send('Target.setAutoAttach', {
    autoAttach: true,
    flatten: true,
    waitForDebuggerOnStart: true,
  });

  const regularRequests = new Set();
  const oopifRequests = new Set();

  // Listen for regular requests
  session.on('Network.requestWillBeSent', ({ request }) =>
    regularRequests.add(request.url)
  );
  await session.send('Network.enable');

  // Listen for OOPIF requests
  session.connection().on('sessionattached', async (session) => {
    console.log('Session attached!');
    session.on('Network.requestWillBeSent', ({ request }) =>
      oopifRequests.add(request.url)
    );
    await session.send('Network.enable');
  });

  // Any URL with a YouTube embed or some ads should work.
  await page.goto(
    'https://www.theverge.com/22265570/mustang-mach-e-review-electric-ford-ev-photos-video'
  );

  await new Promise((r) => setTimeout(r, 4_000));

  const hiddenRequests = new Set(oopifRequests);
  for (const url of regularRequests) hiddenRequests.delete(url);

  console.log('Found', regularRequests.size, 'regular requests');
  console.log('Found', oopifRequests.size, 'OOPIF requests');
  console.log(hiddenRequests.size, 'were only visible from OOPIFs');
  console.log(
    Array.from(hiddenRequests)
      .map((s) => `  ${s}`)
      .slice(0, 5)
      .join('\n')
  );

  await browser.close();
}

go();
Output
Session attached!
# ... and many others

Found 196 regular requests
Found 307 OOPIF requests
305 were only visible from OOPIFs
  https://phonograph2.voxmedia.com/pickup.js
  https://volume.vox-cdn.com/packs/js/2.0/embed.js
  https://volume.vox-cdn.com/packs/js/vendors~embed-chorus~embed-youtube-dca572bb40097a171c0a.chunk.js
  https://volume.vox-cdn.com/packs/js/embed-chorus~embed-youtube-c0dd8b44151457cd8e92.chunk.js
  https://volume.vox-cdn.com/packs/js/embed-youtube-bafee92b3ba0cb37e6d9.chunk.js

@google-cla google-cla bot added the cla: yes label Feb 10, 2021
test/CDPSession.spec.ts Outdated Show resolved Hide resolved
@mathiasbynens
Copy link
Member

Jan and Maksim, could you PTAL and help Patrick with the tests?

@sadym-chromium
Copy link
Collaborator

This approach creates a workaround for the OOPIF case, but from the other side, it would work only in headful chrome (headless doesn't kick frames out).

FYI, there is a way to disable kicking out frame from process by using the launch flag site-per-process in launcher?

@patrickhulce
Copy link
Contributor Author

patrickhulce commented Feb 12, 2021

Thanks for chiming in @sadym-chromium !

This approach creates a workaround for the OOPIF case, but from the other side, it would work only in headful chrome (headless doesn't kick frames out).
FYI, there is a way to disable kicking out frame from process by using the launch flag site-per-process in launcher?

Yes and that's what we're trying to accomplish here. Lighthouse is attempting to migrate to using puppeteer and needs to support Network data from OOPIFs. We have two main cases in mind.

  1. Running Lighthouse on a headful browser that the user already launched. A chromium launch flag does not work for this, we need the headful multi-target workaround found here.
  2. Running Lighthouse on a headless browser that is being launched as part of Lighthouse. There we can control the flags so the multi-target work isn't really necessary as we can already use the disable feature flag you mention.

@sadym-chromium
Copy link
Collaborator

The PR is hacky, but serves its purpose.

Please implement the tests and it's good to go IMO.

@patrickhulce patrickhulce force-pushed the connection_emit_session branch 2 times, most recently from bd50331 to 63f7a69 Compare February 24, 2021 17:43
@patrickhulce patrickhulce marked this pull request as ready for review February 24, 2021 17:44
@patrickhulce patrickhulce force-pushed the connection_emit_session branch 2 times, most recently from 174c232 to 0a4ee91 Compare February 24, 2021 17:48
@patrickhulce
Copy link
Contributor Author

Thanks @sadym-chromium for the pointer to the headful test file!

The PR is hacky, but serves its purpose.

Is there a different API you'd prefer for exposing this information? I acknowledge what the consumer of this API has to do feels quite hacky, but this felt like a reasonable addition to the API surface for information that is currently impossible to obtain even though it can be requested via CDPSession already.

@patrickhulce
Copy link
Contributor Author

Any advice for debugging the windows-only failure? Spin up a VM and try it myself?

@jschfflr
Copy link
Contributor

jschfflr commented Mar 1, 2021

It's not immediately clear to me what might cause the test to fail on windows. Happy to try to reproduce it on windows tomorrow though :)

@sadym-chromium
Copy link
Collaborator

I suspect event orders can cause it.

Another unlikely reason can be in not having a frame kicked out for whatever reason, in which case special flag should be used.

@patrickhulce
Copy link
Contributor Author

Thanks @jschfflr that would be much appreciated!

Just a heads up team, I'll be away from code the next few weeks, so my progress may stall here temporarily, but not indicative of lower importance or interest :)

@jschfflr
Copy link
Contributor

jschfflr commented Mar 4, 2021

I'm working on it. Just have to wait until my windows machine is provided. Currently I'm still missing some admin rights that I need in order to install nodejs.

@patrickhulce
Copy link
Contributor Author

Alright I'm back from leave and ready to work on this again, thanks for your patience :)

Any advice for the Windows debugging?

@jschfflr
Copy link
Contributor

@patrickhulce I tried to reproduce the failing test on windows without success so far :/
Maybe site isolation is not enabled in this case. Not sure if this could happen due to memory pressure for example...
Any idea how we could check if site isolation happened?

@jschfflr
Copy link
Contributor

Maybe you could add a temporary console log or something to see if the iframe got kicked out or if it's still in the same process. That way we could verify if we are having a problem with site isolation or if we should search somewhere else...

@patrickhulce
Copy link
Contributor Author

Thanks for looking into it @jschfflr!

I tried to reproduce the failing test on windows without success so far :/

This means the test is passing for you as well, correct? I'll try to add some additional logging to find the source of the CI failure then 👍

@jschfflr
Copy link
Contributor

Yes, it's passing on my windows machine.

@patrickhulce
Copy link
Contributor Author

Alright I've deflaked the Windows test and rebased against the other failures. I think we should be good here for final review :)

Copy link
Contributor

@jschfflr jschfflr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me!

@jschfflr
Copy link
Contributor

jschfflr commented May 3, 2021

@patrickhulce Could you explain how you deflaked the test on windows?

@patrickhulce
Copy link
Contributor Author

@patrickhulce Could you explain how you deflaked the test on windows?

Sure! I noticed in the github actions logs that the iframe target and extra session were being emitted but just the network requests weren't being captured. There was nothing in the test that actually guaranteed the correct sequence of create target -> listen on network -> fire new network request -> quit chrome and it was just implicit that we would be listening during the stylesheet request.

Experience on other projects' flaky tests and CI/Windows only clued me in that on that underpowered hardware you really need to be explicit about every wait condition, so I added a fetch to observe instead.

@jschfflr
Copy link
Contributor

jschfflr commented May 6, 2021

@patrickhulce Ah ok, thanks for sharing your experience!

@patrickhulce
Copy link
Contributor Author

Anytime! What are the next steps to get this landed? Are there additional approvals needed?

@jschfflr jschfflr enabled auto-merge (squash) May 7, 2021 08:17
@jschfflr
Copy link
Contributor

jschfflr commented May 7, 2021

I think we are good here. I updated the branch and enabled auto merge so this should land shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants