[Fiber] only remove properties from singletons on release (#37112)

Removing attributes does not actually reset all property state on the
singleton instance. It also has the side effect of wiping any 3rd party
set attributes and properties that 3rd party scripts and extensions.

Reimplements the singleton release to clear properties on the instance
based on the last committed props for the singleton fiber.

Notably there is an unfixed path with preamble contribution markers that
still just clears based on attributes which covers SSR'd attributes that
need to be wiped before client recovery on hydration can continue. There
are gated failing tests for this as a TODO.

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

@@ -379,12 +379,18 @@ export function trapClickOnNonInteractiveElement(node: HTMLElement) {
   // listener on the target node.
   // https://www.quirksmode.org/blog/archives/2010/09/click_event_del.html
   // Just set it using the onclick property so that we don't have to manage any
-  // bookkeeping for it. Not sure if we need to clear it when the listener is
-  // removed.
+  // bookkeeping for it. HostSingleton release clears the property only if it
+  // still points to this noop.
   // TODO: Only do this for the relevant Safaris maybe?
   node.onclick = noop;
 }
 
+export function clearClickListener(node: HTMLElement) {
+  if (node.onclick === noop) {
+    node.onclick = null;
+  }
+}
+
 const xlinkNamespace = 'http://www.w3.org/1999/xlink';
 const xmlNamespace = 'http://www.w3.org/XML/1998/namespace';
 
@@ -1489,6 +1495,26 @@ export function setInitialProperties(
   }
 }
 
+export type SingletonType = 'html' | 'head' | 'body';
+
+const emptyProps = {};
+
+export function clearSingletonProperties(
+  domElement: Element,
+  tag: SingletonType,
+  props: Object,
+): void {
+  // This is equivalent to updating to empty props for tags without
+  // tag-specific update logic. Host singletons are limited to html, head, and
+  // body, so they always use this generic path.
+  for (const propKey in props) {
+    const propValue = props[propKey];
+    if (props.hasOwnProperty(propKey) && propValue != null) {
+      setProp(domElement, tag, propKey, null, emptyProps, propValue);
+    }
+  }
+}
+
 export function updateProperties(
   domElement: Element,
   tag: string,

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

@@ -69,10 +69,12 @@ const internalPropsMap:
   | Map<InstanceUnion, Props> = new PossiblyWeakMap();
 
 export function detachDeletedInstance(node: Instance): void {
+  // Don't delete the event listener set. The native event listeners it tracks
+  // remain attached to the node, so this bookkeeping needs to last for the
+  // lifetime of the node to prevent duplicate listeners if it is reused.
   if (enableInternalInstanceMap) {
     internalInstanceMap.delete(node);
     internalPropsMap.delete(node);
-    delete (node as any)[internalEventHandlersKey];
     delete (node as any)[internalEventHandlerListenersKey];
     delete (node as any)[internalEventHandlesSetKey];
     delete (node as any)[internalRootNodeResourcesKey];
@@ -85,7 +87,6 @@ export function detachDeletedInstance(node: Instance): void {
   // these fields are relevant.
   delete (node as any)[internalInstanceKey];
   delete (node as any)[internalPropsKey];
-  delete (node as any)[internalEventHandlersKey];
   delete (node as any)[internalEventHandlerListenersKey];
   delete (node as any)[internalEventHandlesSetKey];
 }

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

@@ -77,15 +77,18 @@ import {compareDocumentPositionForEmptyFragment} from 'shared/ReactDOMFragmentRe
 
 export {detachDeletedInstance};
 import {hasRole} from './DOMAccessibilityRoles';
+import type {SingletonType} from './ReactDOMComponent';
 import {
   setInitialProperties,
   updateProperties,
+  clearSingletonProperties,
   hydrateProperties,
   hydrateText,
   diffHydratedProperties,
   getPropsFromElement,
   diffHydratedText,
   trapClickOnNonInteractiveElement,
+  clearClickListener,
 } from './ReactDOMComponent';
 import {hydrateInput} from './ReactDOMInput';
 import {hydrateTextarea} from './ReactDOMTextarea';
@@ -1269,18 +1272,18 @@ function clearHydrationBoundary(
         // then it contributed to the html tag and we need to reset it.
         const ownerDocument = parentInstance.ownerDocument;
         const documentElement: Element = ownerDocument.documentElement as any;
-        releaseSingletonInstance(documentElement);
+        clearSingletonPreambleContribution(documentElement);
       } else if (data === PREAMBLE_CONTRIBUTION_HEAD) {
         const ownerDocument = parentInstance.ownerDocument;
         const head: Element = ownerDocument.head as any;
-        releaseSingletonInstance(head);
+        clearSingletonPreambleContribution(head);
         // We need to clear the head because this is the only singleton that can have children that
         // were part of this boundary but are not inside this boundary.
         clearHead(head);
       } else if (data === PREAMBLE_CONTRIBUTION_BODY) {
         const ownerDocument = parentInstance.ownerDocument;
         const body: Element = ownerDocument.body as any;
-        releaseSingletonInstance(body);
+        clearSingletonPreambleContribution(body);
       }
     }
     // $FlowFixMe[incompatible-type] we bail out when we get a null
@@ -4844,7 +4847,40 @@ export function acquireSingletonInstance(
   updateFiberProps(instance, props);
 }
 
-export function releaseSingletonInstance(instance: Instance): void {
+export function releaseSingletonInstance(
+  instance: Instance,
+  type: SingletonType,
+  props: Props,
+): void {
+  // Remove the attributes and property-backed state owned by this Fiber.
+  clearSingletonProperties(instance, type, props);
+
+  // These properties aren't cleared by updateProperties when their next
+  // value is null. Normally that is handled by replacing/removing the host
+  // instance, but a singleton cannot be removed.
+  // TODO: HostSingleton updates do not currently schedule ContentReset when
+  // dangerouslySetInnerHTML becomes undefined, so an ordinary update can leave
+  // the previous HTML in place. This only handles the release path.
+  if (props.dangerouslySetInnerHTML != null) {
+    instance.textContent = '';
+  }
+  clearClickListener(instance as any as HTMLElement);
+
+  // Only remove state that was represented by this Fiber's props. Attributes
+  // added imperatively while React owned the singleton must be preserved.
+  detachDeletedInstance(instance);
+}
+
+function clearSingletonPreambleContribution(instance: Instance): void {
+  // This path is only used when clearing a dehydrated boundary that contains a
+  // Fizz preamble contribution marker. The marker tells us which singleton the
+  // boundary contributed to, but it does not include the contributed props and
+  // there is no HostSingleton Fiber to provide them. We therefore cannot tell
+  // which attributes came from React and which were added imperatively by a
+  // script or third party. For now, clearing every attribute is an accepted
+  // edge case.
+  // TODO: Include the contributed properties in the marker so this cleanup can
+  // remove only the attributes owned by the boundary.
   const attributes = instance.attributes;
   while (attributes.length) {
     instance.removeAttributeNode(attributes[0]);

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

@@ -225,6 +225,272 @@ describe('ReactDOM HostSingleton', () => {
     );
   });
 
+  it('resets property-backed state when a singleton is released', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const head = document.head;
+    const body = document.body;
+
+    root.render(
+      <html>
+        <head onClick={() => {}} />
+        <body
+          data-react-owned="true"
+          onClick={() => {}}
+          style={{color: 'red'}}
+          dangerouslySetInnerHTML={{__html: '<div>managed content</div>'}}
+        />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(document.body).toBe(body);
+    expect(head.onclick).not.toBe(null);
+    expect(body.onclick).not.toBe(null);
+    expect(body.textContent).toBe('managed content');
+    expect(body.getAttribute('data-react-owned')).toBe('true');
+    expect(body.style.color).toBe('red');
+
+    // Simulate an inline script or third-party code adding its own attribute,
+    // style, and click listener while React owns the singleton.
+    const externalClickHandler = jest.fn();
+    body.setAttribute('data-external', 'true');
+    body.style.backgroundColor = 'blue';
+    body.onclick = externalClickHandler;
+
+    root.render(<html />);
+    await waitForAll([]);
+
+    expect(document.head).toBe(head);
+    expect(document.body).toBe(body);
+    expect(head.onclick).toBe(null);
+    expect(body.onclick).toBe(externalClickHandler);
+    expect(body.textContent).toBe('');
+    expect(body.hasAttribute('data-react-owned')).toBe(false);
+    expect(body.getAttribute('data-external')).toBe('true');
+    expect(body.style.color).toBe('');
+    expect(body.style.backgroundColor).toBe('blue');
+  });
+
+  // @gate TODO
+  it('clears dangerouslySetInnerHTML when it becomes undefined', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const body = document.body;
+    const undefinedHTML = undefined;
+
+    root.render(
+      <html>
+        <head />
+        <body
+          dangerouslySetInnerHTML={{__html: '<div>managed content</div>'}}
+        />
+      </html>,
+    );
+    await waitForAll([]);
+    expect(body.textContent).toBe('managed content');
+
+    root.render(
+      <html>
+        <head />
+        <body dangerouslySetInnerHTML={undefinedHTML} />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(body.textContent).toBe('');
+  });
+
+  // @gate TODO
+  it('clears dangerouslySetInnerHTML when __html becomes undefined', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const body = document.body;
+
+    root.render(
+      <html>
+        <head />
+        <body
+          dangerouslySetInnerHTML={{__html: '<div>managed content</div>'}}
+        />
+      </html>,
+    );
+    await waitForAll([]);
+    expect(body.textContent).toBe('managed content');
+
+    root.render(
+      <html>
+        <head />
+        <body dangerouslySetInnerHTML={{__html: undefined}} />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(body.textContent).toBe('');
+  });
+
+  it('updates dangerouslySetInnerHTML on a singleton', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const body = document.body;
+
+    root.render(
+      <html>
+        <head />
+        <body dangerouslySetInnerHTML={{__html: '<div>first</div>'}} />
+      </html>,
+    );
+    await waitForAll([]);
+    expect(body.innerHTML).toBe('<div>first</div>');
+
+    root.render(
+      <html>
+        <head />
+        <body dangerouslySetInnerHTML={{__html: '<span>second</span>'}} />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(body.innerHTML).toBe('<span>second</span>');
+  });
+
+  it('replaces singleton children with dangerouslySetInnerHTML', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const body = document.body;
+
+    root.render(
+      <html>
+        <head />
+        <body>
+          <div>managed child</div>
+        </body>
+      </html>,
+    );
+    await waitForAll([]);
+    expect(body.innerHTML).toBe('<div>managed child</div>');
+
+    root.render(
+      <html>
+        <head />
+        <body dangerouslySetInnerHTML={{__html: '<span>managed HTML</span>'}} />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(body.innerHTML).toBe('<span>managed HTML</span>');
+  });
+
+  // @gate TODO
+  it('replaces dangerouslySetInnerHTML with singleton children', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const body = document.body;
+
+    root.render(
+      <html>
+        <head />
+        <body
+          dangerouslySetInnerHTML={{__html: '<div>managed content</div>'}}
+        />
+      </html>,
+    );
+    await waitForAll([]);
+    expect(body.innerHTML).toBe('<div>managed content</div>');
+
+    root.render(
+      <html>
+        <head />
+        <body>
+          <span>managed child</span>
+        </body>
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(body.innerHTML).toBe('<span>managed child</span>');
+  });
+
+  // @gate TODO
+  it('preserves imperative attributes when acquiring a singleton', async () => {
+    const body = document.body;
+    body.setAttribute('data-external', 'true');
+
+    const root = ReactDOMClient.createRoot(document);
+    root.render(
+      <html>
+        <head />
+        <body />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(document.body).toBe(body);
+    expect(body.getAttribute('data-external')).toBe('true');
+  });
+
+  // @gate TODO
+  it('preserves imperative attributes when clearing a preamble contribution', async () => {
+    const body = document.body;
+    body.setAttribute('data-react-owned', 'true');
+    body.setAttribute('data-external', 'true');
+    // This is the shape Fizz emits when a completed Suspense boundary
+    // contributes props to the body singleton.
+    body.innerHTML = '<!--$--><!--body--><div>server</div><!--/$-->';
+
+    ReactDOMClient.hydrateRoot(
+      document,
+      <html>
+        <head />
+        <body data-react-owned="true" suppressHydrationWarning={true}>
+          <React.Suspense fallback={null}>
+            <span>client</span>
+          </React.Suspense>
+        </body>
+      </html>,
+      {
+        onRecoverableError() {},
+      },
+    );
+    await waitForAll([]);
+
+    expect(body.textContent).toBe('client');
+    expect(body.getAttribute('data-external')).toBe('true');
+  });
+
+  it('does not duplicate native listeners when a singleton is reacquired', async () => {
+    const root = ReactDOMClient.createRoot(document);
+    const body = document.body;
+    const onScroll = jest.fn();
+
+    root.render(
+      <html>
+        <head />
+        <body onScroll={onScroll} />
+      </html>,
+    );
+    await waitForAll([]);
+
+    body.dispatchEvent(new document.defaultView.Event('scroll'));
+    expect(onScroll).toHaveBeenCalledTimes(1);
+
+    root.render(
+      <html>
+        <head />
+      </html>,
+    );
+    await waitForAll([]);
+
+    body.dispatchEvent(new document.defaultView.Event('scroll'));
+    expect(onScroll).toHaveBeenCalledTimes(1);
+
+    root.render(
+      <html>
+        <head />
+        <body onScroll={onScroll} />
+      </html>,
+    );
+    await waitForAll([]);
+
+    expect(document.body).toBe(body);
+    body.dispatchEvent(new document.defaultView.Event('scroll'));
+    expect(onScroll).toHaveBeenCalledTimes(2);
+  });
+
   it('renders into html, head, and body persistently so the node identities never change and extraneous styles are retained', async () => {
     // Server render some html that will get replaced with a client render
     await actIntoEmptyDocument(() => {

packages/react-reconciler/src/ReactFiberCommitHostEffects.js

@@ -826,8 +826,14 @@ export function commitHostSingletonRelease(releasingWork: Fiber) {
       releasingWork,
       releaseSingletonInstance,
       releasingWork.stateNode,
+      releasingWork.type,
+      releasingWork.memoizedProps,
     );
   } else {
-    releaseSingletonInstance(releasingWork.stateNode);
+    releaseSingletonInstance(
+      releasingWork.stateNode,
+      releasingWork.type,
+      releasingWork.memoizedProps,
+    );
   }
 }