Avoid some memory leaks when disposing Webamp

captbaritone · captbaritone · commit 14c0d24a47b1 · 2025-07-04T21:25:57.000-07:00
diff --git a/packages/webamp/js/fileUtils.ts b/packages/webamp/js/fileUtils.ts
@@ -58,16 +58,23 @@ export function genMediaDuration(url: string): Promise<number> {
     // got the duration?
     const audio = document.createElement("audio");
     audio.crossOrigin = "anonymous";
+
     const durationChange = () => {
       resolve(audio.duration);
       audio.removeEventListener("durationchange", durationChange);
+      audio.removeEventListener("error", errorHandler);
       audio.src = "";
       // TODO: Not sure if this really gets cleaned up.
     };
-    audio.addEventListener("durationchange", durationChange);
-    audio.addEventListener("error", (e) => {
+
+    const errorHandler = (e: Event) => {
+      audio.removeEventListener("durationchange", durationChange);
+      audio.removeEventListener("error", errorHandler);
       reject(e);
-    });
+    };
+
+    audio.addEventListener("durationchange", durationChange);
+    audio.addEventListener("error", errorHandler);
     audio.src = url;
   });
 }
diff --git a/packages/webamp/js/hooks.ts b/packages/webamp/js/hooks.ts
@@ -66,15 +66,39 @@ export function useWindowSize() {
 }
 
 const cursorPositionRef = { current: { pageX: 0, pageY: 0 } };
-window.document.addEventListener("mousemove", ({ pageX, pageY }) => {
+let listenerRefCount = 0;
+
+// Global mousemove listener - managed with reference counting
+const globalMouseMoveHandler = ({ pageX, pageY }: MouseEvent) => {
   cursorPositionRef.current = { pageX, pageY };
-});
+};
+
+// Add a reference to the global mouse listener
+function addGlobalMouseListener() {
+  if (listenerRefCount === 0) {
+    window.document.addEventListener("mousemove", globalMouseMoveHandler);
+  }
+  listenerRefCount++;
+}
 
-// We use a single global event listener because there is no way to get the
-// mouse position aside from an event. Ideally we could create/clean up the
-// event listener in the hook, but in the case where we want to check the cursor
-// position on mount, that we wouldn't have had time to capture an event.
+// Remove a reference to the global mouse listener
+function removeGlobalMouseListener() {
+  listenerRefCount--;
+  if (listenerRefCount === 0) {
+    window.document.removeEventListener("mousemove", globalMouseMoveHandler);
+  }
+}
+
+// We use a single global event listener with reference counting because there is no way to get the
+// mouse position aside from an event. The listener is only active when at least one component needs it.
 function useCursorPositionRef() {
+  useEffect(() => {
+    addGlobalMouseListener();
+    return () => {
+      removeGlobalMouseListener();
+    };
+  }, []);
+
   return cursorPositionRef;
 }
 
diff --git a/packages/webamp/js/media/elementSource.ts b/packages/webamp/js/media/elementSource.ts
@@ -1,4 +1,5 @@
 import Emitter from "../emitter";
+import Disposable from "../Disposable";
 import { clamp } from "../utils";
 import { MEDIA_STATUS } from "../constants";
 import { MediaStatus } from "../types";
@@ -11,6 +12,7 @@ export default class ElementSource {
   _audio: HTMLAudioElement;
   _stalled: boolean;
   _status: MediaStatus;
+  _disposable: Disposable;
 
   on(eventType: string, cb: (...args: any[]) => void) {
     return this._emitter.on(eventType, cb);
@@ -24,32 +26,44 @@ export default class ElementSource {
     this._audio.crossOrigin = "anonymous";
     this._stalled = false;
     this._status = MEDIA_STATUS.STOPPED;
+    this._disposable = new Disposable();
 
-    // TODO: #leak
-    this._audio.addEventListener("suspend", () => {
+    // Create event handlers and register cleanup
+    const suspendHandler = () => {
       this._setStalled(true);
-    });
+    };
+    this._audio.addEventListener("suspend", suspendHandler);
+    this._disposable.add(() =>
+      this._audio.removeEventListener("suspend", suspendHandler)
+    );
 
-    // TODO: #leak
-    this._audio.addEventListener("durationchange", () => {
+    const durationChangeHandler = () => {
       this._emitter.trigger("loaded");
       this._setStalled(false);
-    });
+    };
+    this._audio.addEventListener("durationchange", durationChangeHandler);
+    this._disposable.add(() =>
+      this._audio.removeEventListener("durationchange", durationChangeHandler)
+    );
 
-    // TODO: #leak
-    this._audio.addEventListener("ended", () => {
+    const endedHandler = () => {
       this._emitter.trigger("ended");
       this._setStatus(MEDIA_STATUS.STOPPED);
-    });
+    };
+    this._audio.addEventListener("ended", endedHandler);
+    this._disposable.add(() =>
+      this._audio.removeEventListener("ended", endedHandler)
+    );
 
-    // TODO: Throttle to 50 (if needed)
-    // TODO: #leak
-    this._audio.addEventListener("timeupdate", () => {
+    const timeUpdateHandler = () => {
       this._emitter.trigger("positionChange");
-    });
+    };
+    this._audio.addEventListener("timeupdate", timeUpdateHandler);
+    this._disposable.add(() =>
+      this._audio.removeEventListener("timeupdate", timeUpdateHandler)
+    );
 
-    // TODO: #leak
-    this._audio.addEventListener("error", (e) => {
+    const errorHandler = (e: Event) => {
       switch (this._audio.error!.code) {
         case 1:
           // The fetching of the associated resource was aborted by the user's request.
@@ -77,7 +91,11 @@ export default class ElementSource {
 
       this._emitter.trigger("ended");
       this._setStatus(MEDIA_STATUS.STOPPED);
-    });
+    };
+    this._audio.addEventListener("error", errorHandler);
+    this._disposable.add(() =>
+      this._audio.removeEventListener("error", errorHandler)
+    );
 
     this._source = this._context.createMediaElementSource(this._audio);
     this._source.connect(destination);
@@ -159,7 +177,9 @@ export default class ElementSource {
   }
 
   dispose() {
-    // TODO: Dispose subscriptions to this.audio
+    // Clean up all event listeners via disposable
+    this._disposable.dispose();
+
     this.stop();
     this._emitter.dispose();
   }
diff --git a/packages/webamp/js/media/index.ts b/packages/webamp/js/media/index.ts
@@ -2,6 +2,7 @@
 import { BANDS, MEDIA_STATUS } from "../constants";
 import { Band } from "../types";
 import Emitter from "../emitter";
+import Disposable from "../Disposable";
 import StereoBalanceNode from "./StereoBalanceNode";
 import ElementSource from "./elementSource";
 
@@ -100,32 +101,39 @@ export default class Media implements IMedia {
   _gainNode: GainNode;
   _source: ElementSource;
   _bands: { [band: number]: BiquadFilterNode };
+  _disposable: Disposable;
 
   constructor() {
     this._emitter = new Emitter();
+    this._disposable = new Disposable();
     // @ts-ignore Typescript does not know about webkitAudioContext
     this._context = new (window.AudioContext || window.webkitAudioContext)();
     // Fix for iOS and Chrome (Canary) which require that the context be created
     // or resumed by a user interaction.
     // https://developers.google.com/web/updates/2017/09/autoplay-policy-changes
     // https://gist.github.com/laziel/7aefabe99ee57b16081c
     // Via: https://stackoverflow.com/a/43395068/1263117
-    // TODO #leak
     if (this._context.state === "suspended") {
-      const resume = async () => {
+      const resumeHandler = async () => {
         await this._context.resume();
 
         if (this._context.state === "running") {
-          // TODO: Add this to the disposable
-          document.body.removeEventListener("touchend", resume, false);
-          document.body.removeEventListener("click", resume, false);
-          document.body.removeEventListener("keydown", resume, false);
+          document.body.removeEventListener("touchend", resumeHandler, false);
+          document.body.removeEventListener("click", resumeHandler, false);
+          document.body.removeEventListener("keydown", resumeHandler, false);
         }
       };
 
-      document.body.addEventListener("touchend", resume, false);
-      document.body.addEventListener("click", resume, false);
-      document.body.addEventListener("keydown", resume, false);
+      document.body.addEventListener("touchend", resumeHandler, false);
+      document.body.addEventListener("click", resumeHandler, false);
+      document.body.addEventListener("keydown", resumeHandler, false);
+
+      // Add cleanup for resume handlers
+      this._disposable.add(() => {
+        document.body.removeEventListener("touchend", resumeHandler, false);
+        document.body.removeEventListener("click", resumeHandler, false);
+        document.body.removeEventListener("keydown", resumeHandler, false);
+      });
     }
 
     // TODO: Maybe we can get rid of this now that we are using AudioAbstraction?
@@ -318,6 +326,9 @@ export default class Media implements IMedia {
   }
 
   dispose() {
+    // Clean up all event listeners via disposable
+    this._disposable.dispose();
+
     this._source.dispose();
     this._emitter.dispose();
   }
diff --git a/packages/webamp/js/webampLazy.tsx b/packages/webamp/js/webampLazy.tsx
@@ -406,8 +406,7 @@ class Webamp {
    */
   onTrackDidChange(cb: (trackInfo: LoadedURLTrack | null) => void): () => void {
     let previousTrackId: number | null = null;
-    // TODO #leak.
-    return this.store.subscribe(() => {
+    const unsubscribe = this.store.subscribe(() => {
       const state = this.store.getState();
       const trackId = Selectors.getCurrentlyPlayingTrackIdIfLoaded(state);
       if (trackId === previousTrackId) {
@@ -416,6 +415,11 @@ class Webamp {
       previousTrackId = trackId;
       cb(trackId == null ? null : Selectors.getCurrentTrackInfo(state));
     });
+
+    // Register cleanup with disposable
+    this._disposable.add(unsubscribe);
+
+    return unsubscribe;
   }
 
   /**
@@ -445,8 +449,7 @@ class Webamp {
    */
   async skinIsLoaded(): Promise<void> {
     // Wait for the skin to load.
-    // TODO #leak.
-    await storeHas(this.store, (state) => !state.display.loading);
+    await this.storeHas((state) => !state.display.loading);
     // We attempt to pre-resolve these promises before we declare the skin
     // loaded. That's because `<EqGraph>` needs these in order to render fully.
     // As long as these are resolved before we attempt to render, we can ensure
@@ -514,8 +517,6 @@ class Webamp {
    * attempt to clean itself up to avoid memory leaks.
    */
   dispose(): void {
-    // TODO: Clean up store subscription in onTrackDidChange.
-    // TODO: Every storeHas call represents a potential race condition.
     this.media.dispose();
     this._actionEmitter.dispose();
     this._disposable.dispose();
@@ -530,8 +531,42 @@ class Webamp {
   }
 
   __onStateChange(cb: () => void): () => void {
-    // TODO #leak.
-    return this.store.subscribe(cb);
+    const unsubscribe = this.store.subscribe(cb);
+
+    // Register cleanup with disposable
+    this._disposable.add(unsubscribe);
+
+    return unsubscribe;
+  }
+
+  /**
+   * Wait for the store to match a predicate condition.
+   * Returns a promise that resolves when the condition is met.
+   * If the instance is disposed, the promise will be rejected.
+   */
+  private storeHas(predicate: (state: AppState) => boolean): Promise<void> {
+    let unsubscribed = false;
+    return new Promise((resolve, reject) => {
+      if (predicate(this.store.getState())) {
+        resolve();
+        return;
+      }
+      const unsubscribe = this.store.subscribe(() => {
+        if (predicate(this.store.getState())) {
+          unsubscribed = true;
+          unsubscribe();
+          resolve();
+        }
+      });
+
+      // Register cleanup with disposable
+      this._disposable.add(() => {
+        if (!unsubscribed) {
+          unsubscribe();
+          reject(new Error("Store was disposed before condition was met."));
+        }
+      });
+    });
   }
 
   _bufferTracks(tracks: Track[]): void {
@@ -542,25 +577,6 @@ class Webamp {
   }
 }
 
-// Return a promise that resolves when the store matches a predicate.
-// TODO #leak.
-const storeHas = (
-  store: Store,
-  predicate: (state: AppState) => boolean
-): Promise<void> =>
-  new Promise((resolve) => {
-    if (predicate(store.getState())) {
-      resolve();
-      return;
-    }
-    const unsubscribe = store.subscribe(() => {
-      if (predicate(store.getState())) {
-        resolve();
-        unsubscribe();
-      }
-    });
-  });
-
 // @ts-ignore
 window.Webamp = Webamp;
 
