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..c7e93d9 --- /dev/null +++ b/src/identity.test.ts @@ -0,0 +1,57 @@ +import { getQuickJS } 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. 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 ctx = (await getQuickJS()).newContext(); + const arena = new Arena(ctx, options); + fn(arena); + arena.dispose(); + ctx.dispose(); +} + +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 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, + ); });