Skip to content

fix(browser): Check for clock drift on page lifecycle events #25091

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

Open
Lms24 wants to merge 1 commit into from
Open

fix(browser): Check for clock drift on page lifecycle events #25091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions packages/browser/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,17 @@ export class BrowserClient extends Client<BrowserClientOptions> {
});
}
});

// `freeze` and `resume` are only fired by Chromium browsers. Other browsers never fire them, so these listeners
// are no-ops there.
WINDOW.document.addEventListener('freeze', () => timestampInSeconds());
WINDOW.document.addEventListener('resume', () => timestampInSeconds());
}

// Pages restored from the back/forward cache were paused while they were cached.
WINDOW.addEventListener?.('pagehide', () => timestampInSeconds());
WINDOW.addEventListener?.('pageshow', () => timestampInSeconds());
Comment thread
cursor[bot] marked this conversation as resolved.

if (userInfo) {
this.on('beforeSendSession', addAutoIpAddressToSession);
}
Comment on lines 170 to 178

@sentry sentry Bot Oct 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bug: New event listeners for freeze, resume, pagehide, and pageshow are not cleaned up when a BrowserClient is destroyed, causing a memory leak.
Severity: MEDIUM

Suggested Fix

Implement a cleanup mechanism in the BrowserClient. The client should track the listeners it adds. A close() or dispose() method should be implemented or overridden to remove these event listeners from the global objects when the client is destroyed, preventing old client instances from being retained in memory.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: packages/browser/src/client.ts#L170-L178

Potential issue: The `BrowserClient` adds global event listeners for `freeze`, `resume`,
`pagehide`, and `pageshow`. However, there is no corresponding cleanup logic to remove
these listeners when a client instance is no longer needed. If multiple `BrowserClient`
instances are created over an application's lifecycle, such as during hot-reloading, in
test suites, or by calling `Sentry.init()` multiple times, old client instances and
their closures will be retained in memory. This leads to a memory leak, as the new
listeners introduced in this change are not removed upon client destruction.

Did we get this right? 👍 / 👎 to inform future reviews.

@logaretm logaretm Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

l: Should be easy to cleanup, no?

Expand Down
14 changes: 14 additions & 0 deletions packages/browser/test/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ describe('BrowserClient', () => {
expect(SentryCore.timestampInSeconds).toHaveBeenCalled();
});

it.each([
['freeze', () => WINDOW.document],
['resume', () => WINDOW.document],
['pagehide', () => WINDOW],
['pageshow', () => WINDOW],
])('checks the clocks for drift on %s', (eventName, getTarget) => {
client = new BrowserClient(getDefaultBrowserClientOptions());
vi.mocked(SentryCore.timestampInSeconds).mockClear();

getTarget().dispatchEvent(new Event(eventName));

expect(SentryCore.timestampInSeconds).toHaveBeenCalled();
});

it('does not flush outcomes when sendClientReports is disabled but still flushes the client', async () => {
client = new BrowserClient(getDefaultBrowserClientOptions({ sendClientReports: false }));
const flushSpy = vi.spyOn(client, 'flush').mockReturnValue(Promise.resolve(true) as any);
Expand Down
Loading