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.
Brian Vaughn committed
Apr 29, 2019 at 15:04 UTC
1b752f1914b677c0bb588d10a40c12f332dfd031
3 files changed
+83
-4
packages/react-reconciler/src/ReactFiberCommitWork.js
+3
-3
@@ -1317,10 +1317,10 @@ function commitSuspenseComponent(finishedWork: Fiber) {
1317
thenables.forEach(thenable => {
1318
// Memoize using the boundary fiber to prevent redundant listeners.
1319
let retry = resolveRetryThenable.bind(null, finishedWork, thenable);
1320
- if (enableSchedulerTracing) {
1321
- retry = Schedule_tracing_wrap(retry);
1322
- }
1320
if (!retryCache.has(thenable)) {
1321
+ if (enableSchedulerTracing) {
1322
+ retry = Schedule_tracing_wrap(retry);
1323
+ }
1324
retryCache.add(thenable);
1325
thenable.then(retry, retry);
1326
}
packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
+75
@@ -2,6 +2,7 @@ let React;
2
let ReactTestRenderer;
3
let ReactFeatureFlags;
4
let Scheduler;
5
+let SchedulerTracing;
6
let ReactCache;
7
let Suspense;
8
let act;
@@ -17,10 +18,12 @@ describe('ReactSuspense', () => {
18
ReactFeatureFlags = require('shared/ReactFeatureFlags');
19
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
20
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
21
+ ReactFeatureFlags.enableSchedulerTracing = true;
22
React = require('react');
23
ReactTestRenderer = require('react-test-renderer');
24
act = ReactTestRenderer.act;
25
Scheduler = require('scheduler');
26
+ SchedulerTracing = require('scheduler/tracing');
27
ReactCache = require('react-cache');
28
29
Suspense = React.Suspense;
@@ -914,6 +917,78 @@ describe('ReactSuspense', () => {
917
]);
918
});
919
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
+
992
it('#14162', () => {
993
const {lazy} = React;
994
packages/react/src/__tests__/ReactProfiler-test.internal.js
+5
-1
@@ -2607,7 +2607,11 @@ describe('Profiler', () => {
2607
]);
2608
onRender.mockClear();
2609
2610
- expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled();
2610
+ expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1);
2611
+ expect(
2612
+ onInteractionScheduledWorkCompleted.mock.calls[0][0],
2613
+ ).toMatchInteraction(highPriUpdateInteraction);
2614
+ onInteractionScheduledWorkCompleted.mockClear();
2615
2616
Scheduler.advanceTime(100);
2617
jest.advanceTimersByTime(100);