fix(replay): Ensure circular references are handled

[mydea]

Currently, we can get circular reference exceptions when JSON-stringifing events for replay.
We can fix this by using our normalize method.

Closes #7742

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	20.73 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified)	64.75 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	19.28 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified)	57.23 KB (0%)
@sentry/browser - Webpack (gzipped + minified)	20.84 KB (0%)
@sentry/browser - Webpack (minified)	68.2 KB (0%)
@sentry/react - Webpack (gzipped + minified)	20.87 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified)	48.69 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	28.3 KB (-0.01% 🔽)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	26.53 KB (-0.01% 🔽)
@sentry/replay ES6 CDN Bundle (gzipped + minified)	44.88 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified)	38.87 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	63.58 KB (+0.01% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified)	56.62 KB (+0.01% 🔺)

[mydea]

OK, so this will actually only normalize custom breadcrumbs. this has two reasons:

1. No need to parse every rrweb payload, as we assume this is not circular (=saving some perf)
2. normalize actually converts undefined to '[undefined]' instead of dropping it, which we don't want

I think this is fine for custom breadcrumbs only..?

[billyvg]

Could we add an option to normalize() to have it leave undefined untouched?

Otherwise, having it only normalize breadcrumbs sgtm

[mydea]

Could we add an option to normalize() to have it leave undefined untouched?

Otherwise, having it only normalize breadcrumbs sgtm

I looked into that a bit, it's possible I guess but a bit more effort, also our normalize is not necessarily 100% JSON compatible even in other areas, so I think it wouldn't be too safe to use that for rrweb stuff as we expect things to be exactly a certain way there 😅 IMHO this should be a good first step, if needed we can later expand this to other payloads!

[billyvg]

I don't think we need to use it at all for rrweb events, I don't think we try to serialize arbitrary user objects there.

I do think we should look into undefined as our UI will display it looking like a string instead of stylizing similar to how the browser does for the undefined value, and it'll be quite confusing.

[mydea]

I don't think we need to use it at all for rrweb events, I don't think we try to serialize arbitrary user objects there.

I do think we should look into undefined as our UI will display it looking like a string instead of stylizing similar to how the browser does for the undefined value, and it'll be quite confusing.

We are currently already normalizing this for other events (errors transactions). The following is normalized:

• breadcrumbs.data
• user
• contexts
• extra

So I guess it would actually be consistent with what is captured in the rest of Sentry?

We can still look at changing this for replay if we feel this is problematic in a follow up!

[billyvg]

Ah, I was thinking about the rrweb event payloads themselves, which don't need it, but yeah the user and contexts on the replay event would make sense.

[sarunast]

@mydea After debugging for a while I am pretty sure because of this change sentry crashes our page every time. Somehow the normalize function starts running infinitely. I can't provide reproduction but we have to disable the Replay because of this issue.

Everything works fine with 7.46 as soon as I try 7.47 page crashes.

[mydea]

Hey,

we are currently looking into this, I think I have an idea what's happening. It may be related to how HTML elements are serialized. I will keep you posted on a fix!

[mydea]

Please see #7916 and #7915 for fixes to this - we'll try to get this released ASAP.

[mydea]

Hi,

we have just released 7.49.0 that should handle more edge cases for this. Could you give this a try, and open a new issue if you're still having problems with this on 7.49.0? Thanks a lot for reporting! We have some further improvements/handling of edge cases on the way here: #7917 but 7.49.0 should already contain a bunch of improvements in this direction!

[sarunast]

@mydea thanks for a quick fix. I tested it on 7.49.0 and it works now 👍

[phil-tutti]

Good morning

after the fix in 7.49.0, we (I work together with @sarunast) updated it from 7.46.0 to 7.49.0 and on the client (browser) side, everything seems to work. However, for some reason, we saw an increase of memory usage on our server (we use server side rendering and also use sentry there). The memory was not freed up, which led to an alert on our monitoring system, that we're running out of memory soon.

Here you can see the constant increase of memory usage over the past 2 days (the increase was slow, but steady):

Here you can see the memory consumption before the release, it's a steady graph:

After the alert, we rolled back only the sentry version from 7.49.0 to 7.46.0 and we're back at the steady graph, without any increase in memory usage.

Interesting enough, we did not receive any SSR errors, but still, the increase in memory happened.

I'm not sure if this is the right place to report it. If not, let me know and I'll open an issue. And I apologize for not being able to provide a reproduction repository, but as the increase happened very slowly over 2 days, it's hard provide one.

[AbhiPrasad]

@phil-tutti - are you running the browser SDK with replay in SSR? That's not supported - you should be running the Node SDK.

If you are using the Node SDK, the issue might be with async local storage introduced with 7.48.0: https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md#7480. Are you using domains in your app to do scope isolation with sentry? You'll need to switch to using Sentry.runWithAsyncContext.

If none of that applies - do you mind opening another GH issue with some details about your setup (framework, other sdks used, etc.) - would help us a lot for debugging.

Sorry for the trouble - memory leaks like this are unacceptable to us and we'll make sure we prio to figure out what is going on!

[phil-tutti]

@AbhiPrasad - thank you for your fast response. I shortly checked what you suggested, and can't see that we're using domains to do scope isolation. We're also using the sentry/node package for our server-side setup.

Here's the issue I opened: #7982