Skip to content

Commit 78400eb

Browse files
authored
fix: native <list> virtualization state leaked on nested removal (#1631)
listStates (ts/miso/native/mts/context.ts) is keyed by __GetElementUniqueID, separately from the nodeId-keyed event registries that dropChildren (ts/miso/native/mts.ts) already tore down per node in a removed subtree. A <list> removed via an ancestor wrapper -- not as the direct argument to removeChild/replaceChild -- never had its entry cleared, leaking its {node, items, known} state and every ElementRef in it for the rest of the session. destroyListState clears a node's list state and is now called alongside destroyNodeEvents for every node dropChildren visits, covering both direct and nested removal.
1 parent dd6c1ee commit 78400eb

3 files changed

Lines changed: 84 additions & 4 deletions

File tree

ts/miso/native/mts.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import type { ElementRef } from "@lynx-js/type-element-api";
2525
import
2626
{ drawingContext
2727
, destroyNodeEvents
28+
, destroyListState
2829
} from './mts/context';
2930

3031
export function mts () {
@@ -145,14 +146,19 @@ function processMessage (m : PATCH, runtime) {
145146
native handle (no `.children`/`.nodeId` JS properties), so read the id from
146147
Config and walk children via the element PAPI. `node` is captured before
147148
detachment, so its subtree is still intact here. */
148-
function dropChildren (nodeMap: Record<number, ElementRef>, node: ElementRef) {
149+
export function dropChildren (nodeMap: Record<number, ElementRef>, node: ElementRef) {
149150
const nodeId = __GetConfig(node)?.nodeId as number | undefined;
150151
if (nodeId !== undefined) {
151152
delete nodeMap[nodeId];
152153
// Tear down this node's event state (main-thread routing registry + native
153154
// direct-bind listeners), else each destroyed node leaks its entries.
154155
destroyNodeEvents(node, nodeId);
155156
}
157+
// Keyed by __GetElementUniqueID rather than nodeId, so this runs
158+
// unconditionally (not gated on nodeId being set) -- a removed <list>,
159+
// wherever it sits in this subtree, must not keep its virtualization
160+
// state (and every ElementRef it holds) alive forever.
161+
destroyListState(node);
156162
for (let child = __FirstElement(node) as ElementRef; child; child = __NextElement(child) as ElementRef) {
157163
dropChildren(nodeMap, child);
158164
}

ts/miso/native/mts/context.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ function nextNodeId () : number {
3636
// hold each list's item elements and serve them back by index — one element per
3737
// cell, with no reuse pool (that's the "full recycler" upgrade path).
3838
type ListState = { node: ElementRef; items: Array<ElementRef>; known: number };
39-
const listStates = new Map<number, ListState>();
39+
// Exported (read/write) for test introspection only -- production code should
40+
// go through listStateOf/destroyListState, not touch this Map directly.
41+
export const listStates = new Map<number, ListState>();
4042

4143
// Main-thread event routing registry, keyed by the element's `nodeId`.
4244
// NOT stored in element config directly: Lynx __SetConfig/__GetConfig round-trips
@@ -75,6 +77,17 @@ export function destroyNodeEvents(node : ElementRef, nodeId : number) : void {
7577
mainThreadKeys.delete(nodeId);
7678
}
7779

80+
// Drop a node's <list> virtualization state, if it has any. Keyed by
81+
// __GetElementUniqueID (not nodeId, unlike destroyNodeEvents -- see
82+
// listStateOf) so this must be called independently, not folded into
83+
// destroyNodeEvents. Called from `dropChildren` (ts/miso/native/mts.ts) for
84+
// every node in a removed subtree -- without this, a <list> removed via an
85+
// ancestor wrapper (rather than directly) keeps its {node, items, known}
86+
// entry, and every ElementRef in it, in this Map forever.
87+
export function destroyListState(node : ElementRef) : void {
88+
listStates.delete(__GetElementUniqueID(node));
89+
}
90+
7891
function nodeIdOf(node : ElementRef) : number | undefined {
7992
return (__GetConfig(node) as any)?.nodeId as number | undefined;
8093
}

ts/spec/native-mts.spec.ts

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@
44
and per-node event-registry teardown. The Lynx PAPI globals (`__GetConfig`,
55
`__AddEvent`) and the `runtime`/`lynx` host objects are stubbed here — in a web
66
build they don't exist, which is exactly why this code has no other coverage. */
7-
import { test, expect, describe, beforeEach, afterAll, beforeAll } from 'bun:test';
8-
import { routeEvent, destroyNodeEvents, drawingContext } from '../miso/native/mts/context';
7+
import { test, expect, describe, beforeEach, afterEach, afterAll, beforeAll } from 'bun:test';
8+
import { routeEvent, destroyNodeEvents, drawingContext, listStates, destroyListState } from '../miso/native/mts/context';
9+
import { dropChildren } from '../miso/native/mts';
910
import { vnode, vfrag, vcomp, vtext } from '../miso/smart';
1011
import type { EventContext, NodeId } from '../miso/types';
1112

@@ -242,3 +243,63 @@ describe('drawingContext.removeAttribute — MTS', () => {
242243
delete (globalThis as any).__SetAttribute;
243244
});
244245
});
246+
247+
// Regression: dropChildren (ts/miso/native/mts.ts) recursively tears down a
248+
// removed subtree's event state via destroyNodeEvents, but previously never
249+
// touched listStates (ts/miso/native/mts/context.ts) -- so a <list> removed
250+
// via an ancestor wrapper (rather than directly) kept its {node, items,
251+
// known} entry, and every ElementRef in it, in that Map forever.
252+
describe('destroyListState / dropChildren — <list> virtualization teardown', () => {
253+
254+
afterEach(() => {
255+
for (const g of ['__GetElementUniqueID', '__FirstElement', '__NextElement'])
256+
delete (globalThis as any)[g];
257+
});
258+
259+
test('destroyListState clears a node\'s own list state', () => {
260+
const node: El = { nodeId: 72 };
261+
(globalThis as any).__GetElementUniqueID = (n: El) => n.nodeId;
262+
263+
listStates.set(72, { node: node as any, items: [{} as any], known: 1 });
264+
expect(listStates.has(72)).toBeTrue();
265+
266+
destroyListState(node as any);
267+
expect(listStates.has(72)).toBeFalse();
268+
});
269+
270+
test('destroyListState on a node with no list state is a harmless no-op', () => {
271+
const node: El = { nodeId: 73 };
272+
(globalThis as any).__GetElementUniqueID = (n: El) => n.nodeId;
273+
expect(() => destroyListState(node as any)).not.toThrow();
274+
});
275+
276+
test('dropChildren clears the listStates entry of a <list> nested under a removed wrapper', () => {
277+
// tree: wrapper(70) -> list(71), removed as a unit via the wrapper --
278+
// the list itself is never the direct argument to removeChild/replaceChild.
279+
const wrapper: El = { nodeId: 70 };
280+
const list: El = { nodeId: 71 };
281+
282+
(globalThis as any).__GetElementUniqueID = (n: El) => n.nodeId;
283+
const childrenOf = new Map<number, El[]>([[70, [list]], [71, []]]);
284+
(globalThis as any).__FirstElement = (n: El) => childrenOf.get(n.nodeId)?.[0] ?? null;
285+
(globalThis as any).__NextElement = (n: El) => {
286+
for (const kids of childrenOf.values()) {
287+
const i = kids.indexOf(n);
288+
if (i >= 0 && i + 1 < kids.length) return kids[i + 1];
289+
}
290+
return null;
291+
};
292+
293+
listStates.set(71, { node: list as any, items: [{} as any, {} as any], known: 2 });
294+
expect(listStates.has(71)).toBeTrue();
295+
296+
const nodeMap: Record<number, El> = { 70: wrapper, 71: list };
297+
dropChildren(nodeMap as any, wrapper as any);
298+
299+
// the list's virtualization state (and every ElementRef it held) is gone,
300+
// not just its runtime.nodes entry
301+
expect(listStates.has(71)).toBeFalse();
302+
expect(nodeMap[70]).toBeUndefined();
303+
expect(nodeMap[71]).toBeUndefined();
304+
});
305+
});

0 commit comments

Comments
 (0)