[Bugfix] Reset `subtreeFlags` in `resetWorkInProgress` (#20948)
* 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>
Andrew Clark committed
Mar 7, 2021 at 11:20 UTC
f04bcb8139cfa341640ea875c2eae15523ae9cd9
3 files changed
+191
-8
packages/react-dom/src/__tests__/ReactWrongReturnPointer-test.js
+189
@@ -9,12 +9,146 @@
9
10
let React;
11
let ReactNoop;
12
+let Scheduler;
13
+let Suspense;
14
+let SuspenseList;
15
+let getCacheForType;
16
+let caches;
17
+let seededCache;
18
19
beforeEach(() => {
20
React = require('react');
21
ReactNoop = require('react-noop-renderer');
22
+ Scheduler = require('scheduler');
23
+
24
+ Suspense = React.Suspense;
25
+ SuspenseList = React.unstable_SuspenseList;
26
+
27
+ getCacheForType = React.unstable_getCacheForType;
28
+
29
+ caches = [];
30
+ seededCache = null;
31
});
32
33
+function createTextCache() {
34
+ if (seededCache !== null) {
35
+ // Trick to seed a cache before it exists.
36
+ // TODO: Need a built-in API to seed data before the initial render (i.e.
37
+ // not a refresh because nothing has mounted yet).
38
+ const cache = seededCache;
39
+ seededCache = null;
40
+ return cache;
41
+ }
42
+
43
+ const data = new Map();
44
+ const version = caches.length + 1;
45
+ const cache = {
46
+ version,
47
+ data,
48
+ resolve(text) {
49
+ const record = data.get(text);
50
+ if (record === undefined) {
51
+ const newRecord = {
52
+ status: 'resolved',
53
+ value: text,
54
+ };
55
+ data.set(text, newRecord);
56
+ } else if (record.status === 'pending') {
57
+ const thenable = record.value;
58
+ record.status = 'resolved';
59
+ record.value = text;
60
+ thenable.pings.forEach(t => t());
61
+ }
62
+ },
63
+ reject(text, error) {
64
+ const record = data.get(text);
65
+ if (record === undefined) {
66
+ const newRecord = {
67
+ status: 'rejected',
68
+ value: error,
69
+ };
70
+ data.set(text, newRecord);
71
+ } else if (record.status === 'pending') {
72
+ const thenable = record.value;
73
+ record.status = 'rejected';
74
+ record.value = error;
75
+ thenable.pings.forEach(t => t());
76
+ }
77
+ },
78
+ };
79
+ caches.push(cache);
80
+ return cache;
81
+}
82
+
83
+function readText(text) {
84
+ const textCache = getCacheForType(createTextCache);
85
+ const record = textCache.data.get(text);
86
+ if (record !== undefined) {
87
+ switch (record.status) {
88
+ case 'pending':
89
+ Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
90
+ throw record.value;
91
+ case 'rejected':
92
+ Scheduler.unstable_yieldValue(`Error! [${text}]`);
93
+ throw record.value;
94
+ case 'resolved':
95
+ return textCache.version;
96
+ }
97
+ } else {
98
+ Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
99
+
100
+ const thenable = {
101
+ pings: [],
102
+ then(resolve) {
103
+ if (newRecord.status === 'pending') {
104
+ thenable.pings.push(resolve);
105
+ } else {
106
+ Promise.resolve().then(() => resolve(newRecord.value));
107
+ }
108
+ },
109
+ };
110
+
111
+ const newRecord = {
112
+ status: 'pending',
113
+ value: thenable,
114
+ };
115
+ textCache.data.set(text, newRecord);
116
+
117
+ throw thenable;
118
+ }
119
+}
120
+
121
+function Text({text}) {
122
+ Scheduler.unstable_yieldValue(text);
123
+ return <span prop={text} />;
124
+}
125
+
126
+function AsyncText({text, showVersion}) {
127
+ const version = readText(text);
128
+ const fullText = showVersion ? `${text} [v${version}]` : text;
129
+ Scheduler.unstable_yieldValue(fullText);
130
+ return <span prop={fullText} />;
131
+}
132
+
133
+// function seedNextTextCache(text) {
134
+// if (seededCache === null) {
135
+// seededCache = createTextCache();
136
+// }
137
+// seededCache.resolve(text);
138
+// }
139
+
140
+function resolveMostRecentTextCache(text) {
141
+ if (caches.length === 0) {
142
+ throw Error('Cache does not exist.');
143
+ } else {
144
+ // Resolve the most recently created cache. An older cache can by
145
+ // resolved with `caches[index].resolve(text)`.
146
+ caches[caches.length - 1].resolve(text);
147
+ }
148
+}
149
+
150
+const resolveText = resolveMostRecentTextCache;
151
+
152
// Don't feel too guilty if you have to delete this test.
153
// @gate dfsEffectsRefactor
154
// @gate __DEV__
@@ -59,3 +193,58 @@ test('warns in DEV if return pointer is inconsistent', async () => {
193
'Internal React error: Return pointer is inconsistent with parent.',
194
);
195
});
196
+
197
+// @gate experimental
198
+// @gate enableCache
199
+test('regression (#20932): return pointer is correct before entering deleted tree', async () => {
200
+ // Based on a production bug. Designed to trigger a very specific
201
+ // implementation path.
202
+ function Tail() {
203
+ return (
204
+ <Suspense fallback={<Text text="Loading Tail..." />}>
205
+ <Text text="Tail" />
206
+ </Suspense>
207
+ );
208
+ }
209
+
210
+ function App() {
211
+ return (
212
+ <SuspenseList revealOrder="forwards">
213
+ <Suspense fallback={<Text text="Loading Async..." />}>
214
+ <Async />
215
+ </Suspense>
216
+ <Tail />
217
+ </SuspenseList>
218
+ );
219
+ }
220
+
221
+ let setAsyncText;
222
+ function Async() {
223
+ const [c, _setAsyncText] = React.useState(0);
224
+ setAsyncText = _setAsyncText;
225
+ return <AsyncText text={c} />;
226
+ }
227
+
228
+ const root = ReactNoop.createRoot();
229
+ await ReactNoop.act(async () => {
230
+ root.render(<App />);
231
+ });
232
+ expect(Scheduler).toHaveYielded([
233
+ 'Suspend! [0]',
234
+ 'Loading Async...',
235
+ 'Loading Tail...',
236
+ ]);
237
+ await ReactNoop.act(async () => {
238
+ resolveText(0);
239
+ });
240
+ expect(Scheduler).toHaveYielded([0, 'Tail']);
241
+ await ReactNoop.act(async () => {
242
+ setAsyncText(x => x + 1);
243
+ });
244
+ expect(Scheduler).toHaveYielded([
245
+ 'Suspend! [1]',
246
+ 'Loading Async...',
247
+ 'Suspend! [1]',
248
+ 'Loading Async...',
249
+ ]);
250
+});
packages/react-reconciler/src/ReactFiber.new.js
+1
-4
@@ -388,10 +388,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
388
workInProgress.lanes = current.lanes;
389
390
workInProgress.child = current.child;
391
- // TODO: `subtreeFlags` should be reset to NoFlags, like we do in
392
- // `createWorkInProgress`. Nothing reads this until the complete phase,
393
- // currently, but it might in the future, and we should be consistent.
394
- workInProgress.subtreeFlags = current.subtreeFlags;
391
+ workInProgress.subtreeFlags = NoFlags;
392
workInProgress.deletions = null;
393
workInProgress.memoizedProps = current.memoizedProps;
394
workInProgress.memoizedState = current.memoizedState;
packages/react-reconciler/src/ReactFiber.old.js
+1
-4
@@ -388,10 +388,7 @@ export function resetWorkInProgress(workInProgress: Fiber, renderLanes: Lanes) {
388
workInProgress.lanes = current.lanes;
389
390
workInProgress.child = current.child;
391
- // TODO: `subtreeFlags` should be reset to NoFlags, like we do in
392
- // `createWorkInProgress`. Nothing reads this until the complete phase,
393
- // currently, but it might in the future, and we should be consistent.
394
- workInProgress.subtreeFlags = current.subtreeFlags;
391
+ workInProgress.subtreeFlags = NoFlags;
392
workInProgress.deletions = null;
393
workInProgress.memoizedProps = current.memoizedProps;
394
workInProgress.memoizedState = current.memoizedState;