Skip to content

Commit 1b752f1

Browse files
author
Brian Vaughn
authored
Fixed potential interaction tracing leak in Suspense thennable memoization (#15531)
Audited the other places we call unstable_wrap() in React DOM and verified that they didn't have this similar problem.
1 parent 12e5a13 commit 1b752f1

File tree

3 files changed

+83
-4
lines changed

3 files changed

+83
-4
lines changed

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1317,10 +1317,10 @@ function commitSuspenseComponent(finishedWork: Fiber) {
13171317
thenables.forEach(thenable => {
13181318
// Memoize using the boundary fiber to prevent redundant listeners.
13191319
let retry = resolveRetryThenable.bind(null, finishedWork, thenable);
1320-
if (enableSchedulerTracing) {
1321-
retry = Schedule_tracing_wrap(retry);
1322-
}
13231320
if (!retryCache.has(thenable)) {
1321+
if (enableSchedulerTracing) {
1322+
retry = Schedule_tracing_wrap(retry);
1323+
}
13241324
retryCache.add(thenable);
13251325
thenable.then(retry, retry);
13261326
}

packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ let React;
22
let ReactTestRenderer;
33
let ReactFeatureFlags;
44
let Scheduler;
5+
let SchedulerTracing;
56
let ReactCache;
67
let Suspense;
78
let act;
@@ -17,10 +18,12 @@ describe('ReactSuspense', () => {
1718
ReactFeatureFlags = require('shared/ReactFeatureFlags');
1819
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
1920
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
21+
ReactFeatureFlags.enableSchedulerTracing = true;
2022
React = require('react');
2123
ReactTestRenderer = require('react-test-renderer');
2224
act = ReactTestRenderer.act;
2325
Scheduler = require('scheduler');
26+
SchedulerTracing = require('scheduler/tracing');
2427
ReactCache = require('react-cache');
2528

2629
Suspense = React.Suspense;
@@ -914,6 +917,78 @@ describe('ReactSuspense', () => {
914917
]);
915918
});
916919

920+
it('should call onInteractionScheduledWorkCompleted after suspending', done => {
921+
const subscriber = {
922+
onInteractionScheduledWorkCompleted: jest.fn(),
923+
onInteractionTraced: jest.fn(),
924+
onWorkCanceled: jest.fn(),
925+
onWorkScheduled: jest.fn(),
926+
onWorkStarted: jest.fn(),
927+
onWorkStopped: jest.fn(),
928+
};
929+
SchedulerTracing.unstable_subscribe(subscriber);
930+
SchedulerTracing.unstable_trace('test', performance.now(), () => {
931+
function App() {
932+
return (
933+
<React.Suspense fallback={<Text text="Loading..." />}>
934+
<AsyncText text="A" ms={1000} />
935+
<AsyncText text="B" ms={2000} />
936+
<AsyncText text="C" ms={3000} />
937+
</React.Suspense>
938+
);
939+
}
940+
941+
const root = ReactTestRenderer.create(null);
942+
root.update(<App />);
943+
944+
expect(Scheduler).toHaveYielded([
945+
'Suspend! [A]',
946+
'Suspend! [B]',
947+
'Suspend! [C]',
948+
'Loading...',
949+
]);
950+
951+
// Resolve A
952+
jest.advanceTimersByTime(1000);
953+
954+
expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
955+
expect(Scheduler).toFlushExpired([
956+
'A',
957+
// The promises for B and C have now been thrown twice
958+
'Suspend! [B]',
959+
'Suspend! [C]',
960+
]);
961+
962+
// Resolve B
963+
jest.advanceTimersByTime(1000);
964+
965+
expect(Scheduler).toHaveYielded(['Promise resolved [B]']);
966+
expect(Scheduler).toFlushExpired([
967+
// Even though the promise for B was thrown twice, we should only
968+
// re-render once.
969+
'B',
970+
// The promise for C has now been thrown three times
971+
'Suspend! [C]',
972+
]);
973+
974+
// Resolve C
975+
jest.advanceTimersByTime(1000);
976+
977+
expect(Scheduler).toHaveYielded(['Promise resolved [C]']);
978+
expect(Scheduler).toFlushExpired([
979+
// Even though the promise for C was thrown three times, we should only
980+
// re-render once.
981+
'C',
982+
]);
983+
984+
done();
985+
});
986+
987+
expect(
988+
subscriber.onInteractionScheduledWorkCompleted,
989+
).toHaveBeenCalledTimes(1);
990+
});
991+
917992
it('#14162', () => {
918993
const {lazy} = React;
919994

packages/react/src/__tests__/ReactProfiler-test.internal.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2607,7 +2607,11 @@ describe('Profiler', () => {
26072607
]);
26082608
onRender.mockClear();
26092609

2610-
expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled();
2610+
expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1);
2611+
expect(
2612+
onInteractionScheduledWorkCompleted.mock.calls[0][0],
2613+
).toMatchInteraction(highPriUpdateInteraction);
2614+
onInteractionScheduledWorkCompleted.mockClear();
26112615

26122616
Scheduler.advanceTime(100);
26132617
jest.advanceTimersByTime(100);

0 commit comments

Comments
 (0)