fix: a throw while removing repeat() leftovers keeps the removed row - #1274
fix: a throw while removing repeat() leftovers keeps the removed row#1274vivek7405 wants to merge 4 commits into
Conversation
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through the whole diff. The shape is right: this is bookkeeping repair, not a rebuild, and making the code strong enough for #1172's existing claim beats hedging the claim. Two real problems, both about the difference between what the code guarantees and what the prose says it does.
The bigger one is the removal loop's ordering. Unmapping the key ahead of the row fixes a dispose throw and creates the mirror-image hole for a removal throw, which lands the row in the document described by neither map. That is the exact untracked-orphan shape the PR is about, so it is not a residual to name, it is the bug in a different position. Removing and unmapping in one finally holds both ends.
The other is the ref contract. applyElement already wraps every callback-ref call on the commit path, so the callback ref was never propagating there and the new prose says it was. The PR body scoped it correctly to the OBJECT writes; the shipped comment and both doc pages did not.
Also the loop half had no browser assertion, only the ref half, and the ref half is precisely the one the guard makes unable to throw.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second look, this time at the ordering fix itself, and it does not survive. Unmapping the leftover AFTER its removal is worse than the ordering it replaced, not better, and the comment I wrote to justify it asserts the opposite of what the code does.
Keeping the key when removeBetween refuses sounds like the conservative choice, but it leaves the row HALF removed: start marker gone, end marker still in place. A re-add then takes the reuse branch, and moveRange re-attaches the lone start marker after its own end marker. The next removal of that key walks forward from a start that never reaches its end, so it takes the repeat part's own marker and every following sibling, and the region never renders again.
Measured on the test fixture, dropping key 3 then adding key 4. Unmap-first: three, one, two then three, one, two, four, so the list keeps reconciling around one untracked remnant. Unmap-last: three then three, dead.
So it goes back to unmapping first, and the residual is one untracked row after a refusing DOM removal, which is the right trade against a destroyed list. The comment now records the measurement so the next reader does not flip it again on the same reasoning I did.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole thing again after the ordering revert, and it turned up the worst kind of finding: the revert had nothing holding it. Flipping the loops back left all 16 cases green, because the revert commit had ALSO shortened the test sequence past the render where the two orderings differ. That render is specifically a re-add of the REFUSED key with nothing in between, since any render that treats it as a leftover again unmaps it under either ordering and collapses the difference. Fixed, and the toggle now reds exactly one case.
Worse, the walk's own shape-mismatch branch was still unmapping AFTER the removal, which is the ordering the leftover loop's comment calls fatal, and it produces the destroyed region verbatim for a row whose shape changes and changes back. Both branches agree now.
And the invariant sentence claimed every instance in the document is in exactly one of the two maps, which the residual three lines below contradicts. Scoped.
| // the removal off the end of the region. Same ordering, same | ||
| // trade, and the two have to agree or the invariant the catch | ||
| // relies on holds on one branch and not the other. | ||
| state.map.delete(key); |
There was a problem hiding this comment.
This branch unmaps after the destructive work, which is the ordering the leftover loop below calls fatal. Same premise, same outcome: for a row whose shape changes and changes back, a refused removal keeps the key, the reuse branch re-attaches the start marker after its end marker, and the region dies.
There was a problem hiding this comment.
Fixed in d1ed7831. Both branches unmap first now, so the invariant holds across the function rather than half of it.
| assert.notEqual(readded, doomed); | ||
| }); | ||
|
|
||
| test('repeat: a throw INSIDE the removal loop leaves no leftover still mapped', () => { |
There was a problem hiding this comment.
This does not discriminate. Flipping the loop ordering back leaves it green, because there is a render in the middle that treats the refused key as a leftover again and unmaps it under either ordering. The re-add has to come first, with nothing between it and the throw.
There was a problem hiding this comment.
Fixed in d1ed7831. The re-add now comes immediately after the throw, in one render, with a comment explaining why an intervening render collapses the difference. Verified by toggling the ordering: it reds exactly this case and nothing else.
A throw inside reconcileRepeat's leftover-removal loop left rows that were already disposed and detached still registered in state.map. The visible result was the opposite of what the app asked for: the row it removed stayed on screen, the survivors reordered, and a later render that re-added that key reinserted the detached instance. Two changes. The object-ref write in disposeInstance and clearInstance is guarded to match the callback-ref branch beside it, because neither site clears lastTarget before that write, so a throw made every later teardown of the same instance throw again at the same line. In clearInstance that was permanent: the throw skipped the container's replaceChildren(), so the old DOM stayed and every future template swap of that container threw. The removal loop then deletes each key before touching its row and removes the nodes in a finally, so at any throw point state.map holds exactly the leftovers still in the document. That closes the class rather than the one trigger, since removeBetween stays inside the loop. teardownRepeat gets the same shape. Swallowing a throwing ref unbind is a deliberate divergence from lit, which propagates from both branches. A teardown has no retry, and it has to be total or it abandons the parts behind it. The commit path is untouched: a throwing ref there still reaches the component boundary.
…e other Unmapping the key before touching the row held the invariant for a dispose throw and broke its mirror image: a removal that itself refused left the row in the document described by neither map, which is the untracked-orphan class this repair exists to close. Removing and unmapping in one finally holds both ends, since a row that could not leave the document keeps its key. Also corrects the comment and both doc surfaces on which ref branch does what. applyElement guards every callback-ref call on the commit path too, so a throwing ref CALLBACK was already swallowed everywhere; the object ref was guarded on neither. This makes the two agree on teardown and leaves the commit path alone, so the claim that a commit throw still reaches the boundary is now true only where it is, the object-ref writes. The residual is scoped rather than asserted away: a refused DOM removal costs that row its position, and cannot be cleaned up later, since removeBetween returns early once the start marker is gone. Named in the comment and asserted in the browser test rather than glossed. Adds the browser assertion the loop shape had no coverage for, driving the throw through removeChild rather than the ref unbind the guard makes unable to throw.
Reverts the ordering flip from the previous commit, which was worse than what it replaced, and says why in the comment so it does not get flipped again. Unmapping after the removal keeps the key when removeBetween itself refuses. That row is then half removed, its start marker gone and its end marker still in place, so a re-add hits the reuse branch and moveRange re-attaches the lone start marker AFTER its own end marker. The next removal of that key walks forward from a start that never reaches its end, so it takes the repeat part's marker and every following sibling with it, and the region is dead for good. Measured on the same fixture the tests use. Unmapping first, then dropping key 3 and adding key 4: three, one, two then three, one, two, four, so the list keeps reconciling around one untracked remnant. Unmapping last: three then three, with no later render ever landing again. So the residual is one untracked row after a refusing DOM removal, and it is the right trade against a destroyed list. Both the comment and the tests now state that rather than the reverse. The ref-contract correction from the previous commit stands: applyElement guards every callback-ref call on the commit path, so only the object writes propagate there.
…alk too The ordering the previous commit restored had nothing holding it: flipping the loops back left every test green, because that commit had also shortened the sequence past the render where the two orderings differ. The test now re-adds the REFUSED key before the next removal, which is the only point they diverge, and asserts both the fresh row beside the remnant and that a later render still lands. The walk's own shape-mismatch branch was still unmapping after the removal, which is the ordering the leftover loop's comment calls fatal, and it produces the destroyed region verbatim for a row whose shape changes and changes back. Both branches now unmap first, so the invariant the catch relies on holds across the whole function rather than half of it. The invariant sentence said every instance in the document is described by exactly one of the two maps, which the residual three lines down contradicts. It now excludes the row whose removal refused, which is in neither, by choice.
d1ed783 to
22a06d3
Compare
1 participant
Closes #1268
A throw inside
reconcileRepeat's leftover-removal loop left rows that were already disposed and detached still registered instate.map, so the app got the opposite of what it asked for: the row it removed stayed on screen, the survivors reordered, and a later render that re-added that key reinserted the detached instance. #1172 repaired the bookkeeping for a throw during the WALK, and its catch comment claimed more than the code delivered on the removal branch. This makes the code strong enough for the claim rather than softening it.What changed
The object-ref write is guarded in both teardown sites (
disposeInstance,clearInstance), matching the callback-ref branch already beside it. This is a fix in its own right, not hardening: neither site clearsp.lastTargetbefore the throwing write, so the same instance throws again on every later teardown attempt. InclearInstancethat is permanent, becauserender()calls it on a template-shape swap and the throw skips the trailingcontainer.replaceChildren(), leaving the old DOM in place withhost[INSTANCE]never reassigned. Every future swap of that container then throws at the same part.The leftover-removal loop deletes each key before touching its row, with
removeBetweenin afinally. At any throw pointstate.maptherefore holds exactly the leftovers this pass has not reached, and a row whose dispose threw still leaves the DOM. That closes the class rather than the one trigger, sinceremoveBetweenstays inside the loop and any step added there later is a fresh way in.teardownRepeatshares the shape and gets the same treatment.The ordering here is deliberate and was measured, because unmapping AFTER the removal looks safer and is not. Keeping the key when
removeBetweenrefuses leaves the row HALF removed, start marker gone and end marker in place, so a re-add takes the reuse branch andmoveRangere-attaches the lone start marker after its own end marker. The next removal of that key then walks forward from a start that never reaches its end, taking the repeat part's own marker and every following sibling with it, and the region never renders again. On the test fixture, dropping key 3 then adding key 4: unmapping first givesthree, one, twothenthree, one, two, four, so the list keeps reconciling around one untracked remnant; unmapping last givesthreethenthree. One untracked row beats a destroyed list, and the comment records the measurement so the next reader does not flip it back.The catch comment is extended rather than weakened, stating the invariant that now holds on both branches (every instance still in the document is described by exactly one of the two maps) and naming the one residual, a throw from
removeBetweenitself, which only callsremoveChildon nodes the renderer owns and leaves that row's remaining nodes untracked.Deliberately excluded
applyElement's two object-ref writes stay unguarded. They are on the COMMIT path, where a throw has a defined route (the component boundary) and a defined repair (theCOMMIT_FAILEDsentinel plus this reconciler's catch), so swallowing there would hide a real author error from the boundary built to receive it. That is the seam: teardown swallows because it has no retry and must be total, commit propagates because it has both.Swallowing a throwing
refunbind is a real behaviour change and a deliberate divergence from lit, which guards neither branch and propagates from both. It is recorded in both doc surfaces. To be precise about which branch moved:applyElementalready wraps every callback-ref call on the COMMIT path, so a throwing ref CALLBACK was already swallowed everywhere and the object ref was guarded in neither place. This makes the two agree on teardown and leaves the commit path alone.removeArrayItem(L1914) was read and confirmed to reachdisposeInstance, which is what #1250's repair forreconcileArrayassumes. #1250 lands on top of this.Test plan
packages/core/test/rendering/directive-commit-throw.test.js): four new cases. Dropping a row whose ref unbind throws now removes it and keeps the survivor's identity; re-adding a key dropped that way builds a fresh row rather than resurrecting the disposed instance; a throw INSIDE the removal loop (driven fromremoveChild, so it does not depend on the guard) leaves no leftover still mapped; and a container holding a throwing ref can swap templates twice. 16/16 pass.75fceaf4.packages/core/test/rendering/browser/directive-commit-throw.test.js): one new case for the identity facts linkedom cannot prove (the survivor is moved rather than rebuilt, the resurrected key is a genuinely new element). Full run green on Chromium, Firefox and WebKit.render-client.jsis the client renderer, not a runtime-sensitive surface, and the parity hook does not match this path.dist/, nobun, noexamples/blog/node_modules) and reproduce identically with this change reverted.Doc surfaces
.agents/skills/webjs/references/components.md: a new paragraph stating teardown totality, the never-removed-but-still-mapped invariant, and the lit divergence.website/app/docs/error-handling/page.ts: the same in that page's prose. It stated the consistency claim as an absolute with no carve-out, so leaving it would have kept overstating.