[ButtonBase] Fix potential memory leak for multi-touch devices (#19333)

mui · Jan 21, 2020 · 2082679 · 2082679
1 parent a9a5f9f
commit 2082679
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 12 deletions.
diff --git a/packages/material-ui/src/ButtonBase/TouchRipple.js b/packages/material-ui/src/ButtonBase/TouchRipple.js
@@ -207,17 +207,22 @@ const TouchRipple = React.forwardRef(function TouchRipple(props, ref) {
 
       // Touche devices
       if (event.touches) {
-        // Prepare the ripple effect.
-        startTimerCommit.current = () => {
-          startCommit({ pulsate, rippleX, rippleY, rippleSize, cb });
-        };
-        // Delay the execution of the ripple effect.
-        startTimer.current = setTimeout(() => {
-          if (startTimerCommit.current) {
-            startTimerCommit.current();
-            startTimerCommit.current = null;
-          }
-        }, DELAY_RIPPLE); // We have to make a tradeoff with this value.
+        // check that this isn't another touchstart due to multitouch
+        // otherwise we will only clear a single timer when unmounting while two
+        // are running
+        if (startTimerCommit.current === null) {
+          // Prepare the ripple effect.
+          startTimerCommit.current = () => {
+            startCommit({ pulsate, rippleX, rippleY, rippleSize, cb });
+          };
+          // Delay the execution of the ripple effect.
+          startTimer.current = setTimeout(() => {
+            if (startTimerCommit.current) {
+              startTimerCommit.current();
+              startTimerCommit.current = null;
+            }
+          }, DELAY_RIPPLE); // We have to make a tradeoff with this value.
+        }
       } else {
         startCommit({ pulsate, rippleX, rippleY, rippleSize, cb });
       }

diff --git a/packages/material-ui/src/ButtonBase/TouchRipple.test.js b/packages/material-ui/src/ButtonBase/TouchRipple.test.js
@@ -18,7 +18,7 @@ describe('<TouchRipple />', () => {
    */
   function renderTouchRipple(other) {
     const touchRippleRef = React.createRef();
-    const { container } = render(
+    const { container, unmount } = render(
       <TouchRipple
         ref={touchRippleRef}
         classes={{
@@ -42,6 +42,7 @@ describe('<TouchRipple />', () => {
       queryRipple() {
         return container.querySelector('.ripple');
       },
+      unmount,
     };
   }
 
@@ -155,6 +156,9 @@ describe('<TouchRipple />', () => {
   });
 
   describe('mobile', () => {
+    /**
+     * @type {ReturnType<typeof useFakeTimers>}
+     */
     let clock;
 
     before(() => {
@@ -227,5 +231,17 @@ describe('<TouchRipple />', () => {
       expect(queryAllActiveRipples()).to.have.lengthOf(0);
       expect(queryAllStoppingRipples()).to.have.lengthOf(0);
     });
+
+    it('should not leak on multi-touch', function multiTouchTest() {
+      const { instance, unmount } = renderTouchRipple();
+
+      instance.start({ type: 'touchstart', touches: [{}] }, () => {});
+      instance.start({ type: 'touchstart', touches: [{}] }, () => {});
+      unmount();
+
+      // expect this to run gracefully without
+      // "react state update on an unmounted component"
+      clock.runAll();
+    });
   });
 });