Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec by everettbu · Pull Request #716 · greptileai/react-mirror

everettbu · 2026-03-15T14:23:01Z

Mirror of facebook/react#36047
Original author: Dhakshin2007

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, 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:

// 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:

// 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 which enabled enableFragmentRefs + enableFragmentRefsInstanceHandles across all builds (merged 3 days ago).

Tests

Added two new regression tests to ReactDOMFragmentRefs-test.js:

removes a capture listener registered with boolean when removed with options object
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.

…ismatch between boolean and object capture optionsn ReactFiberConfigDOM

…lization in FragmentInstance addEventListener/removeEventListenereners in FragmentRefs

greptile-apps · 2026-03-15T14:25:33Z

Greptile Summary

Fixes a listener key mismatch in FragmentInstance where normalizeListenerOptions produced different key strings for boolean vs object capture options (e.g., "c=1" vs "c=1&o=0&p=0"), causing removeEventListener to silently fail when the option form differed from addEventListener. The fix appends &o=0&p=0 to the boolean branch so both forms produce identical keys, matching DOM spec behavior.

Bug fix: Appended &o=0&p=0 to the boolean branch in normalizeListenerOptions so that addEventListener(type, fn, true) and removeEventListener(type, fn, {capture: true}) (and vice versa) correctly cross-match
Tests: Added two regression tests covering both cross-form removal directions
Style: New tests have inconsistent indentation (8 spaces instead of 6) compared to sibling it() blocks

Confidence Score: 5/5

This PR is safe to merge — it's a minimal, correct one-line fix with proper test coverage.
The core fix is a single-line change that correctly normalizes boolean capture options to match the object format per the DOM spec. The logic is straightforward and the two new tests directly validate the fix. The only issue found is a cosmetic indentation inconsistency in the test file.
No files require special attention.

Important Files Changed

Filename	Overview
packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js	One-line fix normalizes the boolean branch of `normalizeListenerOptions` to produce the same key format as the object branch, correctly fixing the listener key mismatch.
packages/react-dom/src/tests/ReactDOMFragmentRefs-test.js	Two well-structured regression tests covering both cross-form removal directions; minor indentation inconsistency with sibling tests (8 spaces vs 6).

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
Line: 817-855

Comment:
**Inconsistent indentation with sibling tests**

Both new `it()` blocks are indented at 8 spaces, but every other `it()` inside this `describe('add/remove event listeners')` block uses 6 spaces (e.g., lines 788, 898). This is a minor style nit but could cause confusion when scanning the file.

```suggestion
      // @gate enableFragmentRefs
      it(
        'removes a capture listener registered with boolean when removed with options object',
        async () => {
          const fragmentRef = React.createRef(null);
          function Test() {
            return (
              <Fragment ref={fragmentRef}>
                <div id="child-a" />
              </Fragment>
            );
          }
          const root = ReactDOMClient.createRoot(container);
          await act(() => {
            root.render(<Test />);
          });

          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([]);
        },
      );
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js
Line: 857-895

Comment:
**Same indentation issue as above**

This second test block also has 8-space indentation where it should be 6 spaces to match the rest of the `describe('add/remove event listeners')` block.

```suggestion
      // @gate enableFragmentRefs
      it(
        'removes a capture listener registered with options object when removed with boolean',
        async () => {
          const fragmentRef = React.createRef(null);
          function Test() {
            return (
              <Fragment ref={fragmentRef}>
                <div id="child-b" />
              </Fragment>
            );
          }
          const root = ReactDOMClient.createRoot(container);
          await act(() => {
            root.render(<Test />);
          });

          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([]);
        },
      );
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: c380e05}

greptile-apps · 2026-03-15T14:25:36Z

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

+        // @gate enableFragmentRefs
+        it(
+          'removes a capture listener registered with boolean when removed with options object',
+          async () => {
+            const fragmentRef = React.createRef(null);
+            function Test() {
+              return (
+                <Fragment ref={fragmentRef}>
+                  <div id="child-a" />
+                </Fragment>
+              );
+            }
+            const root = ReactDOMClient.createRoot(container);
+            await act(() => {
+              root.render(<Test />);
+            });
+
+            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([]);
+          },
+        );


Inconsistent indentation with sibling tests

Both new it() blocks are indented at 8 spaces, but every other it() inside this describe('add/remove event listeners') block uses 6 spaces (e.g., lines 788, 898). This is a minor style nit but could cause confusion when scanning the file.

Suggested change

// @gate enableFragmentRefs

it(

'removes a capture listener registered with boolean when removed with options object',

async () => {

const fragmentRef = React.createRef(null);

function Test() {

return (

<Fragment ref={fragmentRef}>

<div id="child-a" />

</Fragment>

);

}

const root = ReactDOMClient.createRoot(container);

await act(() => {

root.render(<Test />);

});

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 boolean when removed with options object',

async () => {

const fragmentRef = React.createRef(null);

function Test() {

return (

<Fragment ref={fragmentRef}>

<div id="child-a" />

</Fragment>

);

}

const root = ReactDOMClient.createRoot(container);

await act(() => {

root.render(<Test />);

});

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([]);

},

);

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js Line: 817-855 Comment: **Inconsistent indentation with sibling tests** Both new `it()` blocks are indented at 8 spaces, but every other `it()` inside this `describe('add/remove event listeners')` block uses 6 spaces (e.g., lines 788, 898). This is a minor style nit but could cause confusion when scanning the file. ```suggestion // @gate enableFragmentRefs it( 'removes a capture listener registered with boolean when removed with options object', async () => { const fragmentRef = React.createRef(null); function Test() { return ( <Fragment ref={fragmentRef}> <div id="child-a" /> </Fragment> ); } const root = ReactDOMClient.createRoot(container); await act(() => { root.render(<Test />); }); 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([]); }, ); ``` How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

greptile-apps · 2026-03-15T14:25:37Z

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

+        // @gate enableFragmentRefs
+        it(
+          'removes a capture listener registered with options object when removed with boolean',
+          async () => {
+            const fragmentRef = React.createRef(null);
+            function Test() {
+              return (
+                <Fragment ref={fragmentRef}>
+                  <div id="child-b" />
+                </Fragment>
+              );
+            }
+            const root = ReactDOMClient.createRoot(container);
+            await act(() => {
+              root.render(<Test />);
+            });
+
+            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([]);
+          },
+        );


Same indentation issue as above

This second test block also has 8-space indentation where it should be 6 spaces to match the rest of the describe('add/remove event listeners') block.

Suggested change

// @gate enableFragmentRefs

it(

'removes a capture listener registered with options object when removed with boolean',

async () => {

const fragmentRef = React.createRef(null);

function Test() {

return (

<Fragment ref={fragmentRef}>

<div id="child-b" />

</Fragment>

);

}

const root = ReactDOMClient.createRoot(container);

await act(() => {

root.render(<Test />);

});

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(

'removes a capture listener registered with options object when removed with boolean',

async () => {

const fragmentRef = React.createRef(null);

function Test() {

return (

<Fragment ref={fragmentRef}>

<div id="child-b" />

</Fragment>

);

}

const root = ReactDOMClient.createRoot(container);

await act(() => {

root.render(<Test />);

});

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([]);

},

);

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js Line: 857-895 Comment: **Same indentation issue as above** This second test block also has 8-space indentation where it should be 6 spaces to match the rest of the `describe('add/remove event listeners')` block. ```suggestion // @gate enableFragmentRefs it( 'removes a capture listener registered with options object when removed with boolean', async () => { const fragmentRef = React.createRef(null); function Test() { return ( <Fragment ref={fragmentRef}> <div id="child-b" /> </Fragment> ); } const root = ReactDOMClient.createRoot(container); await act(() => { root.render(<Test />); }); 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([]); }, ); ``` How can I resolve this? If you propose a fix, please make it concise.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

…y per DOM spec Simplified return statement for boolean opts by removing unused parameters.Per eps1lon's review: the DOM Living Standard specifies that removeEventListener matches listeners using only (type, callback, capture). The `passive` and `once` options do NOT affect listener identity and must be ignored during removal. The previous fix added `&o=0&p=0` to the boolean branch to match the object branch, but this was wrong in both directions: - Adding passive/once to the key means removeEventListener({passive:true}) would fail to remove a listener added with ({passive:false}), violating the spec. Correct fix: key ONLY on capture in both branches: boolean: `c=${opts ? '1' : '0'}` object: `c=${opts.capture ? '1' : '0'}` Ref: https://dom.spec.whatwg.org/#dom-eventtarget-removeeventlistener Ref: MDN - "Only the capture setting matters to removeEventListener"

Dhakshin2007 added 2 commits March 15, 2026 19:06

Add parameters to options string iFix FragmentInstance listener key m…

ce7699a

…ismatch between boolean and object capture optionsn ReactFiberConfigDOM

Add tests for removing capture listAdd tests for capture option norma…

c380e05

…lization in FragmentInstance addEventListener/removeEventListenereners in FragmentRefs

greptile-apps bot reviewed Mar 15, 2026

View reviewed changes

everettbu added the CLA Signed label Mar 15, 2026

Dhakshin2007 added 2 commits March 17, 2026 23:34

test: add passive option mismatch test for FragmentRef addEventListener

47cd78d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec#716

Fix FragmentInstance listener leak: normalize boolean vs object capture options per DOM spec#716
everettbu wants to merge 4 commits intomainfrom
fix/fragment-ref-capture-option-normalization

everettbu commented Mar 15, 2026

Uh oh!

greptile-apps bot commented Mar 15, 2026

Uh oh!

greptile-apps bot Mar 15, 2026

Uh oh!

greptile-apps bot Mar 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

everettbu commented Mar 15, 2026

Summary

Root Cause

Fix

When Was This Introduced

Tests

How did you test this change?

Changelog

React DOM

Uh oh!

greptile-apps bot commented Mar 15, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

greptile-apps bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants