Uh oh!
There was an error while loading. Please reload this page.
fix(browser): Clean up pageload readystatechange listener - #21632
Merged
andreiborza merged 2 commits intoJun 18, 2026
Conversation
The auto-finish `readystatechange` listener registered for each pageload
span was never removed. Because it closes over the `idleSpan` (and the
rest of the route-span closure), every pageload leaked a listener and the
span it retained. The previous anonymous handler could not be removed, and
the suggested `{ once: true }` would not help in the common case where the
document is already loaded when the span starts, since the listener never
fires.
Pass `emitFinish` directly as the handler and remove it as soon as the
auto-finish signal is emitted, covering both the already-loaded and
load-later cases.
Fixes#21630
Co-Authored-By: Opus 4.8 <noreply@anthropic.com>andreiborza
requested review from
Lms24 and logaretm
and removed request for
a team, Lms24 and logaretmJune 18, 2026 11:55
andreiborza
marked this pull request as draft
June 18, 2026 12:01
Contributor
size-limit report 📦
|
| optionalWindowDocument.addEventListener('readystatechange', () => { | ||
| emitFinish(); | ||
| }); | ||
| optionalWindowDocument.addEventListener('readystatechange', emitFinish); |
Member
There was a problem hiding this comment.
Actually never mind, you want to remove it once it is called down there as well.
logaretm
approved these changes
Jun 18, 2026
Lms24
approved these changes
Jun 18, 2026
andreiborza
marked this pull request as ready for review
June 18, 2026 15:24
andreiborza
commented
Jun 18, 2026
MemberAuthor
For the record: I'm not actually sure this fixes an actual leak in a real-world scenario, because it seems strange to me that we'd have more than one pageload span. That being said, cleaning up here seems like good form. |
Uh oh!
There was an error while loading. Please reload this page.
andreiborza
deleted the
fix/browser-tracing-readystatechange-listener-leak
branch
June 18, 2026 15:58
48 tasks
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The auto-finish
readystatechangelistener registered for each pageload span inbrowserTracingIntegrationwas never removed. Since the handler closes over theidleSpan(and the rest of the route-span closure), every pageload leaked a listener and kept the span it retained alive — the growing set of Sentry functions reported in #21630.The previous handler was an anonymous arrow function, so it could not be removed via
removeEventListener. The{ once: true }option suggested in the issue does not fully address the leak either: in the common case where the document is already loaded when the span starts, the listener never fires and therefore is never auto-removed.This change passes
emitFinishdirectly as the handler and removes it as soon as the auto-finish signal is emitted. This covers both cases:readystatechange, emits, and removes itself.Since the framework integrations (Vue, Astro, Next.js, Remix, SvelteKit, Ember) all wrap the browser package's
browserTracingIntegration, this single fix covers them too.Root cause
optionalWindowDocument.addEventListener('readystatechange', () => { emitFinish(); })added a fresh, unremovable listener for every pageload span without ever detaching it.Fixes#21630