Skip to content

dogfood: SSR mis-detects element boundaries when a tag sits inside a comment #1133

Description

@vivek7405

Problem

findClosingTagInString matches a closing tag that is inside an HTML comment, so a comment
containing </some-tag> is mistaken for the element's real end. This is the mirror image of
#1128 (which was about OPENING tags in comments, fixed in #1132) and it corrupts output the
same way: the emitted markup ends up with an unterminated comment that swallows real closing
tags, plus a duplicated close tag.

Reproduced on the #1132 branch, so this survives that fix:

class Card extends WebComponent { render() { return html`<div class="card"><slot></slot></div>`; } }
Card.register('c-card');

await renderToString(html`<c-card>kid<!-- </c-card> --></c-card><span>ok</span>`);

Actual output:

<c-card data-wj-host><!--webjs-hydrate--><div class="card"><slot data-webjs-light
data-projection="actual" data-wj-slot-owner="c-card">kid<!-- </slot></div></c-card> -->
</c-card><span>ok</span>

The authored children were taken as kid<!-- (truncated at the commented close tag), so the
projected slot content ends with an unterminated <!--. A browser then reads
</slot></div></c-card> as comment data, and a second </c-card> is left over. The visible
result is a broken card and a stray --> in the text.

The same helper is used for <slot> and <webjs-suspense>, so the same input shape breaks
those too. <slot>fb<!-- </slot> --></slot> corrupts through the slot path. A commented
</webjs-suspense> inside a real boundary mis-splits the boundary's inner content from what
follows it, which is byte-harmless in the blocking path but divergent under streaming, where
the boundary id then wraps only the truncated inner content.

Found by the review of #1132, which fixed the opening-tag half of the family.

Design / approach

#1132 added an inertRanges(html) tokenizer to packages/core/src/render-server.js that
returns the byte ranges where a tag-shaped match is text rather than markup (comments, markup
declarations, tag interiors, raw text, RCDATA), plus an inRanges(ranges, index) helper. The
straightforward fix is to make findClosingTagInString consult the same ranges and skip a
candidate close tag that starts inside one.

The thing to decide deliberately is WHERE the ranges get computed, because this helper is
called inside loops and per component:

  • Computing inertRanges inside findClosingTagInString on every call is the simple version
    but recomputes the scan of the same string repeatedly. Review measured the existing scanner
    at a linear 8 to 10 percent overhead on a large page, so a naive per-call recompute inside a
    loop is the one shape that could turn that into something worse.
  • Passing the caller's already-computed ranges in as an optional argument avoids the
    recompute. Each of the four call sites either already has ranges in scope or can hoist the
    computation out of its loop.

Prefer the second unless the first measures fine.

Implementation notes (for the implementing agent)

Where to edit

  • packages/core/src/render-server.js, findClosingTagInString (search for the function; it
    scans forward from an index for </tag). This is the single point of the fix.
  • Its four call sites, which is where the ranges would come from if they are passed in: the
    element walk (two calls, one in the main component path and one in the slot-partition
    path), processSuspenseElements, and substituteSlotsInRender.
  • inertRanges and inRanges in the same file are the helpers to reuse. Do NOT write a
    second scanner; fix(core): stop SSR instantiating a component named inside a comment #1132's whole lesson was that a partial one introduces failures worse than
    the bug.

Landmines

  • fix(core): stop SSR instantiating a component named inside a comment #1132's history is the warning here. The first attempt searched for <!-- and skipped what
    followed, which silently suppressed every component after an <!-- in an attribute value or
    in RCDATA, and mistook <script-probe> for a <script>. Read that PR's review trail before
    touching this.
  • An odd quote inside a tag must not derail the scan. escapeAttr does not escape ', so an
    interpolated apostrophe in a single-quoted attribute emits unbalanced quotes; inertRanges
    already recovers at the > for that, and any new code must not reintroduce the trap.
  • The unclosed case matters as much as the closed one. When the real close tag is missing, the
    callers deliberately fall back to "take the rest of the string" and synthesize a close tag.
    Skipping a commented close tag must land in that fallback rather than silently returning a
    wrong index.
  • The client router's keyed boundary comment pairs (Rebuild the client-router swap on route-keyed comment boundaries #1015, dogfood: client router intermittently falls back to a full page load (tab spinner + whole-document flash) #1114) are load-bearing and its
    scan is strict: a mispaired or duplicated boundary degrades navigation to a full page load.
    Run the nested-layout partial-swap tests, not just the unit suite.

Invariants to respect

Tests + docs surfaces

Acceptance criteria

  • A </tag> inside a comment is not treated as the element's closing tag
  • <c-card>kid<!-- </c-card> --></c-card><span>ok</span> round-trips with the comment
    still closed, no duplicate </c-card>, and the trailing span intact
  • The same holds for a commented </slot> and a commented </webjs-suspense>
  • A genuinely unclosed element still falls back to the synthesized-close behaviour
  • The nested-layout partial-swap tests stay green
  • A counterfactual proves the new tests fail when the guard is removed
  • The cross-runtime script covers the close-tag case on Node and Bun

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions