Skip to content

Commit 5d49b2b

Browse files
authored
[Fiber] Track SuspendedState on stack instead of global (#34486)
Stacked on #34481. We currently track the suspended state temporarily with a global which is safe as long as we always read it during a sync pass. However, we sometimes read it in closures and then we have to be carefully to pass the right one since it's possible another commit on a different root has started at that point. This avoids this footgun. Another reason to do this is that I want to read it in `startViewTransition` which is in an async gap after which point it's no longer safe. So I have to pass that through the `commitRoot` bound function.
1 parent ae22247 commit 5d49b2b

File tree

10 files changed

+165
-88
lines changed

10 files changed

+165
-88
lines changed

packages/react-art/src/ReactFiberConfigART.js

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -609,11 +609,13 @@ export function preloadInstance(type, props) {
609609
return true;
610610
}
611611

612-
export function startSuspendingCommit() {}
612+
export function startSuspendingCommit() {
613+
return null;
614+
}
613615

614-
export function suspendInstance(instance, type, props) {}
616+
export function suspendInstance(state, instance, type, props) {}
615617

616-
export function suspendOnActiveViewTransition(container) {}
618+
export function suspendOnActiveViewTransition(state, container) {}
617619

618620
export function waitForCommitToBeReady(timeoutOffset) {
619621
return null;

packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js

Lines changed: 12 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -2081,6 +2081,7 @@ function forceLayout(ownerDocument: Document) {
20812081
}
20822082

20832083
export function startViewTransition(
2084+
suspendedState: null | SuspendedState,
20842085
rootContainer: Container,
20852086
transitionTypes: null | TransitionTypes,
20862087
mutationCallback: () => void,
@@ -2443,6 +2444,7 @@ function animateGesture(
24432444
}
24442445

24452446
export function startGestureTransition(
2447+
suspendedState: null | SuspendedState,
24462448
rootContainer: Container,
24472449
timeline: GestureTimeline,
24482450
rangeStart: number,
@@ -5904,18 +5906,17 @@ export function preloadResource(resource: Resource): boolean {
59045906
return true;
59055907
}
59065908

5907-
type SuspendedState = {
5909+
export opaque type SuspendedState = {
59085910
stylesheets: null | Map<StylesheetResource, HoistableRoot>,
59095911
count: number, // suspensey css and active view transitions
59105912
imgCount: number, // suspensey images
59115913
imgBytes: number, // number of bytes we estimate needing to download
59125914
waitingForImages: boolean, // false when we're no longer blocking on images
59135915
unsuspend: null | (() => void),
59145916
};
5915-
let suspendedState: null | SuspendedState = null;
59165917

5917-
export function startSuspendingCommit(): void {
5918-
suspendedState = {
5918+
export function startSuspendingCommit(): SuspendedState {
5919+
return {
59195920
stylesheets: null,
59205921
count: 0,
59215922
imgCount: 0,
@@ -5930,19 +5931,14 @@ export function startSuspendingCommit(): void {
59305931
}
59315932

59325933
export function suspendInstance(
5934+
state: SuspendedState,
59335935
instance: Instance,
59345936
type: Type,
59355937
props: Props,
59365938
): void {
59375939
if (!enableSuspenseyImages && !enableViewTransition) {
59385940
return;
59395941
}
5940-
if (suspendedState === null) {
5941-
throw new Error(
5942-
'Internal React Error: suspendedState null when it was expected to exists. Please report this as a React bug.',
5943-
);
5944-
}
5945-
const state = suspendedState;
59465942
if (
59475943
// $FlowFixMe[prop-missing]
59485944
typeof instance.decode === 'function' &&
@@ -5971,16 +5967,11 @@ export function suspendInstance(
59715967
}
59725968

59735969
export function suspendResource(
5970+
state: SuspendedState,
59745971
hoistableRoot: HoistableRoot,
59755972
resource: Resource,
59765973
props: any,
59775974
): void {
5978-
if (suspendedState === null) {
5979-
throw new Error(
5980-
'Internal React Error: suspendedState null when it was expected to exists. Please report this as a React bug.',
5981-
);
5982-
}
5983-
const state = suspendedState;
59845975
if (resource.type === 'stylesheet') {
59855976
if (typeof props.media === 'string') {
59865977
// If we don't currently match media we avoid suspending on this resource
@@ -6060,13 +6051,10 @@ export function suspendResource(
60606051
}
60616052
}
60626053

6063-
export function suspendOnActiveViewTransition(rootContainer: Container): void {
6064-
if (suspendedState === null) {
6065-
throw new Error(
6066-
'Internal React Error: suspendedState null when it was expected to exists. Please report this as a React bug.',
6067-
);
6068-
}
6069-
const state = suspendedState;
6054+
export function suspendOnActiveViewTransition(
6055+
state: SuspendedState,
6056+
rootContainer: Container,
6057+
): void {
60706058
const ownerDocument =
60716059
rootContainer.nodeType === DOCUMENT_NODE
60726060
? rootContainer
@@ -6090,16 +6078,9 @@ const SUSPENSEY_IMAGE_TIME_ESTIMATE = 500;
60906078
let estimatedBytesWithinLimit: number = 0;
60916079

60926080
export function waitForCommitToBeReady(
6081+
state: SuspendedState,
60936082
timeoutOffset: number,
60946083
): null | ((() => void) => () => void) {
6095-
if (suspendedState === null) {
6096-
throw new Error(
6097-
'Internal React Error: suspendedState null when it was expected to exists. Please report this as a React bug.',
6098-
);
6099-
}
6100-
6101-
const state = suspendedState;
6102-
61036084
if (state.stylesheets && state.count === 0) {
61046085
// We are not currently blocked but we have not inserted all stylesheets.
61056086
// If this insertion happens and loads or errors synchronously then we can

packages/react-native-renderer/src/ReactFiberConfigFabric.js

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -602,17 +602,28 @@ export function preloadInstance(
602602
return true;
603603
}
604604

605-
export function startSuspendingCommit(): void {}
605+
export opaque type SuspendedState = null;
606+
607+
export function startSuspendingCommit(): SuspendedState {
608+
return null;
609+
}
606610

607611
export function suspendInstance(
612+
state: SuspendedState,
608613
instance: Instance,
609614
type: Type,
610615
props: Props,
611616
): void {}
612617

613-
export function suspendOnActiveViewTransition(container: Container): void {}
618+
export function suspendOnActiveViewTransition(
619+
state: SuspendedState,
620+
container: Container,
621+
): void {}
614622

615-
export function waitForCommitToBeReady(timeoutOffset: number): null {
623+
export function waitForCommitToBeReady(
624+
state: SuspendedState,
625+
timeoutOffset: number,
626+
): null {
616627
return null;
617628
}
618629

packages/react-native-renderer/src/ReactFiberConfigNative.js

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -664,6 +664,7 @@ export function hasInstanceAffectedParent(
664664
}
665665

666666
export function startViewTransition(
667+
suspendedState: null | SuspendedState,
667668
rootContainer: Container,
668669
transitionTypes: null | TransitionTypes,
669670
mutationCallback: () => void,
@@ -684,6 +685,7 @@ export function startViewTransition(
684685
export type RunningViewTransition = null;
685686

686687
export function startGestureTransition(
688+
suspendedState: null | SuspendedState,
687689
rootContainer: Container,
688690
timeline: GestureTimeline,
689691
rangeStart: number,
@@ -778,17 +780,28 @@ export function preloadInstance(
778780
return true;
779781
}
780782

781-
export function startSuspendingCommit(): void {}
783+
export opaque type SuspendedState = null;
784+
785+
export function startSuspendingCommit(): SuspendedState {
786+
return null;
787+
}
782788

783789
export function suspendInstance(
790+
state: SuspendedState,
784791
instance: Instance,
785792
type: Type,
786793
props: Props,
787794
): void {}
788795

789-
export function suspendOnActiveViewTransition(container: Container): void {}
796+
export function suspendOnActiveViewTransition(
797+
state: SuspendedState,
798+
container: Container,
799+
): void {}
790800

791-
export function waitForCommitToBeReady(timeoutOffset: number): null {
801+
export function waitForCommitToBeReady(
802+
state: SuspendedState,
803+
timeoutOffset: number,
804+
): null {
792805
return null;
793806
}
794807

packages/react-noop-renderer/src/createReactNoop.js

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ type SuspenseyCommitSubscription = {
9090
commit: null | (() => void),
9191
};
9292

93+
export opaque type SuspendedState = SuspenseyCommitSubscription;
94+
9395
export type TransitionStatus = mixed;
9496

9597
export type FormInstance = Instance;
@@ -311,17 +313,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
311313
'pending' | 'fulfilled',
312314
> | null = null;
313315

314-
// Represents a subscription for all the suspensey things that block a
315-
// particular commit. Once they've all loaded, the commit phase can proceed.
316-
let suspenseyCommitSubscription: SuspenseyCommitSubscription | null = null;
317-
318-
function startSuspendingCommit(): void {
319-
// This is where we might suspend on things that aren't associated with a
320-
// particular node, like document.fonts.ready.
321-
suspenseyCommitSubscription = null;
316+
function startSuspendingCommit(): SuspendedState {
317+
// Represents a subscription for all the suspensey things that block a
318+
// particular commit. Once they've all loaded, the commit phase can proceed.
319+
return {
320+
pendingCount: 0,
321+
commit: null,
322+
};
322323
}
323324

324325
function suspendInstance(
326+
state: SuspendedState,
325327
instance: Instance,
326328
type: string,
327329
props: Props,
@@ -338,20 +340,13 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
338340
if (record.status === 'fulfilled') {
339341
// Already loaded.
340342
} else if (record.status === 'pending') {
341-
if (suspenseyCommitSubscription === null) {
342-
suspenseyCommitSubscription = {
343-
pendingCount: 1,
344-
commit: null,
345-
};
346-
} else {
347-
suspenseyCommitSubscription.pendingCount++;
348-
}
343+
state.pendingCount++;
349344
// Stash the subscription on the record. In `resolveSuspenseyThing`,
350345
// we'll use this fire the commit once all the things have loaded.
351346
if (record.subscriptions === null) {
352347
record.subscriptions = [];
353348
}
354-
record.subscriptions.push(suspenseyCommitSubscription);
349+
record.subscriptions.push(state);
355350
}
356351
} else {
357352
throw new Error(
@@ -362,15 +357,14 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
362357
}
363358

364359
function waitForCommitToBeReady(
360+
state: SuspendedState,
365361
timeoutOffset: number,
366362
): ((commit: () => mixed) => () => void) | null {
367-
const subscription = suspenseyCommitSubscription;
368-
if (subscription !== null) {
369-
suspenseyCommitSubscription = null;
363+
if (state.pendingCount > 0) {
370364
return (commit: () => void) => {
371-
subscription.commit = commit;
365+
state.commit = commit;
372366
const cancelCommit = () => {
373-
subscription.commit = null;
367+
state.commit = null;
374368
};
375369
return cancelCommit;
376370
};
@@ -693,13 +687,16 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
693687
startSuspendingCommit,
694688
suspendInstance,
695689

696-
suspendResource(resource: mixed): void {
690+
suspendResource(state: SuspendedState, resource: mixed): void {
697691
throw new Error(
698692
'Resources are not implemented for React Noop yet. This method should not be called',
699693
);
700694
},
701695

702-
suspendOnActiveViewTransition(container: Container): void {
696+
suspendOnActiveViewTransition(
697+
state: SuspendedState,
698+
container: Container,
699+
): void {
703700
// Not implemented
704701
},
705702

0 commit comments

Comments
 (0)