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

perf-diagnostics-third-party is broken with FR runner #14093

Closed
adamraine opened this issue Jun 6, 2022 · 7 comments · Fixed by #14153
Closed

perf-diagnostics-third-party is broken with FR runner #14093

adamraine opened this issue Jun 6, 2022 · 7 comments · Fixed by #14153
Assignees
Labels

Comments

@adamraine
Copy link
Member

adamraine commented Jun 6, 2022

Some OOPIF requests are emitting a Network.responseReceived event but no Network.requestWillBeSent event.

Bisect range on yarn smoke perf-diagnostics-third-party --fraggle-rock:
https://chromium.googlesource.com/chromium/src/+log/e2e042f4c34cff4555412fc4bba955cf963e4e39..26600153b33329939cc181a06f5978552283d9ad

This CL is probably where the issues started:
https://chromium.googlesource.com/chromium/src/+/b03884a3a1ffb3ad8ec269590843203db9e7a386

Disabling some of the features from that CL did fix the issue with perf-diagnostics-third-party #14081

Min repro:
https://gist.github.com/adamraine/e9ae2e811bde239db24a47535008e228

I used the min repro to bisect again, and it returned the same commit range as doing yarn smoke perf-diagnostics-third-party --fraggle-rock, so it could be helpful when debugging.

@adamraine
Copy link
Member Author

adamraine commented Jun 7, 2022

I debugged this with @caseq today.

Puppeteer does it's own Target.setAutoAttach that happens before we do it and they set waitForDebugger to false:
https://github.com/puppeteer/puppeteer/blob/353358a996dcaa1cda75ec79e4111252c91f243d/src/common/FrameManager.ts#L143-L146

When we set our own Target.setAutoAttach after this, it doesn't override the value originally set by Puppeteer, so we are not actually waiting for the debugger when using Lighthouse driven by Puppeteer (i.e. fraggle rock).

Perhaps the issue with oopif-scripts is similar where the DT frontend calls Target.setAutoAttach before us, and our call gets negated. Or maybe it's coming from pptr again because we do use pptr to drive the DT smokes. I haven't verified this yet.

@paulirish
Copy link
Member

Puppeteer does it's own Target.setAutoAttach that happens before we do it and they set waitForDebugger to false

I ran into the same thing today in my debugging. Eesh. (I suspect you're right about DT frontend having the same situation. didn't verify tho)

I've found the Target.attachedToTarget CDP event to be useful as it has a waitingForDebugger boolean describing the state of it.

@brendankenny brendankenny added the test backlog disabled or needed tests label Jun 8, 2022
@connorjclark
Copy link
Collaborator

Or maybe it's coming from pptr again because we do use pptr to drive the DT smokes.

I don't think we do. The DT smoke test runner uses the legacy runner, because that's what yarn devtools does.

@adamraine
Copy link
Member Author

I don't think we do. The DT smoke test runner uses the legacy runner, because that's what yarn devtools does.

It uses puppeteer to launch Chrome though, so maybe pptr is getting involved on the target page before Lighthouse is.

@connorjclark
Copy link
Collaborator

When we set our own Target.setAutoAttach after this, it doesn't override the value originally set by Puppeteer, so we are not actually waiting for the debugger when using Lighthouse driven by Puppeteer (i.e. fraggle rock).

Can you clarify this: If there are two sessions to the same root target, then a new target will be paused on creation only if:

  1. the first Target.setAutoAttach has waitForDebugger: true?
  2. or all Target.setAutoAttach have waitForDebugger: true?

Is this expected behavior or a bug?

@adamraine
Copy link
Member Author

1

@caseq was looking into it if I remember correctly because it was "undefined" behavior.

@adamraine
Copy link
Member Author

puppeteer/puppeteer#8507

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 a pull request may close this issue.

4 participants