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

fix: Always set target for attached sessions #10946

Closed
wants to merge 1 commit into from

Conversation

adamraine
Copy link
Contributor

@adamraine adamraine commented Sep 19, 2023

In Lighthouse we are still getting uncaught exceptions "Target must exist" during our bfcache testing similar to #10892. Unfortunately I haven't created a minimum repro yet, here is what I do know:

  • This happens for CDP sessions created on web worker targest
  • The error is emitted because the session never has _setTarget called, this PR addresses that problem
  • This happens as a result of the history navigation that restores from bfcache. Again similar to [Bug]: Errors thrown after doing history restoration on page with oopifs/workers #10892 but the repro in that bug will not reproduce this.
    • Speculation: When the history navigation is performed, a new CDP session is created for the web worker. Since it's a bfcache restoration the target is the same but the session never has _setTarget called because we do not call into the target factory.

Please let me know if this needs more investigation.

@OrKoN
Copy link
Collaborator

OrKoN commented Sep 20, 2023

@adamraine do you have a repro we could add to the tests? I am not sure that ChromeTargetManager is the right place to set the target here. I have learned that bfcache restoration does not cause a new session to be created so I wonder how exactly the session gets no target. We have a test for bfcache case now https://github.com/puppeteer/puppeteer/blob/main/test/src/bfcache.spec.ts how can we extend it with the situation you are observing?

@adamraine
Copy link
Contributor Author

We have a test for bfcache case now main/test/src/bfcache.spec.ts how can we extend it with the situation you are observing?

That is what I am having difficulty with. I can easily reproduce this on the Lighthouse side but I can't create a min repro. I'll keep trying to get a min repro this week as it's blocking our upgrading of puppeteer at this point.

@adamraine
Copy link
Contributor Author

#10966

@OrKoN
Copy link
Collaborator

OrKoN commented Sep 20, 2023

Thanks for the fix. Closing this PR in favour of #10967

@OrKoN OrKoN closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants