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

rrweb sends message to cross-origin iframe to force snapshot #1465

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

colingm
Copy link
Contributor

@colingm colingm commented May 1, 2024

We often manually take snapshots in cross origin frame situations and were running into problems with no longer having the content of cross origin frames. We noticed that if we sent along a message to tell the cross origin frame to take a new snapshot whenever the parent frame took one then it resolved this issue.

Copy link

changeset-bot bot commented May 1, 2024

🦋 Changeset detected

Latest commit: 01d107f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension Patch
rrvideo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@colingm colingm force-pushed the cgm/cross-origin-snapshot branch 3 times, most recently from 950f836 to fa0208e Compare May 1, 2024 19:42
@colingm colingm force-pushed the cgm/cross-origin-snapshot branch from 37ac6f7 to debf898 Compare May 1, 2024 19:55
@eoghanmurray
Copy link
Contributor

Is there a possibility that this will duplicate the regular way to take an iframe snapshot (when the contentDocument is accessible)?
I'm also curious as to if there is a problem with communicating the snapshot back up to the parent context when the contentDocument can't be accessed.
Some tests to demonstrate the novel scenario and prevent future regression would be appreciated.

@colingm
Copy link
Contributor Author

colingm commented May 9, 2024

@eoghanmurray I am not quite sure if this can be triggered before the inner frame ends up being ready, will need to test that specifically. In practice we don't run into that problem since we don't try to initialize rrweb until the document is ready ourselves so it is worth double checking.

In general the idea that the top frame taking a snapshot (either due to the checkoutN options or manually) means you lose the inner frame entirely was definitely the major concern but it is a good idea to make sure it doesn't mess with delayed initializations for the inner frames.

@@ -216,6 +216,7 @@ export type CrossOriginIframeMessageEventContent<T = eventWithTime> = {
// The origin of the iframe which originally emits this message. It is used to check the integrity of message and to filter out the rrweb messages which are forwarded by some sites.
origin: string;
isCheckout?: boolean;
snapshot?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

This note is a reminder that we need to add a test

@colingm
Copy link
Contributor Author

colingm commented May 15, 2024

Adding a note here as doing some testing with timing of things and have found some oddities where it might not be behaving as expected due to the ordering of definition for takeFullSnapshot and iframeManager. Working through some testing and other options with the team.

@dobrynin
Copy link

fwiw we also have this use case!

Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

Looks really good @colingm! Curious what (edge) cases you've come across, and it would indeed be nice to have a test to make sure this stays working.

Easy-ish way to test this, is load rrweb (but don't record) in a page with cross origin iframe, then after everything is loaded: start recording. Iframe won't be recorded in the way its currently setup, but will with your fix.

@colingm
Copy link
Contributor Author

colingm commented Aug 1, 2024

@Juice10 yeah sorry I have kind of let this languish unfortunately. So we actually found with the original version we had a weird race condition where it wasn't doing what we thought most of the time. We changed how we passed in the function (a setter now being called later) and that seemed to address the odd behavior we saw. Now just to write the tests 😅

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

Successfully merging this pull request may close these issues.

6 participants