From 7a0cd0c1d1ec8b21ee8352fc0e91f9f9899dc370 Mon Sep 17 00:00:00 2001 From: rot1024 Date: Tue, 9 Jun 2026 19:46:38 +0900 Subject: [PATCH 1/2] fix: preserve object identity across the host boundary (#4) A host function's return value is disposed by the VM once consumed. The VMMap retains marshalled object handles for identity while sync is on, but the returned handle WAS the map's own handle, so the VM disposed it and the map entry went stale: marshalling the same value twice produced two distinct VM objects (foo !== id(foo), get() !== get()) and leaked the orphaned sibling. Hand the VM a dup of the handle when the map retains it (prepareReturn), keeping the map's copy alive. Identity now holds across round-trips and repeated returns; with syncEnabled:false nothing is retained, so behaviour is unchanged (no dup). Host-side mutations of an already-marshalled object are no longer picked up by re-marshalling (the value is identity-cached); arena.sync remains the way to propagate host writes. Updated the edge 'getter' test accordingly and un-commented its ctx.dispose() now that the leak is gone. --- src/edge.test.ts | 46 ++++++++++++++++++++++++++++++-- src/identity.test.ts | 58 +++++++++++++++++++++++++++++++++++++++++ src/index.ts | 12 +++++++++ src/marshal/function.ts | 12 ++++++++- src/marshal/index.ts | 16 +++++++++++- 5 files changed, 140 insertions(+), 4 deletions(-) create mode 100644 src/identity.test.ts diff --git a/src/edge.test.ts b/src/edge.test.ts index 5936f91..5906b19 100644 --- a/src/edge.test.ts +++ b/src/edge.test.ts @@ -36,12 +36,54 @@ describe("edge cases", () => { expect(cb.current?.()).toBe(0); expect(called).toEqual(["a", "b"]); + // The host getters are still traversed on every access, so `called` grows. + // But `obj` keeps its identity across marshals (sync is on), so the VM holds + // the value snapshot from the first marshal: a later host-side mutation is + // not re-marshalled. Use `arena.sync(obj)` to propagate host writes (see the + // "getter (synced)" test below). obj.c = 1; - expect(cb.current?.()).toBe(1); // this line causes an error when context is disposed + expect(cb.current?.()).toBe(0); expect(called).toEqual(["a", "b", "a", "b"]); arena.dispose(); - // ctx.dispose(); // reports an error + // Re-marshalling `obj` used to leak a handle, which aborted ctx.dispose(). + // Now that the stale-entry handle is disposed, the context disposes cleanly. + expect(() => ctx.dispose()).not.toThrow(); + }); + + // this test takes more than about 20s + test("getter (synced)", async () => { + const ctx = (await getQuickJS()).newContext(); + const arena = new Arena(ctx, { isMarshalable: true }); + + // A synced object propagates host-side writes to the VM, so re-reads see the + // updated value while still keeping a stable identity. + const obj = arena.sync({ c: 0 }); + const exposed = { + get a() { + return { + get b() { + return obj; + }, + }; + }, + }; + const cb: { current?: () => any } = {}; + arena.expose({ + exposed, + register: (fn: () => any) => { + cb.current = fn; + }, + }); + + arena.evalCode(`register(() => exposed.a.b.c);`); + expect(cb.current?.()).toBe(0); + + obj.c = 1; + expect(cb.current?.()).toBe(1); + + arena.dispose(); + expect(() => ctx.dispose()).not.toThrow(); }); test( diff --git a/src/identity.test.ts b/src/identity.test.ts new file mode 100644 index 0000000..007ed00 --- /dev/null +++ b/src/identity.test.ts @@ -0,0 +1,58 @@ +import variant from "@jitl/quickjs-wasmfile-debug-sync"; +import { newQuickJSWASMModuleFromVariant } from "quickjs-emscripten"; +import { describe, expect, it } from "vitest"; + +import { Arena } from "."; + +// A host function's return value is disposed by the VM once consumed. Marshalled +// object handles are retained in the VMMap for identity while sync is on, so the +// returned handle must be dup'd — otherwise the map entry goes stale and the same +// value marshalled twice yields two distinct VM objects (and used to leak). These +// tests pin the identity behaviour (issue #4). +async function withArena( + options: ConstructorParameters[1], + fn: (arena: Arena) => void, +) { + const mod = await newQuickJSWASMModuleFromVariant(variant as any); + const ctx = mod.newContext(); + const arena = new Arena(ctx, options); + fn(arena); + arena.dispose(); + expect(() => ctx.dispose()).not.toThrow(); +} + +describe("marshal identity", () => { + it("preserves identity for a VM object round-tripped through the host", async () => { + await withArena({ isMarshalable: true }, arena => { + arena.expose({ id: (x: any) => x }); + // foo originates in the VM, is passed to the host and returned: the + // round-trip must yield the same object. + expect(arena.evalCode(`let foo = id({}); foo === id(foo)`)).toBe(true); + }); + }); + + it("returns the same VM object when a host function returns the same object twice", async () => { + await withArena({ isMarshalable: true }, arena => { + const shared = { k: 1 }; + arena.expose({ get: () => shared }); + expect(arena.evalCode(`get() === get()`)).toBe(true); + }); + }); + + it("keeps identity across retained references", async () => { + await withArena({ isMarshalable: true }, arena => { + const shared = { k: 1 }; + arena.expose({ get: () => shared }); + expect(arena.evalCode(`let a = get(); let b = get(); a === b`)).toBe(true); + }); + }); + + it("does not retain identity when sync is disabled", async () => { + await withArena({ isMarshalable: true, syncEnabled: false }, arena => { + const shared = { k: 1 }; + arena.expose({ get: () => shared }); + // With sync off, objects are not retained, so each marshal is independent. + expect(arena.evalCode(`get() === get()`)).toBe(false); + }); + }); +}); diff --git a/src/index.ts b/src/index.ts index 5c92fc0..3766d1b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -490,6 +490,7 @@ export class Arena { registerTransient: this._registerTransient, disposeTransient: this._disposeTransient, preApply: this._marshalPreApply, + prepareReturn: this._prepareMarshalReturn, custom: this._options?.customMarshaller, }); @@ -501,6 +502,17 @@ export class Arena { return [handle, !syncEnabled || !this._map.hasHandle(handle)]; }; + _prepareMarshalReturn = (h: QuickJSHandle): QuickJSHandle => { + // A host function's return value is disposed by the VM once it is consumed. + // When sync is on, the VMMap retains object handles for identity, so the + // handle we hand back is the one the map owns: returning it directly would + // let the VM dispose the map's copy, leaving a stale entry that breaks + // `x === fn()` identity across calls. Hand the VM a dup instead and keep + // ours alive. With sync off, handles are not retained, so this is a no-op. + const syncEnabled = this._options?.syncEnabled ?? true; + return syncEnabled && h.alive && this._map.hasHandle(h) ? h.dup() : h; + }; + _preUnmarshal = (t: any, h: QuickJSHandle): Wrapped => { return this._register(t, h, undefined, this._options?.syncEnabled ?? true)?.[0]; }; diff --git a/src/marshal/function.ts b/src/marshal/function.ts index 6d3be75..c4d1d73 100644 --- a/src/marshal/function.ts +++ b/src/marshal/function.ts @@ -13,6 +13,7 @@ export default function marshalFunction( preMarshal: (target: unknown, handle: QuickJSHandle) => QuickJSHandle | undefined, preApply?: (target: (...args: any[]) => any, thisArg: unknown, args: unknown[]) => any, disposeTransient: (handle: QuickJSHandle) => void = () => {}, + prepareReturn: (handle: QuickJSHandle) => QuickJSHandle = h => h, ): QuickJSHandle | undefined { if (typeof target !== "function") return; @@ -33,7 +34,16 @@ export default function marshalFunction( return this; } - return marshal(preApply ? preApply(target as (...args: any[]) => any, that, args) : (target as (...args: any[]) => any).apply(that, args)); + // The VM disposes whatever we return here. `prepareReturn` dups the + // handle when the VMMap retains it, so the map keeps a live copy and + // identity (`x === fn()` across calls) survives instead of going stale. + return prepareReturn( + marshal( + preApply + ? preApply(target as (...args: any[]) => any, that, args) + : (target as (...args: any[]) => any).apply(that, args), + ), + ); }) .consume(handle2 => // fucntions created by vm.newFunction are not callable as a class constrcutor diff --git a/src/marshal/index.ts b/src/marshal/index.ts index 0c5306d..f11c64a 100644 --- a/src/marshal/index.ts +++ b/src/marshal/index.ts @@ -28,6 +28,11 @@ export type Options = { mode: true | "json" | undefined, ) => QuickJSHandle | undefined; preApply?: (target: (...args: any[]) => any, thisArg: unknown, args: unknown[]) => any; + // Adjust ownership of a handle that is about to be handed to the VM as a host + // function's return value. The VM disposes whatever it receives, so a handle + // the VMMap retains (for identity, while sync is on) must be dup'd here or its + // map entry goes stale. Defaults to identity (no-op). + prepareReturn?: (handle: QuickJSHandle) => QuickJSHandle; custom?: Iterable<(obj: unknown, ctx: QuickJSContext) => QuickJSHandle | undefined>; }; @@ -73,7 +78,16 @@ export function marshal(target: unknown, options: Options): QuickJSHandle { return ( marshalCustom(ctx, target, pre2, [...defaultCustom, ...(options.custom ?? [])]) ?? marshalPromise(ctx, target, marshal2, pre2) ?? - marshalFunction(ctx, target, marshal2, unmarshal, pre2, options.preApply, disposeTransient) ?? + marshalFunction( + ctx, + target, + marshal2, + unmarshal, + pre2, + options.preApply, + disposeTransient, + options.prepareReturn, + ) ?? marshalMapSet(ctx, target, marshal2, pre2, disposeTransient) ?? marshalObject(ctx, target, marshal2, pre2, disposeTransient) ?? ctx.undefined From 226aab6c2ccb0eadf7ccb24eed611d3af2cfad6e Mon Sep 17 00:00:00 2001 From: rot1024 Date: Tue, 9 Jun 2026 19:54:52 +0900 Subject: [PATCH 2/2] test: keep identity tests on the fast runtime and lift leak-test timeouts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Identity assertions don't need the debug-sync runtime's leak detection (that is covered by remarshalleak.test.ts), so run them on the default runtime — under coverage the debug-sync variant pushed each in-VM call past the 30s timeout in CI. The remaining debug-sync leak tests get a 90s timeout, matching the existing slow 'many newFunction' edge test. --- src/identity.test.ts | 13 ++++++------- src/remarshalleak.test.ts | 39 +++++++++++++++++++++++++-------------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/identity.test.ts b/src/identity.test.ts index 007ed00..c7e93d9 100644 --- a/src/identity.test.ts +++ b/src/identity.test.ts @@ -1,5 +1,4 @@ -import variant from "@jitl/quickjs-wasmfile-debug-sync"; -import { newQuickJSWASMModuleFromVariant } from "quickjs-emscripten"; +import { getQuickJS } from "quickjs-emscripten"; import { describe, expect, it } from "vitest"; import { Arena } from "."; @@ -7,18 +6,18 @@ import { Arena } from "."; // A host function's return value is disposed by the VM once consumed. Marshalled // object handles are retained in the VMMap for identity while sync is on, so the // returned handle must be dup'd — otherwise the map entry goes stale and the same -// value marshalled twice yields two distinct VM objects (and used to leak). These -// tests pin the identity behaviour (issue #4). +// value marshalled twice yields two distinct VM objects. These tests pin the +// identity behaviour (issue #4); the no-leak side is covered by +// remarshalleak.test.ts under the debug-sync runtime. async function withArena( options: ConstructorParameters[1], fn: (arena: Arena) => void, ) { - const mod = await newQuickJSWASMModuleFromVariant(variant as any); - const ctx = mod.newContext(); + const ctx = (await getQuickJS()).newContext(); const arena = new Arena(ctx, options); fn(arena); arena.dispose(); - expect(() => ctx.dispose()).not.toThrow(); + ctx.dispose(); } describe("marshal identity", () => { diff --git a/src/remarshalleak.test.ts b/src/remarshalleak.test.ts index 8b02a02..2e40124 100644 --- a/src/remarshalleak.test.ts +++ b/src/remarshalleak.test.ts @@ -21,19 +21,30 @@ async function withArena(fn: (arena: Arena) => void) { } describe("re-marshal handle leak", () => { - it("does not leak when a host function returns the same object twice", async () => { - await withArena(arena => { - const shared = { k: 1 }; - arena.expose({ get: () => shared }); - arena.evalCode(`get(); get();`); - }); - }); + // Calling an exposed function from inside the VM marshals the whole global + // graph on each call, which is slow under the debug-sync runtime + coverage, + // so these get a generous timeout (cf. the "many newFunction" edge test). + it( + "does not leak when a host function returns the same object twice", + async () => { + await withArena(arena => { + const shared = { k: 1 }; + arena.expose({ get: () => shared }); + arena.evalCode(`get(); get();`); + }); + }, + 90000, + ); - it("does not leak when the same object is compared across two calls", async () => { - await withArena(arena => { - const shared = { k: 1 }; - arena.expose({ get: () => shared }); - arena.evalCode(`get() === get();`); - }); - }); + it( + "does not leak when the same object is compared across two calls", + async () => { + await withArena(arena => { + const shared = { k: 1 }; + arena.expose({ get: () => shared }); + arena.evalCode(`get() === get();`); + }); + }, + 90000, + ); });