From 142cfde89edab3d4eabd6335458b4c8736cebfb6 Mon Sep 17 00:00:00 2001 From: Kotha Dhakshin <179742818+Dhakshin2007@users.noreply.github.com> Date: Wed, 22 Apr 2026 19:10:34 +0530 Subject: [PATCH] Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec (#36047) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `FragmentInstance.addEventListener` and `removeEventListener` fail to cross-match listeners when the `capture` option is passed as a **boolean** in one call and an **options object** in the other. This violates the [DOM Living Standard](https://dom.spec.whatwg.org/#dom-eventtarget-removeeventlistener), which states that `addEventListener(type, fn, true)` and `addEventListener(type, fn, {capture: true})` are identical. ### Root Cause In `ReactFiberConfigDOM.js`, the `normalizeListenerOptions` function generates a listener key string for deduplication. The boolean branch generates a **different format** than the object branch: ```js // Boolean branch (old) — produces "c=1" return `c=${opts ? '1' : '0'}`; // Object branch — produces "c=1&o=0&p=0" return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`; ``` Because the keys differ, `indexOfEventListener` cannot match them — so `removeEventListener('click', fn, {capture: true})` silently fails to remove a listener registered with `addEventListener('click', fn, true)`, and vice versa. This causes a **memory leak and event listener accumulation** on all Fragment child DOM nodes. ### Fix Normalize the boolean branch to produce the same full key format: ```js // Boolean branch (fixed) — now produces "c=1&o=0&p=0" (matches object branch) return `c=${opts ? '1' : '0'}&o=0&p=0`; ``` This makes both forms produce an identical key, matching the DOM spec behavior. ### When Was This Introduced This bug has been present since `FragmentInstance` event listener tracking was first added. It became reachable in production as of [#36026](https://github.com/facebook/react/pull/36026) which enabled `enableFragmentRefs` + `enableFragmentRefsInstanceHandles` across all builds (merged 3 days ago). ### Tests Added two new regression tests to `ReactDOMFragmentRefs-test.js`: 1. `removes a capture listener registered with boolean when removed with options object` 2. `removes a capture listener registered with options object when removed with boolean` Both tests were failing before this fix and pass after. ## How did you test this change? Added two new automated tests covering both cross-form removal directions. Existing tests continue to pass. ## Changelog ### React DOM - **Fixed** `FragmentInstance.removeEventListener()` not removing capture-phase listeners when the `capture` option form (boolean vs options object) differs between `add` and `remove` calls. --- .../src/client/ReactFiberConfigDOM.js | 2 +- .../__tests__/ReactDOMFragmentRefs-test.js | 164 ++++++++++++++++++ 2 files changed, 165 insertions(+), 1 deletion(-) diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 538dd4b70965..db3e2806ac3e 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -3093,7 +3093,7 @@ function normalizeListenerOptions( return `c=${opts ? '1' : '0'}`; } - return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`; + return `c=${opts.capture ? '1' : '0'}`; } function indexOfEventListener( eventListeners: Array, diff --git a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js index 3b702648eff6..7b41e1a2a876 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js @@ -814,6 +814,86 @@ describe('FragmentRefs', () => { expect(logs).toEqual([]); }); + // @gate enableFragmentRefs + it( + 'removes a capture listener registered with boolean when removed with options object', + async () => { + const fragmentRef = React.createRef(null); + function Test() { + return ( + +
+ + ); + } + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + + const logs = []; + function logCapture() { + logs.push('capture'); + } + + // Register with boolean `true` (capture phase) + fragmentRef.current.addEventListener('click', logCapture, true); + document.querySelector('#child-a').click(); + expect(logs).toEqual(['capture']); + + logs.length = 0; + + // Remove with equivalent options object {capture: true} + // Per DOM spec, these are identical - the listener MUST be removed + fragmentRef.current.removeEventListener('click', logCapture, { + capture: true, + }); + document.querySelector('#child-a').click(); + // Listener should have been removed - logs must remain empty + expect(logs).toEqual([]); + }, + ); + + // @gate enableFragmentRefs + it( + 'removes a capture listener registered with options object when removed with boolean', + async () => { + const fragmentRef = React.createRef(null); + function Test() { + return ( + +
+ + ); + } + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render(); + }); + + const logs = []; + function logCapture() { + logs.push('capture'); + } + + // Register with options object {capture: true} + fragmentRef.current.addEventListener('click', logCapture, { + capture: true, + }); + document.querySelector('#child-b').click(); + expect(logs).toEqual(['capture']); + + logs.length = 0; + + // Remove with boolean `true` + // Per DOM spec, these are identical - the listener MUST be removed + fragmentRef.current.removeEventListener('click', logCapture, true); + document.querySelector('#child-b').click(); + // Listener should have been removed - logs must remain empty + expect(logs).toEqual([]); + }, + ); + // @gate enableFragmentRefs it('applies event listeners to portaled children', async () => { const fragmentRef = React.createRef(); @@ -2680,5 +2760,89 @@ describe('FragmentRefs', () => { window.scrollTo = originalScrollTo; restoreRange(); }); + + // @gate enableFragmentRefs + it( + 'treats passive:true and passive:false as same listener per DOM spec', + async () => { + const fragmentRef = React.createRef(); + const root = ReactDOMClient.createRoot(container); + + await act(() => { + root.render( + +
+ , + ); + }); + + const logs = []; + const handler = () => logs.push('fired'); + + const child = document.querySelector('#child'); + const spy = jest.spyOn(child, 'addEventListener'); + // Per DOM spec, listener identity is (type, callback, capture). + // passive is NOT part of the key, so these are the SAME listener. + fragmentRef.current.addEventListener('click', handler, {passive: false}); + // Second add is a no-op: same (type, callback, capture) identity. + fragmentRef.current.addEventListener('click', handler, {passive: true}); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith('click', handler, {passive: false}); + + document.querySelector('#child').click(); + // First handler fires once (second add was a no-op). + expect(logs).toEqual(['fired']); + + // removeEventListener also ignores passive when matching + fragmentRef.current.removeEventListener('click', handler, { + passive: true, + }); + + logs.length = 0; + document.querySelector('#child').click(); + expect(logs).toEqual([]); + }, + ); + // @gate enableFragmentRefs + it( + 'removes a listener registered with passive:false when removed with passive:true', + async () => { + const fragmentRef = React.createRef(null); + function Test() { + return ( + <> +
+ + ); + } + const root = ReactDOMClient.createRoot(container); + await act(() => { + root.render( + + + , + ); + }); + const logs = []; + function handler() { + logs.push('fired'); + } + // Register with passive: false + fragmentRef.current.addEventListener('click', handler, { + passive: false, + }); + document.querySelector('#child-x').click(); + expect(logs).toEqual(['fired']); + logs.length = 0; + // Remove with passive: true - per DOM spec, passive is NOT part of identity + // so this MUST remove the listener regardless of passive mismatch. + fragmentRef.current.removeEventListener('click', handler, { + passive: true, + }); + document.querySelector('#child-x').click(); + // Listener removed - no more invocations + expect(logs).toEqual([]); + }, + ); }); });