ReviewOS

stacks/stx

useStorage: 'storage' event listener never removed — leaks across instances and HMR

#1718
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Summary

composables/use-storage.ts:147 attaches a window.addEventListener('storage', ...) but never removes it. The listener captures currentValue, defaultValue, serializer, and notify, so every call to useStorage(...) leaks a closure holding those references plus a DOM reference.

After ~10 HMR reloads or repeated useStorage calls in different scopes, multiple listeners fire for every cross-tab storage event — each against stale closures.

Reproducer

<script client>
  // Each call leaks a listener. Hot-reload this file 10x; check
  // `getEventListeners(window).storage` in DevTools — count grows.
  const session = useStorage('session', null, { listenToStorageChanges: true })
</script>

Root cause

packages/stx/src/composables/use-storage.ts:147-153:

// Listen for storage changes from other tabs
if (isClient && listenToStorageChanges) {
  window.addEventListener('storage', (event) => {
    if (event.key === key && event.storageArea === getStorage()) {
      const prevValue = currentValue
      currentValue = event.newValue ? serializer.read(event.newValue) : defaultValue
      notify(currentValue, prevValue)
    }
  })
}

No onDestroy(() => window.removeEventListener('storage', handler)) pair. The handler is also defined inline (no named binding), so even calling removeEventListener later would need a restructure.

The subscribers array elsewhere in the file IS cleaned up correctly via the explicit unsubscribe returned by .subscribe(). The storage listener was just missed.

Impact

  • Memory leak across HMR.
  • N listeners → N callbacks per storage event → unbounded fanout.
  • Stale closures: leaked listeners read stale defaultValue / serializer, can fire with wrong types.

Same pattern was caught in stacksjs/stx#1709 for three other sites (loading-indicator, reactive-bindings, hot-reload). This is the fourth.

Suggested fix

Hoist the handler to a named function and register cleanup via onDestroy (mirror the pattern used by the runtime version in signals.ts:2908-2913):

if (isClient && listenToStorageChanges) {
  const onStorage = (event: StorageEvent) => {
    if (event.key === key && event.storageArea === getStorage()) {
      const prevValue = currentValue
      currentValue = event.newValue ? serializer.read(event.newValue) : defaultValue
      notify(currentValue, prevValue)
    }
  }
  window.addEventListener('storage', onStorage)
  // onDestroy may be undefined in non-runtime contexts (tests, SSR) — gate it.
  if (typeof (globalThis as any).onDestroy === 'function') {
    (globalThis as any).onDestroy(() => window.removeEventListener('storage', onStorage))
  }
}

Environment

  • stx main
  • Bun 1.3.13

Sign in to comment on this issue.