fix(overlays): fix memory leaks of adopt-styles and OverlayMixin

* fix(overlays): fix memory leaks of adopt-styles and OverlayMixin * fix(overlays): support reconnecting the OverlayController to the OverlayManager * chore: move unregister logic to OverlayController --------- Co-authored-by: Thijs Louisse <[email protected]>
ing-bank · Nov 12, 2024 · 86ca2e0 · 86ca2e0
1 parent 2a989f4
commit 86ca2e0
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 10 deletions.
diff --git a/.changeset/quiet-apricots-sneeze.md b/.changeset/quiet-apricots-sneeze.md
@@ -0,0 +1,5 @@
+---
+'@lion/ui': patch
+---
+
+[overlays] Fixed memory leak caused adopted style cache and the `OverlayMixin` not releasing the `OverlayController` on teardown
diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js
@@ -192,7 +192,6 @@ export class OverlayController extends EventTarget {
       zIndex: 9999,
     };
 
-    this.manager.add(this);
     /** @protected */
     this._contentId = `overlay-content--${Math.random().toString(36).slice(2, 10)}`;
     /** @private */
@@ -475,6 +474,14 @@ export class OverlayController extends EventTarget {
     this._init();
     /** @private */
     this.__elementToFocusAfterHide = undefined;
+
+    if (!this.#isRegisteredOnManager()) {
+      this.manager.add(this);
+    }
+  }
+
+  #isRegisteredOnManager() {
+    return Boolean(this.manager.list.find(ctrl => this === ctrl));
   }
 
   /**
@@ -1354,6 +1361,10 @@ export class OverlayController extends EventTarget {
     this.__handleOverlayStyles({ phase: 'teardown' });
     this._handleFeatures({ phase: 'teardown' });
     this.__wrappingDialogNode?.removeEventListener('cancel', this.__cancelHandler);
+
+    if (this.#isRegisteredOnManager()) {
+      this.manager.remove(this);
+    }
   }
 
   /** @private */

diff --git a/packages/ui/components/overlays/src/OverlayMixin.js b/packages/ui/components/overlays/src/OverlayMixin.js
@@ -182,11 +182,13 @@ export const OverlayMixinImplementation = superclass =>
       this._setupOverlayCtrl();
     }
 
-    disconnectedCallback() {
+    async disconnectedCallback() {
       super.disconnectedCallback();
-      if (this._overlayCtrl) {
-        this._teardownOverlayCtrl();
-      }
+
+      if (!this._overlayCtrl) return;
+      if (await this.#isMovingInDom()) return;
+
+      this._teardownOverlayCtrl();
     }
 
     /**
@@ -253,6 +255,7 @@ export const OverlayMixinImplementation = superclass =>
     _teardownOverlayCtrl() {
       this._teardownOpenCloseListeners();
       this.__teardownSyncFromOverlayController();
+
       /** @type {OverlayController} */
       (this._overlayCtrl).teardown();
     }
@@ -387,5 +390,15 @@ export const OverlayMixinImplementation = superclass =>
         ctrl._popper.update();
       }
     }
+
+    /**
+     * When we're moving around in dom, disconnectedCallback gets called.
+     * Before we decide to teardown, let's wait to see if we were not just moving nodes around.
+     * @return {Promise<boolean>}
+     */
+    async #isMovingInDom() {
+      await this.updateComplete;
+      return this.isConnected;
+    }
   };
 export const OverlayMixin = dedupeMixin(OverlayMixinImplementation);
diff --git a/packages/ui/components/overlays/src/utils/adopt-styles.js b/packages/ui/components/overlays/src/utils/adopt-styles.js
@@ -25,7 +25,7 @@ export const _adoptStyleUtils = {
   adoptStyles: undefined,
 };
 
-const styleCache = new Map();
+const styleCache = new WeakMap();
 
 /**
  * @param {CSSStyleSheet} cssStyleSheet
@@ -81,10 +81,10 @@ function adoptStyleWhenAdoptedStylesheetsNotSupported(
  */
 function handleCache(renderRoot, style, { teardown = false } = {}) {
   let haltFurtherExecution = false;
-  if (!styleCache.has(renderRoot)) {
+  if (renderRoot && !styleCache.has(renderRoot)) {
     styleCache.set(renderRoot, []);
   }
-  const addedStylesForRoot = styleCache.get(renderRoot);
+  const addedStylesForRoot = styleCache.get(renderRoot) ?? [];
   const foundStyle = addedStylesForRoot.find(
     (/** @type {import("lit").CSSResultOrNative} */ addedStyle) => style === addedStyle,
   );

diff --git a/packages/ui/components/overlays/test/OverlayController.test.js b/packages/ui/components/overlays/test/OverlayController.test.js
@@ -36,6 +36,14 @@ async function mimicEscapePress(element) {
   await aTimeout(0);
 }
 
+/**
+ * @param {OverlayController} ctrlToFind
+ * @returns {boolean}
+ */
+function isRegisteredOnManager(ctrlToFind) {
+  return Boolean(ctrlToFind.manager?.list?.find(ctrl => ctrlToFind === ctrl));
+}
+
 /**
  * Make sure that all browsers serialize html in a similar way
  * (Firefox tends to output empty style attrs)
@@ -275,8 +283,18 @@ describe('OverlayController', () => {
     });
   });
 
-  // TODO: Add teardown feature tests
-  describe('Teardown', () => {});
+  // TODO: Add more teardown feature tests
+  describe('Teardown', () => {
+    it('unregisters itself from overlayManager', async () => {
+      const ctrl = new OverlayController(withGlobalTestConfig());
+
+      expect(isRegisteredOnManager(ctrl)).to.be.true;
+
+      ctrl.teardown();
+
+      expect(isRegisteredOnManager(ctrl)).to.be.false;
+    });
+  });
 
   describe('Node Configuration', () => {
     describe('Content', async () => {