[react-interactions] Fix memory leak in event responder system (#17421)
Dominic Gannaway committed
Nov 21, 2019 at 13:15 UTC
007a276b6541f98318adf5d37668fceb5ef37223
3 files changed
+73
-20
packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js
+48
@@ -72,6 +72,7 @@ describe('DOMEventResponderSystem', () => {
72
jest.resetModules();
73
ReactFeatureFlags = require('shared/ReactFeatureFlags');
74
ReactFeatureFlags.enableFlareAPI = true;
75
+ ReactFeatureFlags.enableScopeAPI = true;
76
React = require('react');
77
ReactDOM = require('react-dom');
78
ReactDOMServer = require('react-dom/server');
@@ -552,6 +553,53 @@ describe('DOMEventResponderSystem', () => {
553
expect(onUnmountFired).toEqual(4);
554
});
555
556
+ it('the event responder onUnmount() function should fire using scopes', () => {
557
+ let onUnmountFired = 0;
558
+
559
+ const TestScope = React.unstable_createScope();
560
+ const TestResponder = createEventResponder({
561
+ targetEventTypes: [],
562
+ onUnmount: () => {
563
+ onUnmountFired++;
564
+ },
565
+ });
566
+
567
+ function Test({test}) {
568
+ const listener = React.unstable_useResponder(TestResponder, {});
569
+ if (test === 0) {
570
+ return <TestScope DEPRECATED_flareListeners={[listener]} />;
571
+ } else if (test === 1) {
572
+ return <TestScope DEPRECATED_flareListeners={null} />;
573
+ } else if (test === 2) {
574
+ return <TestScope DEPRECATED_flareListeners={[]} />;
575
+ } else if (test === 3) {
576
+ return <TestScope />;
577
+ } else if (test === 4) {
578
+ return <TestScope DEPRECATED_flareListeners={listener} />;
579
+ }
580
+ }
581
+
582
+ ReactDOM.render(<Test test={0} />, container);
583
+ ReactDOM.render(null, container);
584
+ expect(onUnmountFired).toEqual(1);
585
+
586
+ ReactDOM.render(<Test test={0} />, container);
587
+ ReactDOM.render(<Test test={1} />, container);
588
+ expect(onUnmountFired).toEqual(2);
589
+
590
+ ReactDOM.render(<Test test={0} />, container);
591
+ ReactDOM.render(<Test test={2} />, container);
592
+ expect(onUnmountFired).toEqual(3);
593
+
594
+ ReactDOM.render(<Test test={0} />, container);
595
+ ReactDOM.render(<Test test={3} />, container);
596
+ expect(onUnmountFired).toEqual(4);
597
+
598
+ ReactDOM.render(<Test test={0} />, container);
599
+ ReactDOM.render(<Test test={4} />, container);
600
+ expect(onUnmountFired).toEqual(4);
601
+ });
602
+
603
it('the event responder onUnmount() function should fire with state', () => {
604
let counter = 0;
605
packages/react-reconciler/src/ReactFiberCommitWork.js
+8
-19
@@ -100,7 +100,6 @@ import {
100
hideTextInstance,
101
unhideInstance,
102
unhideTextInstance,
103
- unmountResponderInstance,
103
unmountFundamentalComponent,
104
updateFundamentalComponent,
105
commitHydratedContainer,
@@ -124,7 +123,10 @@ import {
123
} from './ReactHookEffectTags';
124
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
125
import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
127
-import {updateLegacyEventListeners} from './ReactFiberEvents';
126
+import {
127
+ updateLegacyEventListeners,
128
+ unmountResponderListeners,
129
+} from './ReactFiberEvents';
130
131
let didWarnAboutUndefinedSnapshotBeforeUpdate: Set<mixed> | null = null;
132
if (__DEV__) {
@@ -792,23 +794,7 @@ function commitUnmount(
794
}
795
case HostComponent: {
796
if (enableFlareAPI) {
795
- const dependencies = current.dependencies;
796
-
797
- if (dependencies !== null) {
798
- const respondersMap = dependencies.responders;
799
- if (respondersMap !== null) {
800
- const responderInstances = Array.from(respondersMap.values());
801
- for (
802
- let i = 0, length = responderInstances.length;
803
- i < length;
804
- i++
805
- ) {
806
- const responderInstance = responderInstances[i];
807
- unmountResponderInstance(responderInstance);
808
- }
809
- dependencies.responders = null;
810
- }
811
- }
797
+ unmountResponderListeners(current);
798
beforeRemoveInstance(current.stateNode);
799
}
800
safelyDetachRef(current);
@@ -848,6 +834,9 @@ function commitUnmount(
834
return;
835
}
836
case ScopeComponent: {
837
+ if (enableFlareAPI) {
838
+ unmountResponderListeners(current);
839
+ }
840
if (enableScopeAPI) {
841
safelyDetachRef(current);
842
}
packages/react-reconciler/src/ReactFiberEvents.js
+17
-1
@@ -163,7 +163,7 @@ export function updateLegacyEventListeners(
163
}
164
let respondersMap = dependencies.responders;
165
if (respondersMap === null) {
166
- respondersMap = new Map();
166
+ dependencies.responders = respondersMap = new Map();
167
}
168
if (isArray(listeners)) {
169
for (let i = 0, length = listeners.length; i < length; i++) {
@@ -218,3 +218,19 @@ export function createResponderListener(
218
}
219
return eventResponderListener;
220
}
221
+
222
+export function unmountResponderListeners(fiber: Fiber) {
223
+ const dependencies = fiber.dependencies;
224
+
225
+ if (dependencies !== null) {
226
+ const respondersMap = dependencies.responders;
227
+ if (respondersMap !== null) {
228
+ const responderInstances = Array.from(respondersMap.values());
229
+ for (let i = 0, length = responderInstances.length; i < length; i++) {
230
+ const responderInstance = responderInstances[i];
231
+ unmountResponderInstance(responderInstance);
232
+ }
233
+ dependencies.responders = null;
234
+ }
235
+ }
236
+}