Bugfix: Synchronous ping during render phase sometimes unwinds the stack, leading to crash (#25851)
I found this bug when working on a different task. `pingSuspendedRoot` sometimes calls `prepareFreshStack` to interupt the work-in-progress tree and force a restart from the root. The idea is that if the current render is already in a state where it be blocked from committing, and there's new data that could unblock it, we might as well restart from the beginning. The problem is that this is only safe to do if `pingSuspendedRoot` is called from a non-React task, like an event handler or a microtask. While this is usually the case, it's entirely possible for a thenable to resolve (i.e. to call `pingSuspendedRoot`) synchronously while the render phase is already executing. If that happens, and work loop attempts to unwind the stack, it causes the render phase to crash.
Andrew Clark committed
Dec 21, 2022 at 23:55 UTC
5fcf1a4b4c2150a1b9fe0de0144a82a053c63966
2 files changed
+86
-2
packages/react-reconciler/src/ReactFiberWorkLoop.js
+10
-2
@@ -3401,8 +3401,16 @@ function pingSuspendedRoot(
3401
includesOnlyRetries(workInProgressRootRenderLanes) &&
3402
now() - globalMostRecentFallbackTime < FALLBACK_THROTTLE_MS)
3403
) {
3404
- // Restart from the root.
3405
- prepareFreshStack(root, NoLanes);
3404
+ // Force a restart from the root by unwinding the stack. Unless this is
3405
+ // being called from the render phase, because that would cause a crash.
3406
+ if ((executionContext & RenderContext) === NoContext) {
3407
+ prepareFreshStack(root, NoLanes);
3408
+ } else {
3409
+ // TODO: If this does happen during the render phase, we should throw
3410
+ // the special internal exception that we use to interrupt the stack for
3411
+ // selective hydration. That was temporarily reverted but we once we add
3412
+ // it back we can use it here.
3413
+ }
3414
} else {
3415
// Even though we can't restart right now, we might get an
3416
// opportunity later. So we mark this render as having a ping.
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
+76
@@ -4218,4 +4218,80 @@ describe('ReactSuspenseWithNoopRenderer', () => {
4218
});
4219
expect(Scheduler).toHaveYielded(['Unmount Child']);
4220
});
4221
+
4222
+ // @gate enableLegacyCache
4223
+ it(
4224
+ 'regression test: pinging synchronously within the render phase ' +
4225
+ 'does not unwind the stack',
4226
+ async () => {
4227
+ // This is a regression test that reproduces a very specific scenario that
4228
+ // used to cause a crash.
4229
+ const thenable = {
4230
+ then(resolve) {
4231
+ resolve('hi');
4232
+ },
4233
+ status: 'pending',
4234
+ };
4235
+
4236
+ function ImmediatelyPings() {
4237
+ if (thenable.status === 'pending') {
4238
+ thenable.status = 'fulfilled';
4239
+ throw thenable;
4240
+ }
4241
+ return <Text text="Hi" />;
4242
+ }
4243
+
4244
+ function App({showMore}) {
4245
+ return (
4246
+ <div>
4247
+ <Suspense fallback={<Text text="Loading..." />}>
4248
+ {showMore ? (
4249
+ <>
4250
+ <AsyncText text="Async" />
4251
+ </>
4252
+ ) : null}
4253
+ </Suspense>
4254
+ {showMore ? (
4255
+ <Suspense>
4256
+ <ImmediatelyPings />
4257
+ </Suspense>
4258
+ ) : null}
4259
+ </div>
4260
+ );
4261
+ }
4262
+
4263
+ // Initial render. This mounts a Suspense boundary, so that in the next
4264
+ // update we can trigger a "suspend with delay" scenario.
4265
+ const root = ReactNoop.createRoot();
4266
+ await act(async () => {
4267
+ root.render(<App showMore={false} />);
4268
+ });
4269
+ expect(Scheduler).toHaveYielded([]);
4270
+ expect(root).toMatchRenderedOutput(<div />);
4271
+
4272
+ // Update. This will cause two separate trees to suspend. The first tree
4273
+ // will be inside an already mounted Suspense boundary, so it will trigger
4274
+ // a "suspend with delay". The second tree will be a new Suspense
4275
+ // boundary, but the thenable that is thrown will immediately call its
4276
+ // ping listener.
4277
+ //
4278
+ // Before the bug was fixed, this would lead to a `prepareFreshStack` call
4279
+ // that unwinds the work-in-progress stack. When that code was written, it
4280
+ // was expected that pings always happen from an asynchronous task (or
4281
+ // microtask). But this test shows an example where that's not the case.
4282
+ //
4283
+ // The fix was to check if we're in the render phase before calling
4284
+ // `prepareFreshStack`.
4285
+ await act(async () => {
4286
+ root.render(<App showMore={true} />);
4287
+ });
4288
+ expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...', 'Hi']);
4289
+ expect(root).toMatchRenderedOutput(
4290
+ <div>
4291
+ <span prop="Loading..." />
4292
+ <span prop="Hi" />
4293
+ </div>,
4294
+ );
4295
+ },
4296
+ );
4297
});