@samitouri / QOS-React-1 / commits / d07921eeda

Don't modify keyPath until right before recursive renderNode call (#27366)

Currently, if a component suspends, the keyPath has already been modified to include the identity of the component itself; the path is set before the component body is called (akin to the begin phase in Fiber). An accidental consequence is that when the promise resolves and component is retried, the identity gets appended to the keyPath again, leading to a duplicate node in the path. To address this, we should only modify contexts after any code that may suspend. For maximum safety, this should occur as late as possible: right before the recursive renderNode call, before the children are rendered. I did not add a test yet because there's no feature that currently observes it, but I do have tests in my other WIP PR for useFormState: #27321

Andrew Clark committed Sep 12, 2023 at 21:27 UTC d07921eeda62613bfcf52ecdb66322db26393567

1 file changed +94 -19

packages/react-server/src/ReactFizzServer.js

+94 -19

@@ -725,6 +725,7 @@ function fatalError(request: Request, error: mixed): void {

725 function renderSuspenseBoundary(

726 request: Request,

727 task: Task,

728 + keyPath: Root | KeyNode,

729 props: Object,

730 ): void {

731 pushBuiltInComponentStackInDEV(task, 'Suspense');

@@ -742,7 +743,7 @@ function renderSuspenseBoundary(

743 const newBoundary = createSuspenseBoundary(

744 request,

745 fallbackAbortSet,

745 - task.keyPath,

746 + keyPath,

747 );

748 const insertionIndex = parentSegment.chunks.length;

749 // The children of the boundary segment is actually the fallback.

@@ -853,7 +854,8 @@ function renderSuspenseBoundary(

854 parentBoundary,

855 boundarySegment,

856 fallbackAbortSet,

856 - task.keyPath,

857 + // TODO: Should distinguish key path of fallback and primary tasks

858 + keyPath,

859 task.formatContext,

860 task.legacyContext,

861 task.context,

@@ -872,6 +874,7 @@ function renderSuspenseBoundary(

874 function renderBackupSuspenseBoundary(

875 request: Request,

876 task: Task,

877 + keyPath: Root | KeyNode,

878 props: Object,

879 ) {

880 pushBuiltInComponentStackInDEV(task, 'Suspense');

@@ -880,7 +883,10 @@ function renderBackupSuspenseBoundary(

883 const segment = task.blockedSegment;

884

885 pushStartCompletedSuspenseBoundary(segment.chunks);

886 + const prevKeyPath = task.keyPath;

887 + task.keyPath = keyPath;

888 renderNode(request, task, content, -1);

889 + task.keyPath = prevKeyPath;

890 pushEndCompletedSuspenseBoundary(segment.chunks);

891

892 popComponentStackInDEV(task);

@@ -889,6 +895,7 @@ function renderBackupSuspenseBoundary(

895 function renderHostElement(

896 request: Request,

897 task: Task,

898 + keyPath: Root | KeyNode,

899 type: string,

900 props: Object,

901 ): void {

@@ -906,7 +913,9 @@ function renderHostElement(

913 );

914 segment.lastPushedText = false;

915 const prevContext = task.formatContext;

916 + const prevKeyPath = task.keyPath;

917 task.formatContext = getChildFormatContext(prevContext, type, props);

918 + task.keyPath = keyPath;

919

920 // We use the non-destructive form because if something suspends, we still

921 // need to pop back up and finish this subtree of HTML.

@@ -915,6 +924,7 @@ function renderHostElement(

924 // We expect that errors will fatal the whole task and that we don't need

925 // the correct context. Therefore this is not in a finally.

926 task.formatContext = prevContext;

927 + task.keyPath = prevKeyPath;

928 pushEndInstance(

929 segment.chunks,

930 type,

@@ -947,6 +957,7 @@ function renderWithHooks<Props, SecondArg>(

957 function finishClassComponent(

958 request: Request,

959 task: Task,

960 + keyPath: Root | KeyNode,

961 instance: any,

962 Component: any,

963 props: any,

@@ -983,12 +994,16 @@ function finishClassComponent(

994 }

995 }

996

997 + const prevKeyPath = task.keyPath;

998 + task.keyPath = keyPath;

999 renderNodeDestructive(request, task, null, nextChildren, -1);

1000 + task.keyPath = prevKeyPath;

1001 }

1002

1003 function renderClassComponent(

1004 request: Request,

1005 task: Task,

1006 + keyPath: Root | KeyNode,

1007 Component: any,

1008 props: any,

1009 ): void {

@@ -998,7 +1013,7 @@ function renderClassComponent(

1013 : undefined;

1014 const instance = constructClassInstance(Component, props, maskedContext);

1015 mountClassInstance(instance, Component, props, maskedContext);

1001 - finishClassComponent(request, task, instance, Component, props);

1016 + finishClassComponent(request, task, keyPath, instance, Component, props);

1017 popComponentStackInDEV(task);

1018 }

1019

@@ -1017,6 +1032,7 @@ let hasWarnedAboutUsingContextAsConsumer = false;

1032 function renderIndeterminateComponent(

1033 request: Request,

1034 task: Task,

1035 + keyPath: Root | KeyNode,

1036 prevThenableState: ThenableState | null,

1037 Component: any,

1038 props: any,

@@ -1111,7 +1127,7 @@ function renderIndeterminateComponent(

1127 }

1128

1129 mountClassInstance(value, Component, props, legacyContext);

1114 - finishClassComponent(request, task, value, Component, props);

1130 + finishClassComponent(request, task, keyPath, value, Component, props);

1131 } else {

1132 // Proceed under the assumption that this is a function component

1133 if (__DEV__) {

@@ -1129,6 +1145,7 @@ function renderIndeterminateComponent(

1145 finishFunctionComponent(

1146 request,

1147 task,

1148 + keyPath,

1149 value,

1150 hasId,

1151 formStateCount,

@@ -1141,6 +1158,7 @@ function renderIndeterminateComponent(

1158 function finishFunctionComponent(

1159 request: Request,

1160 task: Task,

1161 + keyPath: Root | KeyNode,

1162 children: ReactNodeList,

1163 hasId: boolean,

1164 formStateCount: number,

@@ -1168,6 +1186,8 @@ function finishFunctionComponent(

1186 }

1187 }

1188

1189 + const prevKeyPath = task.keyPath;

1190 + task.keyPath = keyPath;

1191 if (hasId) {

1192 // This component materialized an id. We treat this as its own level, with

1193 // a single "child" slot.

@@ -1192,6 +1212,7 @@ function finishFunctionComponent(

1212 // again, so we can use the destructive recursive form.

1213 renderNodeDestructive(request, task, null, children, -1);

1214 }

1215 + task.keyPath = prevKeyPath;

1216 }

1217

1218 function validateFunctionComponentInDev(Component: any): void {

@@ -1265,6 +1286,7 @@ function resolveDefaultProps(Component: any, baseProps: Object): Object {

1286 function renderForwardRef(

1287 request: Request,

1288 task: Task,

1289 + keyPath: Root | KeyNode,

1290 prevThenableState: null | ThenableState,

1291 type: any,

1292 props: Object,

@@ -1285,6 +1307,7 @@ function renderForwardRef(

1307 finishFunctionComponent(

1308 request,

1309 task,

1310 + keyPath,

1311 children,

1312 hasId,

1313 formStateCount,

@@ -1296,6 +1319,7 @@ function renderForwardRef(

1319 function renderMemo(

1320 request: Request,

1321 task: Task,

1322 + keyPath: Root | KeyNode,

1323 prevThenableState: ThenableState | null,

1324 type: any,

1325 props: Object,

@@ -1306,6 +1330,7 @@ function renderMemo(

1330 renderElement(

1331 request,

1332 task,

1333 + keyPath,

1334 prevThenableState,

1335 innerType,

1336 resolvedProps,

@@ -1316,6 +1341,7 @@ function renderMemo(

1341 function renderContextConsumer(

1342 request: Request,

1343 task: Task,

1344 + keyPath: Root | KeyNode,

1345 context: ReactContext<any>,

1346 props: Object,

1347 ): void {

@@ -1360,12 +1386,16 @@ function renderContextConsumer(

1386 const newValue = readContext(context);

1387 const newChildren = render(newValue);

1388

1389 + const prevKeyPath = task.keyPath;

1390 + task.keyPath = keyPath;

1391 renderNodeDestructive(request, task, null, newChildren, -1);

1392 + task.keyPath = prevKeyPath;

1393 }

1394

1395 function renderContextProvider(

1396 request: Request,

1397 task: Task,

1398 + keyPath: Root | KeyNode,

1399 type: ReactProviderType<any>,

1400 props: Object,

1401 ): void {

@@ -1376,9 +1406,12 @@ function renderContextProvider(

1406 if (__DEV__) {

1407 prevSnapshot = task.context;

1408 }

1409 + const prevKeyPath = task.keyPath;

1410 task.context = pushProvider(context, value);

1411 + task.keyPath = keyPath;

1412 renderNodeDestructive(request, task, null, children, -1);

1413 task.context = popProvider(context);

1414 + task.keyPath = prevKeyPath;

1415 if (__DEV__) {

1416 if (prevSnapshot !== task.context) {

1417 console.error(

@@ -1391,6 +1424,7 @@ function renderContextProvider(

1424 function renderLazyComponent(

1425 request: Request,

1426 task: Task,

1427 + keyPath: Root | KeyNode,

1428 prevThenableState: ThenableState | null,

1429 lazyComponent: LazyComponentType<any, any>,

1430 props: Object,

@@ -1404,6 +1438,7 @@ function renderLazyComponent(

1438 renderElement(

1439 request,

1440 task,

1441 + keyPath,

1442 prevThenableState,

1443 Component,

1444 resolvedProps,

@@ -1412,7 +1447,12 @@ function renderLazyComponent(

1447 popComponentStackInDEV(task);

1448 }

1449

1415 -function renderOffscreen(request: Request, task: Task, props: Object): void {

1450 +function renderOffscreen(

1451 + request: Request,

1452 + task: Task,

1453 + keyPath: Root | KeyNode,

1454 + props: Object,

1455 +): void {

1456 const mode: ?OffscreenMode = (props.mode: any);

1457 if (mode === 'hidden') {

1458 // A hidden Offscreen boundary is not server rendered. Prerendering happens

@@ -1420,13 +1460,17 @@ function renderOffscreen(request: Request, task: Task, props: Object): void {

1460 } else {

1461 // A visible Offscreen boundary is treated exactly like a fragment: a

1462 // pure indirection.

1463 + const prevKeyPath = task.keyPath;

1464 + task.keyPath = keyPath;

1465 renderNodeDestructive(request, task, null, props.children, -1);

1466 + task.keyPath = prevKeyPath;

1467 }

1468 }

1469

1470 function renderElement(

1471 request: Request,

1472 task: Task,

1473 + keyPath: Root | KeyNode,

1474 prevThenableState: ThenableState | null,

1475 type: any,

1476 props: Object,

@@ -1434,12 +1478,13 @@ function renderElement(

1478 ): void {

1479 if (typeof type === 'function') {

1480 if (shouldConstruct(type)) {

1437 - renderClassComponent(request, task, type, props);

1481 + renderClassComponent(request, task, keyPath, type, props);

1482 return;

1483 } else {

1484 renderIndeterminateComponent(

1485 request,

1486 task,

1487 + keyPath,

1488 prevThenableState,

1489 type,

1490 props,

@@ -1448,7 +1493,7 @@ function renderElement(

1493 }

1494 }

1495 if (typeof type === 'string') {

1451 - renderHostElement(request, task, type, props);

1496 + renderHostElement(request, task, keyPath, type, props);

1497 return;

1498 }

1499

@@ -1467,23 +1512,32 @@ function renderElement(

1512 case REACT_STRICT_MODE_TYPE:

1513 case REACT_PROFILER_TYPE:

1514 case REACT_FRAGMENT_TYPE: {

1515 + const prevKeyPath = task.keyPath;

1516 + task.keyPath = keyPath;

1517 renderNodeDestructive(request, task, null, props.children, -1);

1518 + task.keyPath = prevKeyPath;

1519 return;

1520 }

1521 case REACT_OFFSCREEN_TYPE: {

1474 - renderOffscreen(request, task, props);

1522 + renderOffscreen(request, task, keyPath, props);

1523 return;

1524 }

1525 case REACT_SUSPENSE_LIST_TYPE: {

1526 pushBuiltInComponentStackInDEV(task, 'SuspenseList');

1527 // TODO: SuspenseList should control the boundaries.

1528 + const prevKeyPath = task.keyPath;

1529 + task.keyPath = keyPath;

1530 renderNodeDestructive(request, task, null, props.children, -1);

1531 + task.keyPath = prevKeyPath;

1532 popComponentStackInDEV(task);

1533 return;

1534 }

1535 case REACT_SCOPE_TYPE: {

1536 if (enableScopeAPI) {

1537 + const prevKeyPath = task.keyPath;

1538 + task.keyPath = keyPath;

1539 renderNodeDestructive(request, task, null, props.children, -1);

1540 + task.keyPath = prevKeyPath;

1541 return;

1542 }

1543 throw new Error('ReactDOMServer does not yet support scope components.');

@@ -1493,9 +1547,9 @@ function renderElement(

1547 enableSuspenseAvoidThisFallbackFizz &&

1548 props.unstable_avoidThisFallback === true

1549 ) {

1496 - renderBackupSuspenseBoundary(request, task, props);

1550 + renderBackupSuspenseBoundary(request, task, keyPath, props);

1551 } else {

1498 - renderSuspenseBoundary(request, task, props);

1552 + renderSuspenseBoundary(request, task, keyPath, props);

1553 }

1554 return;

1555 }

@@ -1504,23 +1558,38 @@ function renderElement(

1558 if (typeof type === 'object' && type !== null) {

1559 switch (type.$$typeof) {

1560 case REACT_FORWARD_REF_TYPE: {

1507 - renderForwardRef(request, task, prevThenableState, type, props, ref);

1561 + renderForwardRef(

1562 + request,

1563 + task,

1564 + keyPath,

1565 + prevThenableState,

1566 + type,

1567 + props,

1568 + ref,

1569 + );

1570 return;

1571 }

1572 case REACT_MEMO_TYPE: {

1511 - renderMemo(request, task, prevThenableState, type, props, ref);

1573 + renderMemo(request, task, keyPath, prevThenableState, type, props, ref);

1574 return;

1575 }

1576 case REACT_PROVIDER_TYPE: {

1515 - renderContextProvider(request, task, type, props);

1577 + renderContextProvider(request, task, keyPath, type, props);

1578 return;

1579 }

1580 case REACT_CONTEXT_TYPE: {

1519 - renderContextConsumer(request, task, type, props);

1581 + renderContextConsumer(request, task, keyPath, type, props);

1582 return;

1583 }

1584 case REACT_LAZY_TYPE: {

1523 - renderLazyComponent(request, task, prevThenableState, type, props);

1585 + renderLazyComponent(

1586 + request,

1587 + task,

1588 + keyPath,

1589 + prevThenableState,

1590 + type,

1591 + props,

1592 + );

1593 return;

1594 }

1595 }

@@ -1651,14 +1720,20 @@ function renderNodeDestructiveImpl(

1720 const props = element.props;

1721 const ref = element.ref;

1722 const name = getComponentNameFromType(type);

1654 - const prevKeyPath = task.keyPath;

1655 - task.keyPath = [

1723 + const keyPath = [

1724 task.keyPath,

1725 name,

1726 key == null ? (childIndex === -1 ? 0 : childIndex) : key,

1727 ];

1660 - renderElement(request, task, prevThenableState, type, props, ref);

1661 - task.keyPath = prevKeyPath;

1728 + renderElement(

1729 + request,

1730 + task,

1731 + keyPath,

1732 + prevThenableState,

1733 + type,

1734 + props,

1735 + ref,

1736 + );

1737 return;

1738 }

1739 case REACT_PORTAL_TYPE: