Skip to content

🐛 fix resource timing buffer listener not stopped on cleanup #4360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thomas-lebeau wants to merge 3 commits into from
Closed

🐛 fix resource timing buffer listener not stopped on cleanup #4360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f63be22
🐛 fix resource timing buffer listener not stopped on cleanup
thomas-lebeau Mar 20, 2026
e2d5a3c
🐛 fix additional cleanup issues in observable and activity tracking
thomas-lebeau Mar 23, 2026
e0ce198
🐛 fix manageResourceTimingBufferFull called for non-resource entry types
thomas-lebeau Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/core/src/tools/observable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ export class Observable<T> {
protected removeObserver(observer: Observer<T>) {
this.observers = this.observers.filter((other) => observer !== other)
if (!this.observers.length && this.onLastUnsubscribe) {
this.onLastUnsubscribe()
// Clear before calling to prevent re-entrant calls if cleanup triggers another unsubscribe
const cleanup = this.onLastUnsubscribe
this.onLastUnsubscribe = undefined
cleanup()
}
}
}
Expand Down
15 changes: 11 additions & 4 deletions packages/rum-core/src/browser/performanceObservable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Duration, RelativeTime, TimeoutId } from '@datadog/browser-core'
import { addEventListener, Observable, setTimeout, clearTimeout, monitor } from '@datadog/browser-core'
import { addEventListener, noop, Observable, setTimeout, clearTimeout, monitor } from '@datadog/browser-core'
import type { RumConfiguration } from '../domain/configuration'
import { hasValidResourceEntryDuration, isAllowedRequestUrl } from '../domain/resource/resourceUtils'
import { retrieveFirstInputTiming } from './firstInputPolyfill'
Expand Down Expand Up @@ -266,7 +266,8 @@ export function createPerformanceObservable<T extends RumPerformanceEntryType>(
}
isObserverInitializing = false

manageResourceTimingBufferFull(configuration)
const stopManageResourceTimingBufferFull =
options.type === RumPerformanceEntryType.RESOURCE ? manageResourceTimingBufferFull(configuration) : noop

let stopFirstInputTiming: (() => void) | undefined
if (
Expand All @@ -280,6 +281,7 @@ export function createPerformanceObservable<T extends RumPerformanceEntryType>(

return () => {
observer.disconnect()
stopManageResourceTimingBufferFull()
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep shared resource buffer listener alive across observables

Calling stopManageResourceTimingBufferFull() in every createPerformanceObservable cleanup removes the single module-level resourcetimingbufferfull listener even when other performance observables are still subscribed. This is reachable with short-lived subscriptions like createPageActivityObservable (unsubscribed when waitPageActivityEnd completes), while long-lived resource collection remains active, so the resource timing buffer is no longer cleared and later resource entries can be dropped once full. The cleanup needs ref-counting (or equivalent) so the shared listener is only removed when the last dependent observable stops.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

@thomas-lebeau thomas-lebeau Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already handled via ref-counting in manageResourceTimingBufferFull: the listener is only torn down when the last subscriber unsubscribes (resourceTimingBufferFullListenerCount hits 0).

if (stopFirstInputTiming) {
stopFirstInputTiming()
}
Expand All @@ -288,16 +290,21 @@ export function createPerformanceObservable<T extends RumPerformanceEntryType>(
})
}

let resourceTimingBufferFullListener: { stop: () => void }
let resourceTimingBufferFullListener: { stop: () => void } | undefined
let resourceTimingBufferFullListenerCount = 0
function manageResourceTimingBufferFull(configuration: RumConfiguration) {
if (!resourceTimingBufferFullListener && supportPerformanceObject() && 'addEventListener' in performance) {
// https://bugzilla.mozilla.org/show_bug.cgi?id=1559377
resourceTimingBufferFullListener = addEventListener(configuration, performance, 'resourcetimingbufferfull', () => {
performance.clearResourceTimings()
})
}
resourceTimingBufferFullListenerCount++
return () => {
resourceTimingBufferFullListener?.stop()
if (--resourceTimingBufferFullListenerCount === 0) {
resourceTimingBufferFullListener?.stop()
resourceTimingBufferFullListener = undefined
}
}
}

Expand Down
18 changes: 13 additions & 5 deletions packages/rum-core/src/domain/action/trackClickActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,15 @@ export function trackClickActions(
const { stop: stopActionEventsListener } = listenActionEvents<{
clickActionBase: ClickActionBase
hadActivityOnPointerDown: () => boolean
stopHadActivityTracking: () => void
}>(configuration, {
onPointerDown: (pointerDownEvent) =>
processPointerDown(configuration, lifeCycle, domMutationObservable, pointerDownEvent, windowOpenObservable),
onPointerUp: ({ clickActionBase, hadActivityOnPointerDown }, startEvent, getUserActivity) => {
onPointerUp: (
{ clickActionBase, hadActivityOnPointerDown, stopHadActivityTracking },
startEvent,
getUserActivity
) => {
startClickAction(
configuration,
lifeCycle,
Expand All @@ -89,7 +94,8 @@ export function trackClickActions(
clickActionBase,
startEvent,
getUserActivity,
hadActivityOnPointerDown
hadActivityOnPointerDown,
stopHadActivityTracking
)
},
})
Expand Down Expand Up @@ -151,7 +157,7 @@ function processPointerDown(

let hadActivityOnPointerDown = false

waitPageActivityEnd(
const { stop: stopHadActivityTracking } = waitPageActivityEnd(
lifeCycle,
domMutationObservable,
windowOpenObservable,
Expand All @@ -164,7 +170,7 @@ function processPointerDown(
PAGE_ACTIVITY_VALIDATION_DELAY
)

return { clickActionBase, hadActivityOnPointerDown: () => hadActivityOnPointerDown }
return { clickActionBase, hadActivityOnPointerDown: () => hadActivityOnPointerDown, stopHadActivityTracking }
}

function startClickAction(
Expand All @@ -178,7 +184,8 @@ function startClickAction(
clickActionBase: ClickActionBase,
startEvent: MouseEventOnElement,
getUserActivity: () => UserActivity,
hadActivityOnPointerDown: () => boolean
hadActivityOnPointerDown: () => boolean,
stopHadActivityTracking: () => void
) {
const click = newClick(lifeCycle, actionTracker, getUserActivity, clickActionBase, startEvent)
appendClickToClickChain(click)
Expand Down Expand Up @@ -229,6 +236,7 @@ function startClickAction(
click.stopObservable.subscribe(() => {
pageMayExitSubscription.unsubscribe()
viewEndedSubscription.unsubscribe()
stopHadActivityTracking()
stopWaitPageActivityEnd()
stopSubscription.unsubscribe()
})
Expand Down
7 changes: 5 additions & 2 deletions packages/rum-core/src/domain/view/viewMetrics/trackCommonViewMetrics.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ describe('trackCommonViewMetrics', () => {

describe('manual loading time suppresses auto-detected loading time callback', () => {
it('should ignore auto-detected loading time when manual loading time was already set', () => {
const { setLoadEvent, setLoadingTime, getCommonViewMetrics, stop } = trackCommonViewMetrics(
const { setLoadEvent, setLoadingTime, getCommonViewMetrics, stop, stopINPTracking } = trackCommonViewMetrics(
lifeCycle,
domMutationObservable,
windowOpenObservable,
Expand All @@ -40,7 +40,10 @@ describe('trackCommonViewMetrics', () => {
clocksOrigin()
)

registerCleanupTask(stop)
registerCleanupTask(() => {
stop()
stopINPTracking()
})

// Step 1: Trigger page activity and let it end.
// This sets isWaitingForActivityLoadingTime = false with a candidate,
Expand Down
Loading