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: ``` <html> <head> <title>EventListenerMaps memory leak</title> <script type="module"> import { Application, Controller } from "https://unpkg.com/@hotwired/stimulus/dist/stimulus.js" window.Stimulus = Application.start() Stimulus.register("boosts", class extends Controller { doClick() { process(500) } }) </script> <script> function process(count) { let i = 0 let handler = setInterval(function(){ if (++i > count) { clearInterval(handler) } else { document.body.replaceWith(document.body.cloneNode(true)) } }, 1) } </script> </head> <body> <div data-controller="boosts"> To reproduce: <ul> <li>Check heap snapshot</li> <li>Click "trigger leak" button</li> <li>Check heap snapshot again</li> </ul> <button data-action="click->boosts#doClick">trigger leak</button> </div> </body> </html> ```
hotwired · Oct 19, 2022 · 1dc5415 · 1dc5415
1 parent 9686943
commit 1dc5415
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 3 deletions.
diff --git 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<Action> {
@@ -72,7 +72,7 @@ export class BindingObserver implements ValueListObserverDelegate<Action> {
   }
 
   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
@@ -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
@@ -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 = `
+    <div data-controller="${this.identifier}">
+      <button data-action="${this.identifier}#doLog">Log</button>
+      <button data-action="${this.identifier}#doAlert">Alert</button>
+    </div>
+  `
+
+  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)
+  }
+}