search

getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.68k stars 1.49k forks source link

Sentry replay removes insertRule hooks #11837

Open eugene-chang-fs opened 3 weeks ago

eugene-chang-fs commented 3 weeks ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/react

SDK Version

7.112.2

Framework Version

No response

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

As far as I'm aware, this happens to all versions and all SDKs that include rrweb replay.

Any hooks added to insertRule are dropped once rrweb stops recording.

  1. Start capturing
  2. Add a hook to CSSStyleSheet.prototype.insertRule
  3. Stop capturing
  4. Observe that hook added by step 2 is no longer in place

Below you will see two videos of a demo page. Once hooked, adding a rule to change the color of the text snippet at the top should result in an output line, indicating that the hook worked. But in the result with rrweb, this behavior is absent, indicating that the hook was dropped.

Expected Result

Sentry/rrweb should not drop hooks on the prototypical chain (shown in demo under output).

See result without rrweb: https://imgur.com/C0veqB0

Actual Result

Sentry/rrweb does drop hooks installed.

See result with rrweb extension: https://imgur.com/a/DGIoUSv

eugene-chang-fs commented 3 weeks ago

Here's a jsfiddle that includes the same code as shown above: https://jsfiddle.net/pmd5L8v7/

eugene-chang-fs commented 3 weeks ago

Here's the line from rrweb that resets the insertRule back to the original captured value, causing this bug: https://github.com/rrweb-io/rrweb/blob/master/packages/rrweb/src/record/observer.ts#L862

Additional info: clarity also had a similar issue that was later addressed by this PR: https://github.com/microsoft/clarity/pull/231

mydea commented 3 weeks ago

Hmm, that's interesting of course. So the fix would be to basically not reset this when replay ends 🤔 cc @billyvg, what are your thoughts on this?

billyvg commented 2 weeks ago

Ah makes sense to me -- though this probably belongs in https://github.com/rrweb-io/rrweb/