OverlayMixin causes a memory leak

[Sciurus7]

Reproduction Recipe

• Go to a page containing a component using the OverlayMixin that can be removed. (See the example I created for a very basic component containing the OverlayMixin.)
• Check and uncheck the checkbox. That'll create and remove a simple component using the overlay.
• In the Chrome devtools, go to the memory tab.
• Do a GC (the broomstick icon) just to be sure.
• Take a heap snapshot
• In the list, find the entry for "Detached HTMLElement". That'll show you the elements that are not part of the DOM any more, but cannot be garbage collected.

Expected behavior

There are no detached HTMLElements

Actual Behavior

The component with the overlay-mixin can not be garbage collected.

Additional context

This bug has also been tested and reproduced with the LionSelectRich.
Based on research this bug is partially caused by the OverlayManager, which adds every OverlayController to a list, but never removes it.
The OverlayController should probably be removed from the list during the disconnectedCallback of the OverlayMixin. Unfortunately even when taking this step the memory leak persists so there must be another cause for the memory leak.

Lion version

0.7.3

Code base example

https://github.com/Sciurus7/lion-overlay-mixin-memory-leak

Additional research notes

The memory leak definitely seems to be linked to the OverlaysManager. When setting this.manager to null in the OverlayController constructor the memory leak will disappear. Some data collected during the investigation.

While the OverlayConstructor will add itself to a list in the OverlaysManager during its initialization, it never removes itself of this list while it is being removed. This can be remedied by removing the OverlayController from that list when it’s torndown. This should be done done in the OverlayMixin:

/** @protected */
_teardownOverlayCtrl() {
   this._teardownOpenCloseListeners();
   this.__teardownSyncFromOverlayController();
   /** @type {OverlayController} */
   (this._overlayCtrl).teardown();
   this._overlayCtrl.manager.remove(this._overlayCtrl); // To be added in a Lion's code
}

Even after the above change the memory leak persists. This happens even when the OverlayManager is basically an empty class with just empty methods and no props.

During a simple lifecycle of a component with an OverlayMixin where it is just created and removed he following methods are called on the manager:

• add
• hide
• hide
• remove

[riovir]

The memory leak has more than one problem areas. Indeed, the extra line in the OverlayController is necessary to fix the issue. Another reason why the overlay components can't be garbage collected is here: https://github.com/ing-bank/lion/blob/master/packages/ui/components/overlays/src/utils/adopt-styles.js#L82

With minimal edits we could use a WeakMap for caching styles

• const styleCache = new WeakMap();
• then account for the undefined key: if (renderRoot && !styleCache.has(renderRoot)) { before using the renderRoot as a key
• and fall back to an empty array when looking up values: const addedStylesForRoot = styleCache.get(renderRoot) ?? [];

[riovir]

Unfortunately, the fix is not so simple:
https://github.com/ing-bank/lion/actions/runs/9468011350/job/26083401539?pr=2302

Some of the errors can easily be fixed by checking if the manager has the controller to begin with. Still, 2 errors remain with that approach:

 ❌ lion-dialog > Integration tests > OverlayMixin for lion-dialog nested > [global] allows for moving of the element
      AssertionError: expected 0 to equal 1
      + expected - actual

      -0
      +1

      at n.<anonymous> (packages\ui\components\overlays\test-suites\OverlayMixin.suite.js:391:51)

 ❌ lion-dialog > Integration tests > OverlayMixin for lion-dialog nested > reconstructs the overlay when disconnected and reconnected to DOM (support for nested overlay nodes)
      AssertionError: expected 1 to equal 2
      + expected - actual

      -1
      +2

[riovir]

I've looked into using a WeakSet instead of an array in the OverlayManager, but at some point the teardown tries to iterate through it. Unfortunately that doesn't play well with a WeakSet as it's not iterable.

[riovir]

Got it working: #2302
The OverlayController needs to be reconnected to the OverlayManager when the OverlayMixin is reconnected. I tested the different fix, and it would still solve the memory leak.