diff --git a/NOTES-incomplete-first-pass-tview-rebuild.md b/NOTES-incomplete-first-pass-tview-rebuild.md new file mode 100644 index 00000000000..3e18f73d419 --- /dev/null +++ b/NOTES-incomplete-first-pass-tview-rebuild.md @@ -0,0 +1,232 @@ +# WIP: rebuilding a corrupted embedded-view TView after an interrupted first pass + +**Status: experimental, not ready to ship.** If you're picking this up cold, read this file plus +the big comment block in `packages/core/src/render3/view_manipulation.ts` (search for +`EXPERIMENTAL`) and you should be back up to speed without re-doing the investigation. + +## Quick summary, if you only read one paragraph + +There's a real production crash. We shipped a narrow fix that turns the crash into a clear error +message instead of a confusing one. This branch is a deeper fix that removes the crash entirely, +and it's verified to work correctly for the general case (any error interrupting an +`@if`/`@switch` branch's first render). It's specifically the **hydration** flavor of this bug — +where the leftover content already existed on the page before Angular touched it — that's still +broken (silent duplicated content instead of a loud error), so it isn't shipped yet. Read on for +the details. + +## Background: three terms you need (skip if you already know Ivy internals) + +Angular's rendering engine (called Ivy) keeps two separate kinds of data structures for every +template: + +- **`TView`/`TNode`** (the `T` stands for _Template_) — a description of the template's shape, + built **once** and shared by every instance of that template. If you loop over 1000 items with + `@for`, Angular does not build 1000 separate descriptions of what each `
  • ` looks like — it + builds one, and reuses it. Think of it as a **blueprint**. +- **`LView`** (the `L` stands for _Live_, i.e. runtime) — the actual, per-instance data: which + specific DOM element this is, which specific item's data it's showing right now. Every time + Angular needs a new instance, it makes a fresh `LView` by literally **copying** the blueprint + (`TView.blueprint`) and then filling in the instance-specific bits. +- **`@if`/`@for`/`@switch` blocks are their own mini-templates.** The content inside a branch or + loop body isn't just "more nodes in the parent template" — it gets its _own_ `TView`, built the + first time that branch/loop actually runs. + +The bug in this file is about that last point: what happens when building one of these "own +`TView`s" gets interrupted partway through. + +## Where this came from + +We already shipped a fix, on `fix/core-hydration-sibling-skip-crash` (now merged into `main`), for +a real crash a production user hit: + +``` +TypeError: Cannot read properties of null (reading 'parent') +``` + +This came from a function called `getParentRElement()` in `node_manipulation.ts`. Its `tNode` +parameter was typed as "this is never null" — but in one specific situation, it actually could be +null, and the function would crash trying to read `.parent` off of it. The shipped fix just adds a +check: if `tNode` is null, throw a clear, coded error (`NG0510`) instead of letting the raw crash +happen. That's a **patch**, not a real fix — it doesn't stop `tNode` from becoming null, it just +makes the resulting error less confusing. The commit message says so itself. + +This branch is where we started digging into _why_ `tNode` can be null in the first place, and +whether we can stop it from happening at all. + +## The actual root cause (we confirmed this, not just guessed) + +We had an actual minified production stack trace to work with, and matched it line-by-line +against the real source code (see the commit message on `fix/core-hydration-sibling-skip-crash` +for the full decoding work). Here's the call chain, in plain terms: + +``` +An @if/@switch block runs (ɵɵconditional) + → asks for that branch's TNode (getExistingTNode) — reads it straight out of an array, + with only a dev-mode-only safety check + → creates the branch's content as its own little embedded view (createAndRenderEmbeddedLView) + → runs that view's template function for the first time (renderView → executeTemplate) + → while creating one of the branch's child nodes (e.g. a text node)... + → ...it tries to find that node's parent (appendChild → getParentRElement) + → CRASH: the child node's own TNode turned out to be null +``` + +**Here's the actual mechanism**, found in `render.ts`'s `renderView()` function: + +```ts +} catch (error) { + // If we didn't manage to get past the first template pass due to + // an error, mark the view as corrupted so we can try to recover. + if (tView.firstCreatePass) { + tView.incompleteFirstPass = true; + tView.firstCreatePass = false; + } + throw error; +} +``` + +Walk through what happens step by step: + +1. An `@if`/`@switch` branch gets selected for the first time. Angular starts building its `TView` + (the blueprint) by running through its content node by node — first child, second child, etc. + — creating a `TNode` for each one as it goes. +2. Something goes wrong partway through — in the real production case, a **hydration mismatch** + (the server-rendered HTML and what the browser actually has don't match) on the branch's + _second_ child, right after the _first_ child was built successfully. +3. That error gets thrown. But look at the `catch` block above: **before** letting the error + propagate further, it flips `firstCreatePass` to `false` and sets `incompleteFirstPass = true`. + In other words: it marks the blueprint as "finished being built," even though it very much + isn't — the build stopped halfway. +4. Somewhere further up the call stack, this error gets caught and the app recovers (doesn't + crash the whole page) — that part is by design, hydration is supposed to be resilient. +5. **Later**, the _same branch_ gets selected again (e.g. the user toggles whatever condition + controls it). Angular checks `firstCreatePass` — it says `false`, meaning "nothing to build, + just reuse what's there." So instead of creating a fresh `TNode` for that second child, it goes + looking for the already-built one — and finds `null`, because it was never actually built. +6. Whatever code needed that `TNode` now has a null value it wasn't expecting. In the production + case, that was `getParentRElement()`. But there's nothing special about that one function — + _anything_ that reads from that same "already built" array under the same assumption is + equally exposed. We only know about the one path the crash actually happened on. + +**Why doesn't this happen for whole components, only for `@if`/`@for`/`@switch` branches?** +Because components get a second chance. Look at `getOrCreateComponentTView()`: + +```ts +if (tView === null || tView.incompleteFirstPass) { + return (def.tView = createTView(...)); +} +``` + +If a component's blueprint is marked "incomplete," Angular just throws it away and builds a fresh +one next time. There's no equivalent check for `@if`/`@for`/`@switch` branches — their blueprint +gets built exactly once, ever, and nothing ever double-checks whether that one attempt actually +finished. + +## What we tried on this branch + +In `view_manipulation.ts`, inside `createAndRenderEmbeddedLView()` (the function that builds an +`@if`/`@switch` branch's content), we added the exact same kind of check components already +have: if the branch's blueprint is marked "incomplete," rebuild it from scratch instead of +trusting the half-finished one. + +We were able to reconstruct a fresh blueprint just from data the old, broken one already had +stored on it (its template function, how many nodes/bindings it needs, which directives/pipes are +available, etc.) — nothing extra needed to be tracked or passed in. + +**We manually confirmed this works**, in the sense that it makes the crash go away entirely: we +took our real reproduction test (an `@if` branch that hits a hydration mismatch on its second +child, then gets re-selected later) and re-ran it with this fix in place. It no longer throws +anything at all — the branch's blueprint just gets quietly rebuilt and everything proceeds +normally. + +## Why we didn't ship this: it's actually worse in one way + +"No longer crashes" sounded great, so we went further and checked: does the branch's content +actually come out _correct_ the second time around, or does it just fail to crash? We changed the +test to check the actual rendered text instead of just "did it throw," and got: + +``` +"firstorigfirstsecond-pass" +``` + +That's wrong. It should have just shown the fresh content. What actually happened: the **first, +failed attempt** had already managed to create and attach `firstorig` to +the real page before the hydration mismatch interrupted it. Nobody ever cleaned those two elements +up. So when the branch got rebuilt and rendered again, its new content got added _next to_ the +leftover elements from the failed attempt, instead of replacing them — hence the duplicated text. + +So: rebuilding the blueprint fixes "which nodes _should_ exist," but it does nothing about "what's +still sitting in the DOM from the attempt that never finished." That's a separate bug, and +honestly a worse one — trading a **loud, obvious crash with a clear error code** for **silently +wrong, duplicated content that nobody would notice unless they were looking closely**. That's why +this branch stops here instead of shipping. + +## Update: the duplicated-content problem turns out to be hydration-specific + +We built a second, much simpler reproduction of the same "interrupted first pass" mechanism — +no server-rendering involved at all, just a directive whose constructor throws once (see +`control_flow_if_spec.ts`, `"rebuilds an @if branch whose first creation pass was interrupted..."`). +Same root cause, same rebuild fix — but this time, after re-entering the branch, **the content +came out correct**: `"firstsecond"`, no duplication, no leftovers. + +The difference comes down to _when_ a branch's content actually becomes visible. Look at +`ɵɵconditional` in `control_flow.ts`: it calls `createAndRenderEmbeddedLView()` first, and only +_after that returns successfully_ does it call `addLViewToLContainer()` — the thing that actually +splices the new content into the visible DOM. Our interrupting error happens _inside_ +`createAndRenderEmbeddedLView()`, so in **plain client-side rendering**, `addLViewToLContainer()` +for the failed attempt never runs at all — nothing from that attempt was ever spliced into the +page, so there's nothing to leak or duplicate. + +**Hydration is different** because the DOM it's working with doesn't come from +`addLViewToLContainer()` at all — it already exists, sitting on the page from the +server-rendered HTML, before Angular's hydration logic even starts. So `first` +in the hydration reproduction wasn't something Angular inserted and forgot to clean up on +failure — it was already visible independent of any of Angular's own bookkeeping, and nothing +in the interrupted pass ever tells the DOM to forget about it. + +**Net effect: the TView rebuild looks like a genuinely complete fix for the general case** (any +error interrupting an `@if`/`@switch` branch's first creation pass, for any reason). It's +specifically the **hydration** flavor of this bug — where the orphaned content was already +part of the page before Angular touched it — that still needs the cleanup work described below. +The currently-shipped `full_app_hydration_spec.ts` test for this (which asserts `NG0510` gets +thrown) has been disabled (`xit`) on this branch rather than deleted or rewritten, since it's +testing exactly the part that's still genuinely broken. + +## What's left to do (none of this has been started) + +1. **Figure out how to clean up the _hydration-specific_ leftover DOM.** Since the general + (non-hydration) case is now verified fine on its own, this is narrower than it first looked: + specifically, when a hydration mismatch interrupts a branch's first pass, whatever of the + server-rendered DOM Angular had already claimed (`first` in our reproduction) + needs to be torn down before/when the branch gets rebuilt and retried. Likely needs + `render.ts`'s `catch` block (or wherever reacts to the rethrown error) to know it's dealing + with hydration-claimed nodes specifically and remove them, not just flag the TView. +2. **Once that cleanup exists, re-enable and rewrite the disabled hydration test.** It's + currently `xit`-disabled in `full_app_hydration_spec.ts` (asserts `NG0510` gets thrown, which + is no longer true with the rebuild in place). Flip it to check actual rendered content is + correct instead — the same way `control_flow_if_spec.ts`'s non-hydration test already does. +3. **Check whether other code paths have the same weakness.** We only confirmed this for + `@if`/`@switch` (via `ɵɵconditional`). The function it calls to look up a branch's `TNode`, + `getExistingTNode()` in `control_flow.ts`, is _also_ used by `@for` (`ɵɵrepeater`) and has the + exact same "trust it without double-checking" shape. Nobody has confirmed whether `@for` loops + can hit this same corruption independently. +4. **Decide where the eventual fix should actually live** — this experiment put it in + `createAndRenderEmbeddedLView()`, but it might make more sense closer to where the corruption + happens (`render.ts`) or where the corrupted value gets read (`control_flow.ts`). +5. **Once shipped, the original `getParentRElement()` guard can be deleted.** The `NG0510` null + check in `node_manipulation.ts` becomes unreachable dead code once branches can no longer end + up with a corrupted blueprint in the first place — `tNode` genuinely can't be null there + anymore at that point. (There's already a `TODO` comment marking this in that file.) + +## Where things actually stand right now + +- The experimental rebuild is committed on this branch, in `view_manipulation.ts`, with the full + explanation inline (search for `EXPERIMENTAL`). +- `control_flow_if_spec.ts` has a new, passing, non-hydration test proving the rebuild works + correctly for the general case (`"rebuilds an @if branch whose first creation pass was +interrupted..."`) — verified it genuinely fails without the fix (raw `NG0510`) and passes with it. +- `full_app_hydration_spec.ts`'s hydration-specific test for this is `xit`-disabled (not deleted), + since it's testing exactly the part that's still broken (see "Update" section above). +- This branch has been rebased on top of the latest `main` (which already includes the shipped + `fix/core-hydration-sibling-skip-crash` fix), so it's up to date and buildable on its own. +- The shipped fix (the `NG0510` guard + the real reproduction test) already lives on `main` — it's + not part of this branch's diff against `main`, only this experimental rebuild is. diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index 658c57ca0dc..a89b0b92ff4 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -504,6 +504,14 @@ export function getParentRElement( // slot in `tView.data` instead of creating a fresh `TNode` — which is what surfaces here. // Guard against it so production throws a coded RuntimeError instead of a raw TypeError when // dereferencing `tNode.parent` below. + // + // TODO: this whole guard becomes dead code once the root-cause fix on + // `experimental/embedded-tview-incomplete-pass-rebuild` ships (rebuilding the corrupted + // embedded `TView` instead of letting it stay corrupted) — `tNode` can no longer be null at + // that point. That branch isn't shippable yet on its own (see + // NOTES-incomplete-first-pass-tview-rebuild.md there for why), but once it — or whatever + // fixes the LView-cleanup issue it uncovered — lands, remove this null check and narrow + // `tNode`'s type back to non-nullable `TNode`. if (tNode === null) { throw new RuntimeError( RuntimeErrorCode.PARENT_NODE_NOT_FOUND, diff --git a/packages/core/src/render3/view_manipulation.ts b/packages/core/src/render3/view_manipulation.ts index fb27ccb7bd0..1c737d8bed4 100644 --- a/packages/core/src/render3/view_manipulation.ts +++ b/packages/core/src/render3/view_manipulation.ts @@ -16,8 +16,15 @@ import {assertDefined} from '../util/assert'; import {assertLContainer, assertTNodeForLView} from './assert'; import {renderView} from './instructions/render'; import {TNode} from './interfaces/node'; -import {DECLARATION_LCONTAINER, FLAGS, LView, LViewFlags, QUERIES} from './interfaces/view'; -import {createLView} from './view/construction'; +import { + DECLARATION_LCONTAINER, + FLAGS, + HEADER_OFFSET, + LView, + LViewFlags, + QUERIES, +} from './interfaces/view'; +import {createLView, createTView} from './view/construction'; export function createAndRenderEmbeddedLView( declarationLView: LView, @@ -31,8 +38,77 @@ export function createAndRenderEmbeddedLView( ): LView { const prevConsumer = setActiveConsumer(null); try { - const embeddedTView = templateTNode.tView!; + // ===================================================================================== + // EXPERIMENTAL — see NOTES-incomplete-first-pass-tview-rebuild.md at the repo root for + // the full writeup (production trace decode, root cause, why this alone isn't a finished + // fix). One-paragraph recap for orientation: + // + // An @if/@switch branch's content is its own embedded template with its own TView, built + // lazily (templateCreate() in instructions/template.ts) the first time that branch is + // actually selected. If an error interrupts that *first* creation pass — e.g. a hydration + // mismatch on the branch's second child, after the first child's TNode was already + // created — render.ts's `catch` block still flips `TView.firstCreatePass` to false and + // sets `incompleteFirstPass = true` before rethrowing. Component TViews get rebuilt from + // scratch next time via getOrCreateComponentTView()'s `incompleteFirstPass` check + // (view/construction.ts), but *embedded* view TViews have no equivalent — nothing ever + // reassigns `templateTNode.tView` again after templateCreate()'s one-time assignment. So + // the next time this exact branch is selected again, its instructions read straight from + // the corrupted (partially-null) `tView.data` instead of creating fresh TNodes, and + // whichever instruction hits the first still-null slot crashes — `getParentRElement()` + // (node_manipulation.ts, NG0510) is just the specific spot the production trace hit; + // other instructions reading `tView.data[slot]` are presumably equally exposed. + // + // What this block does: mirrors getOrCreateComponentTView()'s rebuild-on-incompleteFirstPass + // check, but for the embedded-view TView case. CONFIRMED (manually, not asserted by any + // test in this branch) that this alone makes the NG0510 crash in + // full_app_hydration_spec.ts's "re-entered after a hydration mismatch..." test disappear + // entirely — no error at all on the retried tick(). + // + // WHY THIS ISN'T SHIPPED: making the crash disappear is not the same as making the + // behavior correct. With only this fix applied, the retried branch's DOM ended up as + // `"firstorigfirstsecond-pass"` — the *aborted first attempt's* partially-created LView + // (whose firstorig nodes DID get attached to the real DOM + // before the interrupting error) is never torn down when the branch toggles off, so the + // second attempt's fresh content gets appended alongside the orphaned leftovers instead + // of replacing them. Rebuilding the TView fixes "which template shape to use" but exposes + // that the LView from the failed attempt was never properly cleaned up either — trading a + // loud, debuggable crash (current shipped fix: NG0510 in getParentRElement) for silent + // content duplication, which is arguably worse. + // + // NEXT STEPS (not started): + // 1. Figure out where a partially-created LView from an interrupted renderView() should + // be torn down — likely needs render.ts's `catch` block (or a caller of it, e.g. + // wherever createAndRenderEmbeddedLView()/ɵɵconditional's caller reacts to a thrown + // error) to explicitly detach/destroy whatever nodes DID get attached before + // rethrowing, not just mark the TView as corrupted for next time. + // 2. Once that exists, re-verify this TView-rebuild block against a test that checks + // actual DOM *content* after the retry (not just "does it throw"), e.g. asserting + // `doc.querySelector('app')?.textContent` equals only the fresh second-pass content. + // 3. Consider whether this rebuild belongs here at all, or higher up (e.g. wherever + // `ɵɵconditional`/`ɵɵrepeater` call `getExistingTNode` — control_flow.ts — since + // those also read potentially-corrupted TView-adjacent state via the same + // `!tView.firstCreatePass` pattern and were never audited for this). + // 4. If pursued, this should replace (not sit alongside) the current shipped NG0510 + // guard in node_manipulation.ts — that guard becomes unreachable dead code once + // embedded TViews are never corrupted-and-reused. + // ===================================================================================== + let embeddedTView = templateTNode.tView!; ngDevMode && assertDefined(embeddedTView, 'TView must be defined for a template node.'); + if (embeddedTView.incompleteFirstPass) { + embeddedTView = templateTNode.tView = createTView( + embeddedTView.type, + embeddedTView.declTNode, + embeddedTView.template, + embeddedTView.bindingStartIndex - HEADER_OFFSET, + embeddedTView.expandoStartIndex - embeddedTView.bindingStartIndex, + embeddedTView.directiveRegistry, + embeddedTView.pipeRegistry, + embeddedTView.viewQuery, + embeddedTView.schemas, + embeddedTView.consts, + embeddedTView.ssrId, + ); + } ngDevMode && assertTNodeForLView(templateTNode, declarationLView); // Embedded views follow the change detection strategy of the view they're declared in. diff --git a/packages/core/test/acceptance/control_flow_if_spec.ts b/packages/core/test/acceptance/control_flow_if_spec.ts index ac7bbeb17ac..c9fc330fafb 100644 --- a/packages/core/test/acceptance/control_flow_if_spec.ts +++ b/packages/core/test/acceptance/control_flow_if_spec.ts @@ -33,6 +33,65 @@ class MultiplyPipe implements PipeTransform { } describe('control flow - if', () => { + it( + 'rebuilds an @if branch whose first creation pass was interrupted, without leaving ' + + 'orphaned content behind (non-hydration case)', + () => { + // Regression coverage for the experimental incompleteFirstPass rebuild in + // view_manipulation.ts (see NOTES-incomplete-first-pass-tview-rebuild.md). Cheaper and + // faster than the full SSR/hydration reproduction in full_app_hydration_spec.ts: a + // directive whose constructor throws once reproduces the same "interrupted first pass" + // mechanism directly, without needing hydration at all. + let shouldThrow = true; + + @Directive({selector: '[boom]'}) + class BoomDirective { + constructor() { + if (shouldThrow) { + shouldThrow = false; + throw new Error('boom'); + } + } + } + + @Component({ + imports: [BoomDirective], + template: ` + @if (show()) { + first + second + } + `, + }) + class TestComponent { + show = signal(false); + } + + const fixture = TestBed.createComponent(TestComponent); + fixture.detectChanges(); + + // First attempt: the directive throws partway through the branch's first creation pass, + // after the first was already created but before the second one was. + fixture.componentInstance.show.set(true); + expect(() => fixture.detectChanges()).toThrowError('boom'); + + // Unlike the hydration case, nothing from the failed attempt is visible: the branch's + // embedded view is only spliced into the DOM *after* it finishes creating successfully + // (see ɵɵconditional in control_flow.ts), so an interrupted attempt never gets attached + // in the first place — there's nothing to leak into view. + expect(fixture.nativeElement.textContent).toBe(''); + + // Re-enter the same branch. With the rebuild fix, this succeeds cleanly. + fixture.componentInstance.show.set(false); + fixture.detectChanges(); + fixture.componentInstance.show.set(true); + expect(() => fixture.detectChanges()).not.toThrow(); + + // Correct, non-duplicated content — no leftover from the failed first attempt. + expect(fixture.nativeElement.textContent).toBe('firstsecond'); + }, + ); + it('should add and remove views based on conditions change', async () => { @Component({template: '@if (show()) {Something} @else {Nothing}'}) class TestComponent { diff --git a/packages/platform-server/test/full_app_hydration_spec.ts b/packages/platform-server/test/full_app_hydration_spec.ts index b5ca863cd0f..e799a8321ca 100644 --- a/packages/platform-server/test/full_app_hydration_spec.ts +++ b/packages/platform-server/test/full_app_hydration_spec.ts @@ -6460,7 +6460,17 @@ describe('platform-server full application hydration integration', () => { }, ); - it( + // Disabled on this experimental branch: the incompleteFirstPass rebuild in + // view_manipulation.ts (see NOTES-incomplete-first-pass-tview-rebuild.md) makes this + // scenario stop throwing NG0510 entirely, so this assertion no longer holds here. That's + // expected, not a regression — the rebuild is confirmed to work correctly for ordinary + // (non-hydration) interrupted-first-pass cases (see the new test in + // control_flow_if_spec.ts), but the *hydration* case specifically still has the + // separate, unfixed orphaned-DOM bug documented in the notes file. Once that's fixed, + // this test should be rewritten to assert correct content instead of NG0510 being + // thrown; until then it's left disabled here rather than deleted or rewritten to assert + // the currently-broken behavior. + xit( 'should throw a coded RuntimeError, not a raw TypeError, when an @if branch is ' + 're-entered after a hydration mismatch corrupted its template on the first pass', async () => {