ReviewOS

stacks/stx

Scope leak on :if / :for unmount — scopes only cleaned up on router-driven swaps, not dynamic toggles

#1727
Closed glennmichael123 opened this 10 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
10 days ago

Summary

window.stx._scopes is the registry for [data-stx-scope] elements' reactive state. SPA navigation correctly cleans these up — cleanupContainer at signals.ts:4180 walks the departing container's [data-stx-scope] elements and delete window.stx._scopes[scopeId] on each.

But scope cleanup is only wired to the SPA-navigation teardown path. Dynamic-unmount paths — :if="false" removing a subtree, :for removing an item when its data is filtered out — call el.remove() directly without walking the removed subtree for scope cleanup. So scopes leak.

Long-lived apps that toggle components (modals opening/closing, lists with frequently-removed items, conditional sections) accumulate stale entries in window.stx._scopes indefinitely — each holding references to old DOM nodes, signals, and any composable state captured during init.

Reproducer

<script client>
  const show = state(false)
  onMount(() => setInterval(() => show.set(!show()), 1000))
</script>

<div :if="show()" data-stx-scope="x-data:counter">…</div>

Toggle every 1s for a minute → ~30 stale entries in window.stx._scopes, each carrying its own scope object. DevTools memory snapshot shows retained DOM and signal objects per toggled-off instance.

Root cause

bindIf at signals.ts:2485-2490:

else if (!value && isInserted) {
  console.log('[stx] bindIf REMOVING element for :if=' + expr, ...);
  el.remove();           // ← removes from DOM
  isInserted = false;
  // No walk of removed subtree → no scope cleanup
}

Compare with cleanupContainer at signals.ts:4229-4243 which DOES handle this for the router-driven teardown:

container.querySelectorAll('[data-stx-scope]').forEach(function(el) {
  var scopeId = el.getAttribute('data-stx-scope');
  if (scopeId && window.stx._scopes && window.stx._scopes[scopeId]) {
    // fire destroy callbacks, then:
    delete window.stx._scopes[scopeId];
  }
});

The same walk needs to happen before el.remove() in bindIf (and presumably in bindFor's item-removal path).

Impact

  • Memory leak that accumulates with usage. Worst on long-running tabs with frequent toggling — admin dashboards, multi-step wizards, chat UIs with dynamic message lists.
  • Stale signals in window.stx._scopes retain DOM node references → prevents GC.
  • Component-level onDestroy callbacks registered during scope init would also never fire (they're inside scope.__destroyCallbacks), so any user-registered cleanup (closing a WebSocket, removing global listeners — see #1718) silently doesn't run on :if-driven unmount.

The last point makes this worse than a pure memory issue: it means our existing listener-leak fixes (e.g. useStorage #1718, useEventListener #1721) work for router-driven unmounts but NOT for :if/:for-driven unmounts.

Suggested fix

Factor the scope-cleanup walk in cleanupContainer into a helper:

function disposeSubtreeScopes(root) {
  // Include the root itself + all descendants with data-stx-scope
  const nodes = [root, ...root.querySelectorAll('[data-stx-scope]')];
  for (const el of nodes) {
    const scopeId = el.getAttribute && el.getAttribute('data-stx-scope');
    if (!scopeId) continue;
    const scope = window.stx._scopes && window.stx._scopes[scopeId];
    if (!scope) continue;
    if (Array.isArray(scope.__destroyCallbacks)) {
      scope.__destroyCallbacks.forEach(fn => { try { fn(); } catch (e) { console.warn('[stx] scope destroy error:', e); } });
    }
    delete window.stx._scopes[scopeId];
  }
}

Call it from:

  • bindIf immediately before el.remove() (line 2487)
  • bindFor immediately before each removed item's el.remove()
  • Any other site that calls el.remove() on a subtree that could contain [data-stx-scope] elements

Existing cleanupContainer should also call disposeSubtreeScopes instead of inlining the walk, to keep behavior consistent.

Environment

  • stx main
  • Bun 1.3.13

Sign in to comment on this issue.