Skip to content

Conversation

@mydea
Copy link
Member

@mydea mydea commented Feb 13, 2023

This may be the underlying cause here, maybe an e.g. undefined sneaked in there somehow. IMHO this should also safe some bytes.

@mydea mydea requested review from Lms24 and billyvg February 13, 2023 08:48
@mydea mydea self-assigned this Feb 13, 2023
const notInDoc =
!this.doc.contains(n) &&
(rootShadowHost === null || !this.doc.contains(rootShadowHost));
(!rootShadowHost || !this.doc.contains(rootShadowHost));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm would you mind explaining how this fixes the reported issue?
Not sure what I'm missing here but I tried reproducing this locally, by explicitly calling this.doc.contains(undefined) but the call seemed to return false and didn't crash.

Also, any chance that this is caused by the line above (n)?

Copy link
Member

Choose a reason for hiding this comment

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

Ofc chances are that in my simple test app, doc is handling the contains call somewhat differently than in Sentry...

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, oof, I thought that contains(undefined) would break it. If that works, then this isn't a fix at all, then it obv. is a different issue. Then I guess we'd need to do a rootShadowHost instanceof Node check or something along these lines 🤔

@billyvg
Copy link
Member

billyvg commented Feb 13, 2023

Note this was reported upstream as well: rrweb-io#1114

@YunFeng0817
Copy link

https://github.com/rrweb-io/rrweb/pull/891/files
image

This is a similar fix.

@mydea
Copy link
Member Author

mydea commented Feb 20, 2023

https://github.com/rrweb-io/rrweb/pull/891/files image

This is a similar fix.

Ah, interesting, so I guess we can merge this after all!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Then let's go 🚀

@Lms24
Copy link
Member

Lms24 commented Feb 23, 2023

Should fix getsentry/sentry-javascript#7274

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.

5 participants