[BUG] Memory leak via anchorsBySelect

[ivan-palatov]

Bug description
The Tooltip component accumulates anchors to elements in anchorsBySelect state without ever releasing old, already unused elements. It causes applications to have massive memory leaks and Detached elements in memory, which eventualy leads to the death of the app.

Version of Package
v5.21.5

To Reproduce
Sorry, I dont have time to setup a proper bench for it, but this should suffice.

1. open codesandbox
2. focus the window on the right and hold the right arrow for a bit
3. open dev tools and make a memory snapshot. See lots of detached nodes
4. repeat steps 2-3 and see the detached nodes increase (not all of those are because of react-tooltip, but many are. Some are from devtools, some from codesandbox dev env)

Expected behavior
Detached nodes should not be increasing. anchorsBySelect should not be updated with the old + new, old values must be filtered first.

Screenshots
The screenshot from our app in production using react-tooltip. If we remore <Tooltip /> from the root of the app, those detached nodes are gone.

Additional context
The problem happens because you are not filtering out the old anchorsBySelect state and just add the new elements on top of it here

So those elements are never actualy released from memory and always hang there, causing massive memory leaks.

To fix this either make use of the removedNodes from MutationObserver like so

    const documentObserverCallback: MutationCallback = (mutationList) => {
      const oldAnchors: HTMLElement[] = [];
      const newAnchors: HTMLElement[] = [];
      for (const mutation of mutationList) {
        if (
          mutation.type === "attributes" &&
          mutation.attributeName === "data-tooltip-id"
        ) {
          const newId = (mutation.target as HTMLElement).getAttribute(
            "data-tooltip-id"
          );

          if (newId === id) {
            newAnchors.push(mutation.target as HTMLElement);
          }
        }

        if (mutation.type !== "childList") {
          continue;
        }

        for (const node of Array.from(mutation.removedNodes)) {
          oldAnchors.push(
            node as HTMLElement,
            // eslint-disable-next-line unicorn/prefer-spread
            ...(Array.from(
              (node as HTMLElement).querySelectorAll(selector)
            ) as HTMLElement[])
          );

          if (!activeAnchor || !node?.contains?.(activeAnchor)) {
            continue;
          }

          setRendered(false);
          handleShow(false);
          setActiveAnchor(null);

          if (tooltipShowDelayTimerRef.current) {
            clearTimeout(tooltipShowDelayTimerRef.current);
          }

          if (tooltipHideDelayTimerRef.current) {
            clearTimeout(tooltipHideDelayTimerRef.current);
          }
        }

        if (!selector) {
          continue;
        }

        const elements = Array.from(mutation.addedNodes).filter(
          (node) => node.nodeType === 1
        );
        newAnchors.push(
          // the element itself is an anchor
          ...(elements.filter((element) =>
            (element as HTMLElement).matches(selector)
          ) as HTMLElement[]),
          // the element has children which are anchors
          ...elements.flatMap(
            (element) =>
              // eslint-disable-next-line unicorn/prefer-spread
              Array.from(
                (element as HTMLElement).querySelectorAll(selector)
              ) as HTMLElement[]
          )
        );
      }

      if (newAnchors.length > 0 || oldAnchors.length > 0) {
        setAnchorsBySelect((anchors) => [
          ...anchors.filter((anchor) => !oldAnchors.includes(anchor)),
          ...newAnchors,
        ]);
      }
    };

Or just do a document.querySelectorAll(selector) on mutation, which, tbh, could be faster than those iterations, transforms and array merges.

[gabrieljablonski]

Hi @ivan-palatov.

Thanks for reporting it. We already have a fix for this in #1091, but we're probably not merging the new prop the PR introduces, it was something we we're just testing out.

We'll clean up the PR and merge the fix for this soon.

Thanks again.

[ivan-palatov]

@gabrieljablonski It still leaves a smaller leak, I've made a review comment showing it in the pr.

[ivan-palatov]

Also, I believe there might be a possible leak here. The tooltipRef.current might be pointing to a different element in the cleanup function already.

I might be wrong on this one, but I think the code should be changed like this:

useEffect(() => {
    // rest of useEffect ...
 
    // Notice this line here
    const tooltip = tooltipRef.current;

    return () => {
      if (closeOnScroll) {
        window.removeEventListener("scroll", handleScrollResize);
        anchorScrollParent?.removeEventListener("scroll", handleScrollResize);
        tooltipScrollParent?.removeEventListener("scroll", handleScrollResize);
      }

      if (closeOnResize) {
        window.removeEventListener("resize", handleScrollResize);
      } else {
        updateTooltipCleanup?.();
      }

      if (shouldOpenOnClick) {
        window.removeEventListener("click", handleClickOutsideAnchors);
      }

      if (closeOnEsc) {
        window.removeEventListener("keydown", handleEsc);
      }

      if (clickable && !shouldOpenOnClick) {
        tooltip?.removeEventListener("mouseenter", handleMouseEnterTooltip);
        tooltip?.removeEventListener("mouseleave", handleMouseLeaveTooltip);
      }

      for (const { event, listener } of enabledEvents) {
        for (const ref of elementRefs) {
          ref.removeEventListener(event, listener);
        }
      }

      enabledEvents.forEach(({ event, listener }) => {
        elementRefs.forEach((ref) => {
          ref.current?.removeEventListener(event, listener)
        })
      })
    };
    /**
     * rendered is also a dependency to ensure anchor observers are re-registered
     * since `tooltipRef` becomes stale after removing/adding the tooltip to the DOM
     */
  }, [
    activeAnchor,
    updateTooltipPosition,
    anchorRefs,
    rendered,
    anchorsBySelect,
    closeOnEsc,
    events,
  ]);

[gabrieljablonski]

It still leaves a smaller leak, I've made a review comment showing it in the pr.

Good catch, will update it.

The tooltipRef.current might be pointing to a different element in the cleanup function already.

I'm like 99% sure this wouldn't leak the listeners, since the ref is being set through the React ref prop. This means that if the ref changed, the HTML element necessarily got unmounted, which removes all attached listeners by garbage collection (more info).

Too lazy to test this specifically (it would probably take a bit of work to isolate it since the leak would be veeery minor). I'll probably just do the const tooltip = ..., since it definitely can't hurt.

---

I'll have the time to properly review the PR and test it out only next week. If this is affecting your app badly enough, let us know so we can release a usable beta version. That way you can keep working until the official release with the fix.

[ivan-palatov]

We have temporarily removed react-tooltip from our app for now. Would add it back in again when the update is ready. The reason I started digging and found the issue is because it was making the app lag really hard after ~100 "page" changes. Tbh, there weren't that many data-tooltip-id elements per page, but those add up really fast once you add things like virtualized tables.

About the ref thing I think the same, but my eslint was complaining about it, so I thought it was worth pointing it out. It wouldn't hurt either way.

[gabrieljablonski]

Fixed on react-tooltip@5.21.6