Skip to content

fix(browser-utils): prevent DOM handler leaks - #24723

Closed
DorianChn wants to merge 1 commit into
getsentry:developfrom
DorianChn:fix/browser-dom-listener-cleanup-24702
Closed

DorianChn wants to merge 1 commit into
getsentry:developfrom
DorianChn:fix/browser-dom-listener-cleanup-24702

Conversation

@DorianChn

Copy link
Copy Markdown

The DOM instrumentation wrapper currently decrements its refcount for every removeEventListener call, including no-op removals. When a capture-phase listener is the first registration, a later bubble-phase no-op removal can reach zero with the wrong capture mode and leave the internal handler attached.

Why: Track actual listener/capture registrations and normalize the internal handler to a stable capture boolean. This prevents unrelated removals from dropping the bookkeeping early and preserves correct cleanup when multiple listeners use different capture modes.

Fixes #24702

Validation: The targeted DOM regression test, browser-utils build, package lint, type-aware lint, targeted format check, and git diff checks pass. The package-wide test run reached 251/252; its only failure was the unrelated networkUtils Request/URLSearchParams mismatch in this local jsdom environment.

Co-Authored-By: OpenAI GPT-5 <codex@openai.com>
@DorianChn
DorianChn marked this pull request as ready for review September 25, 2026 03:05
@DorianChn
DorianChn requested a review from a team as a code owner September 25, 2026 03:05
@DorianChn
DorianChn requested review from logaretm and msonnb and removed request for a team September 25, 2026 03:05
@mstevens843

Copy link
Copy Markdown

Hi @DorianChn , I independently tested your dom.ts implementation at c39c75f against additional listener-lifecycle regressions.

All 33 jsdom cases and four Chromium tests pass. The browser tests cover the 20-cycle document-listener leak and missing ui.click breadcrumbs after no-op removals, with and without BrowserApiErrors. All four browser tests fail on the unpatched daacc2bd base.

I have two browser integration suites ready to contribute. Would you prefer to include them in this PR or have a tests-only follow-up after it merges?

Tested locally with Node 22.22.1 and Chromium

@Lms24

Lms24 commented Sep 25, 2026

Copy link
Copy Markdown
Member

Thanks for contributing! unfortunately this is too large and not a minimal fix (see #24727).

@Lms24 Lms24 closed this Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

instrumentDOM leaks a document click handler when the refcount hits zero on a removal with different capture options (e.g. Radix DismissableLayer)

3 participants