instrumentDOM leaks a document click handler when the refcount hits zero on a removal with different capture options (e.g. Radix DismissableLayer)
メンテナーはふだん 1 日以内に返信
まだ誰も着手していません。
評価
- 難易度
- 4/5
- 見積もり時間
- 3〜5日
- 初心者へのやさしさ
- 74/100
- issue の種類
- バグ
- 明瞭さ
- 明確に書かれている
- 活発さ
- 活発
- 技術スタック
- javascript, react, typescript
- 領域
- frontend
調査の方向性
Start in packages/browser-utils/src/instrumentation/dom.ts and run the supplied jsdom reproduction, focusing on the refcount and capture options used by the per-target handler. Done means repeated Radix-style add/remove cycles leave no extra document click handlers while preserving instrumentation coverage; add or update focused tests if the existing suite provides an appropriate entry point.
索引モデルが issue の本文から書いたものです。
説明
Is there an existing issue for this?
- I have checked for existing issues
- I have reviewed the documentation
- I am using the latest SDK release: observed on 10.72.0;
instrumentDOMis byte-identical in 10.75.3 and 11.0.0 (build/esm/instrumentation/dom.js)
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/react (the code is in @sentry/browser-utils)
SDK Version
10.72.0 (also checked 10.75.3 and 11.0.0)
Framework Version
React 19.2.8, @radix-ui/react-dismissable-layer 1.1.19 (via Radix Dialog)
Link to Sentry event
N/A. This is a listener leak, not an error event.
Reproduction Example/SDK Setup
// @vitest-environment jsdom (Chrome behaves the same way)
import * as Sentry from "@sentry/react";
// Count what really attaches to document, underneath the SDK's wrapper.
const live = { true: new Set(), false: new Set() };
const P = EventTarget.prototype, add = P.addEventListener, rm = P.removeEventListener;
const cap = (o) => (typeof o === "boolean" ? o : !!o?.capture);
P.addEventListener = function (t, fn, o) { if (this === document && t === "click") live[cap(o)].add(fn); return add.call(this, t, fn, o); };
P.removeEventListener = function (t, fn, o) { if (this === document && t === "click") live[cap(o)].delete(fn); return rm.call(this, t, fn, o); };
const count = () => live.true.size + live.false.size;
Sentry.init({ dsn: "https://[email protected]/0", defaultIntegrations: false, integrations: [Sentry.breadcrumbsIntegration()] });
const baseline = count(); // 1: the global handler
// Radix DismissableLayer's document click listeners: mount, a pointerdown
// inside the layer, then unmount.
const never = () => {}, onCapture = () => {}, onBubble = () => {};
for (let i = 0; i < 20; i++) {
document.addEventListener("click", onCapture, true);
document.addEventListener("click", onBubble);
document.removeEventListener("click", never); // on pointerdown: removes a listener it never added
document.removeEventListener("click", never); // on unmount: same no-op removal
document.removeEventListener("click", onCapture, true);
document.removeEventListener("click", onBubble);
}
console.log(count() - baseline); // 20, expected 0
Steps to Reproduce
- Run the snippet above. Or, in a real app: open and close any Radix
Dialog/Popover/DropdownMenuby clicking inside it (for example a Cancel button), 20 times. - In Chrome, run
getEventListeners(document).click, or use CDPDOMDebugger.getEventListeners.
Expected Result
The SDK's per-target handler is removed when the last click listener on document goes away, so the count stays at the baseline.
Actual Result
Each cycle leaves one more capture-phase click listener on document, and all of them come from the SDK. In our app that was +20 after 20 dialog cycles closed by a click, and 0 when closed with Escape (no pointerdown).
Additional Context
The cause is in instrumentDOM (packages/browser-utils/src/instrumentation/dom.ts):
- The SDK keeps one handler and one
refCountper event type on each target. The handler is registered with theoptionsof whicheveraddEventListenercall first found the count at 0. - It is removed with the
optionsof whicheverremoveEventListenercall brings the count to<= 0. - The count is decremented for every
removeEventListener, even one for a listener that was never added. The browser treats that call as a no-op, but the SDK still counts it.
So when the first add used capture: true and the call that reaches zero is a bubble-phase no-op removal, originalRemoveEventListener(type, handler, undefined) misses the capture-phase handler. The bookkeeping is then deleted anyway, and the next add registers a new handler. DismissableLayer triggers this every time: it calls ownerDocument.removeEventListener("click", handleClickRef.current) on pointerdown and on unmount, even though handleClickRef.current is its initial no-op that was never added.
Because the handler is registered with the caller's full options object, a first caller's { once: true } or signal also applies to the SDK's handler. That can remove the handler while the bookkeeping still says it is attached.
We patched it locally by registering and removing with one normalized capture flag, which brought the growth to 0:
handlerForType.handler = handler;
- originalAddEventListener.call(this, type, handler, options);
+ handlerForType.capture = typeof options === "boolean" ? options : !!options?.capture;
+ originalAddEventListener.call(this, type, handler, handlerForType.capture);
...
- originalRemoveEventListener.call(this, type, handlerForType.handler, options);
+ originalRemoveEventListener.call(this, type, handlerForType.handler, handlerForType.capture);
The refcount can still reach zero early because of the no-op removals. When that happens, the per-target handler is dropped while real listeners remain, so there is a small coverage gap but nothing accumulates. A complete fix would track which (listener, capture) pairs are actually registered. breadcrumbsIntegration({ dom: false }) is not a workaround, because Replay calls addClickKeypressInstrumentationHandler too.
Priority
React with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding +1 or me too, to help us triage it.
- 主要言語
- TypeScript
- スター
- 8.7k
- フォーク
- 1.9k
- 平均マージ
- 1日 17時間
- マージ済み PR(30日)
- 543
環境構築
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
getsentry/sentry-javascript のほかの issue
-
Browser Bug Next.js Traces
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100
getsentry/sentry-javascript#24672 · コメント 2 件 · 担当者 1 名 ·
メンテナーはふだん 1 日以内に返信
-
javascript
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100
getsentry/sentry-javascript#24200 · コメント 2 件 ·
メンテナーはふだん 1 日以内に返信
-
javascript Task
難易度 2/5 1〜3時間 初心者へのやさしさ 82/100
getsentry/sentry-javascript#24134 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
-
Cloudflare Workers javascript Tests
難易度 2/5 1〜3時間 初心者へのやさしさ 78/100
getsentry/sentry-javascript#24051 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
-
Bug Bun javascript
難易度 2/5 1〜3時間 初心者へのやさしさ 92/100
getsentry/sentry-javascript#24045 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
getsentry/sentry-javascript の issue をすべて見る
似ている issue
-
module-request
難易度 2/5 1〜3時間 初心者へのやさしさ 68/100
メンテナーはふだん 1 日以内に返信
-
難易度 1/5 1時間未満 初心者へのやさしさ 90/100
appandflow/stim#1604 · コメント 1 件 ·
メンテナーはふだん 1 日以内に返信
-
community first-timers-only good first issue hacktoberfest help wanted low hanging fruit up-for-grabs
難易度 1/5 1時間未満 初心者へのやさしさ 92/100
lingdojo/kana-dojo#31060 · コメント 1 件 · リアクション 5 件 ·
メンテナーはふだん 1 日以内に返信
-
難易度 1/5 1時間未満 初心者へのやさしさ 90/100
paperclipai/paperclip#14173 ·
メンテナーはふだん 1 日以内に返信
-
needs-triage
難易度 2/5 1〜3時間 初心者へのやさしさ 85/100
メンテナーはふだん 1 日以内に返信