The possibility of memory leak

[ojj1123]

Problem

After merging this PR https://github.com/toss/overlay-kit/pull/33, I observed that it is possible to happen the memory leak.

The repro on this issue. You can check the memory profiling. https://github.com/ojj1123/overlay-kit-memory-leak

The points of the repro is below:

1. SPA (using react-router-dom)
2. I didn't call unmount callback and didn't set the custom overlayId.

Every calling overlay.open() w/o the custom overlayId, the new random id is generated by randomId() and then the overlayItem with that id is stored to overlayData. Even if unmounting OverlayProvider, the overlayItems are not GC'd. I think that the problem is not to call unmount. In other word, the users of overlay-kit need to use unmount callback for preventing memory leak. And as SPA, there are no refresh or reload so the overlay state is not refreshed.

When close is executed, the overlay changes isOpen to false, making it disappear from the screen, but it remains in memory and the DOM. Additionally, I assumed that executing close without unmount means the user is aware of this and intentionally keeps the overlay in memory. As a result, close does not modify current. https://github.com/toss/overlay-kit/issues/44#issuecomment-2220294751

You said the users intent to remain the overlays in memory, but without the custom id, the same overlay is never re-used since every calling overlay.open, the new overlay is generated. So I'm wondering what's the advantage of storing the overlay states.

Workarounds

I came up with some workaround, but I didn't try that yet.

Clean-up

As useOverlay of slash, it has clean-up function to unmount overlay. Like this, overlay-kit need to clean up the overlay state.

WeakMap

If you want to cache the state, we can use WeakMap. So you change overlayData from object to WeakMap setting the keys to overlayItem:

export type OverlayData = {
  current: OverlayId | null;
  overlayOrderList: OverlayId[];
  overlayData: WeakMap<OverlayItem, any>;
};

If no refer to OverlayItem, that overlayItem will be GC'd. But as the situation below, maybe that are not GC'd:

1. In OverlayProvider, all of overlayItem are already referred.
2. If the users use overlayData using useOverlayData, it can't be GC'd.

Revert https://github.com/toss/overlay-kit/pull/33

As https://github.com/toss/overlay-kit/pull/33, you changed the management of the overlay state from React's internal state(useReducer) to external state(useSyncExternalStore). So even if OverlayProvider is unmounted, the overlay state is not cleared. Therefore, if you manage it as React's internal state, you will be able to initialize the state when OverlayProvider is unmounted.

Docs

You can just let the users know you need to call unmount in the Document. But if the user don't take care of it, the memory leak occurs. So I think that it is good for this library to provider unmounting overlay properly.

Other bug

Making the repro, I found other bug:

If unmounting OverlayProvider and mounting OverlayProvider again, all of isOpen set to true. This is because when mounting OverlayProvider, effect function is called in ContentOverlayController. You can see this bug in my repro.

https://github.com/toss/overlay-kit/blob/7ca508795c95d8679860fe6336ca9ea3e9d22646/packages/src/context/provider.tsx#L64-L66

[ojj1123]

cc. @manudeli

[jungpaeng]

Thank you for your comments.

I’d like to share some thoughts on the points you raised regarding the potential memory leak when not specifying a customOverlayId and the subsequent random generation of overlayId when overlays are closed without unmounting.

While it’s true that not specifying a customOverlayId can potentially lead to memory leaks if overlays are not reused properly, there is still a way to reuse overlays even without a custom ID. The overlay.open() function returns the ID of the overlay it opens. This ID can be stored in the state and reused; if the ID doesn’t exist, a new one is generated, and if it does exist, it’s used to reopen the overlay.

Here’s a simple example to illustrate this:

function DemoWithEsOverlay() {
  const current = useCurrentOverlay();
  const [overlayId, setOverlayId] = useState<string>();

  return (
    <div>
      <p>current: {current}</p>
      <button
        onClick={() => {
          setOverlayId(
            overlay.open(
              ({ isOpen, close }) => {
                return <CustomModal isOpen={isOpen} onClose={close} />;
              },
              { overlayId }
            )
          );
        }}
      >
        open modal count
      </button>
    </div>
  );
}

https://github.com/user-attachments/assets/12c0d3eb-ba6d-466e-8215-60fa6aacf24e

Thus, if it’s necessary to unmount the overlay when the component that called it is unmounted, you simply unmount the overlay using the list of overlay IDs you’ve collected.

You also mentioned that the state of the overlay does not reset when the OverlayProvider is unmounted. This is correct; the state remains external and is not cleared on unmount because it’s stored outside the React component state.

However, as indicated in the documentation, OverlayProvider is intended to be placed at the very top of the service components and operate as a singleton within a service. The unmounting of OverlayProvider should coincide with the termination of the service. This design intends for only one OverlayProvider per service.

Could you elaborate on the scenarios where you foresee needing to unmount OverlayProvider?

[jungpaeng]

Fixed it #58

While preparing the example, I discovered a hidden bug.

https://github.com/user-attachments/assets/69f2d9ac-5fe4-40e6-b338-0bb6ffe79225

I believe that when an overlay is closed without unmounting and then opened again, it should function as shown in the video above.

https://github.com/user-attachments/assets/70bc56a7-3d71-4b65-b25a-9a2f8e7128d5

However, even though the overlay is not removed from the DOM tree when closed, the component resets when reopened. This behavior is unintentional and appears to be a bug that needs to be addressed.

[ojj1123]

While it’s true that not specifying a customOverlayId can potentially lead to memory leaks if overlays are not reused properly, there is still a way to reuse overlays even without a custom ID. The overlay.open() function returns the ID of the overlay it opens.

I just thought that it's good for this library to unmount the unused overlay. I used to close the overlays without unmount as I didn't re-use the overlays.

Could you elaborate on the scenarios where you foresee needing to unmount OverlayProvider?

I don't think it's necessary to unmount OverlayProvider. I just told you that if the users don't re-use the overlay and don't call unmount callback, overlay.unmount() or overlay.unmountAll(), overlayItems don't be removed although unmounting OverlayProvider. So It is possible to lead to memory leak.

[ojj1123]

The unmounting of OverlayProvider should coincide with the termination of the service. This design intends for only one OverlayProvider per service.

If you want the users to unmount the overlays manually and to make OverlayProvider as a singleton, it's okay to close this issue!

[jungpaeng]

if the users don't re-use the overlay and don't call unmount callback, overlay.unmount() or overlay.unmountAll(), overlayItems don't be removed although unmounting OverlayProvider. So It is possible to lead to memory leak.

Yes, in such cases, memory can be used unnecessarily.

It would be good to elaborate on this context more in the documentation. Thank you. 🙇

[jungpaeng]

I have noted that there is a warning about the potential for memory leaks when unmount is not used.

Link: https://overlay-kit.slash.page/usage/overlay.html#closing-the-overlay