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: Switch to real dom before rebuilding fullsnapshot #1139

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

YunFeng0817
Copy link
Member

@YunFeng0817 YunFeng0817 commented Feb 16, 2023

This pull request makes rrdom optimization robust.

  1. Normally rebuilding full snapshots should not be under a virtual dom environment.
    But if the order of data events has some issues, it might be possible.
    Adding a check to avoid any potential issues.

  2. Fix the bug: If an RRNode appends the same child twice, the child nodes will become an infinite linked list. This is a bug imported in improve rrdom performance #1127

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2023

🦋 Changeset detected

Latest commit: d63cc1e

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

This PR includes changesets to release 7 packages
Name Type
rrweb Patch
rrdom Patch
rrweb-snapshot Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension 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

@YunFeng0817 YunFeng0817 requested a review from Juice10 February 16, 2023 05:03
@Juice10
Copy link
Contributor

Juice10 commented Feb 16, 2023

By setting usingVirtualDom to false, aren’t you essentially turning off all rrdom/virtual dom optimizations whenever you hit a full snapshot?

@YunFeng0817
Copy link
Member Author

YunFeng0817 commented Feb 16, 2023

By setting usingVirtualDom to false, aren’t you essentially turning off all rrdom/virtual dom optimizations whenever you hit a full snapshot?

Yes, because if a new fullsnapshot is rebuilt, the status of the existing virtual dom doesn't mean anything as it represents the previous snapshot. usingVirtualDom is an internal switch that determines the current dom environment. It's different from the replayer's configuration useVirtualDom.

Virtual dom optimization shall only be activated if there are incremental node mutations. Refer to the code: https://github.com/rrweb-io/rrweb/blob/master/packages/rrweb/src/replay/index.ts#L1340-L1343

This pull request won't change any logic for correct event data. It is made for data with flaws. Previous parentFragment optimization can tolerate those flaws. So I think the new version should also have the capability.

If a rrnode appends the same child twice, the child nodes will become an infinite linked list
@eoghanmurray
Copy link
Contributor

For future work: I feel like the usingVirtualDom global is dangerous; I'd prefer that this variable be passed down the call chain to each function rather than relying on global state. I've come across a situation in last few weeks where after an error, more errors were generated because it's state wasn't reset.

@YunFeng0817
Copy link
Member Author

Your suggestion can also help. I think if this pull request is merged, there are not many serious bugs left.

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.

[Bug]: Seeking repeatedly crashes chrome tab (when using virtual dom)
3 participants