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 BrowsingContext re-initialisation race condition #629

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

sadym-chromium
Copy link
Collaborator

@sadym-chromium sadym-chromium commented Apr 20, 2023

The root cause of the #620 was exactly as described @Lightning00Blade. While Page.frameNavigated event was fired before Runtime.executionContextCreated, as it's async, it's last part with cleaning realms was run after Runtime.executionContextCreated handled.

This fix includes 2 parts:

  1. Remove all the async event handlers.
  2. Rely on the Runtime.executionContextsCleared event rather then Page.frameNavigated to clean Realms.

Tested manually with Puppeteer and repro script from the puppeteer/puppeteer#10032. Couldn't reproduce with Mapper Tab and therefor couldn't add e2e test.

@sadym-chromium sadym-chromium merged commit 55680d5 into main Apr 20, 2023
@sadym-chromium sadym-chromium deleted the sadym/realms branch April 20, 2023 14:06
@Lightning00Blade
Copy link
Collaborator

Closed #620, Closed #621

thiagowfx pushed a commit that referenced this pull request Apr 21, 2023
* Rely on `Runtime.executionContextCreated` event
* Remove unnecessary async event handlers

---------

Co-authored-by: Maksim Sadym <sadym@google.com>
thiagowfx pushed a commit that referenced this pull request Apr 21, 2023
* Rely on `Runtime.executionContextCreated` event
* Remove unnecessary async event handlers

---------

Co-authored-by: Maksim Sadym <sadym@google.com>
thiagowfx pushed a commit that referenced this pull request Apr 21, 2023
* Rely on `Runtime.executionContextCreated` event
* Remove unnecessary async event handlers

---------

Co-authored-by: Maksim Sadym <sadym@google.com>
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

3 participants