From f04bcb8139cfa341640ea875c2eae15523ae9cd9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sun, 7 Mar 2021 11:20:01 -0600 Subject: [PATCH] [Bugfix] Reset `subtreeFlags` in `resetWorkInProgress` (#20948) * Add failing regression test Based on #20932 Co-Authored-By: Dan Abramov * Reset `subtreeFlags` in `resetWorkInProgress` Alternate fix to #20942 There was already a TODO to make this change, but at the time I left it, I couldn't think of a way that it would actually cause a bug, and I was hesistant to change something without fully understanding the ramifications. This was during a time when we were hunting down a different bug, so we were especially risk averse. What I should have done in retrospect is put the change behind a flag and tried rolling it out once the other bug had been flushed out. OTOH, now we have a regression test, which wouldn't have otherwise, and the bug it caused rarely fired in production. Co-authored-by: Dan Abramov --- .../__tests__/ReactWrongReturnPointer-test.js | 189 ++++++++++++++++++ .../react-reconciler/src/ReactFiber.new.js | 5 +- .../react-reconciler/src/ReactFiber.old.js | 5 +- 3 files changed, 191 insertions(+), 8 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js index 9329a2aa34fc4..bb3e040cbeb6e 100644 --- a/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js +++ b/packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js @@ -9,12 +9,146 @@ let React; let ReactNoop; +let Scheduler; +let Suspense; +let SuspenseList; +let getCacheForType; +let caches; +let seededCache; beforeEach(() => { React = require('react'); ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + + Suspense = React.Suspense; + SuspenseList = React.unstable_SuspenseList; + + getCacheForType = React.unstable_getCacheForType; + + caches = []; + seededCache = null; }); +function createTextCache() { + if (seededCache !== null) { + // Trick to seed a cache before it exists. + // TODO: Need a built-in API to seed data before the initial render (i.e. + // not a refresh because nothing has mounted yet). + const cache = seededCache; + seededCache = null; + return cache; + } + + const data = new Map(); + const version = caches.length + 1; + const cache = { + version, + data, + resolve(text) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'resolved', + value: text, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'resolved'; + record.value = text; + thenable.pings.forEach(t => t()); + } + }, + reject(text, error) { + const record = data.get(text); + if (record === undefined) { + const newRecord = { + status: 'rejected', + value: error, + }; + data.set(text, newRecord); + } else if (record.status === 'pending') { + const thenable = record.value; + record.status = 'rejected'; + record.value = error; + thenable.pings.forEach(t => t()); + } + }, + }; + caches.push(cache); + return cache; +} + +function readText(text) { + const textCache = getCacheForType(createTextCache); + const record = textCache.data.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + throw record.value; + case 'rejected': + Scheduler.unstable_yieldValue(`Error! [${text}]`); + throw record.value; + case 'resolved': + return textCache.version; + } + } else { + Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + + const thenable = { + pings: [], + then(resolve) { + if (newRecord.status === 'pending') { + thenable.pings.push(resolve); + } else { + Promise.resolve().then(() => resolve(newRecord.value)); + } + }, + }; + + const newRecord = { + status: 'pending', + value: thenable, + }; + textCache.data.set(text, newRecord); + + throw thenable; + } +} + +function Text({text}) { + Scheduler.unstable_yieldValue(text); + return ; +} + +function AsyncText({text, showVersion}) { + const version = readText(text); + const fullText = showVersion ? `${text} [v${version}]` : text; + Scheduler.unstable_yieldValue(fullText); + return ; +} + +// function seedNextTextCache(text) { +// if (seededCache === null) { +// seededCache = createTextCache(); +// } +// seededCache.resolve(text); +// } + +function resolveMostRecentTextCache(text) { + if (caches.length === 0) { + throw Error('Cache does not exist.'); + } else { + // Resolve the most recently created cache. An older cache can by + // resolved with `caches[index].resolve(text)`. + caches[caches.length - 1].resolve(text); + } +} + +const resolveText = resolveMostRecentTextCache; + // Don't feel too guilty if you have to delete this test. // @gate dfsEffectsRefactor // @gate __DEV__ @@ -59,3 +193,58 @@ test('warns in DEV if return pointer is inconsistent', async () => { 'Internal React error: Return pointer is inconsistent with parent.', ); }); + +// @gate experimental +// @gate enableCache +test('regression (#20932): return pointer is correct before entering deleted tree', async () => { + // Based on a production bug. Designed to trigger a very specific + // implementation path. + function Tail() { + return ( + }> + + + ); + } + + function App() { + return ( + + }> + + + + + ); + } + + let setAsyncText; + function Async() { + const [c, _setAsyncText] = React.useState(0); + setAsyncText = _setAsyncText; + return ; + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Suspend! [0]', + 'Loading Async...', + 'Loading Tail...', + ]); + await ReactNoop.act(async () => { + resolveText(0); + }); + expect(Scheduler).toHaveYielded([0, 'Tail']); + await ReactNoop.act(async () => { + setAsyncText(x => x + 1); + }); + expect(Scheduler).toHaveYielded([ + 'Suspend! [1]', + 'Loading Async...', + 'Suspend! [1]', + 'Loading Async...', + ]); +}); diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 5a56fb0592c4b..3c49e56431c39 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -388,10 +388,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) { workInProgress.lanes = current.lanes; workInProgress.child = current.child; - // TODO: `subtreeFlags` should be reset to NoFlags, like we do in - // `createWorkInProgress`. Nothing reads this until the complete phase, - // currently, but it might in the future, and we should be consistent. - workInProgress.subtreeFlags = current.subtreeFlags; + workInProgress.subtreeFlags = NoFlags; workInProgress.deletions = null; workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index 8536c024d66ed..e98634b37cfd6 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -388,10 +388,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) { workInProgress.lanes = current.lanes; workInProgress.child = current.child; - // TODO: `subtreeFlags` should be reset to NoFlags, like we do in - // `createWorkInProgress`. Nothing reads this until the complete phase, - // currently, but it might in the future, and we should be consistent. - workInProgress.subtreeFlags = current.subtreeFlags; + workInProgress.subtreeFlags = NoFlags; workInProgress.deletions = null; workInProgress.memoizedProps = current.memoizedProps; workInProgress.memoizedState = current.memoizedState;