From 03462cfc7a96f399d79010f1986c0abdbe0bbd15 Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Sat, 4 Mar 2023 11:26:59 -0500 Subject: [PATCH] [Fizz] External runtime: fix bug in processing existing elements (#26303) ## Summary Fix bug in how the Fizz external runtime processes existing template elements. Bug: - getElementsByTagName returns a HTMLCollection, which is live. - while iterating over an HTMLCollection, we call handleNode which removes nodes Fix: - Call Array.from to copy children of `document.body` before processing. - We could use `querySelectorAll` instead, but that is likely slower due to reading more nodes. ## How did you test this change? Did ad-hoc testing on Facebook home page by commenting out the mutation observer and adding the following. ```javascript window.addEventListener('DOMContentLoaded', function () { handleExistingNodes(document.body); }); ``` --- .../server/ReactDOMServerExternalRuntime.js | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js b/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js index 08012fbc1a..045412b074 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerExternalRuntime.js @@ -21,35 +21,37 @@ if (!window.$RC) { window.$RM = new Map(); } -if (document.readyState === 'loading') { - if (document.body != null) { +if (document.body != null) { + if (document.readyState === 'loading') { installFizzInstrObserver(document.body); - } else { - // body may not exist yet if the fizz runtime is sent in - // (e.g. as a preinit resource) - // $FlowFixMe[recursive-definition] - const domBodyObserver = new MutationObserver(() => { - // We expect the body node to be stable once parsed / created - if (document.body) { - if (document.readyState === 'loading') { - installFizzInstrObserver(document.body); - } - handleExistingNodes(); - // We can call disconnect without takeRecord here, - // since we only expect a single document.body - domBodyObserver.disconnect(); - } - }); - // documentElement must already exist at this point - // $FlowFixMe[incompatible-call] - domBodyObserver.observe(document.documentElement, {childList: true}); } + // $FlowFixMe[incompatible-cast] + handleExistingNodes((document.body /*: HTMLElement */)); +} else { + // Document must be loading -- body may not exist yet if the fizz external + // runtime is sent in (e.g. as a preinit resource) + // $FlowFixMe[recursive-definition] + const domBodyObserver = new MutationObserver(() => { + // We expect the body node to be stable once parsed / created + if (document.body != null) { + if (document.readyState === 'loading') { + installFizzInstrObserver(document.body); + } + // $FlowFixMe[incompatible-cast] + handleExistingNodes((document.body /*: HTMLElement */)); + + // We can call disconnect without takeRecord here, + // since we only expect a single document.body + domBodyObserver.disconnect(); + } + }); + // documentElement must already exist at this point + // $FlowFixMe[incompatible-call] + domBodyObserver.observe(document.documentElement, {childList: true}); } -handleExistingNodes(); - -function handleExistingNodes() { - const existingNodes = document.getElementsByTagName('template'); +function handleExistingNodes(target /*: HTMLElement */) { + const existingNodes = target.querySelectorAll('template'); for (let i = 0; i < existingNodes.length; i++) { handleNode(existingNodes[i]); } @@ -60,8 +62,8 @@ function installFizzInstrObserver(target /*: Node */) { for (let i = 0; i < mutations.length; i++) { const addedNodes = mutations[i].addedNodes; for (let j = 0; j < addedNodes.length; j++) { - if (addedNodes.item(j).parentNode) { - handleNode(addedNodes.item(j)); + if (addedNodes[j].parentNode) { + handleNode(addedNodes[j]); } } }