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: bug when handling shadow doms #1041

Merged
merged 1 commit into from
Nov 7, 2022
Merged

Conversation

YunFeng0817
Copy link
Member

@YunFeng0817 YunFeng0817 commented Oct 31, 2022

Bug

On the website https://mixpanel.com/project/2195193/view/139237/app/dashboards#id=3679845, the bottom chart makes the recorder crash (in an infinite loop)

How to reproduce:

  1. Create an account and log in
  2. goto the website and start recording
  3. scroll down to the bottom of the page
  4. when the chart is loaded, the whole browser would freeze (fall into an infinite loop)

image

How to fix:

The chart is an SVG inside a shadow dom. The complicated dom structure raises a corner case for the recording code of shadow dom #956. In this case, the unhandled node is not a direct child of the shadow host and its parent node is not supposed to be the shadow host element. So I add code to check whether the unhandled node is a direct child of shadow dom or not. I tried to reproduce this through a simplified test case but I failed.

On the website https://mixpanel.com/project/2195193/view/139237/app/dashboards#id=3679845, the bottom chart makes the recorder crash (infinite loop)
@Yuyz0112
Copy link
Member

LGTM, any chance to add some tests before we merge this?

@YunFeng0817
Copy link
Member Author

I find it really hard to construct a test case for this. I tried to partially extracted the dom structure and the corner case didn't show up.

Vadman97 added a commit to highlight/highlight that referenced this pull request Nov 4, 2022
## Summary

Updates rrweb and adds shadow-dom test under `/1/buttons` that is invoked from cypress.

## How did you test this change?

New buttons changes that crash without fix (rrweb-io/rrweb#1041) and work after

before: https://www.loom.com/share/12c58018a8314d2fbeb691e462a94aed
after: https://www.loom.com/share/c490d0b8bc194214a47e61832ae57d43

cypress test frozen without the fix
https://www.loom.com/share/8f35a9c5cb164cec956a263c91001f2a
and frozen in CI
https://github.com/highlight-run/highlight/actions/runs/3390553816/jobs/5634828642

## Are there any deployment considerations?

Updating firstload version.
@idosal
Copy link

idosal commented Nov 5, 2022

Hey, thanks for fixing the bug! Do you happen to have an ETA for the fixed version?

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

Successfully merging this pull request may close these issues.

4 participants