Fix missing return pointer assignment (#15700)
* Fix missing return pointer assignment I found a bug using the fuzz tester that manifested as incorrect ordering of children in the host tree, but whose root cause was a missing `return` pointer assignment on a work-in-progress fiber. Usually return pointers are set during reconciliation (`reconcileChildFibers`) but this particular assignment happens inside the custom reconciliation implementation used by Suspense boundaries. I would not be surprised if there were similar bugs related to incorrect return pointers. You're supposed to update the return pointer whenever a work-in-progress fiber is created, but there's nothing in the contract of the `createFiber` or `createWorkInProgress` function that implies this. I propose that we update their signatures to accept the return fiber as an argument. I will do this in a follow-up. In this commit, I rearranged `updateSuspenseComponent` slightly so that every call to `createWorkInProgress` or a `createFiber*` function is immediately followed by a return pointer assignment. I hardcoded the fuzz test case that surfaced the bug. * Update all progressed children in list `progressedPrimaryChild` is a list, not a single fiber. Need to iterate through every child and update their return pointers.
Andrew Clark committed
May 21, 2019 at 16:56 UTC
7829d8cf99c20d567b3baceab8e9c2a1f60062de
2 files changed
+105
-29
packages/react-reconciler/src/ReactFiberBeginWork.js
+30
-7
@@ -1550,6 +1550,7 @@ function updateSuspenseComponent(
1550
NoWork,
1551
null,
1552
);
1553
+ primaryChildFragment.return = workInProgress;
1554
1555
if ((workInProgress.mode & BatchedMode) === NoMode) {
1556
// Outside of batched mode, we commit the effects from the
@@ -1560,6 +1561,11 @@ function updateSuspenseComponent(
1561
? (workInProgress.child: any).child
1562
: (workInProgress.child: any);
1563
primaryChildFragment.child = progressedPrimaryChild;
1564
+ let progressedChild = progressedPrimaryChild;
1565
+ while (progressedChild !== null) {
1566
+ progressedChild.return = primaryChildFragment;
1567
+ progressedChild = progressedChild.sibling;
1568
+ }
1569
}
1570
1571
const fallbackChildFragment = createFiberFromFragment(
@@ -1568,12 +1574,12 @@ function updateSuspenseComponent(
1574
renderExpirationTime,
1575
null,
1576
);
1577
+ fallbackChildFragment.return = workInProgress;
1578
primaryChildFragment.sibling = fallbackChildFragment;
1579
child = primaryChildFragment;
1580
// Skip the primary children, and continue working on the
1581
// fallback children.
1582
next = fallbackChildFragment;
1576
- child.return = next.return = workInProgress;
1583
} else {
1584
// Mount the primary children without an intermediate fragment fiber.
1585
const nextPrimaryChildren = nextProps.children;
@@ -1603,6 +1609,7 @@ function updateSuspenseComponent(
1609
currentPrimaryChildFragment.pendingProps,
1610
NoWork,
1611
);
1612
+ primaryChildFragment.return = workInProgress;
1613
1614
if ((workInProgress.mode & BatchedMode) === NoMode) {
1615
// Outside of batched mode, we commit the effects from the
@@ -1614,6 +1621,11 @@ function updateSuspenseComponent(
1621
: (workInProgress.child: any);
1622
if (progressedPrimaryChild !== currentPrimaryChildFragment.child) {
1623
primaryChildFragment.child = progressedPrimaryChild;
1624
+ let progressedChild = progressedPrimaryChild;
1625
+ while (progressedChild !== null) {
1626
+ progressedChild.return = primaryChildFragment;
1627
+ progressedChild = progressedChild.sibling;
1628
+ }
1629
}
1630
}
1631
@@ -1632,17 +1644,18 @@ function updateSuspenseComponent(
1644
1645
// Clone the fallback child fragment, too. These we'll continue
1646
// working on.
1635
- const fallbackChildFragment = (primaryChildFragment.sibling = createWorkInProgress(
1647
+ const fallbackChildFragment = createWorkInProgress(
1648
currentFallbackChildFragment,
1649
nextFallbackChildren,
1650
currentFallbackChildFragment.expirationTime,
1639
- ));
1651
+ );
1652
+ fallbackChildFragment.return = workInProgress;
1653
+ primaryChildFragment.sibling = fallbackChildFragment;
1654
child = primaryChildFragment;
1655
primaryChildFragment.childExpirationTime = NoWork;
1656
// Skip the primary children, and continue working on the
1657
// fallback children.
1658
next = fallbackChildFragment;
1645
- child.return = next.return = workInProgress;
1659
} else {
1660
// No longer suspended. Switch back to showing the primary children,
1661
// and remove the intermediate fragment fiber.
@@ -1680,7 +1693,11 @@ function updateSuspenseComponent(
1693
NoWork,
1694
null,
1695
);
1696
+ primaryChildFragment.return = workInProgress;
1697
primaryChildFragment.child = currentPrimaryChild;
1698
+ if (currentPrimaryChild !== null) {
1699
+ currentPrimaryChild.return = primaryChildFragment;
1700
+ }
1701
1702
// Even though we're creating a new fiber, there are no new children,
1703
// because we're reusing an already mounted tree. So we don't need to
@@ -1696,6 +1713,11 @@ function updateSuspenseComponent(
1713
? (workInProgress.child: any).child
1714
: (workInProgress.child: any);
1715
primaryChildFragment.child = progressedPrimaryChild;
1716
+ let progressedChild = progressedPrimaryChild;
1717
+ while (progressedChild !== null) {
1718
+ progressedChild.return = primaryChildFragment;
1719
+ progressedChild = progressedChild.sibling;
1720
+ }
1721
}
1722
1723
// Because primaryChildFragment is a new fiber that we're inserting as the
@@ -1712,19 +1734,20 @@ function updateSuspenseComponent(
1734
}
1735
1736
// Create a fragment from the fallback children, too.
1715
- const fallbackChildFragment = (primaryChildFragment.sibling = createFiberFromFragment(
1737
+ const fallbackChildFragment = createFiberFromFragment(
1738
nextFallbackChildren,
1739
mode,
1740
renderExpirationTime,
1741
null,
1720
- ));
1742
+ );
1743
+ fallbackChildFragment.return = workInProgress;
1744
+ primaryChildFragment.sibling = fallbackChildFragment;
1745
fallbackChildFragment.effectTag |= Placement;
1746
child = primaryChildFragment;
1747
primaryChildFragment.childExpirationTime = NoWork;
1748
// Skip the primary children, and continue working on the
1749
// fallback children.
1750
next = fallbackChildFragment;
1727
- child.return = next.return = workInProgress;
1751
} else {
1752
// Still haven't timed out. Continue rendering the children, like we
1753
// normally do.
packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js
+75
-22
@@ -322,28 +322,6 @@ describe('ReactSuspenseFuzz', () => {
322
);
323
});
324
325
- it('hard-coded cases', () => {
326
- const {Text, testResolvedOutput} = createFuzzer();
327
-
328
- testResolvedOutput(
329
- <React.Fragment>
330
- <Text
331
- initialDelay={20}
332
- text="A"
333
- updates={[{beginAfter: 10, suspendFor: 20}]}
334
- />
335
- <Suspense fallback="Loading... (B)">
336
- <Text
337
- initialDelay={10}
338
- text="B"
339
- updates={[{beginAfter: 30, suspendFor: 50}]}
340
- />
341
- <Text text="C" />
342
- </Suspense>
343
- </React.Fragment>,
344
- );
345
- });
346
-
325
it('generative tests', () => {
326
const {generateTestCase, testResolvedOutput} = createFuzzer();
327
@@ -367,4 +345,79 @@ ${prettyFormat(randomTestCase)}
345
}
346
}
347
});
348
+
349
+ describe('hard-coded cases', () => {
350
+ it('1', () => {
351
+ const {Text, testResolvedOutput} = createFuzzer();
352
+ testResolvedOutput(
353
+ <React.Fragment>
354
+ <Text
355
+ initialDelay={20}
356
+ text="A"
357
+ updates={[{beginAfter: 10, suspendFor: 20}]}
358
+ />
359
+ <Suspense fallback="Loading... (B)">
360
+ <Text
361
+ initialDelay={10}
362
+ text="B"
363
+ updates={[{beginAfter: 30, suspendFor: 50}]}
364
+ />
365
+ <Text text="C" />
366
+ </Suspense>
367
+ </React.Fragment>,
368
+ );
369
+ });
370
+
371
+ it('2', () => {
372
+ const {Text, Container, testResolvedOutput} = createFuzzer();
373
+ testResolvedOutput(
374
+ <React.Fragment>
375
+ <Suspense fallback="Loading...">
376
+ <Text initialDelay={7200} text="A" />
377
+ </Suspense>
378
+ <Suspense fallback="Loading...">
379
+ <Container>
380
+ <Text initialDelay={1000} text="B" />
381
+ <Text initialDelay={7200} text="C" />
382
+ <Text initialDelay={9000} text="D" />
383
+ </Container>
384
+ </Suspense>
385
+ </React.Fragment>,
386
+ );
387
+ });
388
+
389
+ it('3', () => {
390
+ const {Text, Container, testResolvedOutput} = createFuzzer();
391
+ testResolvedOutput(
392
+ <React.Fragment>
393
+ <Suspense fallback="Loading...">
394
+ <Text
395
+ initialDelay={3183}
396
+ text="A"
397
+ updates={[
398
+ {
399
+ beginAfter: 2256,
400
+ suspendFor: 6696,
401
+ },
402
+ ]}
403
+ />
404
+ <Text initialDelay={3251} text="B" />
405
+ </Suspense>
406
+ <Container>
407
+ <Text
408
+ initialDelay={2700}
409
+ text="C"
410
+ updates={[
411
+ {
412
+ beginAfter: 3266,
413
+ suspendFor: 9139,
414
+ },
415
+ ]}
416
+ />
417
+ <Text initialDelay={6732} text="D" />
418
+ </Container>
419
+ </React.Fragment>,
420
+ );
421
+ });
422
+ });
423
});