fix: Prevent ST from holding onto Event refs in memory when aborted (#1491)

Author: cwli24

src/features/session_trace/aggregate/index.js

@@ -86,7 +86,7 @@ export class Aggregate extends AggregateBase {
     registerHandler('bstResource', (...args) => this.events.storeResources(...args), this.featureName, this.ee)
     registerHandler('bstHist', (...args) => this.events.storeHist(...args), this.featureName, this.ee)
     registerHandler('bstXhrAgg', (...args) => this.events.storeXhrAgg(...args), this.featureName, this.ee)
-    registerHandler('bstApi', (...args) => this.events.storeSTN(...args), this.featureName, this.ee)
+    registerHandler('bstApi', (...args) => this.events.storeNode(...args), this.featureName, this.ee)
     registerHandler('trace-jserror', (...args) => this.events.storeErrorAgg(...args), this.featureName, this.ee)
     registerHandler('pvtAdded', (...args) => this.events.processPVT(...args), this.featureName, this.ee)
 

src/features/session_trace/aggregate/trace/storage.js

@@ -41,15 +41,18 @@ export class TraceStorage {
     this.parent = parent
   }
 
-  /** Central function called by all the other store__ & addToTrace API to append a trace node. */
-  storeSTN (stn) {
-    if (this.parent.blocked) return
+  #canStoreNewNode () {
+    if (this.parent.blocked) return false
     if (this.nodeCount >= MAX_NODES_PER_HARVEST) { // limit the amount of pending data awaiting next harvest
-      if (this.parent.mode !== MODE.ERROR) return
+      if (this.parent.mode !== MODE.ERROR) return false
       const openedSpace = this.trimSTNs(ERROR_MODE_SECONDS_WINDOW) // but maybe we could make some space by discarding irrelevant nodes if we're in sessioned Error mode
-      if (openedSpace === 0) return
+      if (openedSpace === 0) return false
     }
+    return true
+  }
 
+  /** Central internal function called by all the other store__ & addToTrace API to append a trace node. They MUST all have checked #canStoreNewNode before calling this func!! */
+  #storeSTN (stn) {
     if (this.trace[stn.n]) this.trace[stn.n].push(stn)
     else this.trace[stn.n] = [stn]
 
@@ -135,6 +138,11 @@ export class TraceStorage {
     }
   }
 
+  storeNode (node) {
+    if (!this.#canStoreNewNode()) return
+    this.#storeSTN(node)
+  }
+
   processPVT (name, value, attrs) {
     this.storeTiming({ [name]: value })
   }
@@ -160,13 +168,16 @@ export class TraceStorage {
           Math.floor(this.parent.timeKeeper.correctAbsoluteTimestamp(val))
         )
       }
-      this.storeSTN(new TraceNode(key, val, val, 'document', 'timing'))
+      if (!this.#canStoreNewNode()) return // at any point when no new nodes can be stored, there's no point in processing the rest of the timing entries
+      this.#storeSTN(new TraceNode(key, val, val, 'document', 'timing'))
     }
   }
 
   // Tracks the events and their listener's duration on objects wrapped by wrap-events.
   storeEvent (currentEvent, target, start, end) {
     if (this.shouldIgnoreEvent(currentEvent, target)) return
+    if (!this.#canStoreNewNode()) return // need to check if adding node will succeed BEFORE storing event ref below (*cli Jun'25 - addressing memory leak in aborted ST issue #NR-420780)
+
     if (this.prevStoredEvents.has(currentEvent)) return // prevent multiple listeners of an event from creating duplicate trace nodes per occurrence. Cleared every harvest. near-zero chance for re-duplication after clearing per harvest since the timestamps of the event are considered for uniqueness.
     this.prevStoredEvents.add(currentEvent)
 
@@ -178,7 +189,7 @@ export class TraceStorage {
     } catch (e) {
       evt.o = eventOrigin(null, target, this.parent.ee)
     }
-    this.storeSTN(evt)
+    this.#storeSTN(evt)
   }
 
   shouldIgnoreEvent (event, target) {
@@ -216,36 +227,42 @@ export class TraceStorage {
 
   // Tracks when the window history API specified by wrap-history is used.
   storeHist (path, old, time) {
-    this.storeSTN(new TraceNode('history.pushState', time, time, path, old))
+    if (!this.#canStoreNewNode()) return
+    this.#storeSTN(new TraceNode('history.pushState', time, time, path, old))
   }
 
   #laststart = 0
   // Processes all the PerformanceResourceTiming entries captured (by observer).
   storeResources (resources) {
     if (!resources || resources.length === 0) return
 
-    resources.forEach((currentResource) => {
-      if ((currentResource.fetchStart | 0) <= this.#laststart) return // don't recollect already-seen resources
+    for (let i = 0; i < resources.length; i++) {
+      const currentResource = resources[i]
+      if ((currentResource.fetchStart | 0) <= this.#laststart) continue // don't recollect already-seen resources
+      if (!this.#canStoreNewNode()) break // stop processing if we can't store any more resource nodes anyways
 
       const { initiatorType, fetchStart, responseEnd, entryType } = currentResource
       const { protocol, hostname, port, pathname } = parseUrl(currentResource.name)
       const res = new TraceNode(initiatorType, fetchStart | 0, responseEnd | 0, `${protocol}://${hostname}:${port}${pathname}`, entryType)
-      this.storeSTN(res)
-    })
+
+      this.#storeSTN(res)
+    }
 
     this.#laststart = resources[resources.length - 1].fetchStart | 0
   }
 
   // JavascriptError (FEATURE) events pipes into ST here.
   storeErrorAgg (type, name, params, metrics) {
     if (type !== 'err') return // internal errors are purposefully ignored
-    this.storeSTN(new TraceNode('error', metrics.time, metrics.time, params.message, params.stackHash))
+    if (!this.#canStoreNewNode()) return
+    this.#storeSTN(new TraceNode('error', metrics.time, metrics.time, params.message, params.stackHash))
   }
 
   // Ajax (FEATURE) events--XML & fetches--pipes into ST here.
   storeXhrAgg (type, name, params, metrics) {
     if (type !== 'xhr') return
-    this.storeSTN(new TraceNode('Ajax', metrics.time, metrics.time + metrics.duration, `${params.status} ${params.method}: ${params.host}${params.pathname}`, 'ajax'))
+    if (!this.#canStoreNewNode()) return
+    this.#storeSTN(new TraceNode('Ajax', metrics.time, metrics.time + metrics.duration, `${params.status} ${params.method}: ${params.host}${params.pathname}`, 'ajax'))
   }
 
   /* Below are the interface expected & required of whatever storage is used across all features on an individual basis. This allows a common `.events` property on Trace shared with AggregateBase.
@@ -271,7 +288,12 @@ export class TraceStorage {
   }
 
   reloadSave () {
-    Object.values(this.#backupTrace).forEach(stnsArray => stnsArray.forEach(stn => this.storeSTN(stn)))
+    for (const stnsArray of Object.values(this.#backupTrace)) {
+      for (const stn of stnsArray) {
+        if (!this.#canStoreNewNode()) return // stop attempting to re-store nodes
+        this.#storeSTN(stn)
+      }
+    }
   }
 
   clearSave () {

tests/components/session_trace/aggregate.test.js

@@ -104,7 +104,7 @@ test('when max nodes per harvest is reached, no node is further added in FULL mo
   sessionTraceAggregate.events.nodeCount = MAX_NODES_PER_HARVEST
   sessionTraceAggregate.mode = MODE.FULL
 
-  sessionTraceAggregate.events.storeSTN({ n: 'someNode', s: 123 })
+  sessionTraceAggregate.events.storeNode({ n: 'someNode', s: 123 })
   expect(sessionTraceAggregate.events.nodeCount).toEqual(MAX_NODES_PER_HARVEST)
   expect(Object.keys(sessionTraceAggregate.events.trace).length).toEqual(0)
 })
@@ -114,7 +114,19 @@ test('when max nodes per harvest is reached, node is still added in ERROR mode',
   sessionTraceAggregate.mode = MODE.ERROR
   jest.spyOn(sessionTraceAggregate.events, 'trimSTNs').mockReturnValue(MAX_NODES_PER_HARVEST)
 
-  sessionTraceAggregate.events.storeSTN({ n: 'someNode', s: 123 })
+  sessionTraceAggregate.events.storeNode({ n: 'someNode', s: 123 })
   expect(sessionTraceAggregate.events.nodeCount).toEqual(MAX_NODES_PER_HARVEST + 1)
   expect(Object.keys(sessionTraceAggregate.events.trace).length).toEqual(1)
 })
+
+test('aborted ST feat does not continue to hog event ref in memory from storeEvent', () => {
+  sessionTraceAggregate.blocked = true // simulate aborted condition
+  sessionTraceAggregate.events.clear()
+  expect(sessionTraceAggregate.events.prevStoredEvents.size).toEqual(0)
+
+  const someClick = new Event('click')
+  sessionTraceAggregate.events.storeEvent(someClick, 'document', 0, 100)
+  expect(sessionTraceAggregate.events.prevStoredEvents.size).toEqual(0)
+
+  sessionTraceAggregate.blocked = false // reset blocked state
+})