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

Jump to bottom

fix(page): Page.createIsolatedWorld error catching has been added #7848

Merged
merged 11 commits into from Mar 2, 2022
Merged

fix(page): Page.createIsolatedWorld error catching has been added #7848

merged 11 commits into from Mar 2, 2022

Conversation

DimaShustal
Copy link
Contributor

What kind of change does this PR introduce?
Bug fix

Did you add tests for your changes?
No

If relevant, did you update the documentation?
Not relevant

Summary
When navigating, frames are destroyed and sending events leads to an error that crashes the whole process. This error was mentioned here. Error example:

ProtocolError: Protocol error (Page.createIsolatedWorld): No frame for given id found
at /.../node_modules/puppeteer/src/common/Connection.ts:300:16
at new Promise (<anonymous>)
at CDPSession.send (/.../node_modules/puppeteer/src/common/Connection.ts:296:12)
at /.../node_modules/puppeteer/src/common/FrameManager.ts:397:19
at Array.map (<anonymous>)
at FrameManager._ensureIsolatedWorld (/.../node_modules/puppeteer/src/common/FrameManager.ts:396:10)
at async Promise.all (index 1)
at async FrameManager.initialize (/.../node_modules/puppeteer/src/common/FrameManager.ts:143:7)
at async FrameManager._onAttachedToTarget (/.../node_modules/puppeteer/src/common/FrameManager.ts:262:5) {
originalMessage: 'No frame for given id found'
}

Does this PR introduce a breaking change?
As I can see there is no breaking change

fix(page): Page.createIsolatedWorld error catching has been added
3c20ab4 
when navigating, frames are destroyed and sending events leads to an error that crashes the whole process
eemelipa
eemelipa reviewed Dec 29, 2021
View reviewed changes
src/common/FrameManager.ts Outdated
@@ -398,7 +398,7 @@ export class FrameManager extends EventEmitter {
frameId: frame._id,
worldName: name,
grantUniveralAccess: true,
})
}).catch(console.error)
Copy link

Choose a reason for hiding this comment

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

Earlier this was

Suggested change
}).catch(console.error)
}).catch(debugError)

imported with import {debugError} from './helper';. Should it be the debugError instead?

(For some reason it got removed in 4d9dc8c)

Copy link

Choose a reason for hiding this comment

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

👍

Looks good now

eemelipa
eemelipa approved these changes Dec 30, 2021
View reviewed changes
Copy link

@eemelipa eemelipa left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

I tried the fix in my use case and it seemed to work as expected

Someone from Puppeteer team should give their blessing as well. I don't know all the implications of the change, and why it broke now. I believe the catch clause hasn't been there for a few puppeteer releases so some other change has made this issue surface again (e.g. with puppeteer 11.0.0 it wasn't there but v11 worked ok for me)

@jozsi jozsi mentioned this pull request Jan 19, 2022
Closed
DimaShustal added 2 commits February 11, 2022 15:26
@OrKoN
fix(page): Page.createIsolatedWorld error catching has been added
0eebe4e 
when navigating, frames are destroyed and sending events leads to an error that crashes the whole process
@OrKoN
fix(page): console.error was replaced by debugError
32d4312
@OrKoN
Copy link
Collaborator

OrKoN commented Feb 17, 2022

Hey, thanks for the PR. Could you please sign a CLA and rebase (with code style fixes npm run eslint-fix) to get this landed?

@OrKoN OrKoN closed this Feb 17, 2022
@OrKoN OrKoN reopened this Feb 17, 2022
@OrKoN
Copy link
Collaborator

OrKoN commented Feb 17, 2022

Didn't mean to close :D

OrKoN
OrKoN requested changes Feb 17, 2022
View reviewed changes
Copy link
Collaborator

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

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

marking as requiring changes (see my previous comment)

@DimaShustal DimaShustal reopened this Feb 23, 2022
@DimaShustal
Copy link
Contributor Author

I will update PR tomorrow

@OrKoN OrKoN enabled auto-merge (squash) March 1, 2022 09:05
@dmyerscough
Copy link

When will this change make it into Puppeteer?

@OrKoN OrKoN merged commit e11fe71 into puppeteer:main Mar 2, 2022
OrKoN added a commit that referenced this pull request Mar 3, 2022
@DimaShustal @OrKoN
fix(page): page.createIsolatedWorld error catching has been added (#7848
309e8b8 
)

Co-authored-by: DimaShustal <dzmitry.shustal@gmail.com>
Co-authored-by: Alex Rudenko <OrKoN@users.noreply.github.com>
This was referenced May 30, 2022
Closed
Closed
This was referenced May 30, 2022
Closed
Closed
Closed
This was referenced May 30, 2022
Closed
Closed
This was referenced Jun 13, 2022
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrKoN OrKoN OrKoN approved these changes

@eemelipa eemelipa eemelipa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@DimaShustal @OrKoN @dmyerscough @eemelipa