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: don't emit an internal error when eval causes navigation #3008

Merged
merged 2 commits into from Aug 1, 2018

Conversation

Projects
None yet
2 participants
@JoelEinbinder
Copy link
Collaborator

commented Aug 1, 2018

When an evaluation causes a navigation, for example:

await page.evaluate(() => window.reload());

sometimes we process the ExecutionContextDestroyed event before the ack from the evaluate. When we do get the ack from the evaluate, we try to build a JSHandle for it, and try to find the execution by id. But it is gone, and we throw an error. This patch switches createJSHandle to accept an ExecutionContext instead of just an id.

This bug was making the test should throw a nice error after a navigation flaky.

@JoelEinbinder JoelEinbinder requested a review from aslushnikov Aug 1, 2018

@aslushnikov aslushnikov merged commit 95d867a into GoogleChrome:master Aug 1, 2018

7 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
node6 (linux)
Details
node8 (linux)
Details
node8 (macOS)
Details
pr title lint PR title is good to go, boss
Details

LarenDorr added a commit to LarenDorr/puppeteer that referenced this pull request Mar 19, 2019

fix: don't emit an internal error when eval causes navigation (Google…
…Chrome#3008)

When an evaluation causes a navigation, for example:
```js
await page.evaluate(() => window.reload());
```
sometimes we process the ExecutionContextDestroyed event before the ack from the evaluate. When we do get the ack from the evaluate, we try to build a JSHandle for it, and try to find the execution by id. But it is gone, and we throw an error. This patch switches createJSHandle to accept an ExecutionContext instead of just an id.

This bug was making the test `should throw a nice error after a navigation` flaky.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.