From 764a045ec6ff982de2444784cce8e838f9912fa9 Mon Sep 17 00:00:00 2001 From: Arne Link Date: Wed, 11 Dec 2024 18:09:46 +0100 Subject: [PATCH 1/6] Fix memory leaks and failed cleanups in destroyables Mutation Observer --- src/util/destroyables.ts | 107 ++++++++++++++++++++++++++------------- 1 file changed, 73 insertions(+), 34 deletions(-) diff --git a/src/util/destroyables.ts b/src/util/destroyables.ts index d794a585563..a8bcd66906d 100644 --- a/src/util/destroyables.ts +++ b/src/util/destroyables.ts @@ -2,11 +2,7 @@ import Sortable from "sortablejs"; import { ErrorPF2e } from "./misc.ts"; class DestroyableManager { - #bodyObserver: MutationObserver; - - #appObservers = new Map(); - - #destroyables = new Map(); + #appObservers = new Map(); declare static instance: DestroyableManager; @@ -22,52 +18,81 @@ class DestroyableManager { DestroyableManager.instance ??= new DestroyableManager(); } - constructor() { - this.#bodyObserver = new MutationObserver(this.#onMutate.bind(this)); - this.#bodyObserver.observe(document.body, DestroyableManager.#OBSERVE_OPTIONS); - } + constructor() {} observe(destroyable: Destroyable): void { - const contentEl = + const destroyableEl = destroyable instanceof Sortable - ? destroyable.el.closest(".app, .application")?.querySelector(".window-content") + ? destroyable.el : destroyable instanceof TooltipsterTarget ? destroyable.element - : destroyable.DOM.input.closest(".app, .application")?.querySelector(".window-content"); - if (!contentEl) return console.warn(ErrorPF2e("No application element found").message); + : destroyable.DOM.input; + const contentEl = destroyableEl?.closest(".app, .application")?.querySelector(".window-content"); + if (!contentEl && !destroyableEl.closest(".chat-message")) + return console.warn(ErrorPF2e("No application element found").message); + if (!contentEl) return; + + let context = this.#appObservers.get(contentEl); + if (context) { + context.elements.push(destroyableEl, contentEl); + context.destroyables.push(destroyable); + return; + } - const destroyables = this.#destroyables.get(contentEl) ?? []; - destroyables.push(destroyable); - this.#destroyables.set(contentEl, destroyables); + context = { + observer: null, + contextKey: contentEl, + elements: [destroyableEl], + destroyables: [destroyable], + }; + const observer = new MutationObserver(this.#onMutate(context)); + context.observer = observer; - if (!this.#appObservers.has(contentEl)) { - const observer = new MutationObserver(this.#onMutate.bind(this)); - observer.observe(contentEl, DestroyableManager.#OBSERVE_OPTIONS); - this.#appObservers.set(contentEl, observer); - } + this.#appObservers.set(contentEl, context); + + observer.observe(contentEl, DestroyableManager.#OBSERVE_OPTIONS); + observer.observe(document.body, DestroyableManager.#OBSERVE_OPTIONS); } - /** Destroy destroyable instances in closed applications and replaced window content. */ - #onMutate(mutations: MutationRecord[]): void { - for (const mutation of mutations) { - for (const element of mutation.removedNodes) { - for (const destroyable of this.#destroyables.get(element) ?? []) { - destroyable.destroy(); + #onMutate(context: MutationObserverContext): (mutations: MutationRecord[]) => void { + return (mutations: MutationRecord[]) => { + for (const mutation of mutations) { + for (const element of mutation.removedNodes) { + if (!context.elements.some((contextElement) => element.contains(contextElement))) { + continue; + } + for (const destroyable of context.destroyables) { + destroyable.destroy(); + } + if (context.observer) { + context.observer.disconnect(); + } + this.#appObservers.delete(context.contextKey); + context.observer = null; + context.destroyables = []; + context.elements = []; } - this.#destroyables.delete(element); - this.#appObservers.delete(element); } - } + }; } } +interface MutationObserverContext { + observer: MutationObserver | null; + contextKey: Node; + elements: Node[]; + destroyables: Destroyable[]; +} + type Destroyable = Tagify<{ id: string; value: string }> | Tagify | Sortable | TooltipsterTarget; class TooltipsterTarget { $element: JQuery; + instance: Destroyable; - constructor($element: JQuery) { + constructor($element: JQuery, instance: Destroyable) { this.$element = $element; + this.instance = instance; } get element(): HTMLElement { @@ -75,7 +100,7 @@ class TooltipsterTarget { } destroy(): void { - this.$element.tooltipster("destroy"); + this.instance.destroy(); } } @@ -87,8 +112,22 @@ function createSortable(list: HTMLElement, options: Sortable.Options): Sortable function createTooltipster(target: HTMLElement, options: JQueryTooltipster.ITooltipsterOptions): JQuery { const $element = $(target); - DestroyableManager.instance.observe(new TooltipsterTarget($element)); - return $element.tooltipster(options); + const $tooltipsterEl = $element.tooltipster(options); + // get tooltipster namespace key + const tooltipsterNs: string | undefined = $tooltipsterEl.data("tooltipster-ns")?.[0]; + if (!tooltipsterNs) { + console.warn(ErrorPF2e("No tooltipster namespace found").message); + return $tooltipsterEl; + } + // get internal tooltipster instance + const tooltipsterInstance: Destroyable | undefined = $tooltipsterEl.data(tooltipsterNs); + if (!tooltipsterInstance) { + console.warn(ErrorPF2e("No tooltipster instance found").message); + return $tooltipsterEl; + } + // create wrapper of instance and tooltipster element for cleanup after element has been removed from DOM + DestroyableManager.instance.observe(new TooltipsterTarget($tooltipsterEl, tooltipsterInstance)); + return $tooltipsterEl; } export { DestroyableManager, createSortable, createTooltipster }; From 7d772cdedffb75a45b46c6cd275ba689a04b78c4 Mon Sep 17 00:00:00 2001 From: Arne Link Date: Thu, 12 Dec 2024 10:46:05 +0100 Subject: [PATCH 2/6] More granular approach to destroyable cleanup. The previous approach of just destroying every destroyable for an application whenever the content was replaced had potential issues for cases when only a subset of the DOM was being replaced, as is potentially planned for the kingdom and/or party sheet --- src/util/destroyables.ts | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/util/destroyables.ts b/src/util/destroyables.ts index a8bcd66906d..69b8cc4e664 100644 --- a/src/util/destroyables.ts +++ b/src/util/destroyables.ts @@ -34,16 +34,14 @@ class DestroyableManager { let context = this.#appObservers.get(contentEl); if (context) { - context.elements.push(destroyableEl, contentEl); - context.destroyables.push(destroyable); + context.elements.add({ node: destroyableEl, destroyable }); return; } context = { observer: null, contextKey: contentEl, - elements: [destroyableEl], - destroyables: [destroyable], + elements: new Set([{ node: destroyableEl, destroyable }]), }; const observer = new MutationObserver(this.#onMutate(context)); context.observer = observer; @@ -57,20 +55,22 @@ class DestroyableManager { #onMutate(context: MutationObserverContext): (mutations: MutationRecord[]) => void { return (mutations: MutationRecord[]) => { for (const mutation of mutations) { - for (const element of mutation.removedNodes) { - if (!context.elements.some((contextElement) => element.contains(contextElement))) { - continue; + for (const removedNode of mutation.removedNodes) { + for (const element of context.elements) { + if (removedNode.contains(element.node)) { + element.destroyable.destroy(); + context.elements.delete(element); + } } - for (const destroyable of context.destroyables) { - destroyable.destroy(); + if (context.elements.size > 0) { + continue; } if (context.observer) { context.observer.disconnect(); } this.#appObservers.delete(context.contextKey); context.observer = null; - context.destroyables = []; - context.elements = []; + return; } } }; @@ -80,8 +80,7 @@ class DestroyableManager { interface MutationObserverContext { observer: MutationObserver | null; contextKey: Node; - elements: Node[]; - destroyables: Destroyable[]; + elements: Set<{ node: Node; destroyable: Destroyable }>; } type Destroyable = Tagify<{ id: string; value: string }> | Tagify | Sortable | TooltipsterTarget; From 9ef4a0c4f08fc6488234dbc6415642feaf7585f2 Mon Sep 17 00:00:00 2001 From: Arne Link Date: Thu, 12 Dec 2024 17:20:40 +0100 Subject: [PATCH 3/6] Use a single mutation observer to watch for body closing of apps --- src/util/destroyables.ts | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/src/util/destroyables.ts b/src/util/destroyables.ts index 69b8cc4e664..7ebee9675a7 100644 --- a/src/util/destroyables.ts +++ b/src/util/destroyables.ts @@ -2,6 +2,8 @@ import Sortable from "sortablejs"; import { ErrorPF2e } from "./misc.ts"; class DestroyableManager { + #bodyObserver: MutationObserver; + #appObservers = new Map(); declare static instance: DestroyableManager; @@ -18,7 +20,10 @@ class DestroyableManager { DestroyableManager.instance ??= new DestroyableManager(); } - constructor() {} + constructor() { + this.#bodyObserver = new MutationObserver(this.#onMutateBody.bind(this)); + this.#bodyObserver.observe(document.body, DestroyableManager.#OBSERVE_OPTIONS); + } observe(destroyable: Destroyable): void { const destroyableEl = @@ -43,16 +48,15 @@ class DestroyableManager { contextKey: contentEl, elements: new Set([{ node: destroyableEl, destroyable }]), }; - const observer = new MutationObserver(this.#onMutate(context)); + const observer = new MutationObserver(this.#onMutateContent(context)); context.observer = observer; this.#appObservers.set(contentEl, context); observer.observe(contentEl, DestroyableManager.#OBSERVE_OPTIONS); - observer.observe(document.body, DestroyableManager.#OBSERVE_OPTIONS); } - #onMutate(context: MutationObserverContext): (mutations: MutationRecord[]) => void { + #onMutateContent(context: MutationObserverContext): (mutations: MutationRecord[]) => void { return (mutations: MutationRecord[]) => { for (const mutation of mutations) { for (const removedNode of mutation.removedNodes) { @@ -75,6 +79,26 @@ class DestroyableManager { } }; } + + #onMutateBody(mutations: MutationRecord[]) { + for (const mutation of mutations) { + for (const removedNode of mutation.removedNodes) { + for (const [node, context] of this.#appObservers.entries()) { + if (!removedNode.contains(node)) { + continue; + } + for (const element of context.elements) { + element.destroyable.destroy(); + } + if (context.observer) { + context.observer.disconnect(); + } + this.#appObservers.delete(node); + context.observer = null; + } + } + } + } } interface MutationObserverContext { From f51aba223827804cdc1db76f72f41a71bc1e4a33 Mon Sep 17 00:00:00 2001 From: Arne Link Date: Fri, 13 Dec 2024 08:36:52 +0100 Subject: [PATCH 4/6] Remove TooltipsterTarget wrapper class --- src/util/destroyables.ts | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/util/destroyables.ts b/src/util/destroyables.ts index 7ebee9675a7..a8e32bc8810 100644 --- a/src/util/destroyables.ts +++ b/src/util/destroyables.ts @@ -29,8 +29,8 @@ class DestroyableManager { const destroyableEl = destroyable instanceof Sortable ? destroyable.el - : destroyable instanceof TooltipsterTarget - ? destroyable.element + : "elementOrigin" in destroyable + ? destroyable.elementOrigin() : destroyable.DOM.input; const contentEl = destroyableEl?.closest(".app, .application")?.querySelector(".window-content"); if (!contentEl && !destroyableEl.closest(".chat-message")) @@ -107,26 +107,13 @@ interface MutationObserverContext { elements: Set<{ node: Node; destroyable: Destroyable }>; } -type Destroyable = Tagify<{ id: string; value: string }> | Tagify | Sortable | TooltipsterTarget; - -class TooltipsterTarget { - $element: JQuery; - instance: Destroyable; - - constructor($element: JQuery, instance: Destroyable) { - this.$element = $element; - this.instance = instance; - } - - get element(): HTMLElement { - return this.$element[0]; - } - - destroy(): void { - this.instance.destroy(); - } +interface Tooltipster { + destroy(): void; + elementOrigin(): HTMLElement; } +type Destroyable = Tagify<{ id: string; value: string }> | Tagify | Sortable | Tooltipster; + function createSortable(list: HTMLElement, options: Sortable.Options): Sortable { const sortable = new Sortable(list, options); DestroyableManager.instance.observe(sortable); @@ -149,7 +136,7 @@ function createTooltipster(target: HTMLElement, options: JQueryTooltipster.ITool return $tooltipsterEl; } // create wrapper of instance and tooltipster element for cleanup after element has been removed from DOM - DestroyableManager.instance.observe(new TooltipsterTarget($tooltipsterEl, tooltipsterInstance)); + DestroyableManager.instance.observe(tooltipsterInstance); return $tooltipsterEl; } From 3a37a52b29bbff419f37c9175f942ea0e340bdd8 Mon Sep 17 00:00:00 2001 From: Arne Link Date: Fri, 13 Dec 2024 08:52:33 +0100 Subject: [PATCH 5/6] Remove special handling for tooltipster in chat messages --- src/util/destroyables.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/destroyables.ts b/src/util/destroyables.ts index a8e32bc8810..ce008a8f4e5 100644 --- a/src/util/destroyables.ts +++ b/src/util/destroyables.ts @@ -33,9 +33,7 @@ class DestroyableManager { ? destroyable.elementOrigin() : destroyable.DOM.input; const contentEl = destroyableEl?.closest(".app, .application")?.querySelector(".window-content"); - if (!contentEl && !destroyableEl.closest(".chat-message")) - return console.warn(ErrorPF2e("No application element found").message); - if (!contentEl) return; + if (!contentEl) return console.warn(ErrorPF2e("No application element found").message); let context = this.#appObservers.get(contentEl); if (context) { From 8acdfec48df48e192d5781988b6541e6a8d069cc Mon Sep 17 00:00:00 2001 From: Arne Link Date: Fri, 13 Dec 2024 11:24:54 +0100 Subject: [PATCH 6/6] streamline accessing of tooltipster instance --- src/util/destroyables.ts | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/src/util/destroyables.ts b/src/util/destroyables.ts index ce008a8f4e5..e8c99dfa2d3 100644 --- a/src/util/destroyables.ts +++ b/src/util/destroyables.ts @@ -105,12 +105,11 @@ interface MutationObserverContext { elements: Set<{ node: Node; destroyable: Destroyable }>; } -interface Tooltipster { - destroy(): void; - elementOrigin(): HTMLElement; -} - -type Destroyable = Tagify<{ id: string; value: string }> | Tagify | Sortable | Tooltipster; +type Destroyable = + | Tagify<{ id: string; value: string }> + | Tagify + | Sortable + | JQueryTooltipster.ITooltipsterInstance; function createSortable(list: HTMLElement, options: Sortable.Options): Sortable { const sortable = new Sortable(list, options); @@ -119,23 +118,9 @@ function createSortable(list: HTMLElement, options: Sortable.Options): Sortable } function createTooltipster(target: HTMLElement, options: JQueryTooltipster.ITooltipsterOptions): JQuery { - const $element = $(target); - const $tooltipsterEl = $element.tooltipster(options); - // get tooltipster namespace key - const tooltipsterNs: string | undefined = $tooltipsterEl.data("tooltipster-ns")?.[0]; - if (!tooltipsterNs) { - console.warn(ErrorPF2e("No tooltipster namespace found").message); - return $tooltipsterEl; - } - // get internal tooltipster instance - const tooltipsterInstance: Destroyable | undefined = $tooltipsterEl.data(tooltipsterNs); - if (!tooltipsterInstance) { - console.warn(ErrorPF2e("No tooltipster instance found").message); - return $tooltipsterEl; - } - // create wrapper of instance and tooltipster element for cleanup after element has been removed from DOM - DestroyableManager.instance.observe(tooltipsterInstance); - return $tooltipsterEl; + const $element = $(target).tooltipster(options); + DestroyableManager.instance.observe($element.tooltipster("instance")); + return $element; } export { DestroyableManager, createSortable, createTooltipster };