🐛 [RUM-4436] fix detached node memory leak on CLS

[thomas-lebeau]

Motivation

trackCumulativeLayoutShift keep track of the LCP target Element using a sliding window of layout shift events but this window is cleared 5 minutes after the view ends.

Within these 5 minutes, many new view events might be started meaning an increase in memory consumption du to the reference to the LCP target Element kept and preventing garbage collection of these elements (and potentially large amount of descendent)

Changes

Hold onto the LCP target element using a weak reference (WeakRef) to not prevent it to be garbage-collected.

Testing

• Local
• Staging
• Unit
• End to end

---

I have gone over the contributing documentation.

[cit-pr-commenter]

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	157.60 KiB	157.55 KiB	-44 B	-0.03%	✅
Logs	56.29 KiB	56.29 KiB	0 B	0.00%	✅
Rum Slim	104.09 KiB	104.04 KiB	-48 B	-0.05%	✅
Worker	25.21 KiB	25.21 KiB	0 B	0.00%	✅

🚀 CPU Performance

Action Name	Base Average Cpu Time (ms)	Local Average Cpu Time (ms)	𝚫
adderror	0.030	0.038	0.008
addaction	0.014	N/A	N/A
logmessage	0.005	0.005	0.000
startview	0.804	0.955	0.151
startstopsessionreplayrecording	0.762	0.836	0.074
addtiming	0.001	0.001	0.000
addglobalcontext	0.001	0.001	0.000

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.28%. Comparing base (b0d638c) to head (ca2802a).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2749      +/-   ##
==========================================
+ Coverage   93.27%   93.28%   +0.01%     
==========================================
  Files         241      241              
  Lines        7034     7032       -2     
  Branches     1554     1555       +1     
==========================================
- Hits         6561     6560       -1     
+ Misses        473      472       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BenoitZugmeyer]

Thought: maybe a simpler solution could be to avoid storing the target altogether.
In trackCumulativeLayoutShift, we could have something like this:

const isLargestLayoutShift = window.update(entry)
// ... then:
if (isLargestLayoutShift) {
  const clsTarget = find(
            entry.sources,
            (s): s is { node: HTMLElement } => !!s.node && isElementNode(s.node)
          )?.node;

          if (
            clsTarget &&
            // Check if the CLS target have been removed from the DOM between the time we collect the target reference and when we compute the selector
            clsTarget.isConnected
          ) {
            clsTargetSelector = getSelectorFromElement(clsTarget, configuration.actionNameAttribute)
          }
  }

}

and remove largestLayoutShiftTarget from the window target. Also we could do the same for largestLayoutShiftTime.

[thomas-lebeau]

@BenoitZugmeyer
I think this is not possible. The problem is that we need to calculate the selector of the largest layout shift within the window and check if it's still connected. Because of this we need to keep a reference to target of the largest layout shift for as long as the window is open.

Edit: As discussed during our 1:1, the isConnected check is only relevant to calculate the selector (Because performance event are async, it's possible that the node is disconnected at the time the event is handled).
With the changes in this PR, it is now possible to report a selector instead of undefined for nodes that are responsible for the largest shift but are disconnected before the folowing shift within the same window.

[thomas-lebeau]

/to-staging

[dd-devflow]

🚂 Branch Integration: starting soon, merge in < 9m

Commit 4ebadc8b01 will soon be integrated into staging-20.

This build is going to start soon! (estimated merge in less than 9m)

Use /to-staging -c to cancel this operation!

[dd-devflow]

🚂 Branch Integration: This commit was successfully integrated

Commit 4ebadc8b01 has been merged into staging-20 in merge commit 173bf4e0d7.

Check out the triggered pipeline on Gitlab 🦊

[amortemousque]

🔨 warning: ‏With this implementation we are computing the CSS selector lot more often:

• For each window even if it's not a maxClsValue
• For each shift entry if the value is superior to the precedent
  Since layout shift can be frequently triggered by some UI elements, it can have an impact on the processing.