Skip to content

Commit 98164cb

Browse files
acdliteacdlite
committed
Fix: Stylesheet in error UI suspends indefinitely (#27265)
This fixes the regression test added in the previous commit. The "Suspensey commit" implementation relies on the `shouldRemainOnPreviousScreen` function to determine whether to 1) suspend the commit 2) activate a parent fallback and schedule a retry. The issue was that we were sometimes attempting option 2 even when there was no parent fallback. Part of the reason this bug landed is due to how `throwException` is structured. In the case of Suspensey commits, we pass a special "noop" thenable to `throwException` as a way to trigger the Suspense path. This special thenable must never have a listener attached to it. This is not a great way to structure the logic, it's just a consequence of how the code evolved over time. We should refactor it into multiple functions so we can trigger a fallback directly without having to check the type. In the meantime, I added an internal warning to help detect similar mistakes in the future. DiffTrain build for commit dd480ef.
1 parent 5043aee commit 98164cb

13 files changed

Lines changed: 331 additions & 316 deletions

File tree

‎compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-dev.js‎

Lines changed: 53 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* @noflow
88
* @nolint
99
* @preventMunge
10-
* @generated SignedSource<<a90e685585292cdb00d4e8ba1b363382>>
10+
* @generated SignedSource<<f4e294d7d90195ed2bc85d15207c702c>>
1111
*/
1212

1313
'use strict';
@@ -4071,7 +4071,14 @@ var SuspenseyCommitException = new Error(
40714071
// for now this will do.
40724072

40734073
var noopSuspenseyCommitThenable = {
4074-
then: function () {}
4074+
then: function () {
4075+
{
4076+
error(
4077+
"Internal React error: A listener was unexpectedly attached to a " +
4078+
'"noop" thenable. This is a bug in React. Please file an issue.'
4079+
);
4080+
}
4081+
}
40754082
};
40764083
function createThenableState() {
40774084
// The ThenableState is created the first time a component suspends. If it
@@ -11072,10 +11079,16 @@ function throwException(
1107211079
suspenseBoundary.updateQueue = new Set([wakeable]);
1107311080
} else {
1107411081
retryQueue.add(wakeable);
11082+
} // We only attach ping listeners in concurrent mode. Legacy
11083+
// Suspense always commits fallbacks synchronously, so there are
11084+
// no pings.
11085+
11086+
if (suspenseBoundary.mode & ConcurrentMode) {
11087+
attachPingListener(root, wakeable, rootRenderLanes);
1107511088
}
1107611089
}
1107711090

11078-
break;
11091+
return;
1107911092
}
1108011093

1108111094
case OffscreenComponent: {
@@ -11106,28 +11119,21 @@ function throwException(
1110611119
_retryQueue.add(wakeable);
1110711120
}
1110811121
}
11109-
}
1111011122

11111-
break;
11112-
} // Fall through
11113-
}
11123+
attachPingListener(root, wakeable, rootRenderLanes);
11124+
}
1111411125

11115-
default: {
11116-
throw new Error(
11117-
"Unexpected Suspense handler tag (" +
11118-
suspenseBoundary.tag +
11119-
"). This " +
11120-
"is a bug in React."
11121-
);
11126+
return;
11127+
}
1112211128
}
11123-
} // We only attach ping listeners in concurrent mode. Legacy Suspense always
11124-
// commits fallbacks synchronously, so there are no pings.
11125-
11126-
if (suspenseBoundary.mode & ConcurrentMode) {
11127-
attachPingListener(root, wakeable, rootRenderLanes);
1112811129
}
1112911130

11130-
return;
11131+
throw new Error(
11132+
"Unexpected Suspense handler tag (" +
11133+
suspenseBoundary.tag +
11134+
"). This " +
11135+
"is a bug in React."
11136+
);
1113111137
} else {
1113211138
// No boundary was found. Unless this is a sync update, this is OK.
1113311139
// We can suspend and wait for more data to arrive.
@@ -20983,9 +20989,19 @@ function shouldRemainOnPreviousScreen() {
2098320989
// on the previous screen, versus showing a fallback as soon as possible. It
2098420990
// takes into account both the priority of render and also whether showing a
2098520991
// fallback would produce a desirable user experience.
20986-
// TODO: Once `use` has fully replaced the `throw promise` pattern, we should
20992+
var handler = getSuspenseHandler();
20993+
20994+
if (handler === null) {
20995+
// There's no Suspense boundary that can provide a fallback. We have no
20996+
// choice but to remain on the previous screen.
20997+
// NOTE: We do this even for sync updates, for lack of any better option. In
20998+
// the future, we may change how we handle this, like by putting the whole
20999+
// root into a "detached" mode.
21000+
return true;
21001+
} // TODO: Once `use` has fully replaced the `throw promise` pattern, we should
2098721002
// be able to remove the equivalent check in finishConcurrentRender, and rely
2098821003
// just on this one.
21004+
2098921005
if (includesOnlyTransitions(workInProgressRootRenderLanes)) {
2099021006
if (getShellBoundary() === null) {
2099121007
// We're rendering inside the "shell" of the app. Activating the nearest
@@ -21001,26 +21017,21 @@ function shouldRemainOnPreviousScreen() {
2100121017
}
2100221018
}
2100321019

21004-
var handler = getSuspenseHandler();
21005-
21006-
if (handler === null);
21007-
else {
21008-
if (
21009-
includesOnlyRetries(workInProgressRootRenderLanes) || // In this context, an OffscreenLane counts as a Retry
21010-
// TODO: It's become increasingly clear that Retries and Offscreen are
21011-
// deeply connected. They probably can be unified further.
21012-
includesSomeLane(workInProgressRootRenderLanes, OffscreenLane)
21013-
) {
21014-
// During a retry, we can suspend rendering if the nearest Suspense boundary
21015-
// is the boundary of the "shell", because we're guaranteed not to block
21016-
// any new content from appearing.
21017-
//
21018-
// The reason we must check if this is a retry is because it guarantees
21019-
// that suspending the work loop won't block an actual update, because
21020-
// retries don't "update" anything; they fill in fallbacks that were left
21021-
// behind by a previous transition.
21022-
return handler === getShellBoundary();
21023-
}
21020+
if (
21021+
includesOnlyRetries(workInProgressRootRenderLanes) || // In this context, an OffscreenLane counts as a Retry
21022+
// TODO: It's become increasingly clear that Retries and Offscreen are
21023+
// deeply connected. They probably can be unified further.
21024+
includesSomeLane(workInProgressRootRenderLanes, OffscreenLane)
21025+
) {
21026+
// During a retry, we can suspend rendering if the nearest Suspense boundary
21027+
// is the boundary of the "shell", because we're guaranteed not to block
21028+
// any new content from appearing.
21029+
//
21030+
// The reason we must check if this is a retry is because it guarantees
21031+
// that suspending the work loop won't block an actual update, because
21032+
// retries don't "update" anything; they fill in fallbacks that were left
21033+
// behind by a previous transition.
21034+
return handler === getShellBoundary();
2102421035
} // For all other Lanes besides Transitions and Retries, we should not wait
2102521036
// for the data to load.
2102621037

@@ -23966,7 +23977,7 @@ function createFiberRoot(
2396623977
return root;
2396723978
}
2396823979

23969-
var ReactVersion = "18.3.0-canary-e76a5aca7-20230822";
23980+
var ReactVersion = "18.3.0-canary-dd480ef92-20230822";
2397023981

2397123982
// Might add PROFILE later.
2397223983

‎compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-prod.js‎

Lines changed: 28 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* @noflow
88
* @nolint
99
* @preventMunge
10-
* @generated SignedSource<<855c2a9f51c40c6c16565d44997b08cb>>
10+
* @generated SignedSource<<433e051293b336d0e7452d6c87975a14>>
1111
*/
1212

1313
"use strict";
@@ -6731,23 +6731,20 @@ function handleThrow(root, thrownValue) {
67316731
ReactCurrentOwner.current = null;
67326732
thrownValue === SuspenseException
67336733
? ((thrownValue = getSuspendedThenable()),
6734-
(workInProgressRootRenderLanes & 8388480) ===
6735-
workInProgressRootRenderLanes
6736-
? (root = null === shellBoundary ? !0 : !1)
6737-
: ((root = suspenseHandlerStackCursor.current),
6738-
(root =
6739-
null === root ||
6740-
((workInProgressRootRenderLanes & 125829120) !==
6741-
workInProgressRootRenderLanes &&
6742-
0 === (workInProgressRootRenderLanes & 1073741824))
6743-
? !1
6744-
: root === shellBoundary)),
6734+
(root = suspenseHandlerStackCursor.current),
67456735
(workInProgressSuspendedReason =
6746-
root &&
6747-
0 === (workInProgressRootSkippedLanes & 268435455) &&
6748-
0 === (workInProgressRootInterleavedUpdatedLanes & 268435455)
6749-
? 2
6750-
: 3))
6736+
(null !== root &&
6737+
((workInProgressRootRenderLanes & 8388480) ===
6738+
workInProgressRootRenderLanes
6739+
? null !== shellBoundary
6740+
: ((workInProgressRootRenderLanes & 125829120) !==
6741+
workInProgressRootRenderLanes &&
6742+
0 === (workInProgressRootRenderLanes & 1073741824)) ||
6743+
root !== shellBoundary)) ||
6744+
0 !== (workInProgressRootSkippedLanes & 268435455) ||
6745+
0 !== (workInProgressRootInterleavedUpdatedLanes & 268435455)
6746+
? 3
6747+
: 2))
67516748
: thrownValue === SuspenseyCommitException
67526749
? ((thrownValue = getSuspendedThenable()),
67536750
(workInProgressSuspendedReason = 4))
@@ -7074,8 +7071,10 @@ function throwAndUnwindWorkLoop(unitOfWork, thrownValue) {
70747071
null === retryQueue
70757072
? (suspenseBoundary.updateQueue = new Set([wakeable]))
70767073
: retryQueue.add(wakeable);
7074+
suspenseBoundary.mode & 1 &&
7075+
attachPingListener(root, wakeable, thrownValue);
70777076
}
7078-
break;
7077+
break a;
70797078
case 22:
70807079
if (suspenseBoundary.mode & 1) {
70817080
suspenseBoundary.flags |= 65536;
@@ -7096,20 +7095,18 @@ function throwAndUnwindWorkLoop(unitOfWork, thrownValue) {
70967095
? (offscreenQueue.retryQueue = new Set([wakeable]))
70977096
: retryQueue$29.add(wakeable);
70987097
}
7098+
attachPingListener(root, wakeable, thrownValue);
70997099
}
7100-
break;
7100+
break a;
71017101
}
7102-
default:
7103-
throw Error(
7104-
"Unexpected Suspense handler tag (" +
7105-
suspenseBoundary.tag +
7106-
"). This is a bug in React."
7107-
);
71087102
}
7109-
suspenseBoundary.mode & 1 &&
7110-
attachPingListener(root, wakeable, thrownValue);
7111-
break a;
7112-
} else if (1 === root.tag) {
7103+
throw Error(
7104+
"Unexpected Suspense handler tag (" +
7105+
suspenseBoundary.tag +
7106+
"). This is a bug in React."
7107+
);
7108+
}
7109+
if (1 === root.tag) {
71137110
attachPingListener(root, wakeable, thrownValue);
71147111
renderDidSuspendDelayIfPossible();
71157112
break a;
@@ -8615,7 +8612,7 @@ var devToolsConfig$jscomp$inline_1029 = {
86158612
throw Error("TestRenderer does not support findFiberByHostInstance()");
86168613
},
86178614
bundleType: 0,
8618-
version: "18.3.0-canary-e76a5aca7-20230822",
8615+
version: "18.3.0-canary-dd480ef92-20230822",
86198616
rendererPackageName: "react-test-renderer"
86208617
};
86218618
var internals$jscomp$inline_1228 = {
@@ -8646,7 +8643,7 @@ var internals$jscomp$inline_1228 = {
86468643
scheduleRoot: null,
86478644
setRefreshHandler: null,
86488645
getCurrentFiber: null,
8649-
reconcilerVersion: "18.3.0-canary-e76a5aca7-20230822"
8646+
reconcilerVersion: "18.3.0-canary-dd480ef92-20230822"
86508647
};
86518648
if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
86528649
var hook$jscomp$inline_1229 = __REACT_DEVTOOLS_GLOBAL_HOOK__;

‎compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react-test-renderer/cjs/ReactTestRenderer-profiling.js‎

Lines changed: 28 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* @noflow
88
* @nolint
99
* @preventMunge
10-
* @generated SignedSource<<ecceacf440b4791ce7dad51926bafd21>>
10+
* @generated SignedSource<<e6092a9eaf897724b4998006d8d964b1>>
1111
*/
1212

1313
"use strict";
@@ -7071,23 +7071,20 @@ function handleThrow(root, thrownValue) {
70717071
ReactCurrentOwner.current = null;
70727072
thrownValue === SuspenseException
70737073
? ((thrownValue = getSuspendedThenable()),
7074-
(workInProgressRootRenderLanes & 8388480) ===
7075-
workInProgressRootRenderLanes
7076-
? (root = null === shellBoundary ? !0 : !1)
7077-
: ((root = suspenseHandlerStackCursor.current),
7078-
(root =
7079-
null === root ||
7080-
((workInProgressRootRenderLanes & 125829120) !==
7081-
workInProgressRootRenderLanes &&
7082-
0 === (workInProgressRootRenderLanes & 1073741824))
7083-
? !1
7084-
: root === shellBoundary)),
7074+
(root = suspenseHandlerStackCursor.current),
70857075
(workInProgressSuspendedReason =
7086-
root &&
7087-
0 === (workInProgressRootSkippedLanes & 268435455) &&
7088-
0 === (workInProgressRootInterleavedUpdatedLanes & 268435455)
7089-
? 2
7090-
: 3))
7076+
(null !== root &&
7077+
((workInProgressRootRenderLanes & 8388480) ===
7078+
workInProgressRootRenderLanes
7079+
? null !== shellBoundary
7080+
: ((workInProgressRootRenderLanes & 125829120) !==
7081+
workInProgressRootRenderLanes &&
7082+
0 === (workInProgressRootRenderLanes & 1073741824)) ||
7083+
root !== shellBoundary)) ||
7084+
0 !== (workInProgressRootSkippedLanes & 268435455) ||
7085+
0 !== (workInProgressRootInterleavedUpdatedLanes & 268435455)
7086+
? 3
7087+
: 2))
70917088
: thrownValue === SuspenseyCommitException
70927089
? ((thrownValue = getSuspendedThenable()),
70937090
(workInProgressSuspendedReason = 4))
@@ -7426,8 +7423,10 @@ function throwAndUnwindWorkLoop(unitOfWork, thrownValue) {
74267423
null === retryQueue
74277424
? (suspenseBoundary.updateQueue = new Set([wakeable]))
74287425
: retryQueue.add(wakeable);
7426+
suspenseBoundary.mode & 1 &&
7427+
attachPingListener(root, wakeable, thrownValue);
74297428
}
7430-
break;
7429+
break a;
74317430
case 22:
74327431
if (suspenseBoundary.mode & 1) {
74337432
suspenseBoundary.flags |= 65536;
@@ -7448,20 +7447,18 @@ function throwAndUnwindWorkLoop(unitOfWork, thrownValue) {
74487447
? (offscreenQueue.retryQueue = new Set([wakeable]))
74497448
: retryQueue$29.add(wakeable);
74507449
}
7450+
attachPingListener(root, wakeable, thrownValue);
74517451
}
7452-
break;
7452+
break a;
74537453
}
7454-
default:
7455-
throw Error(
7456-
"Unexpected Suspense handler tag (" +
7457-
suspenseBoundary.tag +
7458-
"). This is a bug in React."
7459-
);
74607454
}
7461-
suspenseBoundary.mode & 1 &&
7462-
attachPingListener(root, wakeable, thrownValue);
7463-
break a;
7464-
} else if (1 === root.tag) {
7455+
throw Error(
7456+
"Unexpected Suspense handler tag (" +
7457+
suspenseBoundary.tag +
7458+
"). This is a bug in React."
7459+
);
7460+
}
7461+
if (1 === root.tag) {
74657462
attachPingListener(root, wakeable, thrownValue);
74667463
renderDidSuspendDelayIfPossible();
74677464
break a;
@@ -9041,7 +9038,7 @@ var devToolsConfig$jscomp$inline_1071 = {
90419038
throw Error("TestRenderer does not support findFiberByHostInstance()");
90429039
},
90439040
bundleType: 0,
9044-
version: "18.3.0-canary-e76a5aca7-20230822",
9041+
version: "18.3.0-canary-dd480ef92-20230822",
90459042
rendererPackageName: "react-test-renderer"
90469043
};
90479044
var internals$jscomp$inline_1269 = {
@@ -9072,7 +9069,7 @@ var internals$jscomp$inline_1269 = {
90729069
scheduleRoot: null,
90739070
setRefreshHandler: null,
90749071
getCurrentFiber: null,
9075-
reconcilerVersion: "18.3.0-canary-e76a5aca7-20230822"
9072+
reconcilerVersion: "18.3.0-canary-dd480ef92-20230822"
90769073
};
90779074
if ("undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__) {
90789075
var hook$jscomp$inline_1270 = __REACT_DEVTOOLS_GLOBAL_HOOK__;

‎compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-dev.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ if (
2727
}
2828
"use strict";
2929

30-
var ReactVersion = "18.3.0-canary-e76a5aca7-20230822";
30+
var ReactVersion = "18.3.0-canary-dd480ef92-20230822";
3131

3232
// ATTENTION
3333
// When adding new symbols to this file,

‎compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-prod.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -616,4 +616,4 @@ exports.useSyncExternalStore = function (
616616
exports.useTransition = function () {
617617
return ReactCurrentDispatcher.current.useTransition();
618618
};
619-
exports.version = "18.3.0-canary-e76a5aca7-20230822";
619+
exports.version = "18.3.0-canary-dd480ef92-20230822";

‎compiled-rn/facebook-fbsource/xplat/js/RKJSModules/vendor/react/cjs/React-profiling.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -619,7 +619,7 @@ exports.useSyncExternalStore = function (
619619
exports.useTransition = function () {
620620
return ReactCurrentDispatcher.current.useTransition();
621621
};
622-
exports.version = "18.3.0-canary-e76a5aca7-20230822";
622+
exports.version = "18.3.0-canary-dd480ef92-20230822";
623623

624624
/* global __REACT_DEVTOOLS_GLOBAL_HOOK__ */
625625
if (

‎compiled-rn/facebook-fbsource/xplat/js/react-native-github/Libraries/Renderer/REVISION‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
e76a5aca781abdc237f044131790ea615b500532
1+
dd480ef923930c8906a02664b01bcdea50707b5d

0 commit comments

Comments
 (0)