From 5012aac8e169a4b349763efc4e6e317eb858d8e8 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 21:44:12 +0530 Subject: [PATCH 01/10] fix: removeBetween leaves the end marker it was asked to remove Every rendered template instance is bracketed by a wjm-s / wjm-e comment pair, and removeBetween is what takes a range back out. The guard deciding whether to remove the end marker read start.parentNode AFTER the walk had already detached start, so it compared the live parent against null, could never fire, and every teardown left one orphan comment in the document. They accumulate for the life of the region and nothing collects them, so a long-lived list that churns rows grows comment nodes without limit. A 3-row repeat() cycled empty and back five times went from 3 end markers to 18. Capture the parent before the walk consumes start. The guard stays, because a marker moved under a different parent is not this region's to remove, and reaching into it would both steal a node the renderer does not own and throw NotFoundError out of a teardown that has to stay total. The terminator stays end itself rather than a stop sentinel captured up front. removeChild runs disconnectedCallback synchronously, so a sentinel pointing at a sibling this region does not own can move mid-walk, and the walk would then run off the end of the child list and take the part's own marker with it. The leak is invisible to textContent and to querySelectorAll, which is why nothing caught it, so the new tests count comment nodes directly. --- .agents/skills/webjs/references/components.md | 2 +- packages/core/src/render-client.js | 35 ++- .../rendering/marker-leak-on-teardown.test.js | 243 ++++++++++++++++++ website/app/docs/error-handling/page.ts | 2 +- 4 files changed, 277 insertions(+), 5 deletions(-) create mode 100644 packages/core/test/rendering/marker-leak-on-teardown.test.js diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index aae9eab63..1f98b20d7 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -183,7 +183,7 @@ The boundary also covers `watch(signal)` (its notify microtask) and `until()` (i **A commit that throws leaves the directive's own state consistent, so the NEXT valid render is correct.** This matters because the corruption is otherwise silent: the renders that expose it are fully valid and log nothing after the first throw. The hole whose commit threw is marked so the next render re-applies it rather than skipping it as unchanged (its recorded value is never advanced past a throw, and would otherwise match exactly what the recovering render supplies, leaving a child region blank for good). Both list reconcilers additionally repair their own bookkeeping so it describes the DOM again, and the next render is an ordinary reconcile rather than a rebuild of the region, which would discard the node identity the reconcilers exist to preserve. `repeat()` re-unites its key map and repositions every row (the failure was a permanently duplicated row). A plain `.map()` array splices the part of its slot list the failed pass never reached back on, which matters whenever a slot is REPLACED rather than updated in place (its template shape changed, its kind changed between text, template and empty, or the array grew past its old length), since that is the branch that inserts the replacement before removing what it replaced (the failure was a stranded row that outlived even a render of an empty array). `guard()` records its new deps only once the commit succeeds, so a later render with those same deps re-renders the region instead of short-circuiting past a region the throw had blanked; `until()` advances its resolved priority only after the commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it. -**Teardown is total as well.** Removing a row is not a commit and has no retry, so a throw while tearing one down cannot be allowed to abandon the rest. Unbinding a `ref` during teardown can never abort the removal of the remaining rows, and `repeat()` drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed (which used to leave the row the app DELETED on screen, reorder the survivors, and let a later render that re-added that key reinsert the disposed instance). To make that hold, a `ref` whose object `value` setter throws is now SWALLOWED on teardown, matching the ref CALLBACK, which was already swallowed everywhere. That is a deliberate divergence from lit, which guards neither and propagates from both. It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches `renderError()`, because there the boundary can report it and the next render can repair it. +**Teardown is total as well.** Removing a row is not a commit and has no retry, so a throw while tearing one down cannot be allowed to abandon the rest. Unbinding a `ref` during teardown can never abort the removal of the remaining rows, and `repeat()` drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed (which used to leave the row the app DELETED on screen, reorder the survivors, and let a later render that re-added that key reinsert the disposed instance). To make that hold, a `ref` whose object `value` setter throws is now SWALLOWED on teardown, matching the ref CALLBACK, which was already swallowed everywhere. That is a deliberate divergence from lit, which guards neither and propagates from both. It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches `renderError()`, because there the boundary can report it and the next render can repair it. Total also means a removal takes the row's own boundary markers with it, so a list that grows and shrinks all day is net zero on the nodes the renderer added, rather than accruing one invisible comment per removed row for the life of the region. Decision rules. Use `async render()` for request-time server data that should be in the first paint (the default). Add `renderFallback()` when a client re-fetch's stale content would mislead. Use `Task` / signals for genuinely client-only data (a click, viewport, live updates). For SLOW data where blocking the first byte hurts, wrap the region in `` to stream it (the only way to show a first-paint fallback; see `client-router-and-streaming.md`). Do NOT fetch in `connectedCallback` for data knowable server-side, and do NOT prop-drill what a leaf can fetch itself. diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index e4012c8c1..af2589fdd 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1701,16 +1701,45 @@ function nodesToFrag(nodes) { return frag; } -/** @param {Node} start @param {Node} end */ +/** + * Remove a template instance's whole range, its bookend markers INCLUDED. + * + * Every caller discards the instance right after (the map entry or slot that + * held it is dropped, and any replacement gets fresh markers from + * `buildDetached`), so this is a REMOVE and never lit's clear-and-reuse. A + * caller that wants to keep the bookends and render into them again needs its + * OWN function, because the two want opposite answers for the end marker. + * + * `parent` is read BEFORE the walk because the walk removes `start` on its + * first iteration, which nulls `start.parentNode`. Reading it afterwards + * compared the end marker's live parent against `null`, so the guard could + * never fire and every teardown left one `wjm-e` comment in the document, + * unbounded for the life of the region. + * + * The terminator stays `end` ITSELF rather than an `end.nextSibling` stop + * sentinel captured up front. `removeChild` runs a custom element's + * `disconnectedCallback` synchronously, so a sentinel pointing at a sibling + * this region does not own can be detached or moved mid-walk, and the walk + * would then run off the end of the child list and take the part's own marker + * with it. `end` is renderer-created and reachable only through the instance. + * + * The `end.parentNode === parent` comparison is a refusal, not a formality. A + * marker moved under a different parent is not this region's to remove, and + * `parent.removeChild(end)` on it throws NotFoundError from inside a teardown + * that has to stay total. + * + * @param {Node} start @param {Node} end + */ function removeBetween(start, end) { - if (!start.parentNode) return; + const parent = start.parentNode; + if (!parent) return; let n = start; while (n && n !== end) { const next = n.nextSibling; n.parentNode?.removeChild(n); n = next; } - if (end.parentNode === start.parentNode) end.parentNode?.removeChild(end); + if (end.parentNode === parent) parent.removeChild(end); } /* ================================================================ diff --git a/packages/core/test/rendering/marker-leak-on-teardown.test.js b/packages/core/test/rendering/marker-leak-on-teardown.test.js new file mode 100644 index 000000000..febc7e5d1 --- /dev/null +++ b/packages/core/test/rendering/marker-leak-on-teardown.test.js @@ -0,0 +1,243 @@ +/** + * Tearing an instance out takes its OWN bookend markers with it. + * + * Every rendered template instance is bracketed by a `wjm-s` / `wjm-e` comment + * pair, and `removeBetween` is what takes a range back out. It used to remove + * the start marker, the content, and then skip the end marker, because the + * guard deciding whether to remove it read `start.parentNode` AFTER the walk + * had already detached `start`. One orphan comment per teardown, accumulating + * for the life of the region. + * + * The leak is invisible to `textContent` and to `querySelectorAll`, which is + * exactly why nothing caught it for so long. So every case here counts comment + * nodes directly, and asserts BOTH halves of the accounting: the pair count is + * balanced (`s === e`), and it is back to the baseline the first render + * established. Each case also asserts the rendered output, so no test here can + * pass by rendering nothing at all. + * + * One case per distinct caller, since they reach `removeBetween` through + * different paths and a repair on one branch says nothing about the others. + */ +import { test, before } from 'node:test'; +import assert from 'node:assert/strict'; +import { parseHTML } from 'linkedom'; + +before(() => { + const { window } = parseHTML(''); + globalThis.document = window.document; + globalThis.DocumentFragment = window.DocumentFragment; + globalThis.Node = window.Node; + globalThis.Element = window.Element; + globalThis.Comment = window.Comment; + globalThis.Text = window.Text; + globalThis.NodeFilter = window.NodeFilter; + globalThis.HTMLElement = window.HTMLElement; +}); + +let html, render, repeat, MARKER; +before(async () => { + ({ html, MARKER } = await import('../../src/html.js')); + ({ render } = await import('../../src/render-client.js')); + ({ repeat } = await import('../../src/repeat.js')); +}); + +/** + * Count renderer bookends under `root`, at any depth. Reads MARKER rather than + * hardcoding the prefix, so a rename of the marker text cannot leave this + * silently counting nothing and passing. + */ +function countBookends(root) { + let s = 0; + let e = 0; + const walk = (n) => { + for (const c of n.childNodes) { + if (c.nodeType === 8) { + if (c.data === `${MARKER}s`) s++; + else if (c.data === `${MARKER}e`) e++; + } else walk(c); + } + }; + walk(root); + return { s, e }; +} + +/** Assert the bookends are paired AND back to `baseline`. */ +function assertBookends(el, baseline, label) { + const got = countBookends(el); + assert.equal(got.s, got.e, `${label}: bookends unpaired (${got.s} start, ${got.e} end)`); + assert.deepEqual(got, baseline, `${label}: bookend count drifted from baseline`); +} + +const texts = (el, sel) => [...el.querySelectorAll(sel)].map((n) => n.textContent); +const rows = (n) => Array.from({ length: n }, (_, i) => ({ id: i, t: `row ${i}` })); + +/* ------------------------------------------------------------------ * + * U1: the leftover-removal loop in reconcileRepeat. + * ------------------------------------------------------------------ */ + +test('repeat(): rows removed across many cycles leave no orphan markers', () => { + const el = document.createElement('div'); + const view = (items) => + html``; + + render(view(rows(4)), el); + const baseline = countBookends(el); + assert.equal(baseline.s, baseline.e, 'baseline is paired'); + // The row at index 0 is present in every render below, so keyed + // reconciliation must hand back the SAME element each time. This is the + // teardown-only guarantee: removing rows 2 and 3 must not perturb the rows + // that stayed. + const survivor = el.querySelector('li'); + + for (let i = 0; i < 5; i++) { + render(view(rows(2)), el); + render(view(rows(4)), el); + } + + assertBookends(el, baseline, 'repeat after 5 shrink/grow cycles'); + assert.deepEqual(texts(el, 'li'), ['row 0', 'row 1', 'row 2', 'row 3']); + assert.equal(el.querySelector('li'), survivor, 'a row that never left kept its node identity'); +}); + +/* ------------------------------------------------------------------ * + * U2: the shape-mismatch branch in reconcileRepeat, where a key SURVIVES + * but its template shape changes, so the old instance is torn out and a + * fresh one built under the same key. + * ------------------------------------------------------------------ */ + +test('repeat(): a same-key template swap leaves no orphan markers', () => { + const el = document.createElement('div'); + const view = (flip) => + html``; + + render(view(false), el); + const baseline = countBookends(el); + + for (let i = 0; i < 5; i++) { + render(view(true), el); + render(view(false), el); + } + + assertBookends(el, baseline, 'repeat after 5 same-key shape swaps'); + assert.deepEqual(texts(el, 'li b'), ['row 0', 'row 1', 'row 2']); +}); + +/* ------------------------------------------------------------------ * + * U3: removeArrayItem, the plain .map() path. A different reconciler + * (positional, not keyed) reaching the same helper. + * ------------------------------------------------------------------ */ + +test('plain .map() array: shrinking the array leaves no orphan markers', () => { + const el = document.createElement('div'); + const view = (n) => html``; + + render(view(4), el); + const baseline = countBookends(el); + + for (let i = 0; i < 5; i++) { + render(view(1), el); + render(view(4), el); + } + + assertBookends(el, baseline, 'array after 5 shrink/grow cycles'); + assert.deepEqual(texts(el, 'li'), ['row 0', 'row 1', 'row 2', 'row 3']); +}); + +test('plain .map() array: emptying it entirely leaves no orphan markers', () => { + const el = document.createElement('div'); + const view = (n) => html``; + + render(view(3), el); + const baseline = countBookends(el); + + for (let i = 0; i < 5; i++) { + render(view(0), el); + render(view(3), el); + } + + assertBookends(el, baseline, 'array after 5 empty/refill cycles'); + assert.deepEqual(texts(el, 'li'), ['row 0', 'row 1', 'row 2']); +}); + +/* ------------------------------------------------------------------ * + * U4: applyChildInnerRaw, a template-shape swap at a plain child hole + * (no list involved). + * ------------------------------------------------------------------ */ + +test('child hole: swapping template shape leaves no orphan markers', () => { + const el = document.createElement('div'); + const a = () => html`A`; + const b = () => html`B`; + + render(html`
${a()}
`, el); + const baseline = countBookends(el); + + for (let i = 0; i < 5; i++) { + render(html`
${b()}
`, el); + render(html`
${a()}
`, el); + } + + assertBookends(el, baseline, 'child hole after 5 shape swaps'); + assert.equal(el.querySelector('span').textContent, 'A'); + assert.equal(el.querySelector('em'), null); +}); + +/* ------------------------------------------------------------------ * + * U5: teardownChild reaching teardownRepeat, when a hole stops being a + * list at all. + * ------------------------------------------------------------------ */ + +test('child hole: swapping a repeat() out for a single template leaves no orphan markers', () => { + const el = document.createElement('div'); + const list = () => + html`
${repeat(rows(3), (it) => it.id, (it) => html`

${it.t}

`)}
`; + const single = () => html`
${html`solo`}
`; + + render(list(), el); + const baseline = countBookends(el); + + for (let i = 0; i < 5; i++) { + render(single(), el); + assert.equal(el.querySelector('span').textContent, 'solo'); + render(list(), el); + } + + assertBookends(el, baseline, 'child hole after 5 list/single swaps'); + assert.deepEqual(texts(el, 'p'), ['row 0', 'row 1', 'row 2']); +}); + +/* ------------------------------------------------------------------ * + * U6: the guard itself. This one pins a DECISION rather than the fix. + * ------------------------------------------------------------------ */ + +test('a marker moved under a foreign parent is refused, not reached into', () => { + // The removal is deliberately NOT an unconditional `end.remove()`. A marker + // that has been moved somewhere else is not this region's to remove, and + // removing it would rip a node out of a tree the renderer does not own. + // + // Two things this case is NOT claiming. The walk still runs off the end of + // the child list when the range is desynced like this, taking following + // siblings with it; that is the pre-existing residual documented on + // `reconcileArray`'s catch and it is out of scope here. And linkedom does + // not necessarily throw for a `removeChild` of a foreign node, so the + // survival assertion, not an absence of throw, is the arm that holds in + // every environment. + const el = document.createElement('div'); + const view = (n) => html``; + render(view(2), el); + + const ul = el.querySelector('ul'); + const stolen = [...ul.childNodes].find((n) => n.nodeType === 8 && n.data === `${MARKER}e`); + assert.ok(stolen, 'found an end marker to move'); + + const foreign = document.createElement('section'); + el.appendChild(foreign); + foreign.appendChild(stolen); + + assert.doesNotThrow(() => render(view(0), el)); + assert.equal(stolen.parentNode, foreign, 'the moved marker was left where it was moved to'); +}); diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index 16f616945..46db8ce7e 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -128,7 +128,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are covered the same way: a chunk's own commit throw, and a watch() or until() nested inside a chunk, both reach the owning component's renderError(). A chunk's own commit throw also stops the stream, since the boundary is about to render an error state and appending into a region it may have replaced is not a recovery; a nested directive throws from its own handler outside that loop, so it reaches the boundary without stopping the stream. Your own code is the exception: a throw from the iterable or from a mapper you passed alongside it is a generator failing rather than a render, so it is still logged to the console and you are expected to handle it. That ends the stream too.

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a region blank for good. Both list reconcilers additionally repair their own bookkeeping so it describes the DOM again, and the next render is an ordinary reconcile rather than a rebuild of the region, which would throw away the node identity they exist to preserve. repeat() re-unites its key map and repositions every row (the symptom was a permanently duplicated row). A plain .map() array splices back the part of its slot list the failed pass never reached, which is what a slot REPLACED rather than updated in place needs (its template shape changed, its kind changed between text, template and empty, or the array grew past its old length), since that is the branch that inserts the replacement before removing what it replaced (the symptom was a stranded row that outlived even a render of an empty array). guard() records its new deps only once the commit succeeds, so a later render with those deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

-

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

+

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError(). Covered also means a removal takes the row's own boundary markers with it, so a list that grows and shrinks all day is net zero on the nodes the renderer added, rather than accruing one invisible comment per removed row for the life of the region.

Server action errors

Errors thrown from server actions are sanitized in production: the client gets a generic "Internal server error" message plus a short digest, never the raw thrown message or the stack trace. The full error is logged server-side keyed by that digest, so a client-reported digest maps back to the server log line. A redirect() / notFound() control-flow throw passes through. To surface a specific user-facing message, return an ActionResult { success: false, error } envelope instead of throwing.

From e1fa267798bb131cdafcead51731c3ec53ed1d26 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 21:46:17 +0530 Subject: [PATCH 02/10] test: make the child-hole marker cases observe the leak they claim Two cases rendered the outer shell from two separate template literal sites, so the container saw two different `strings` identities, rebuilt the whole instance on every render, and wiped the orphan markers with `replaceChildren` before the assertion could see them. Both stayed green with the bug present, which makes them worse than no test. Render the shell from ONE site so the outer instance updates in place and the swap actually reaches the child-hole teardown. Both now fail on the reverted fix, 2 start markers against 12 end markers over five swaps. --- .../rendering/marker-leak-on-teardown.test.js | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/packages/core/test/rendering/marker-leak-on-teardown.test.js b/packages/core/test/rendering/marker-leak-on-teardown.test.js index febc7e5d1..e611f5189 100644 --- a/packages/core/test/rendering/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/marker-leak-on-teardown.test.js @@ -170,15 +170,21 @@ test('plain .map() array: emptying it entirely leaves no orphan markers', () => test('child hole: swapping template shape leaves no orphan markers', () => { const el = document.createElement('div'); + // ONE outer template site, so the outer instance UPDATES in place and the + // swap really reaches the child-hole teardown. Rendering two separate outer + // literals instead would give the container two different `strings` + // identities, rebuild the whole instance each time, and wipe the leak with + // `replaceChildren` before this could ever see it. + const view = (inner) => html`
${inner}
`; const a = () => html`A`; const b = () => html`B`; - render(html`
${a()}
`, el); + render(view(a()), el); const baseline = countBookends(el); for (let i = 0; i < 5; i++) { - render(html`
${b()}
`, el); - render(html`
${a()}
`, el); + render(view(b()), el); + render(view(a()), el); } assertBookends(el, baseline, 'child hole after 5 shape swaps'); @@ -193,17 +199,18 @@ test('child hole: swapping template shape leaves no orphan markers', () => { test('child hole: swapping a repeat() out for a single template leaves no orphan markers', () => { const el = document.createElement('div'); - const list = () => - html`
${repeat(rows(3), (it) => it.id, (it) => html`

${it.t}

`)}
`; - const single = () => html`
${html`solo`}
`; + // One outer template site, for the reason spelled out on the case above. + const view = (inner) => html`
${inner}
`; + const list = () => repeat(rows(3), (it) => it.id, (it) => html`

${it.t}

`); + const single = () => html`solo`; - render(list(), el); + render(view(list()), el); const baseline = countBookends(el); for (let i = 0; i < 5; i++) { - render(single(), el); + render(view(single()), el); assert.equal(el.querySelector('span').textContent, 'solo'); - render(list(), el); + render(view(list()), el); } assertBookends(el, baseline, 'child hole after 5 list/single swaps'); From 6b74ace4adc487a033c23730a41c83cffad211bb Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 21:49:15 +0530 Subject: [PATCH 03/10] test: cover the marker teardown in a real browser and inside a slot linkedom never runs disconnectedCallback, so the unit suite cannot show the removal staying on its rails while author code executes synchronously from inside removeChild. That re-entrancy is why the walk terminates on the end marker rather than on a stop sentinel, so it needs a real browser. The slot case is the one that is not a duplicate of the renderer cases: the fix hands the light-DOM backstop one extra removedNodes record per torn-down row, and those comments were never in the authored record, so churning a slotted list must leave both the record and the assigned set untouched. Also correct the cache suite's comment-stripping helper, which described a marker syntax the renderer does not use and gave no hint that stripping comments makes marker accounting unobservable. --- .../browser/directives-cache.test.js | 7 +- .../browser/marker-leak-on-teardown.test.js | 166 ++++++++++++++++++ .../slots/browser/record-self-heal.test.js | 50 ++++++ 3 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 packages/core/test/rendering/browser/marker-leak-on-teardown.test.js diff --git a/packages/core/test/directives/browser/directives-cache.test.js b/packages/core/test/directives/browser/directives-cache.test.js index 4b5328107..bf9986a44 100644 --- a/packages/core/test/directives/browser/directives-cache.test.js +++ b/packages/core/test/directives/browser/directives-cache.test.js @@ -22,8 +22,11 @@ import { cache } from '../../../src/directives.js'; import { assert } from '../../../../../test/browser-assert.js'; /** - * Strip webjs marker comments (the framework injects `` - * style comments around dynamic parts; tests assert plain HTML). + * Strip the renderer's marker comments (it brackets each instance with a + * `wjm-s` / `wjm-e` pair and marks each hole with a `wjm-N`; tests here assert + * plain HTML). Note this hides marker ACCOUNTING by construction, so it cannot + * be used to show markers are balanced. Assertions about that live in + * `rendering/marker-leak-on-teardown.test.js`, which counts them directly. */ function stripExpressionComments(s) { return s.replace(//g, ''); diff --git a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js new file mode 100644 index 000000000..ec390a975 --- /dev/null +++ b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js @@ -0,0 +1,166 @@ +/** + * Real-browser assertions for the teardown that takes its own end marker. + * + * The unit suite proves the accounting under linkedom. Two things it cannot + * reach live here. A component drives its re-renders through the update + * pipeline rather than a bare `render()` call, so the leak has to be shown on + * that path too. And linkedom never runs `disconnectedCallback`, while a real + * browser runs it SYNCHRONOUSLY from inside `removeChild`, which means author + * code executes in the middle of the removal walk. That re-entrancy is the + * whole reason the walk terminates on the end marker itself rather than on a + * stop sentinel captured before the walk starts: a sentinel is an arbitrary + * sibling this region does not own, and a `disconnectedCallback` that moved it + * would send the walk off the end of the child list. + */ +import { html } from '../../../src/html.js'; +import { MARKER } from '../../../src/html.js'; +import { repeat } from '../../../src/repeat.js'; +import { WebComponent } from '../../../src/component.js'; + +import { assert } from '../../../../../test/browser-assert.js'; + +let uid = 0; +const tagName = (p) => `${p}-${uid++}`; + +const tick = () => new Promise((r) => queueMicrotask(() => queueMicrotask(r))); + +/** Count renderer bookends under `root`, at any depth. */ +function countBookends(root) { + let s = 0; + let e = 0; + const walk = (n) => { + for (const c of n.childNodes) { + if (c.nodeType === 8) { + if (c.data === `${MARKER}s`) s++; + else if (c.data === `${MARKER}e`) e++; + } else walk(c); + } + }; + walk(root); + return { s, e }; +} + +const rows = (n) => Array.from({ length: n }, (_, i) => ({ id: i, t: `row ${i}` })); + +suite('teardown takes its own end marker', () => { + test('a component churning a repeat() list stays flat on marker count', async () => { + const tag = tagName('marker-churn'); + class C extends WebComponent({ items: Array }) { + constructor() { + super(); + this.items = rows(4); + } + render() { + return html`
    ${repeat(this.items, (it) => it.id, (it) => html`
  • ${it.t}
    • `)}
`; + } + } + C.register(tag); + + const host = document.createElement(tag); + document.body.appendChild(host); + await host.updateComplete; + await tick(); + + try { + const baseline = countBookends(host); + assert.equal(baseline.s, baseline.e, 'baseline is paired'); + assert.ok(baseline.e > 0, 'the list really rendered bookends'); + const survivor = host.querySelector('li'); + + for (let i = 0; i < 5; i++) { + host.items = rows(2); + await host.updateComplete; + host.items = rows(4); + await host.updateComplete; + } + await tick(); + + const got = countBookends(host); + assert.equal(got.s, got.e, `bookends unpaired: ${got.s} start, ${got.e} end`); + assert.equal(got.e, baseline.e, 'end markers drifted from baseline'); + assert.equal(host.querySelectorAll('li').length, 4, 'all four rows rendered'); + assert.equal(host.querySelector('li'), survivor, 'a row that never left kept its identity'); + } finally { + host.remove(); + } + }); + + test('a disconnectedCallback running mid-walk does not derail the removal', async () => { + // This reds on the reverted fix like the case above (it counts bookends + // too), but that is not what it is for. It exists to prove the removal + // stays on its rails while synchronous author code runs INSIDE the walk, + // which is the property linkedom cannot express and the reason the walk + // terminates on the end marker rather than on a stop sentinel. + const seen = []; + // A fixed tag, because a tag NAME is not a hole position in an `html` + // template; only attribute and child positions are. + const cellTag = 'marker-leak-cell'; + class Cell extends WebComponent({ label: String }) { + disconnectedCallback() { + super.disconnectedCallback?.(); + // Author code, running synchronously from inside `removeChild`, that + // writes to a container OUTSIDE the range being torn down. + seen.push(this.label); + const note = document.createElement('i'); + note.className = 'gone'; + note.textContent = this.label; + document.querySelector('#marker-sink')?.appendChild(note); + } + render() { + return html`${this.label}`; + } + } + Cell.register(cellTag); + + const tag = tagName('marker-host'); + class C extends WebComponent({ items: Array }) { + constructor() { + super(); + this.items = rows(4); + } + render() { + return html`
    ${repeat( + this.items, + (it) => it.id, + (it) => html`
    • `, + )}
`; + } + } + C.register(tag); + + const sink = document.createElement('div'); + sink.id = 'marker-sink'; + document.body.appendChild(sink); + const host = document.createElement(tag); + document.body.appendChild(host); + await host.updateComplete; + await tick(); + + try { + const baseline = countBookends(host); + assert.equal(baseline.s, baseline.e, 'baseline is paired'); + + host.items = rows(1); + await host.updateComplete; + await tick(); + + assert.ok(seen.length > 0, 'a disconnectedCallback really ran during the removal'); + assert.equal(host.querySelectorAll('li').length, 1, 'the shrink completed'); + assert.equal(host.querySelector('li span').textContent, 'row 0', 'the survivor is intact'); + + // The region is still usable: the part marker survived the walk, so a + // later grow renders back into the same place. + host.items = rows(4); + await host.updateComplete; + await tick(); + + assert.equal(host.querySelectorAll('li').length, 4, 'the region still renders after the walk'); + const got = countBookends(host); + assert.equal(got.s, got.e, `bookends unpaired: ${got.s} start, ${got.e} end`); + assert.equal(got.e, baseline.e, 'end markers drifted from baseline'); + } finally { + host.remove(); + sink.remove(); + } + }); +}); diff --git a/packages/core/test/slots/browser/record-self-heal.test.js b/packages/core/test/slots/browser/record-self-heal.test.js index b96b52f87..b01f0fef4 100644 --- a/packages/core/test/slots/browser/record-self-heal.test.js +++ b/packages/core/test/slots/browser/record-self-heal.test.js @@ -131,6 +131,56 @@ suite('Record self-heal + overlay coherence (review round 16)', () => { } }); + test('a list churning inside the slot leaves no bookends behind and does not perturb the record', async () => { + // A teardown removes the row's own boundary markers, and inside a slot that + // removal is seen by the backstop as one extra `removedNodes` entry per + // row. Those comments were never in the authored record (they sit between + // the host instance's own bookends, so `instanceOwns` claims them for the + // renderer), so churning the list must leave the record and the assignment + // exactly where they started. The marker count is asserted directly, + // because the leak is invisible to `querySelectorAll`. + ensureFixedShell(); + const parentTag = tagName('heal-churn'); + const rows = (n) => Array.from({ length: n }, (_, i) => `i${i}`); + class P extends WebComponent({ items: Array }) { + constructor() { super(); this.items = rows(4); } + render() { + return html`${this.items.map( + (i) => html`

${i}

`, + )}`; + } + } + P.register(parentTag); + const parent = document.createElement(parentTag); + document.body.appendChild(parent); + await tick(); + try { + const shell = parent.querySelector(fixedShell); + const slot = shell.querySelector('slot[data-webjs-light]'); + const endMarkers = (root) => + [...root.childNodes].filter((n) => n.nodeType === 8 && n.data === 'wjm-e').length; + + assert.equal(slot.querySelectorAll('.item').length, 4, 'four items projected'); + const baseMarkers = endMarkers(slot); + const baseAssigned = slot.assignedNodes().length; + + for (let i = 0; i < 5; i++) { + parent.items = rows(1); + await parent.updateComplete; + await tick(); + parent.items = rows(4); + await parent.updateComplete; + await tick(); + } + + assert.equal(endMarkers(slot), baseMarkers, 'end markers under the slot drifted'); + assert.equal(slot.querySelectorAll('.item').length, 4, 'all four items are projected again'); + assert.equal(slot.assignedNodes().length, baseAssigned, 'the assigned set drifted'); + } finally { + parent.remove(); + } + }); + test('a library write into the assigned container survives the next apply', async () => { const tag = tagName('lib-write'); const host = await mount(tag, () => html`
`); From f20386cb335570b34d9a81c8128791c65d341ceb Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:13:00 +0530 Subject: [PATCH 04/10] fix: state the terminator argument as the claim the walk actually needs The docblock said the end marker is "reachable only through the instance", which the paragraph immediately below it contradicts: that guard exists precisely because the marker CAN be moved under a different parent, and the new unit case does exactly that. Say the smaller, true thing instead. The walk needs to be bounded by the range the instance defines, not for its terminator to be immovable. Also read MARKER in the slot case rather than hardcoding the prefix, and assert the baseline count is non-zero. Without both, a rename of the marker text leaves that case counting nothing before and after, passing while asserting nothing, which is the failure mode the unit file's own docblock claims the suite avoids. --- packages/core/src/render-client.js | 12 ++++++++---- .../core/test/slots/browser/record-self-heal.test.js | 8 ++++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index af2589fdd..356a12743 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1718,10 +1718,14 @@ function nodesToFrag(nodes) { * * The terminator stays `end` ITSELF rather than an `end.nextSibling` stop * sentinel captured up front. `removeChild` runs a custom element's - * `disconnectedCallback` synchronously, so a sentinel pointing at a sibling - * this region does not own can be detached or moved mid-walk, and the walk - * would then run off the end of the child list and take the part's own marker - * with it. `end` is renderer-created and reachable only through the instance. + * `disconnectedCallback` synchronously, so author code runs part-way through + * the walk. A sentinel is whatever node happened to follow the range, which + * this region does not own and has no claim on: move or detach it mid-walk + * and the walk never meets its terminator, runs off the end of the child + * list, and takes the part's own marker with it. Terminating on `end` bounds + * the walk by the range the instance itself defines. That is a smaller claim + * than `end` being immovable, which it is not (the guard below is there + * precisely because it can be moved), and it is the claim the walk needs. * * The `end.parentNode === parent` comparison is a refusal, not a formality. A * marker moved under a different parent is not this region's to remove, and diff --git a/packages/core/test/slots/browser/record-self-heal.test.js b/packages/core/test/slots/browser/record-self-heal.test.js index b01f0fef4..3c0b6ecd9 100644 --- a/packages/core/test/slots/browser/record-self-heal.test.js +++ b/packages/core/test/slots/browser/record-self-heal.test.js @@ -8,7 +8,7 @@ * Runs in a REAL browser via WTR + Playwright. */ import { WebComponent } from '../../../src/component.js'; -import { html } from '../../../src/html.js'; +import { html, MARKER } from '../../../src/html.js'; import { repeat, cache, asyncAppend } from '../../../src/directives.js'; import { projectAuthored, isAuthoredContentSlot } from '../../../src/slot.js'; @@ -157,11 +157,15 @@ suite('Record self-heal + overlay coherence (review round 16)', () => { try { const shell = parent.querySelector(fixedShell); const slot = shell.querySelector('slot[data-webjs-light]'); + // Reads MARKER rather than hardcoding the prefix: a rename would + // otherwise leave this counting zero markers before AND after, so it + // would pass while asserting nothing at all. const endMarkers = (root) => - [...root.childNodes].filter((n) => n.nodeType === 8 && n.data === 'wjm-e').length; + [...root.childNodes].filter((n) => n.nodeType === 8 && n.data === `${MARKER}e`).length; assert.equal(slot.querySelectorAll('.item').length, 4, 'four items projected'); const baseMarkers = endMarkers(slot); + assert.ok(baseMarkers > 0, 'the projected rows really carry end markers to count'); const baseAssigned = slot.assignedNodes().length; for (let i = 0; i < 5; i++) { From b32989995047dc98f175a3ae6679a2d493bbe1e7 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:21:37 +0530 Subject: [PATCH 05/10] docs: stop the docblock arguing a choice this change never made The terminator was already the end marker before this fix; only the parent capture changed. Arguing for it in the docblock meant defending a property the walk does not have, since stepping nextSibling to `end` overruns exactly like a stop sentinel would if the range is mutated underneath it. Drop the comparison and say the two things that are true: the guard refuses a marker that sits somewhere else, and the walk assumes an intact range, which it did before this change too. The consequence of a desynced range is already written up on reconcileRepeat's catch, so point there. The unit case cited the wrong catch for that overrun, and the browser case claimed to justify the terminator while its disconnectedCallback only writes outside the range, so it proved no such thing. Both now say what they do. --- packages/core/src/render-client.js | 19 +++++++---------- .../browser/marker-leak-on-teardown.test.js | 21 +++++++++++-------- .../rendering/marker-leak-on-teardown.test.js | 12 ++++++----- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 356a12743..68e140fc9 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1716,22 +1716,19 @@ function nodesToFrag(nodes) { * never fire and every teardown left one `wjm-e` comment in the document, * unbounded for the life of the region. * - * The terminator stays `end` ITSELF rather than an `end.nextSibling` stop - * sentinel captured up front. `removeChild` runs a custom element's - * `disconnectedCallback` synchronously, so author code runs part-way through - * the walk. A sentinel is whatever node happened to follow the range, which - * this region does not own and has no claim on: move or detach it mid-walk - * and the walk never meets its terminator, runs off the end of the child - * list, and takes the part's own marker with it. Terminating on `end` bounds - * the walk by the range the instance itself defines. That is a smaller claim - * than `end` being immovable, which it is not (the guard below is there - * precisely because it can be moved), and it is the claim the walk needs. - * * The `end.parentNode === parent` comparison is a refusal, not a formality. A * marker moved under a different parent is not this region's to remove, and * `parent.removeChild(end)` on it throws NotFoundError from inside a teardown * that has to stay total. * + * The walk assumes the range is INTACT: it steps `nextSibling` from `start` + * and stops on `end`, so a range whose end no longer follows its start runs + * off the child list and takes the part's own marker with it. That is not + * something this function defends against, before or after the parent capture, + * and the consequence is spelled out where it bites, on `reconcileRepeat`'s + * catch below. The guard is the narrower promise: whatever the walk did, a + * marker that is somewhere else is left alone. + * * @param {Node} start @param {Node} end */ function removeBetween(start, end) { diff --git a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js index ec390a975..4e0fd8eff 100644 --- a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js @@ -5,12 +5,9 @@ * reach live here. A component drives its re-renders through the update * pipeline rather than a bare `render()` call, so the leak has to be shown on * that path too. And linkedom never runs `disconnectedCallback`, while a real - * browser runs it SYNCHRONOUSLY from inside `removeChild`, which means author - * code executes in the middle of the removal walk. That re-entrancy is the - * whole reason the walk terminates on the end marker itself rather than on a - * stop sentinel captured before the walk starts: a sentinel is an arbitrary - * sibling this region does not own, and a `disconnectedCallback` that moved it - * would send the walk off the end of the child list. + * browser runs it SYNCHRONOUSLY from inside `removeChild`, so author code + * executes part-way through the removal walk, on a range the walk is still + * stepping through. Nothing in the unit suite exercises that at all. */ import { html } from '../../../src/html.js'; import { MARKER } from '../../../src/html.js'; @@ -88,9 +85,15 @@ suite('teardown takes its own end marker', () => { test('a disconnectedCallback running mid-walk does not derail the removal', async () => { // This reds on the reverted fix like the case above (it counts bookends // too), but that is not what it is for. It exists to prove the removal - // stays on its rails while synchronous author code runs INSIDE the walk, - // which is the property linkedom cannot express and the reason the walk - // terminates on the end marker rather than on a stop sentinel. + // finishes, and the region stays renderable, while a `disconnectedCallback` + // runs synchronously in the middle of the walk, which is the property + // linkedom cannot express at all. + // + // The callback here writes OUTSIDE the range being torn down, which is what + // ordinary author code does. It deliberately does not move a node the walk + // is about to step onto: a range mutated underneath the walk overruns, that + // is true before and after this fix, and this case is not the place to + // claim otherwise. const seen = []; // A fixed tag, because a tag NAME is not a hole position in an `html` // template; only attribute and child positions are. diff --git a/packages/core/test/rendering/marker-leak-on-teardown.test.js b/packages/core/test/rendering/marker-leak-on-teardown.test.js index e611f5189..5cdfe53c2 100644 --- a/packages/core/test/rendering/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/marker-leak-on-teardown.test.js @@ -228,11 +228,13 @@ test('a marker moved under a foreign parent is refused, not reached into', () => // // Two things this case is NOT claiming. The walk still runs off the end of // the child list when the range is desynced like this, taking following - // siblings with it; that is the pre-existing residual documented on - // `reconcileArray`'s catch and it is out of scope here. And linkedom does - // not necessarily throw for a `removeChild` of a foreign node, so the - // survival assertion, not an absence of throw, is the arm that holds in - // every environment. + // siblings with it. That overrun is a property of the walk both before and + // after this fix, described on `reconcileRepeat`'s catch as the reason a + // half-removed row is never re-reached, and it is out of scope here: the + // guard's job starts after the walk, not during it. And linkedom does not + // necessarily throw for a `removeChild` of a foreign node, so the survival + // assertion, not an absence of throw, is the arm that holds in every + // environment. const el = document.createElement('div'); const view = (n) => html`
    ${rows(n).map((it) => html`
  • ${it.t}
    • `)}
`; render(view(2), el); From 9f1c068232a2748e6b921df3e89b9fd9f5663666 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:29:14 +0530 Subject: [PATCH 06/10] docs: drop the last overclaim from the browser suite header The header still said the callback runs "on a range the walk is still stepping through", which is the same overclaim the case's own inline comment now denies: it writes outside the range on purpose. What is true, and all that is needed, is that the callback runs part-way through the removal rather than after it. --- .../test/rendering/browser/marker-leak-on-teardown.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js index 4e0fd8eff..139f55ce5 100644 --- a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js @@ -6,8 +6,8 @@ * pipeline rather than a bare `render()` call, so the leak has to be shown on * that path too. And linkedom never runs `disconnectedCallback`, while a real * browser runs it SYNCHRONOUSLY from inside `removeChild`, so author code - * executes part-way through the removal walk, on a range the walk is still - * stepping through. Nothing in the unit suite exercises that at all. + * executes part-way through the removal rather than after it. Nothing in the + * unit suite exercises that at all. */ import { html } from '../../../src/html.js'; import { MARKER } from '../../../src/html.js'; From 08d0134d336d287d7428c7ef2a3bf4fd17928c70 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:45:35 +0530 Subject: [PATCH 07/10] test: correct what the browser cases claim over the unit suite I wrote that linkedom never runs disconnectedCallback. It does: it flips a reactive flag on the first customElements.define and fires the callback synchronously from removeChild, walking the removed subtree, and the repo's own lifecycle unit test already depends on that. So the browser case earns its place on fidelity, running the real engines, not on linkedom being incapable. Say that, and leave a note so the stronger claim does not come back. The unit file's header also claimed every case counts markers and asserts rendered output. The guard case does neither, by design, so scope the claim to the six leak cases and say what the seventh is for. Add the non-zero baseline assertion to the second browser case, which is the one place the earlier sweep for it missed, so a marker rename reds it rather than leaving every count comparing zero against zero. --- .../browser/marker-leak-on-teardown.test.js | 22 +++++++++++-------- .../rendering/marker-leak-on-teardown.test.js | 17 ++++++++------ 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js index 139f55ce5..3dcc498d4 100644 --- a/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/browser/marker-leak-on-teardown.test.js @@ -1,13 +1,16 @@ /** * Real-browser assertions for the teardown that takes its own end marker. * - * The unit suite proves the accounting under linkedom. Two things it cannot - * reach live here. A component drives its re-renders through the update - * pipeline rather than a bare `render()` call, so the leak has to be shown on - * that path too. And linkedom never runs `disconnectedCallback`, while a real - * browser runs it SYNCHRONOUSLY from inside `removeChild`, so author code - * executes part-way through the removal rather than after it. Nothing in the - * unit suite exercises that at all. + * The unit suite proves the accounting under linkedom, driving the renderer + * with bare `render()` calls. Two things are left for a real engine. A + * component reaches the same teardown through the update pipeline instead, so + * the leak is shown on the path an app actually takes. And `removeChild` runs + * a custom element's `disconnectedCallback` synchronously, which means author + * code lands part-way through the removal walk; that is worth pinning on the + * engines people run rather than on a shim, and no unit case covers it. + * + * (linkedom does fire `disconnectedCallback`, so the second one is a question + * of fidelity rather than of capability. Do not write the stronger claim.) */ import { html } from '../../../src/html.js'; import { MARKER } from '../../../src/html.js'; @@ -86,8 +89,8 @@ suite('teardown takes its own end marker', () => { // This reds on the reverted fix like the case above (it counts bookends // too), but that is not what it is for. It exists to prove the removal // finishes, and the region stays renderable, while a `disconnectedCallback` - // runs synchronously in the middle of the walk, which is the property - // linkedom cannot express at all. + // runs synchronously in the middle of the walk, on the engines people + // actually run. // // The callback here writes OUTSIDE the range being torn down, which is what // ordinary author code does. It deliberately does not move a node the walk @@ -142,6 +145,7 @@ suite('teardown takes its own end marker', () => { try { const baseline = countBookends(host); assert.equal(baseline.s, baseline.e, 'baseline is paired'); + assert.ok(baseline.e > 0, 'the list really rendered bookends to count'); host.items = rows(1); await host.updateComplete; diff --git a/packages/core/test/rendering/marker-leak-on-teardown.test.js b/packages/core/test/rendering/marker-leak-on-teardown.test.js index 5cdfe53c2..caade7ff1 100644 --- a/packages/core/test/rendering/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/marker-leak-on-teardown.test.js @@ -9,14 +9,17 @@ * for the life of the region. * * The leak is invisible to `textContent` and to `querySelectorAll`, which is - * exactly why nothing caught it for so long. So every case here counts comment - * nodes directly, and asserts BOTH halves of the accounting: the pair count is - * balanced (`s === e`), and it is back to the baseline the first render - * established. Each case also asserts the rendered output, so no test here can - * pass by rendering nothing at all. + * exactly why nothing caught it for so long. So the six LEAK cases count + * comment nodes directly, and assert BOTH halves of the accounting: the pair + * count is balanced (`s === e`), and it is back to the baseline the first + * render established. Each also asserts the rendered output, so none of them + * can pass by rendering nothing at all. One case per distinct caller, since + * they reach `removeBetween` through different paths and a repair on one + * branch says nothing about the others. * - * One case per distinct caller, since they reach `removeBetween` through - * different paths and a repair on one branch says nothing about the others. + * The LAST case is not one of those and does not count markers at all. It + * pins the guard, which is a separate decision from the leak, and it is green + * with or without the fix by design. Its own comment says so. */ import { test, before } from 'node:test'; import assert from 'node:assert/strict'; From d0708ae099604ec06f2122c04660a853384287ec Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 22:48:26 +0530 Subject: [PATCH 08/10] docs: cut the last three claims that outrun what I verified The docblock said a replacement instance gets its markers from buildDetached. The child-hole caller creates them inline, so say markers of its own and stop naming a source that is only sometimes right. The unit header said one case per caller when the two array cases share removeArrayItem, one shrinking and one emptying. The guard case hedged that linkedom might not throw for a removeChild of a foreign node. It does throw, same as a browser, so both of that case's arms discriminate: an unguarded removeChild reds the first, an unconditional end.remove() reds the second. Verified by toggling each. It also pointed at reconcileRepeat's catch for the wrong reason; that catch describes the same overrun, but as grounds for rejecting an alternative. --- packages/core/src/render-client.js | 6 ++-- .../rendering/marker-leak-on-teardown.test.js | 28 +++++++++++-------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 68e140fc9..0613ef7e4 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1704,9 +1704,9 @@ function nodesToFrag(nodes) { /** * Remove a template instance's whole range, its bookend markers INCLUDED. * - * Every caller discards the instance right after (the map entry or slot that - * held it is dropped, and any replacement gets fresh markers from - * `buildDetached`), so this is a REMOVE and never lit's clear-and-reuse. A + * Every caller discards the instance right after: the map entry or slot that + * held it is dropped, and a replacement, where there is one, is built with + * markers of its own. So this is a REMOVE, never lit's clear-and-reuse. A * caller that wants to keep the bookends and render into them again needs its * OWN function, because the two want opposite answers for the end marker. * diff --git a/packages/core/test/rendering/marker-leak-on-teardown.test.js b/packages/core/test/rendering/marker-leak-on-teardown.test.js index caade7ff1..9b478974a 100644 --- a/packages/core/test/rendering/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/marker-leak-on-teardown.test.js @@ -13,9 +13,10 @@ * comment nodes directly, and assert BOTH halves of the accounting: the pair * count is balanced (`s === e`), and it is back to the baseline the first * render established. Each also asserts the rendered output, so none of them - * can pass by rendering nothing at all. One case per distinct caller, since - * they reach `removeBetween` through different paths and a repair on one - * branch says nothing about the others. + * can pass by rendering nothing at all. They cover every `removeBetween` + * caller, since each reaches it through a different path and a repair on one + * branch says nothing about the others (the two array cases share a caller, + * one shrinking the list and one emptying it). * * The LAST case is not one of those and does not count markers at all. It * pins the guard, which is a separate decision from the leak, and it is green @@ -229,15 +230,18 @@ test('a marker moved under a foreign parent is refused, not reached into', () => // that has been moved somewhere else is not this region's to remove, and // removing it would rip a node out of a tree the renderer does not own. // - // Two things this case is NOT claiming. The walk still runs off the end of - // the child list when the range is desynced like this, taking following - // siblings with it. That overrun is a property of the walk both before and - // after this fix, described on `reconcileRepeat`'s catch as the reason a - // half-removed row is never re-reached, and it is out of scope here: the - // guard's job starts after the walk, not during it. And linkedom does not - // necessarily throw for a `removeChild` of a foreign node, so the survival - // assertion, not an absence of throw, is the arm that holds in every - // environment. + // What this case is NOT claiming: that the walk copes with a desynced + // range. It does not. It still runs off the end of the child list here, + // taking following siblings with it, before this fix and after it. + // `reconcileRepeat`'s catch describes that same overrun, as the reason it + // rejects an alternative that would leave a start marker stranded past its + // end. Out of scope either way, because the guard's job starts after the + // walk, not during it. + // + // Both assertions below discriminate. linkedom throws for a `removeChild` + // of a node under a different parent, exactly as a browser does, so an + // unguarded `parent.removeChild(end)` reds the first arm, and an + // unconditional `end.remove()` reds the second by stealing the comment. const el = document.createElement('div'); const view = (n) => html`
    ${rows(n).map((it) => html`
  • ${it.t}
    • `)}
`; render(view(2), el); From 949aa9a0cc152b431729b9e27b10c5b558c9065e Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:11:02 +0530 Subject: [PATCH 09/10] test: make every leak case fail when it stops counting anything countBookends matches MARKER plus an `s` / `e` suffix spelled in the test, so it tracks half the marker text. Rename the other half in the renderer and all six leak cases stayed green, comparing zero against zero: the assertions were satisfied by a counter that had stopped seeing anything. The rendered output assertions do not cover this, since they catch rendering nothing rather than counting nothing. Take every baseline through a helper that refuses an empty or unpaired count. Mutating the suffix now reds all seven cases where it reded one. U5's label also named teardownChild reaching teardownRepeat, a path no case in the file takes. The two directions go through applyChildInnerRaw's own branch and teardownChild's strings branch, so say that. --- .../rendering/marker-leak-on-teardown.test.js | 44 ++++++++++++++----- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/packages/core/test/rendering/marker-leak-on-teardown.test.js b/packages/core/test/rendering/marker-leak-on-teardown.test.js index 9b478974a..a8f72f6d7 100644 --- a/packages/core/test/rendering/marker-leak-on-teardown.test.js +++ b/packages/core/test/rendering/marker-leak-on-teardown.test.js @@ -46,9 +46,14 @@ before(async () => { }); /** - * Count renderer bookends under `root`, at any depth. Reads MARKER rather than - * hardcoding the prefix, so a rename of the marker text cannot leave this - * silently counting nothing and passing. + * Count renderer bookends under `root`, at any depth. + * + * It reads MARKER for the prefix but spells the `s` / `e` suffix here, so it + * tracks only half of the marker text and a rename of the other half would + * leave it counting zero. That is what `baselineBookends` is for: nothing in + * this file compares two counts without first proving the baseline is + * non-zero, so a counter that has stopped seeing anything fails instead of + * passing on `0 === 0`. */ function countBookends(root) { let s = 0; @@ -65,6 +70,19 @@ function countBookends(root) { return { s, e }; } +/** + * Capture the post-first-render count every leak case measures against, and + * refuse a baseline that is empty or unpaired. Without the non-zero arm a + * counter that matches nothing makes every later comparison `0 === 0`, and + * the case passes while observing exactly nothing. + */ +function baselineBookends(el, label) { + const got = countBookends(el); + assert.equal(got.s, got.e, `${label}: baseline unpaired (${got.s} start, ${got.e} end)`); + assert.ok(got.e > 0, `${label}: baseline counted no bookends, so nothing below can fail`); + return got; +} + /** Assert the bookends are paired AND back to `baseline`. */ function assertBookends(el, baseline, label) { const got = countBookends(el); @@ -85,8 +103,7 @@ test('repeat(): rows removed across many cycles leave no orphan markers', () => html`
    ${repeat(items, (it) => it.id, (it) => html`
  • ${it.t}
    • `)}
`; render(view(rows(4)), el); - const baseline = countBookends(el); - assert.equal(baseline.s, baseline.e, 'baseline is paired'); + const baseline = baselineBookends(el, 'repeat'); // The row at index 0 is present in every render below, so keyed // reconciliation must hand back the SAME element each time. This is the // teardown-only guarantee: removing rows 2 and 3 must not perturb the rows @@ -119,7 +136,7 @@ test('repeat(): a same-key template swap leaves no orphan markers', () => { )}`; render(view(false), el); - const baseline = countBookends(el); + const baseline = baselineBookends(el, 'same-key swap'); for (let i = 0; i < 5; i++) { render(view(true), el); @@ -140,7 +157,7 @@ test('plain .map() array: shrinking the array leaves no orphan markers', () => { const view = (n) => html`
    ${rows(n).map((it) => html`
  • ${it.t}
    • `)}
`; render(view(4), el); - const baseline = countBookends(el); + const baseline = baselineBookends(el, 'array shrink'); for (let i = 0; i < 5; i++) { render(view(1), el); @@ -156,7 +173,7 @@ test('plain .map() array: emptying it entirely leaves no orphan markers', () => const view = (n) => html`
    ${rows(n).map((it) => html`
  • ${it.t}
    • `)}
`; render(view(3), el); - const baseline = countBookends(el); + const baseline = baselineBookends(el, 'array empty'); for (let i = 0; i < 5; i++) { render(view(0), el); @@ -184,7 +201,7 @@ test('child hole: swapping template shape leaves no orphan markers', () => { const b = () => html`B`; render(view(a()), el); - const baseline = countBookends(el); + const baseline = baselineBookends(el, 'child hole swap'); for (let i = 0; i < 5; i++) { render(view(b()), el); @@ -197,8 +214,11 @@ test('child hole: swapping template shape leaves no orphan markers', () => { }); /* ------------------------------------------------------------------ * - * U5: teardownChild reaching teardownRepeat, when a hole stops being a - * list at all. + * U5: a hole that stops being a list, and becomes one again. Each direction + * takes a different route: list to single tears the repeat down through + * `applyChildInnerRaw`'s own branch, and single to list removes the plain + * instance through `teardownChild`'s `strings` branch. `teardownChild`'s + * repeat branch is NOT what this reaches, and nothing in this file does. * ------------------------------------------------------------------ */ test('child hole: swapping a repeat() out for a single template leaves no orphan markers', () => { @@ -209,7 +229,7 @@ test('child hole: swapping a repeat() out for a single template leaves no orphan const single = () => html`solo`; render(view(list()), el); - const baseline = countBookends(el); + const baseline = baselineBookends(el, 'child hole list/single'); for (let i = 0; i < 5; i++) { render(view(single()), el); From 21a6021179aca16452fade351e978647d0c02be2 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 5 Aug 2026 23:31:13 +0530 Subject: [PATCH 10/10] test: correct what protects the slot counter from a vacuous pass The comment credited reading MARKER with stopping a rename from leaving the counter blind. It does not: the helper spells the `e` suffix locally, so it tracks half the marker text, and the non-zero assertion two lines below is what actually holds. That is the same conclusion the unit file reached, and this comment was still asserting the opposite mechanism. --- packages/core/test/slots/browser/record-self-heal.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/core/test/slots/browser/record-self-heal.test.js b/packages/core/test/slots/browser/record-self-heal.test.js index 3c0b6ecd9..b725a5d5d 100644 --- a/packages/core/test/slots/browser/record-self-heal.test.js +++ b/packages/core/test/slots/browser/record-self-heal.test.js @@ -157,9 +157,10 @@ suite('Record self-heal + overlay coherence (review round 16)', () => { try { const shell = parent.querySelector(fixedShell); const slot = shell.querySelector('slot[data-webjs-light]'); - // Reads MARKER rather than hardcoding the prefix: a rename would - // otherwise leave this counting zero markers before AND after, so it - // would pass while asserting nothing at all. + // Reads MARKER for the prefix, but spells the `e` suffix here, so it + // still tracks only half the marker text. The non-zero assertion below + // is what actually stops a counter that has gone blind from passing on + // 0 against 0; reading MARKER on its own would not. const endMarkers = (root) => [...root.childNodes].filter((n) => n.nodeType === 8 && n.data === `${MARKER}e`).length;