Fix memory leaks and failed cleanups in destroyables Mutation Observer

[Codas]

I really do like the Idea of using Mutation Observer for the otherwise errorprone task of cleanup up the tooltipster, Sortable and Tagify instances. I wish I had thought of that when authoring my PR.

However, I have identified some issues with the current implementation that this PR aims to address:

1. Tooltipster cleanup timing
   Tooltipster instances normally to be destroyed before their associated DOM elements are removed. Tooltipster relies on jQuery’s .data attribute to store references to its instances, and these attributes are cleared when elements are removed via jQuery. If the destroy method is called after this happens, Tooltipster logs warnings or even throws errors.
   Additionally, the instance itself is not automatically destroyed, and it retains a reference to the original HTML element. This alone isn’t enough to cause a memory leak, but Tooltipster's internal setInterval that closes over the tooltipster instance creates one.

2. Handling removed nodes in MutationObserver
   The removedNodes property in the MutationObserver callback does not include the node it was registered for (the target). While the current implementation addresses this by observing a parent element (the closest .window-content element), the mutation handler mistakenly tries to use removed nodes as keys to access destroyable items. This approach will never succeed since the observed target is not included in removedNodes.

3. Memory Leaks from unmanaged MutationObservers
   MutationObservers themselves can lead to memory leaks if not explicitly disconnect()ed. They will continue observing their target nodes even after those nodes are removed from the DOM. An explicit call to disconnect is necessary to release connected nodes and prevent memory leaks.

Proposed Fixes

This PR resolves these issues by:

• Ensuring Tooltipster instances are properly destroyed. That part uses some internal details like $(el).data('tooltipster-ns') to access the tooltipster instance, but is unavoidable.
• Correctly managing the mapping and cleanup of destroyable objects.
• Explicitly disconnecting MutationObservers to avoid residual references.

I have verified that after these changes, no memory leaks occur in NPC sheets, item/feat sheets, and character sheets. However, IWR tooltips in the chat log still exhibit memory leaks. This is because chat messages and their tooltips are rendered before being inserted into the popped-out chat log application, complicating the setup of MutationObservers. Addressing this specific issue will require a separate investigation.

[stwlam]

Do you have any documentation showing 3 is true? Everything I could find states they're cleaned up once the observation target is garbage-collected.

[stwlam]

I'm also not clear on what is meant by 2: using the observed nodes as keys was not a mistake.

[Codas]

Do you have any documentation showing 3 is true? Everything I could find states they're cleaned up once the observation target is garbage-collected.

No real documentation, just a closed (because duplicate) issue on the whatwg github whatwg/dom#484 that then links to the issue of being able to disconnect single targets instead of all or nothing: whatwg/dom#126

There are no details or measurements available in these issues unfortunately.

This prompted me to do my own testing though, and from my testing in chrome at least, MutationObserver does indeed keep a strong reference to the html element it is observing.
This is the memory view of a profiling run with the code of this current PR:

And this with the same code base, but just the following part removed:

context.observer.disconnect();

The methodology is the same as in my previous PR: opening, then closing a sheet (character sheet in this example), then running manual garbage collection, repeating a few times.

[Codas]

I'm also not clear on what is meant by 2: using the observed nodes as keys was not a mistake.

The observed nodes will never appear in the removedNodes list. I will try to explain this as best I can with an example.

Example HTML structure: NPC sheet

Consider the following simplified HTML structure for an NPC sheet:

<body>
  <div class="window window-app npc">
    <section class="window-content">
      <form>
        <tagify-tags></tagify-tags>
      </form>
    </section>
  </div>
</body>

In this example, suppose the tagify-tags element is used as the destroyable parameter when calling the observe method. The contentEl variable will point to the <section class="window-content"> element.

For simplicity, let's assume this is the first and only observe call for this character sheet.

Adding to the #destroyables map

The following code (taken from the current implementation) adds the tagify destroyable to the #destroyables map with .window-content as the key (stored as an element, not a string):

const destroyables = this.#destroyables.get(contentEl) ?? [];
destroyables.push(destroyable);
this.#destroyables.set(contentEl, destroyables);

After this, the #destroyables map looks like this:

{ .window-content -> [tagify] }

Observers in use

The system employs two observers:

1. Content-specific mutation observers:

observer.observe(contentEl, DestroyableManager.#OBSERVE_OPTIONS);

This observes the .window-content node and ignores subtree changes, only monitoring direct child changes.

2. Global Observer: Registered in the DestroyablesManager constructor to watch for app root node removal (e.g., when the app is closed):

constructor() {
  this.#bodyObserver = new MutationObserver(this.#onMutate.bind(this));
  this.#bodyObserver.observe(document.body, DestroyableManager.#OBSERVE_OPTIONS);
}

Why cleanup fails

Despite the setup, cleanup fails in both relevant scenarios. Given the following slightly simplified cleanup function:

#onMutate() {
  for (const mutation of mutations) {
    for (const element of mutation.removedNodes) {
      for (const destroyable of this.#destroyables.get(element) ?? []) {
        destroyable.destroy();
      }
    }
  }
}

here is why the cleanup fails:

• Application is closed

  • The global mutation observer detects the .window.window-app.npc node removal.
  • In the #onMutate function, the element variable points to .window.window-app.npc. The #destroyables.get() call returns undefined because the map key is .window-content, not .window.window-app.npc.

• Application content is replaced

  • The .window-content observer detects changes when the form element is removed.
  • The element variable in #onMutate points to the form node. The #destroyables.get('form') call returns undefined because form is not a key in the map.

• Tooltipster Elements are registered

  • For tooltipster elements, contentEl is set to the element tooltipster is registered for, not the closest .window window-app node.
  • This causes the cleanup to fail for similar reasons: the keys in #destroyables do not match the values in the removedNodes, as they will never contain the target - the tooltipster element - itself.

[stwlam]

I think the fix for the body observer is to simply set the keys to the application frame rather than the content-window element.

[Codas]

That should work, yes.
But to still be able to fix the other issues, we would probably need two separate mutation observer handler functions, one for the global body observer and one for the local application content.

[Codas]

This should be better now, using only a single observer to watch for changes of the document.body childList.

[stwlam]

I think you can drop the TooltipsterTarget class entirely now as well.

[Codas]

I think you can drop the TooltipsterTarget class entirely now as well.

I'm not sure about that. I currently check for the inclusion of the html node linked to the destroyable instance in the removed html element. As far as I could tell, the plain Tooltipster instance value keeps no accessible reference to the html node, which makes the class still necessary.

We could as of now probably live without the contains check and just destroy every destroyable in the scope of the application that is being re-rendered, but I'm not sure I like that honestly. The contains check is reasonably cheap, only runs in very specific circumstances and gives some meassure of future proofing against partial html updates. (only thing that would need to change as of now to support this is also watching the subtree in the content mutation observer)

[stwlam]

I'm not sure about that. I currently check for the inclusion of the html node linked to the destroyable instance in the removed html element. As far as I could tell, the plain Tooltipster instance value keeps no accessible reference to the html node, which makes the class still necessary.

Try checking the return of the tooltipster instance's elementOrigin method.

[Codas]

Your are absolutely right, don't know why I missed that. Probably only looked at properties, not functions...

[Codas]

The ad-hoc tooltipster instance type can also be removed as this is already available as JQueryTooltipster.ITooltipsterInstance.