ReviewOS

stacks/stx

Removed :for rows and hidden <template :if> branches never dispose their effects, so work per update grows without bound

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

Removing a :for row, or hiding a <template :if> branch, removes the elements but never disposes the effects that bound them. Those effects stay subscribed to every signal they read and keep re-running on each later change. Each shrink or toggle adds more, so the work done per update grows without bound for as long as the page stays open. Nothing errors and the DOM is always correct, which makes it easy to miss.

Reproduction 1: :for

@extends('layouts/default')

@section('content')
<script client>
  const items = state([0, 1, 2, 3, 4, 5, 6, 7, 8, 9])
  const tick = state(0)
  const report = state('ready')
  let evaluations = 0

  function probe(i) {
    evaluations++
    return tick() >= 0 ? `row row-${i}` : 'row'
  }

  async function bump(label) {
    const before = evaluations
    tick.set(tick() + 1)
    await new Promise(resolve => setTimeout(resolve, 100))
    report.set(`${label}|rows=${items().length}|evaluations=${evaluations - before}`)
  }

  function shrink() { items.set([0]) }
  function grow() { items.set([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]) }
</script>

<div>
  <button id="bump" @click="bump('bump')">bump</button>
  <button id="shrink" @click="shrink()">shrink</button>
  <button id="grow" @click="grow()">grow</button>
  <p id="report">{{ report() }}</p>
  <ul id="rows">
    <template :for="i in items">
      <li x-class="probe(i)">{{ i }}</li>
    </template>
  </ul>
</div>
@endsection

bump changes a signal the row binding reads and reports how many row bindings ran. Measured in Chrome:

Step<li> in DOMRow bindings run by one bump
Initial, 10 rows1010
shrink to 1 row110 (expected 1)
5 more grow + shrink cycles155 (10 + 9 × 5)
10 cycles in total1100 (10 + 9 × 10)

Every cycle leaks exactly the nine rows it removed.

Reproduction 2: <template :if>

@extends('layouts/default')

@section('content')
<script client>
  const shown = state(true)
  const tick = state(0)
  const report = state('ready')
  let evaluations = 0

  function probe() {
    evaluations++
    return tick() >= 0 ? 'branch' : ''
  }

  async function bump(label) {
    const before = evaluations
    tick.set(tick() + 1)
    await new Promise(resolve => setTimeout(resolve, 100))
    report.set(`${label}|shown=${shown()}|evaluations=${evaluations - before}`)
  }

  function toggle() { shown.set(!shown()) }
</script>

<div>
  <button id="bump" @click="bump('bump')">bump</button>
  <button id="toggle" @click="toggle()">toggle</button>
  <p id="report">{{ report() }}</p>
  <div id="host">
    <template :if="shown()">
      <span class="probe" x-class="probe()">branch</span>
    </template>
  </div>
</div>
@endsection
Step.probe in DOMBindings run by one bump
Branch shown11
After 5 hide + show cycles16
After 10 cycles111
Then hidden011

One leaked effect set per cycle, and they keep running while nothing is rendered.

Cause

Read at main db32fc6618faeda6eb4c3a7dd65ce5b2b2f5c5c6. The client runtime is the template literal returned by generateSignalsRuntimeDev() in packages/stx/src/signals.ts:110. Published dist/signals.js lines are these minus 59.

:for rows never get a disposer handle.

  • Rows are hydrated with a bare processElement(el, itemScope) at signals.ts:4204-4205, not wrapped in trackEffects (signals.ts:854), so no __stx_disposers is stored on the row.
  • effect() registers its disposer only when activeDisposers is set (signals.ts:846). On first render that is the page root's tracker, freed only on SPA navigation by cleanupContainer. On any later re-render, fired by signal.set() from an event handler, activeDisposers is null, so the new rows' effects are registered nowhere and nothing can ever dispose them. That includes SPA navigation. This point comes from reading the code and was not measured.
  • Only component roots receive __stx_disposers, in hydrateComponentScopes at signals.ts:3674.

The removal paths only dispose scopes.

  • removeGroup (signals.ts:3837-3846) calls disposeSubtreeScopes(e) and then e.remove().
  • disposeSubtreeScopes (signals.ts:7616) runs __destroyCallbacks for [data-stx-scope] nodes and deletes their registry entries. It never touches __stx_disposers, __stx_destroy or __stx_hydration_cancel.
  • The keyed removal loop (signals.ts:4215-4244) reaches it through removeGroup (4239). Its TransitionGroup branch (4226-4236) also only disposes scopes. clearRenderedItems (signals.ts:3848) is currentGroups.forEach(removeGroup).
  • Unkeyed loops use index keys (getItemKey, signals.ts:3983) and take the same path.
  • The only code that does dispose descendants' effects is cleanupContainer, for SPA navigation (signals.ts:7904-7940, the __stx_disposers call at 7919-7922).

<template :if> re-clones on every show.

  • Show re-clones el.content and runs processTemplateNodes (signals.ts:4607-4611, processing at 4573-4586) with no trackEffects.
  • Hide removes the clones and clears currentNodes (signals.ts:4620-4623) with no disposal.
  • The clones are never reused, so disposing their effects on hide is safe with respect to #1737.

The same class of leak exists here too. Found by reading the code, not measured:

  • showEmpty/hideEmpty: showEmpty hydrates the empty-state element at signals.ts:3927 without tracking, and hideEmpty only removes it at 3934.
  • A nested single-element :if inside a :for row: it hydrates its children in a setTimeout (signals.ts:4674, processElement at 4684) while activeDisposers is null, so those effects escape any row-level tracker.

Deliberately not in scope: bindIfChain and single-element bindIf reuse the same nodes on re-show and process them once (signals.ts:4423, 4435-4436, 4656, 4668). Their hidden effects keep running but do not accumulate, and disposing them would break re-show.

Suggested fix

Inside the runtime template literal in packages/stx/src/signals.ts. Mind its escaping rules: no backticks, and backslashes doubled (see the comment near line 4022).

  1. Factor the cleanupContainer walk into a helper placed next to disposeSubtreeScopes, and have cleanupContainer call it:
function disposeSubtreeEffects(root) {
  if (!root) return;
  var nodes = [root];
  if (root.querySelectorAll) { var all = root.querySelectorAll('*'); for (var i = 0; i < all.length; i++) nodes.push(all[i]); }
  for (var n = 0; n < nodes.length; n++) {
    var el = nodes[n];
    if (typeof el.__stx_hydration_cancel === 'function') { try { el.__stx_hydration_cancel(); } catch (e) {} el.__stx_hydration_cancel = null; }
    if (Array.isArray(el.__stx_destroy)) { el.__stx_destroy.forEach(function (fn) { try { fn(); } catch (e) { console.warn('[stx] destroy hook error:', e); } }); el.__stx_destroy = null; }
    if (typeof el.__stx_disposers === 'function') { var d = el.__stx_disposers; el.__stx_disposers = null; d(); }
  }
}
  1. Give every :for row a disposer handle at signals.ts:4204-4205:
if (!el.__stx_disposers && el.tagName !== 'SCRIPT')
  el.__stx_disposers = trackEffects(function () { processElement(el, itemScope); });

This also lets cleanupContainer dispose re-rendered rows on SPA navigation.

  1. Dispose before removing. Call disposeSubtreeEffects(node) before disposeSubtreeScopes/remove() in removeGroup (3840, 3843), and in the TransitionGroup branch (4228, 4234) before tgLeave.

  2. The same pattern for the other two paths.

    • <template :if>: in processTemplateNodes, assign node.__stx_disposers = trackEffects(...). On hide, call disposeSubtreeEffects(node) before node.remove(). Effects only: keep not calling disposeSubtreeScopes there, per #1737.
    • showEmpty/hideEmpty: apply the same change.

Known gap after this change: the nested single-element :if deferred hydration (signals.ts:4674) still creates untracked effects. Covering it needs the deferred trackEffects result stored on a separate property, e.g. el.__stx_if_disposers, that the walker also calls. Not __stx_disposers, which 4681-4684 treats as a component-root marker.

Follow-up: a disposed effect stays in its signal's effects Set (signals.ts:690, 711). It only bails at run time (796), so it still retains its closure and the detached nodes. Pruning disposed effects from those sets would release the memory as well as the work.

Regression test

packages/stx/test/signals/for-minimal-move.test.ts and template-for-dom.test.ts already drive the full runtime under happy-dom through packages/stx/test-utils/dom-runtime-shim.ts. A test counting binding evaluations after N grow/shrink cycles, and after N <template :if> toggles, should see a constant equal to what is rendered. Both tables above currently fail that.

Impact

Any list that shrinks and any template branch that toggles leaks: search suggestions, filtered or paginated lists, live feeds, modals and tabs built on <template :if>. Found in an autocomplete dropdown whose :for shrank on nearly every keystroke. The workaround there, rendering a fixed number of rows and toggling hidden, avoids the leak but not the bug.

Environment

@stacksjs/stx 0.2.286 (npm latest; main at db32fc6 has the same runtime), Bun, Chrome.

9 days ago

Fixed across 5b8dedda45, 092ed2d12d and 1e82602053.

Measured in headless Chrome through the real render pipeline — bindings run by one update:

mainnow
:for shrunk to 1 row, after 10 cycles911
:for emptied, 0 rows rendered1000
<template :if> after 10 toggles111
<template :if> then hidden110
:for empty placeholder, 10 cycles122, constant
nested :if in a row, 10 rows23510
@if/:if chain in rows, after 10 flip-shrink cycles1822

All four paths in the issue are covered, including the two that were found by reading rather than measuring, and the "known gap" for the deferred nested :if pass.

Two things beyond the original report:

  • The @if form was still leaking after the first fix. A signal-reading becomes an @if/