From 1dc541567568ac1f835dcf6cabda6952a84ce6b2 Mon Sep 17 00:00:00 2001 From: Jacopo Date: Wed, 19 Oct 2022 16:19:57 +0200 Subject: [PATCH] Clear dangling EventListeners and Detached Nodes when a controller is removed from the DOM `eventListenerMaps` maps HTML elements handled by Stimulus to `eventListenerMap`, a map of Stimulus EventListeners in this way: `{ HTMLelement: { eventName: EventListener } }`. When a controller HTML is removed from the DOM the dangling EventListeners aren't removed, moreover, the removed HTML elements are still referenced via the `eventListenerMaps` keys. This leads to having multiple detached HTMLelements and eventListeners which can't be GCed hence a memory leak. The leak is fixed by removing the dangling eventListeners and clearing unused `eventListenerMaps` keys. When the HTMLelement is attached again to the DOM, the `eventListenerMaps` and the related `eventListeners` are automatically re-created by Stimulus via the MutationObservers, no data is lost by doing this. Below is an example to reproduce the issue: ``` EventListenerMaps memory leak
To reproduce:
``` --- src/core/binding_observer.ts | 4 ++-- src/core/dispatcher.ts | 16 +++++++++++++++- src/tests/modules/core/memory_tests.ts | 22 ++++++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 src/tests/modules/core/memory_tests.ts diff --git a/src/core/binding_observer.ts b/src/core/binding_observer.ts index 99a5019f..fd56295e 100644 --- a/src/core/binding_observer.ts +++ b/src/core/binding_observer.ts @@ -7,7 +7,7 @@ import { Token, ValueListObserver, ValueListObserverDelegate } from "../mutation export interface BindingObserverDelegate extends ErrorHandler { bindingConnected(binding: Binding): void - bindingDisconnected(binding: Binding): void + bindingDisconnected(binding: Binding, clearEventListeners?: boolean): void } export class BindingObserver implements ValueListObserverDelegate { @@ -72,7 +72,7 @@ export class BindingObserver implements ValueListObserverDelegate { } private disconnectAllActions() { - this.bindings.forEach((binding) => this.delegate.bindingDisconnected(binding)) + this.bindings.forEach((binding) => this.delegate.bindingDisconnected(binding, true)) this.bindingsByAction.clear() } diff --git a/src/core/dispatcher.ts b/src/core/dispatcher.ts index cfa9e365..5cabae33 100644 --- a/src/core/dispatcher.ts +++ b/src/core/dispatcher.ts @@ -41,8 +41,9 @@ export class Dispatcher implements BindingObserverDelegate { this.fetchEventListenerForBinding(binding).bindingConnected(binding) } - bindingDisconnected(binding: Binding) { + bindingDisconnected(binding: Binding, clearEventListeners: boolean = false) { this.fetchEventListenerForBinding(binding).bindingDisconnected(binding) + if (clearEventListeners) this.clearEventListenersForBinding(binding) } // Error handling @@ -51,6 +52,19 @@ export class Dispatcher implements BindingObserverDelegate { this.application.handleError(error, `Error ${message}`, detail) } + private clearEventListenersForBinding(binding: Binding) { + const eventListener = this.fetchEventListenerForBinding(binding) + if (eventListener.bindings.length == 0) { + const { eventTarget, eventName, eventOptions } = binding + const eventListenerMap = this.fetchEventListenerMapForEventTarget(eventTarget) + const cacheKey = this.cacheKey(eventName, eventOptions) + eventListener.disconnect() + eventListenerMap.delete(cacheKey) + + if (eventListenerMap.size == 0) this.eventListenerMaps.delete(eventTarget) + } + } + private fetchEventListenerForBinding(binding: Binding): EventListener { const { eventTarget, eventName, eventOptions } = binding return this.fetchEventListener(eventTarget, eventName, eventOptions) diff --git a/src/tests/modules/core/memory_tests.ts b/src/tests/modules/core/memory_tests.ts new file mode 100644 index 00000000..ae73a28c --- /dev/null +++ b/src/tests/modules/core/memory_tests.ts @@ -0,0 +1,22 @@ +import { ControllerTestCase } from "../../cases/controller_test_case" + +export default class MemoryTests extends ControllerTestCase() { + controllerElement!: Element + + async setup() { + this.controllerElement = this.controller.element + } + + fixtureHTML = ` +
+ + +
+ ` + + async "test removing a controller clears dangling eventListeners"() { + this.assert.equal(this.application.dispatcher.eventListeners.length, 2) + await this.fixtureElement.removeChild(this.controllerElement) + this.assert.equal(this.application.dispatcher.eventListeners.length, 0) + } +}