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

feat(oop iframes): integrate OOP iframes with the frame manager #7556

Merged
merged 36 commits into from Oct 28, 2021
Merged

Conversation

@jschfflr
Copy link
Contributor

@jschfflr jschfflr commented Sep 10, 2021

This pull request tries to add better support for OOP iframes (see #2548)

The current problem with OOP iframes is that they are moved to a different target. Because of this, the current implementation of Puppeteer pretty much ignores them.

This change extends the FrameManager to already take OOP iframes into account and hides the fact that those frames are actually in different targets.

Further work needs to be done to also make the NetworkManager aware of these and to make sure that settings like emulations etc. are also properly passed down to the new targets.

@google-cla google-cla bot added the cla: yes label Sep 10, 2021
@jschfflr jschfflr requested a review from OrKoN Sep 16, 2021
@jschfflr
Copy link
Contributor Author

@jschfflr jschfflr commented Sep 16, 2021

@OrKoN Let me know what you think about this pr and if we should incrementally add OOP iframe support or have everything in one big batch?

@OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Sep 16, 2021

Could you describe on a high-level what was missing for OOPIF support and how we are adding it (and what are the difficulties)?

@jschfflr
Copy link
Contributor Author

@jschfflr jschfflr commented Sep 16, 2021

Could you describe on a high-level what was missing for OOPIF support and how we are adding it (and what are the difficulties)?

Sure, I updated the description.

mocha-config/puppeteer-unit-tests.js Outdated Show resolved Hide resolved
mocha-config/puppeteer-unit-tests.js Outdated Show resolved Hide resolved
@OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Sep 16, 2021

What would be the effects of landing partial support? I guess it'd break some clients so maybe it's better to have all changes in one batch?

src/common/DOMWorld.ts Show resolved Hide resolved
src/common/FrameManager.ts Outdated Show resolved Hide resolved
src/common/FrameManager.ts Outdated Show resolved Hide resolved
src/common/Page.ts Outdated Show resolved Hide resolved
@OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Sep 16, 2021

It looks good to me but I am a bit fuzzy about implications for other functionality. Is it easily possible to implement OOPIF support behind a flag? Not suggesting, but just curious if it'd be safer to allow users to opt-in and test at first.

@jschfflr
Copy link
Contributor Author

@jschfflr jschfflr commented Sep 16, 2021

It looks good to me but I am a bit fuzzy about implications for other functionality. Is it easily possible to implement OOPIF support behind a flag? Not suggesting, but just curious if it'd be safer to allow users to opt-in and test at first.

As we currently don't support OOP iframes at all this isn't changing any existing behaviour but only adding new things. As long as we don't regress on existing behaviour, I think we should be fine without a flag.

@OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Sep 16, 2021

As we currently don't support OOP iframes at all this isn't changing any existing behaviour but only adding new things. As long as we don't regress on existing behaviour, I think we should be fine without a flag.

Yeah but I mean new frames might show up unexpectedly for the users and things like this. E.g., they expected frames.length === 1 and suddenly an OOPIF shows up making frames.length === 2. Do you think that won't be common?

@jschfflr jschfflr marked this pull request as ready for review Sep 29, 2021
@jschfflr
Copy link
Contributor Author

@jschfflr jschfflr commented Sep 29, 2021

@OrKoN Could you take another look at this?

src/common/FrameManager.ts Outdated Show resolved Hide resolved
src/common/Page.ts Outdated Show resolved Hide resolved
@jschfflr jschfflr requested a review from OrKoN Oct 28, 2021
src/common/FrameManager.ts Outdated Show resolved Hide resolved
OrKoN
OrKoN approved these changes Oct 28, 2021
Copy link
Collaborator

@OrKoN OrKoN left a comment

Still LGTM. Please make sure that is marked as a breaking change in the release notes.

@jschfflr jschfflr merged commit 4d9dc8c into main Oct 28, 2021
6 checks passed
@jschfflr jschfflr deleted the oopiframes branch Oct 28, 2021
@jschfflr
Copy link
Contributor Author

@jschfflr jschfflr commented Oct 28, 2021

Landed and marked as breaking change 🎉

@mattzeunert
Copy link
Contributor

@mattzeunert mattzeunert commented Nov 13, 2021

I'll need to do more work to find a shareable repro, but in case someone else runs into this, I sometimes get this error after upgrading to v11:

Cannot read property '_updateClient' of undefined
node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:183:15

Update: no repro so far, but this is some of the target info:

attached: true
canAccessOpener: false
type: "iframe"
url: "chrome-untrusted://new-tab-page/one-google-bar?paramsencoded="

I think _onAttachedToTarget is firing 1ms before _onFrameAttached.

@cetico
Copy link

@cetico cetico commented Nov 13, 2021

@mattzeuner I am getting that error. How to fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants