Skip to content

Commit 142cfde

Browse files
authored
Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec (#36047)
## 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](#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.
1 parent 94643c3 commit 142cfde

2 files changed

Lines changed: 165 additions & 1 deletion

File tree

packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3093,7 +3093,7 @@ function normalizeListenerOptions(
30933093
return `c=${opts ? '1' : '0'}`;
30943094
}
30953095

3096-
return `c=${opts.capture ? '1' : '0'}&o=${opts.once ? '1' : '0'}&p=${opts.passive ? '1' : '0'}`;
3096+
return `c=${opts.capture ? '1' : '0'}`;
30973097
}
30983098
function indexOfEventListener(
30993099
eventListeners: Array<StoredEventListener>,

packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -814,6 +814,86 @@ describe('FragmentRefs', () => {
814814
expect(logs).toEqual([]);
815815
});
816816

817+
// @gate enableFragmentRefs
818+
it(
819+
'removes a capture listener registered with boolean when removed with options object',
820+
async () => {
821+
const fragmentRef = React.createRef(null);
822+
function Test() {
823+
return (
824+
<Fragment ref={fragmentRef}>
825+
<div id="child-a" />
826+
</Fragment>
827+
);
828+
}
829+
const root = ReactDOMClient.createRoot(container);
830+
await act(() => {
831+
root.render(<Test />);
832+
});
833+
834+
const logs = [];
835+
function logCapture() {
836+
logs.push('capture');
837+
}
838+
839+
// Register with boolean `true` (capture phase)
840+
fragmentRef.current.addEventListener('click', logCapture, true);
841+
document.querySelector('#child-a').click();
842+
expect(logs).toEqual(['capture']);
843+
844+
logs.length = 0;
845+
846+
// Remove with equivalent options object {capture: true}
847+
// Per DOM spec, these are identical - the listener MUST be removed
848+
fragmentRef.current.removeEventListener('click', logCapture, {
849+
capture: true,
850+
});
851+
document.querySelector('#child-a').click();
852+
// Listener should have been removed - logs must remain empty
853+
expect(logs).toEqual([]);
854+
},
855+
);
856+
857+
// @gate enableFragmentRefs
858+
it(
859+
'removes a capture listener registered with options object when removed with boolean',
860+
async () => {
861+
const fragmentRef = React.createRef(null);
862+
function Test() {
863+
return (
864+
<Fragment ref={fragmentRef}>
865+
<div id="child-b" />
866+
</Fragment>
867+
);
868+
}
869+
const root = ReactDOMClient.createRoot(container);
870+
await act(() => {
871+
root.render(<Test />);
872+
});
873+
874+
const logs = [];
875+
function logCapture() {
876+
logs.push('capture');
877+
}
878+
879+
// Register with options object {capture: true}
880+
fragmentRef.current.addEventListener('click', logCapture, {
881+
capture: true,
882+
});
883+
document.querySelector('#child-b').click();
884+
expect(logs).toEqual(['capture']);
885+
886+
logs.length = 0;
887+
888+
// Remove with boolean `true`
889+
// Per DOM spec, these are identical - the listener MUST be removed
890+
fragmentRef.current.removeEventListener('click', logCapture, true);
891+
document.querySelector('#child-b').click();
892+
// Listener should have been removed - logs must remain empty
893+
expect(logs).toEqual([]);
894+
},
895+
);
896+
817897
// @gate enableFragmentRefs
818898
it('applies event listeners to portaled children', async () => {
819899
const fragmentRef = React.createRef();
@@ -2680,5 +2760,89 @@ describe('FragmentRefs', () => {
26802760
window.scrollTo = originalScrollTo;
26812761
restoreRange();
26822762
});
2763+
2764+
// @gate enableFragmentRefs
2765+
it(
2766+
'treats passive:true and passive:false as same listener per DOM spec',
2767+
async () => {
2768+
const fragmentRef = React.createRef();
2769+
const root = ReactDOMClient.createRoot(container);
2770+
2771+
await act(() => {
2772+
root.render(
2773+
<Fragment ref={fragmentRef}>
2774+
<div id="child" />
2775+
</Fragment>,
2776+
);
2777+
});
2778+
2779+
const logs = [];
2780+
const handler = () => logs.push('fired');
2781+
2782+
const child = document.querySelector('#child');
2783+
const spy = jest.spyOn(child, 'addEventListener');
2784+
// Per DOM spec, listener identity is (type, callback, capture).
2785+
// passive is NOT part of the key, so these are the SAME listener.
2786+
fragmentRef.current.addEventListener('click', handler, {passive: false});
2787+
// Second add is a no-op: same (type, callback, capture) identity.
2788+
fragmentRef.current.addEventListener('click', handler, {passive: true});
2789+
expect(spy).toHaveBeenCalledTimes(1);
2790+
expect(spy).toHaveBeenCalledWith('click', handler, {passive: false});
2791+
2792+
document.querySelector('#child').click();
2793+
// First handler fires once (second add was a no-op).
2794+
expect(logs).toEqual(['fired']);
2795+
2796+
// removeEventListener also ignores passive when matching
2797+
fragmentRef.current.removeEventListener('click', handler, {
2798+
passive: true,
2799+
});
2800+
2801+
logs.length = 0;
2802+
document.querySelector('#child').click();
2803+
expect(logs).toEqual([]);
2804+
},
2805+
);
2806+
// @gate enableFragmentRefs
2807+
it(
2808+
'removes a listener registered with passive:false when removed with passive:true',
2809+
async () => {
2810+
const fragmentRef = React.createRef(null);
2811+
function Test() {
2812+
return (
2813+
<>
2814+
<div id="child-x" />
2815+
</>
2816+
);
2817+
}
2818+
const root = ReactDOMClient.createRoot(container);
2819+
await act(() => {
2820+
root.render(
2821+
<Fragment ref={fragmentRef}>
2822+
<Test />
2823+
</Fragment>,
2824+
);
2825+
});
2826+
const logs = [];
2827+
function handler() {
2828+
logs.push('fired');
2829+
}
2830+
// Register with passive: false
2831+
fragmentRef.current.addEventListener('click', handler, {
2832+
passive: false,
2833+
});
2834+
document.querySelector('#child-x').click();
2835+
expect(logs).toEqual(['fired']);
2836+
logs.length = 0;
2837+
// Remove with passive: true - per DOM spec, passive is NOT part of identity
2838+
// so this MUST remove the listener regardless of passive mismatch.
2839+
fragmentRef.current.removeEventListener('click', handler, {
2840+
passive: true,
2841+
});
2842+
document.querySelector('#child-x').click();
2843+
// Listener removed - no more invocations
2844+
expect(logs).toEqual([]);
2845+
},
2846+
);
26832847
});
26842848
});

0 commit comments

Comments
 (0)