Memory Leak: lastCapturedEvent holds a strong reference to an event that prevents DOM subtrees from being garbage collected

[jblas]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Self-hosted/on-premise

Which SDK are you using?

@sentry/react

SDK Version

7.57.0

Framework Version

React 17.0.0

Link to Sentry event

No response

SDK Setup

No response

Steps to Reproduce

1. Click on any element on screen
2. Observe that lastCaptureElement in instrument.ts is maintained until next click

There is some code in makeDOMEventHandler() inside instrument.ts that stores the last click/keypress event in a module global called lastCapturedEvent:

https://github.com/getsentry/sentry-javascript/blob/develop/packages/utils/src/instrument.ts#L500

for the sole purpose of preventing further processing of the same event as it propagates up through the ancestor hierarchy.

https://github.com/getsentry/sentry-javascript/blob/develop/packages/utils/src/instrument.ts#L482

We were wondering if it would be possible for this code to NOT hold a hard reference to the last event, and instead employ some other method that allows the code to figure out that the current event was already seen. Some possible alternate methods could be:

• Tag the event the first time it is seen with a property on the event that indicates that it has already been seen/processed by the event handler and NOT maintain a lastCapturedEvent at all.
• Have lastCapturedEvent be a WeakRef instead of a hard ref to the event in question.

Within our development workflow, we have tests that try to ensure various aspects of our UI (DOM subtrees) are released/garbage-collected after they are dismissed, usually via a click event. We noticed that when using sentry.js, these tests fail because the lastCapturedEvent reference requires a 2nd click/keypress event to remove the hard reference to the click event that dismissed the UI. The main issue here is that as long as lastCapturedEvent holds a reference to this event, its detail and target properties also hold hard references to data and the target DOM element that can prevent an entire DOM subtree from being garbage collected.

Expected Result

No hard references maintained that would impact the application's functionality or testing.

Actual Result

Within our development workflow, we have tests that try to ensure various aspects of our UI (DOM subtrees) are released/garbage-collected after they are dismissed, usually via a click event. We noticed that when using sentry.js, these tests fail because the lastCapturedEvent reference requires a 2nd click/keypress event to remove the hard reference to the click that dismissed the UI. The main issue here is that as long as lastCapturedEvent holds a reference to this event, its detail and target properties also hold hard references to data and the target DOM element that can prevent an entire DOM subtree from being garbage collected.

[lforst]

I think this is a reasonable ask! Both of your suggestions seem like an option to me - it probably doesn't matter which we go with. Thanks for raising this!

[mydea]

I see two key challenges for the two options:

1. WeakRef is not supported in IE11 & Node < 14... so we'd need to build this in a way that also works for older environments.
2. We do use the lastEvent for shouldShortcircuitPreviousDebounce(lastEvent, event) which we couldn't do anymore if we just put a property on the event, to check if it was already captured.

Probably because of constraint 2. we should go with option 1 and build it in a way that works when WeakRef is not available 🤔

[jblas]

@lforst: So if I interpret the fix that was just merged ... the only difference between the original behavior and what just landed is that a hard reference is only maintained for 1 sec?

If the reference is only necessary to filter different events with the same type and element within a timed window ... perhaps tagging the target with a unique sentry id, and then saving that id and the event type in lastEventCaptured can help avoid using a hard ref.

[lforst]

@jblas Correct. I assume this doesn't fully solve your case yet as your tests would have to wait for the target to be unreferenced and gc'd. We can change that I guess.

[jblas]

@lforst: Yes, our application integrates with several 3rd party libraries, the ideal would be for libraries that monkey-patch browser APIs to behave transparently and not alter the default behaviors of those APIs and/or not introduce behaviors that may interfere or alter Garbage Collection since we often run under high memory pressure on devices with minimal resources.

cc: @amccloud