[Bugfix] Reset subtreeFlags in resetWorkInProgress (#20948)

acdlite · gaearon · web-flow · commit f04bcb8139cf · 2021-03-07T09:20:01.000-08:00
* Add failing regression test Based on #20932 Co-Authored-By: Dan Abramov <dan.abramov@gmail.com> * 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 <dan.abramov@gmail.com>
diff --git 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 <span prop={text} />;
+}
+
+function AsyncText({text, showVersion}) {
+  const version = readText(text);
+  const fullText = showVersion ? `${text} [v${version}]` : text;
+  Scheduler.unstable_yieldValue(fullText);
+  return <span prop={fullText} />;
+}
+
+// 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 (
+      <Suspense fallback={<Text text="Loading Tail..." />}>
+        <Text text="Tail" />
+      </Suspense>
+    );
+  }
+
+  function App() {
+    return (
+      <SuspenseList revealOrder="forwards">
+        <Suspense fallback={<Text text="Loading Async..." />}>
+          <Async />
+        </Suspense>
+        <Tail />
+      </SuspenseList>
+    );
+  }
+
+  let setAsyncText;
+  function Async() {
+    const [c, _setAsyncText] = React.useState(0);
+    setAsyncText = _setAsyncText;
+    return <AsyncText text={c} />;
+  }
+
+  const root = ReactNoop.createRoot();
+  await ReactNoop.act(async () => {
+    root.render(<App />);
+  });
+  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
@@ -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
@@ -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;
