[react-interactions] Fix memory leak in event responder system (#17421)

facebook · Nov 21, 2019 · 007a276 · 007a276
1 parent 3fdfa23
commit 007a276
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 20 deletions.
diff --git a/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js
@@ -72,6 +72,7 @@ describe('DOMEventResponderSystem', () => {
     jest.resetModules();
     ReactFeatureFlags = require('shared/ReactFeatureFlags');
     ReactFeatureFlags.enableFlareAPI = true;
+    ReactFeatureFlags.enableScopeAPI = true;
     React = require('react');
     ReactDOM = require('react-dom');
     ReactDOMServer = require('react-dom/server');
@@ -552,6 +553,53 @@ describe('DOMEventResponderSystem', () => {
     expect(onUnmountFired).toEqual(4);
   });
 
+  it('the event responder onUnmount() function should fire using scopes', () => {
+    let onUnmountFired = 0;
+
+    const TestScope = React.unstable_createScope();
+    const TestResponder = createEventResponder({
+      targetEventTypes: [],
+      onUnmount: () => {
+        onUnmountFired++;
+      },
+    });
+
+    function Test({test}) {
+      const listener = React.unstable_useResponder(TestResponder, {});
+      if (test === 0) {
+        return <TestScope DEPRECATED_flareListeners={[listener]} />;
+      } else if (test === 1) {
+        return <TestScope DEPRECATED_flareListeners={null} />;
+      } else if (test === 2) {
+        return <TestScope DEPRECATED_flareListeners={[]} />;
+      } else if (test === 3) {
+        return <TestScope />;
+      } else if (test === 4) {
+        return <TestScope DEPRECATED_flareListeners={listener} />;
+      }
+    }
+
+    ReactDOM.render(<Test test={0} />, container);
+    ReactDOM.render(null, container);
+    expect(onUnmountFired).toEqual(1);
+
+    ReactDOM.render(<Test test={0} />, container);
+    ReactDOM.render(<Test test={1} />, container);
+    expect(onUnmountFired).toEqual(2);
+
+    ReactDOM.render(<Test test={0} />, container);
+    ReactDOM.render(<Test test={2} />, container);
+    expect(onUnmountFired).toEqual(3);
+
+    ReactDOM.render(<Test test={0} />, container);
+    ReactDOM.render(<Test test={3} />, container);
+    expect(onUnmountFired).toEqual(4);
+
+    ReactDOM.render(<Test test={0} />, container);
+    ReactDOM.render(<Test test={4} />, container);
+    expect(onUnmountFired).toEqual(4);
+  });
+
   it('the event responder onUnmount() function should fire with state', () => {
     let counter = 0;
 

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -100,7 +100,6 @@ import {
   hideTextInstance,
   unhideInstance,
   unhideTextInstance,
-  unmountResponderInstance,
   unmountFundamentalComponent,
   updateFundamentalComponent,
   commitHydratedContainer,
@@ -124,7 +123,10 @@ import {
 } from './ReactHookEffectTags';
 import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
 import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
-import {updateLegacyEventListeners} from './ReactFiberEvents';
+import {
+  updateLegacyEventListeners,
+  unmountResponderListeners,
+} from './ReactFiberEvents';
 
 let didWarnAboutUndefinedSnapshotBeforeUpdate: Set<mixed> | null = null;
 if (__DEV__) {
@@ -792,23 +794,7 @@ function commitUnmount(
     }
     case HostComponent: {
       if (enableFlareAPI) {
-        const dependencies = current.dependencies;
-
-        if (dependencies !== null) {
-          const respondersMap = dependencies.responders;
-          if (respondersMap !== null) {
-            const responderInstances = Array.from(respondersMap.values());
-            for (
-              let i = 0, length = responderInstances.length;
-              i < length;
-              i++
-            ) {
-              const responderInstance = responderInstances[i];
-              unmountResponderInstance(responderInstance);
-            }
-            dependencies.responders = null;
-          }
-        }
+        unmountResponderListeners(current);
         beforeRemoveInstance(current.stateNode);
       }
       safelyDetachRef(current);
@@ -848,6 +834,9 @@ function commitUnmount(
       return;
     }
     case ScopeComponent: {
+      if (enableFlareAPI) {
+        unmountResponderListeners(current);
+      }
       if (enableScopeAPI) {
         safelyDetachRef(current);
       }

diff --git a/packages/react-reconciler/src/ReactFiberEvents.js b/packages/react-reconciler/src/ReactFiberEvents.js
@@ -163,7 +163,7 @@ export function updateLegacyEventListeners(
     }
     let respondersMap = dependencies.responders;
     if (respondersMap === null) {
-      respondersMap = new Map();
+      dependencies.responders = respondersMap = new Map();
     }
     if (isArray(listeners)) {
       for (let i = 0, length = listeners.length; i < length; i++) {
@@ -218,3 +218,19 @@ export function createResponderListener(
   }
   return eventResponderListener;
 }
+
+export function unmountResponderListeners(fiber: Fiber) {
+  const dependencies = fiber.dependencies;
+
+  if (dependencies !== null) {
+    const respondersMap = dependencies.responders;
+    if (respondersMap !== null) {
+      const responderInstances = Array.from(respondersMap.values());
+      for (let i = 0, length = responderInstances.length; i < length; i++) {
+        const responderInstance = responderInstances[i];
+        unmountResponderInstance(responderInstance);
+      }
+      dependencies.responders = null;
+    }
+  }
+}