fix(core): stop SSR instantiating a component named inside a comment - #1132
Conversation
The custom-element walk matched tags with a flat regex over assembled markup, so a registered tag name written inside an HTML comment was constructed and rendered as a real element. The damage went well past a wasted render: the replacement consumed the rest of the comment INCLUDING its closing `-->`, so the markup after it was swallowed by an unterminated comment. Whether it happened at all depended on whether the name in the comment happened to be a registered component, which is what made it look random rather than reproducible. Found by writing an ordinary explanatory comment in the website's root layout that mentioned copy-cmd, which grew a real copy button out of nowhere. The walk now computes the comment ranges once and skips any match inside one. Two details keep it faithful to how a browser reads the same bytes: an unterminated `<!--` comments out everything to EOF, and raw-text elements have no comment syntax, so a `<!--` inside a script or style must not open a region. That second one is the important guard: treating it as a comment would silently stop every component after that script from rendering, which would be worse than the bug being fixed. Raw-text CONTENT is deliberately not skipped for tag matching. Whether a tag-shaped string inside a script should be instantiated is a separate question, so the existing behaviour there is left alone.
My first pass searched for `<!--` and skipped what followed. Review found that introduces failures worse than the bug: an `<!--` inside an attribute value, inside a textarea or title, or the spec's `--!>` and `<!-->` forms each opened or failed to close a region, and every component after it silently stopped rendering. A component named `script-probe` was also mistaken for a `<script>` (a hyphen is a non-word character, so the word boundary matched), which aborted scanning to EOF and left the original bug live for the rest of the document. All five reproduced. Deciding whether a `<` is markup requires knowing the context, which means tokenizing, so `inertRanges` now walks the input once tracking comments (including the short and abrupt-closing forms), markup declarations, tags with their quoted attribute values, raw text, and RCDATA. Tags inside any of those are text and are skipped. That also covers raw-text CONTENT, which the first pass deliberately left alone as "a separate question". It is not separate: a component tag inside a style comment destroyed the rest of the document through the identical path, so leaving it out left half the bug shipping. Two more scanners had the same blind spot and are fixed here rather than left for later, because both lose content: - processSuspenseElements runs BEFORE the element walk and hands a boundary's children to a fresh injectDSD as a standalone string, so a commented-out boundary still ran its children. Under streaming it also consumed an id and emitted a swap script targeting an element that only exists inside a comment, so it could never resolve. - substituteSlotsInRender substituted a `<slot>` written in a comment. A commented slot has no `</slot>`, so the fallback scan swallowed the rest of the template, the real slot was never substituted, and the authored children vanished from the page entirely. The client router's keyed boundary comment pairs are now covered by a test that asserts components between them still render and both markers survive verbatim, replacing an earlier test whose input contained no comment at all and so asserted nothing about comment handling.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
This came back badly, and rightly so. The first version searched for <!-- and skipped what followed, which introduces failures worse than the bug it fixes. I reproduced all of them: an <!-- inside an attribute value, inside a textarea or title, and the spec's --!> and <!--> forms each opened or failed to close a region, and every component after that point silently stopped rendering. A component named script-probe was also mistaken for a <script>, since a hyphen is a non-word character and the word boundary matched, which aborted scanning to EOF and left the original bug live for the rest of the document.
The lesson is that you cannot decide whether a < is markup without knowing the context, and knowing the context means tokenizing. So the scanner now walks the input once tracking comments (including the short and abrupt-closing forms), markup declarations, tags with their quoted attribute values, raw text, and RCDATA.
Two claims I made in the body were also wrong and I have corrected both. processSuspenseElements DOES have the bug: it runs before the element walk and hands the boundary's children to a fresh injectDSD as a standalone string, so a commented-out boundary ran its children, and under streaming it consumed an id and emitted a swap script pointing at an element that only exists inside a comment. And substituteSlotsInRender is the worst of the family: a commented <slot> has no closing tag, so the fallback scan swallowed the rest of the template and the authored children vanished from the page.
I also dropped the raw-text carve-out. Calling it "a separate question" was wrong: a component tag inside a style comment destroys the rest of the document through the identical path, so leaving it out was shipping half the bug.
render-server.js is a runtime-sensitive surface, so the parity gate wants a cross-runtime assertion rather than a Node-only result. The reason this one is worth having is specific, not categorical. The scanner leans on indexOf / startsWith offsets, a RegExp with a lookahead built per call, and matchAll index arithmetic that has to line up exactly with those offsets. A divergence between V8 and JSC would not throw, it would shift a range boundary by a character, and the symptom is a component that renders on one runtime and silently vanishes on the other. So the script asserts both directions, the inert cases AND the cases that must still render, plus the content-preservation check. Green on node 26.1.0 and bun 1.3.14.
The behaviour is what anyone would already expect from a browser, so it would not normally be worth a line. It is worth one now because the opposite used to be true and could silently delete part of a page, and because the <template> exception is genuinely non-obvious.
Two more from review, one of them mine. An odd quote inside any tag made the scanner consume to EOF, so it returned a truncated range list and silently re-enabled this whole bug for the rest of the document. It fails open, which is exactly what the last commit claimed to be avoiding. It is reachable from ordinary app code: escapeAttr does not escape `'`, so an interpolated apostrophe inside a single-quoted attribute emits three unbalanced quotes. A quote now only opens a value when it directly follows `=`, which is how a browser recovers at the `>`. The docstring also claimed a `<` inside an attribute was inert, and only half of that was true. A component tag written literally in an attribute value (`title="renders a <my-card> element"`) was still instantiated in place, destroying the rest of the document through the identical path. The tag interior is now recorded as inert, starting one byte in so the tag's own opening `<` still matches. Pre-existing rather than a regression from this branch, but it is the same bug in a context the scanner already knows the boundaries of, so fixing it here rather than leaving an arbitrary line through the family.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass on the tokenizer, and it found one of mine plus two that predate the branch.
Mine: an odd quote anywhere in a tag made the scan consume to EOF, so it returned a truncated range list and silently re-enabled this entire bug for the rest of the page. It fails open, which is precisely what I said the rewrite was avoiding, and it is reachable from ordinary app code because escapeAttr does not escape an apostrophe. A quote now only opens a value when it directly follows =, which is how a browser recovers at the >.
Also fixed the attribute-interior case. My own docstring claimed a < inside an attribute was inert and only half of that was true: a component tag written literally in a title or placeholder was still instantiated and destroyed the rest of the document. Pre-existing rather than a regression here, but it is the same bug in a context the scanner already knows the boundaries of, so leaving it would have drawn an arbitrary line through the family.
The third one I am deliberately not fixing here. findClosingTagInString matches a CLOSING tag inside a comment, which is the mirror image of this PR and corrupts output the same way, but it is a different helper reached from four call sites and it needs its own decision about where the ranges get computed so a per-call recompute does not land inside a loop. Filed as #1133 with the repro and that analysis.
Worth recording what the reviewer checked and found sound, since it is the part I would otherwise keep re-testing: the comment state machine across the short and abrupt-closing forms, i advancing on every branch, bare < in text, attribute values containing >, EOF-truncated tags, the raw-text close regex being safe because the tag can only be one of four literals, the consumed arithmetic in the suspense scanner on both paths, and the cursor handling in the slot scanner. Overhead measured linear at 8 to 10 percent on a large page, not quadratic.
|
Follow-up: the closing-tag half is filed as #1133 Review turned up that Same family as this PR and it corrupts output the same way, but it is a different helper reached from four call sites, and the fix needs a deliberate decision about where the ranges are computed so a per-call recompute does not land inside a loop. Filed as #1133 with the repro, the four call sites, and that analysis rather than growing this branch further. |
…docs Review found three of the new tests passed with their guard reverted, so they were describing the fix rather than holding it. The unbalanced-quote test used a COMMENTED probe, which stays inert either way: consuming to EOF produces one giant inert range, so the assertion passed for the wrong reason. It now asserts a REAL component after the bad tag still renders, which is the thing that actually breaks. Same for the attribute test, which needed a component named INSIDE the value rather than a value containing only a `>`. Its comment also had the mechanism backwards: an early `>` makes the interior range shorter, so it can only under-cover, and the failure is an element inside the value wrongly rendering, not one after it vanishing. The docs line I added claimed more than ships. Nesting depth is still counted without regard for comments, so a comment holding the open or close tag of the element it sits inside still mis-detects where that element ends. #1133 covers it; the line now says so instead of promising it works. Also names the Bun script in CI explicitly rather than relying on the node:test wrapper for a runtime the docstring claimed was covered directly, and renames the call-site binding now that the ranges are more than comments.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass, and the useful finding this time was about the tests rather than the code: three of them passed with their own guard reverted. The unbalanced-quote one used a commented probe, which stays inert either way once the scan runs to EOF, so it was asserting the right outcome for the wrong reason. They assert a real component still renders now, which is the thing that actually breaks, and reverting the quote condition alone reds the suite.
The docs line I added also claimed more than ships. Nesting depth is still counted without regard for comments, so a comment holding the open or close tag of the element it sits inside still mis-detects where that element ends. I rescoped #1133 to cover both directions rather than just closing tags, and the docs line now points at the gap instead of promising it works.
The PR body had drifted badly across four commits, still describing the raw-text carve-out and the suspense scanner as untouched when both had changed. Rewritten to match what actually ships.
Structural checks came back sound: 200k random inputs produce no inverted, overlapping, or out-of-range spans, and 40k inputs diffed against parse5 agree on whether a tag is a real element. Overhead is linear at 8 to 10 percent of scan time on a large page.
Fourth review round, four things.
A differential fuzz against parse5 found one remaining context gap: `</`
followed by a non-letter (`</1`, `</<`, `</ `) is the third bogus-comment
form and also runs to the next `>`. Without that branch the bytes after it
were scanned as markup and a tag inside was instantiated, which is the
original bug in a shape nobody had tried.
partitionAuthoredBySlot was a fourth scanner deciding where a comment ends,
with a bare indexOf('-->') that disagreed with inertRanges on exactly the
short forms it was taught. The disagreement silently routed a slot="head"
child into the default slot. Both now call one shared endOfComment.
The perf claim in the body was wrong, measured 2.5x rather than the 8 to 10
percent I repeated from an earlier round. Two causes, both fixed: a
per-character RegExp test that ran over every byte of every tag (worst on a
Tailwind page, where most bytes are long class attributes) is now a char
compare, and the membership test now carries a cursor instead of restarting
at range zero on every match, which was an O(tags x components) term.
inRanges became inertAt; its old doc claimed the left-to-right optimisation
it never actually did.
The headline comment guard also turned out to be unpinned: disabling the
entire `<!--` branch left 15 of 16 tests green, because `<!--` then fell
through to the markup-declaration path that stops at the first `>`, and
every fixture had its tag before that `>`. A comment containing a `>` ahead
of the tag is the discriminator, and disabling the branch now reds three.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fuzzing this against parse5 was the right call. One context gap left: </ followed by a non-letter is the third bogus-comment form, and without that branch the bytes after it were scanned as markup, which is the original bug in a shape I had not tried.
The one that bothers me most is that the headline guard was not actually pinned. Disabling the whole <!-- branch left 15 of 16 tests green, because <!-- then falls through to the markup-declaration path that stops at the first >, and every fixture I had written happened to put its tag before that >. A comment containing a > ahead of the tag is what discriminates. That is the second time on this branch a test looked green while holding nothing, so it is worth naming as a pattern rather than a one-off.
Also found a fourth scanner. partitionAuthoredBySlot decides where a comment ends with a bare indexOf, which disagreed with the tokenizer on exactly the short forms it had been taught, and silently routed a slotted child into the wrong slot. Both call one shared endOfComment now.
And the perf number I had been repeating was wrong: 2.5x, not 8 to 10 percent. A per-character RegExp over every byte of every tag was most of it, which on a Tailwind page means most bytes on the page, plus a membership test that restarted at range zero for every match. Char compare and a forward cursor bring it back to parity with the merge base.
Fifth round. Raw text is not just script and style. iframe, xmp, noembed, noframes and plaintext are all elements whose content the tokenizer never reads as markup, and an <iframe> with fallback markup destroyed the document exactly like the comment case. Widened through a predicate scoped to this scanner rather than isRawtextTag, which is shared with the template tokenizer where it decides hole escaping. noscript is deliberately left out: its content IS parsed when scripting is disabled, which for a progressive-enhancement framework is the case that matters, so components inside it must keep rendering. Three guards were load-bearing with no test at all, each verified by reverting it alone: the markup-declaration branch (a tag inside <![CDATA[ was instantiated without it), the consumed offset arithmetic in the suspense scanner (a comment following a REAL boundary was mis-mapped and its contents ran, which the existing test could not reach because it had no preceding boundary), and the partition scanner sharing endOfComment (a short-form comment ran past its end and routed a slot="head" child into the default slot). Extracting endOfComment last round also orphaned the inertRanges doc block onto it, leaving two stacked comments on one function and the real one undocumented.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Round five, and the pattern from round three repeated: three guards were load-bearing with no test at all, each confirmed by reverting it alone. The markup-declaration branch, the consumed offset arithmetic in the suspense scanner, and the partition scanner sharing endOfComment. The suspense one is the interesting miss, because the existing test could never reach it: it had no REAL boundary before the commented one, so it only ever exercised the offset-zero path.
The real correctness find is that raw text is not just script and style. iframe, xmp, noembed, noframes and plaintext all hold content the tokenizer never reads as markup, and an iframe with fallback markup destroyed the document exactly like the comment case. Widened through a predicate scoped to this scanner rather than isRawtextTag, which the template tokenizer shares for hole escaping. noscript stays out deliberately: its content IS parsed when scripting is disabled, which for a PE-first framework is the case that matters.
Every guard in the scanner now reds a test when reverted on its own, which is the bar I should have held from the start rather than after five rounds of being told.
The widening from last round was itself unpinned: dropping xmp, noembed, noframes and plaintext left every test green, and so did wrongly ADDING noscript, which would have stopped components rendering for exactly the reader server-rendered output exists for. Each is now a counterfactual. components.md still described the old two-element list and named template as the only exception, so an agent reading it would conclude an iframe fallback still instantiates components and would never learn noscript is the exception that matters. It now lists the real set and both exclusions. Also documents #1134, the one residual divergence a differential fuzz against parse5 found across ~221k fixtures: a script body that contains `<!--` followed by `<script` enters the parser's double-escaped state, where the next `</script>` does not close the element, and this scanner ends the content there anyway. Narrow enough to file rather than fix here, loud enough that it should not sit undocumented while the docs claim script content is handled.
The end boundary of every text-only skip was unpinned, and its failure mode is the one this whole change exists to avoid. Making the range never end left all 27 tests green, yet under that patch a single <style> in a root layout takes out every component on every page. The existing assertions could not catch it: an inert range never deletes text, so "markup after it survives" is true wherever the range stops. It was redundant with the negative assertion rather than a second check. Only a real component AFTER the element proves scanning resumed, which is what the script and RCDATA tests already had and these five did not. Reverting the boundary now reds five tests. Same gap in the cross-runtime script, which claimed to guard exactly this kind of boundary divergence. The commented-suspense test also only ever ran the blocking branch, since plain renderToString passes no suspense context, so the serious half went unasserted: under streaming a commented boundary would consume an id, queue a chunk, and emit a swap script targeting an element that exists only inside a comment, leaving the fallback stuck forever. Now asserted against a real context.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Seventh round, and the finding is the same shape as rounds three and five: a guard with a test that could not fail. The END of each text-only skip was unpinned. Making the range never end left all 27 green, and under that patch a single style block in a root layout takes out every component on every page.
The reason the assertions could not catch it is worth writing down, because I wrote them and thought they were doing something. An inert range never deletes text, so "markup after it survives" is true no matter where the range stops. It reads like a second check and is actually a restatement of the first. Only a real component AFTER the element proves scanning resumed, which the script and RCDATA tests happened to have and these five did not.
Also asserted the streaming half of the commented-boundary case. Plain renderToString passes no suspense context, so the test only ever ran the blocking branch, and the serious damage (a consumed id, a queued chunk, a swap script pointing inside a comment) sits on the other one.
Three body inaccuracies fixed too, including a paragraph that read as though noscript had been wrongly excluded when the retraction was about raw text generally, and the deferred section that documented #1133 but not #1134.
Two new divergences, both the silent-vanish class. A doubled `=` (`<a title==">`) re-opened the unbalanced-quote hole from a different door. Per spec the character after `=` is reconsumed in attribute-value-unquoted state, so that quote is an ordinary value character and the tag ends at the first `>`. Keying off "previous character was `=`" opened a value there instead, leaving an odd quote count that ran the scan to EOF. Now a flag set by `=` and cleared by the first non-space character after it, which is what the spec describes. Not reachable through interpolation, since the renderer quotes holes and escapes `"`, but reachable through unsafeHTML or third-party markup. Self-closing text-only tags were not handled at all, and this one is a failure this branch introduced rather than inherited. In SVG and MathML foreign content `<title/>` genuinely closes, so there is no `</title` to find, the range ran to EOF, and every component in the rest of the document went inert. Honouring `/>` costs only malformed HTML (`<style/>`, already broken authoring) and errs toward components rendering. Six guards also had tests that could not fail on them. The partition one is the instructive case: its fixture used `<!-->`, which CONTAINS `-->`, so a bare indexOf found the identical end. `--!>` is the discriminating form. Same for the `<!--->` branch, the close-tag boundary lookahead, and the plaintext special case, none of which any fixture reached. All six now red when reverted alone.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Eighth round. Two real divergences and six more tests that could not fail on their own guard.
The doubled = is the one I would not have found by reading. <a title=="> is spec-legal and the quote there is an ordinary value character, so the tag ends at the first >. My rule of "the previous character was =" opened a quoted value instead, which left an odd quote count and ran the scan to EOF, re-entering the exact failure the quote guard was added to prevent. Different door, same room. It needs unsafeHTML or third-party markup to reach, since the renderer quotes and escapes its own holes.
The self-closing one is a failure this branch introduced rather than inherited. In SVG foreign content <title/> genuinely closes, so there is no </title to find, the skip ran to EOF, and every component after it went inert.
On the tests, the partition case is the one worth remembering: the fixture used <!-->, which CONTAINS -->, so a bare indexOf found the identical end and the test was structurally incapable of failing. --!> is the discriminating form. Same story for the <!---> branch, the close-tag boundary lookahead, and the plaintext case.
That is now four rounds in a row where the finding was a test that looked right and held nothing. I have stopped trusting a green suite as evidence that a guard is covered and started reverting each guard individually; all of them red now.
Two regressions from the previous commit, both found by review, both the destructive direction. `<a href=>` is a missing-value parse error and the `>` still ends the tag. My expectValue branch sat above the `>` check, so it ate the tag end and the scan ran to the NEXT `>`. Effects: the component after it stopped rendering, and a following <style> never armed its skip, so a tag inside the style was instantiated and swallowed the rest of the document. The `>` check now comes first, which is also where the spec puts it. `selfClosing = html[p - 1] === '/'` misread a `/` that ends an UNQUOTED value, so `<iframe src=/embed/x/>` looked self-closing, the text-only skip was suppressed, and the tag inside the iframe was instantiated. Per spec `/` is an ordinary character there. The comment I wrote justified a false NEGATIVE on `<style/>`; this was a false POSITIVE on markup that is perfectly normal, which is not the same trade at all. Both come from the same missing piece: unquoted values had no state of their own. They have one now, and it is what makes both `>` and `/` behave correctly from inside a value. Also pins the whitespace skip between `=` and a quoted value, which no fixture reached: without it `title= "a > b <probe> c"` ends the tag at the `>` inside the value and resumes scanning mid-attribute.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Ninth round, and both findings were regressions from the previous commit rather than anything inherited, which is the honest summary of how this one has gone: the quote handling has now broken twice, in two different ways, each time while fixing the previous break.
<a href=> is a missing-value parse error where the > still ends the tag. My branch sat above the > check and ate it, so the scan ran to the next >, the component after it stopped rendering, and a following style element never armed its skip. <iframe src=/embed/x/> was the mirror: a / ending an unquoted value read as a self-closing solidus, so the skip was suppressed and the tag inside the iframe was instantiated. The comment I wrote justified a false negative on <style/>, and this was a false positive on ordinary markup, which is not the same trade.
Both trace to one missing piece. Unquoted attribute values had no state of their own, and both > and / behave differently from inside one. They have one now, which is what the spec describes and what I should have written when I first touched this.
Also pinned the whitespace skip between = and a quoted value, which no fixture reached.
Closes out the two deferred halves of the family in this same PR rather than leaving them as follow-ups. findClosingTagInString kept a nesting ledger that counted every same-name tag, so a commented open tag inflated the depth forever and a commented close tag ended the element at the comment, truncating the authored children mid-comment and leaving an unterminated `<!--` that commented out the real close tags in the browser. It now consults the same inert ranges as the element walk, so a tag inside a comment, raw text, RCDATA, or an attribute value counts for neither side. Callers that already computed the ranges for the exact string pass them; the two that operate on shifted or private strings compute internally. Script data gets the double-escape rule: `<!--` followed by `<script` means the next `</script>` is text and the element ends at the one after, which is what the legacy comment-wrapped inline script produces. A lone `<!--` with no inner script tag still closes at the first `</script>`, same as a browser. Both counterfactualled: dropping the range consult reds the boundary tests, and disabling the double-escape state reds the script test while the ordinary-script tests hold. Bun parity script covers both on node 26.1.0 and bun 1.3.14. The partial-swap e2e fails in this worktree identically with main's render-server (environmental, spawn-based); CI's E2E job is the check of record and runs on this push.
…bad revert The previous commit was supposed to carry these render-server changes and carried only their tests: a `git checkout HEAD --` used to restore a baseline ran BEFORE the fixes were committed, so it silently reverted them, and "3 files changed" on a commit meant to touch four was the tell I missed. This is the exact counterfactual trap the workflow documents (commit first, then revert through git), hit from a baseline comparison instead of a counterfactual. The two new tests going red is what caught it, which is the system working.
…eploy The client router reads x-webjs-build / x-webjs-src off its own fetches to detect a deploy. Both fetch sites used the default HTTP cache mode, so on a page served with a browser max-age the fetch could be satisfied wholly from cache, replaying pre-deploy ids; the check then compared two equally stale values and skipped the snapshot eviction, leaving a deploy invisible for the freshness window plus one stale-while-revalidate serving per URL. Observed in production on webjs.dev the day max-age went from 0 to 60. Both fetches now send cache: no-cache, which is revalidate, not bypass: with the stable page ETags shipped alongside that header change, the forced revalidation is answered with an empty 304, so soft navs stay cheap while the ids stay live. Document navigations (new tab, back/forward) still serve straight from disk within the window, which is where the browser-cache win actually lives. Browser test captures the exact RequestInit through the real click and viewport-prefetch paths on Chromium, Firefox and WebKit; dropping either cache option reds it. The website layout comment that documented this as an accepted tradeoff now documents the resolution instead.
…--> exit Review of the absorbed fixes found two real gaps and a doc mess. The suspense scanner's skip path re-slices its input just past a commented boundary match, which can land MID-comment. The close-tag search for the next real boundary then re-tokenized that suffix from a clean state, so a text-only opener named later in the same comment (a <textarea> in prose) read as a real unclosed element, everything to EOF went inert, and the boundary swallowed its own close tag plus the trailing markup. The search now receives the FULL-input ranges shifted into slice coordinates, so the mid-comment truth survives the slice. endOfScriptContent missed that the dash-dash state entered by <!-- exits straight back to plain script data on >, so a lone "<!-->" in a script armed the escape and a later "<script>" string deferred the close past its real end, silently unrendering every component after the script. The restore commit had also left two JSDoc blocks detached from their functions and a comment duplicated; reattached. The suspense test needed its assertions replaced once more: the swallowed output CONTAINS the literal close-tag-then-span sequence, so the original regex matched in the broken state too. The discriminators are the close tag count (the failure synthesizes a duplicate) and the document ending with </div> rather than the synthesized close. Both fixes red their test when reverted alone.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Round on the absorbed scope (#1131, #1133, #1134 folded into this PR). Two real finds, both fixed.
The suspense one is subtle and worth understanding: the skip path slices the input just past a commented boundary match, which lands mid-comment, and the close-tag search then re-tokenized that suffix from a clean state. A tokenizer restarted inside a comment has no idea it is in one, so a text-only element named later in the same comment read as real and unclosed, and the next genuine boundary swallowed its close tag and everything after it. The search now gets the full-input ranges shifted into slice coordinates.
The script one is the spec's dash-dash exit: drops straight back to plain script data, so it must not arm the double-escape.
Also hit the non-discriminating-test trap AGAIN on my own new test: the swallowed output literally contains the close-then-span sequence the regex looked for, so it passed in the broken state. Tag COUNT and the document's final bytes are what discriminate. That is now three separate rounds where an assertion looked load-bearing and held nothing; counting occurrences and asserting ends, not includes, is the pattern that survives.
Also restored the render-server source that a baseline git checkout HEAD silently dropped before its commit (the two red tests caught it, which is the system working), and reattached the JSDoc blocks that restore had scrambled.
The <!--> exit was only honoured when entering fresh. The token's trailing dashes reach the dash-dash state from the escaped and double-escaped states too, and dash-dash exits to plain data on >, so inside an already-escaped body the missed exit let a later "<script" string arm the double-escape and move the element end past its real close, silently unrendering every component after the script. Confirmed against parse5; one unified branch now clears both flags, pinned by a revert-alone counterfactual.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
One genuine find this round, confirmed against parse5: the dash-dash exit was honoured only when entering fresh, but the token's trailing dashes reach the dash-dash state from the escaped and double-escaped states too, so inside an already-escaped script body the missed exit let a later script-tag string arm the double-escape and push the element end past its real close. Unified into one branch that clears both flags, pinned by a revert-alone counterfactual.
Everything else held under mutation and adversarial probes, including the shifted-ranges suspense path and the router fetch options.
The exit's dbl=false was load-bearing but unpinned: clearing only the escaped flag survived every test, while a stale double-escape made the real close tag read as a step-down and ran the element to EOF. The new case reaches the exit with the double-escape armed and reds that mutant. Review fuzzed the scanner itself against parse5 across 40k script bodies with zero divergences, so this closes the last unpinned guard.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
The scanner source came through clean this time: 40k fuzzed script bodies against parse5, zero divergences, and the older guards held under mutation. The one find was a coverage gap on the newest guard, the dbl-clearing half of the dash-dash exit, which no fixture reached with the double-escape armed. One added case pins it; the escaped-only mutant now reds.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Final pass came back clean. Mutation-spot-checked two guards (the quote-after-equals recovery and both halves of the script dash-dash exit) and each mutant was killed by exactly the assertion written for it. The whole final state agrees with itself: scanner, tests, the Bun script, the CI step, both docs surfaces, the layout comment, and the router change. Nothing left to find here.
1 participant
Closes #1128
Closes #1131
Closes #1133
Closes #1134
A registered tag name written inside an HTML comment was constructed and rendered as a real element during SSR. The damage went past a wasted render: the replacement consumed the rest of the comment INCLUDING its closing
-->, so the markup after it was swallowed by an unterminated comment. Whether it happened at all depended on whether the name in the comment happened to be a registered component, which is what made it look random rather than reproducible.Found while writing an ordinary explanatory comment in the website's root layout that mentioned
copy-cmd. The layout grew a real copy button out of nowhere, and I only noticed because an unrelated test caught it.The fix
Deciding whether a
<is markup requires knowing the context, soinertRangeswalks the assembled HTML once and returns the byte ranges where a tag-shaped match is text: comments, markup declarations, tag interiors (so attribute values), raw text, and RCDATA. The three scanners that match tags over assembled markup consult it.I tried the obvious version first, searching for
<!--and skipping what followed, and it was worse than the bug. Review reproduced five ways it fails: an<!--inside an attribute value, inside a<textarea>or<title>, and the spec's--!>and<!-->forms each opened or failed to close a region, and every component after that point silently stopped rendering. A component namedscript-probewas also read as a<script>, since a hyphen is a non-word character, which aborted scanning to EOF and left the original bug live for the rest of the document. Half-tokenizing HTML does not work; that is the whole lesson of this change.Four scanners share the blind spot and all four are fixed here, because each one loses content rather than merely wasting a render:
injectDSD, the element walk, which is the reported bug.processSuspenseElements, which runs BEFORE the element walk and hands a boundary's children to a freshinjectDSDas a standalone string, so a commented-out boundary still ran its children. Under streaming it also consumed an id and emitted a swap script pointing at an element that exists only inside a comment, so it could never resolve.substituteSlotsInRender, the worst of the family: a commented<slot>has no</slot>, so the fallback scan swallowed the rest of the template, the real slot was never substituted, and the authored children vanished from the page entirely.partitionAuthoredBySlot, which decided where a comment ends with a bareindexOf('-->')and so disagreed with the tokenizer on the spec short forms, silently routing aslot="head"child into the default slot. Both now call one sharedendOfComment.Text-only element CONTENT is skipped too, and that means every such element rather than just
scriptandstyle:iframe,xmp,noembed,noframes,plaintext,textarea,title. An<iframe>with fallback markup is the realistic trigger and it destroyed the document the same way. I first carved raw-text content out entirely as "a separate question", and that was wrong: a component tag inside a<style>comment hits the identical path, so leaving it out was shipping half the bug.Two elements are excluded on purpose.
<template>content is parsed and legitimately carries components, which Declarative Shadow DOM and the streamed swap templates both depend on.<noscript>content is parsed too, because a browser with scripting disabled reads it as markup, and that reader is exactly who server-rendered output exists for, so components inside it must keep rendering. Both exclusions have a counterfactual test, since wrongly ADDINGnoscriptto the list was otherwise silent.Two further problems came out of review and are fixed here as well. An odd quote anywhere in a tag made the scan consume to EOF, returning a truncated range list that silently re-enabled the whole bug for the rest of the page; it is reachable from ordinary app code because
escapeAttrdoes not escape', so an interpolated apostrophe in a single-quoted attribute emits unbalanced quotes. A quote now only opens a value when it directly follows=, which is how a browser recovers at the>. And a component tag written literally in an attribute value (title="renders a <my-card> element") was still instantiated in place; tag interiors are now inert, starting one byte in so the tag's own<still matches.The rest of the family, absorbed rather than deferred
Originally two edges were filed as follow-ups; they are fixed here instead, so this PR closes the whole family and leaves nothing open.
#1133, element nesting.
findClosingTagInStringcounted every same-name tag for its depth ledger, so a commented open tag inflated the depth forever and a commented close tag ended the element at the comment, truncating the authored children mid-comment. It now consults the same inert ranges as the element walk, threaded through from callers that already computed them.#1134, script double-escape. A
<script>body containing<!--followed by<scriptenters the parser's double-escaped state, where the next</script>is text and the element ends at the one after. The scanner now models that; an ordinary script, and a script with a lone<!--, still end at their first close tag.#1131, from the caching PR, also absorbed. The client router read its deploy-detection headers (
x-webjs-build/x-webjs-src) off fetches made with the default HTTP cache mode, so a page cached with a browsermax-agecould satisfy the fetch from cache and replay pre-deploy ids, hiding a deploy for the freshness window plus one stale-while-revalidate serving per URL. Both router fetch sites now sendcache: 'no-cache'(revalidate, not bypass): with the stable page ETags shipped alongside that header change, the revalidation is an empty 304, so soft navs stay cheap while the ids stay live. Document navigations still serve straight from disk within the window. A browser test captures the exactRequestInitthrough the real click and viewport-prefetch paths on Chromium, Firefox, and WebKit; dropping either option reds it.The client side needs no fix. Templates are parsed there by the real HTML parser, and the HTML spec defines everything between
<!--and-->as comment data, so no element construction can occur.Test plan
packages/core/test/rendering/ssr-comment-not-an-element.test.js, 45 cases: the comment case and its content-loss shape, unterminated comments, the spec short forms, attribute values, RCDATA, raw text, a component whose name starts withscript, unbalanced quotes, the suspense and slot scanners, and the client router's<!--wj:children:...-->boundary pairs round-tripping with a component between them.main, and this took several passes to actually achieve. Review repeatedly found tests that could not fail on the guard they were written for: a commented probe stays inert either way once the scan runs to EOF; "markup after it survives" is true wherever an inert range ends, since a range never deletes text; and a<!-->fixture contains-->, so a bareindexOffinds the identical end. Every guard in the scanner now reds a test when reverted on its own, verified one at a time.test/bun/comment-not-an-element.mjsasserts both directions on Node and Bun, and is named explicitly in the CI Bun job. Green on node 26.1.0 and bun 1.3.14. This surface is offset arithmetic overindexOfresults plus a per-callRegExp, where a V8-versus-JSC divergence would not throw, it would shift a range boundary and make a component render on one runtime and vanish on the other./and/docs/getting-startedand ui-website/boot 200 in prod mode with no broken modulepreloads./ui404s in this worktree because that route lives on an unmerged branch, not on main.Docs
references/components.mdgains the rule (a tag name in a comment is not instantiated, with the<template>exception) plus the #1133 gap. Perf is back at parity with the merge base on a markup-dense benchmark (median 1.23ms over 30 warmed runs). An earlier draft of this scanner was 2.5x, from a per-character RegExp over every tag byte and a membership test that restarted for every match; both are gone.