ReviewOS

zig-utils/zig-js

Finalization: retain precise cleanup ownership through callback admission and relocation

#906
Closed chris opened this 9 days ago ·
9 days ago

Parents: #474, #476 Related: #896, #480, #493, #888

Source finding

At 65dbb40, Interpreter.drainFinalizationCleanupJobs keeps registry and its callback in native locals across repeated callback invocations. It also calls registry.finRecordTakeReady() before the fallible callValue(cb, held) preparation. The shared cleanup queue roots the registry, but its relocation does not rewrite those native locals. The drain also uses a per-invocation queue index even though nested cleanup can clear/repopulate the same queue. These ownership boundaries require deterministic witnesses.

This is a source audit, not a measured use-after-free or failing-allocator result. #896 now separates unowned cleanup OOM from the host's original evaluation result and retains untouched records after a callback has actually run. That error-reporting boundary does not itself establish record admission or precise ownership inside the finalization drain.

Specification boundary

ECMA-262 CleanupFinalizationRegistry, steps 3.b–3.c removes a cell before invoking HostCallJobCallback. The commit point must respect that order. Drain-owned preparation that fails before an admitted Call should leave the selected cell owned; once Call is admitted, an abrupt result consumes that cell. In particular, do not replay a proxy apply getter, parameter initializer, native callback, or callback body that already performed effects. The original source audit did not establish a generic requirement to restore cells after every call-setup OOM.

Required work

  • Produce deterministic allocation-failure witnesses distinguishing drain preparation before Call admission from an abrupt admitted Call, with exact ready/held-value ownership.
  • Give each selected registry, callback and held value precise relocatable ownership across callback preparation, execution and nested cleanup. Reload relocated values before subsequent operations.
  • Make the spec-aligned admission point explicit: retain work on failed drain preparation; never replay an admitted Call, including observable proxy/parameter preparation.
  • Preserve remaining registry/record order and reentrant queue insertion without holding realm/object locks while invoking JS or allocating through recovery.
  • Preserve exact release on success, ordinary callback throw, engine OOM, registry unregister and Context teardown. No disabling moving GC, arbitrary queue limits, conservative-stack dependence, emergency pool or swallowed failure.

Acceptance

Deterministic failing-allocator and actual nursery-relocation tests with native-stack scanning disabled; nested cleanup and callback-side unregister/new registration; exact callback counts and queue/held-value ownership before failure and after recovery; concurrent/no-GIL and suppression-free TSan; affected FinalizationRegistry/WeakRef Test262, full units and leak checks. Link implementation commits and keep source findings distinct from measured failures.

Security scope is engine state and lifetime integrity under script-controlled graph shape and memory pressure; no embedding permission or sandbox layer is required.

9 days ago

Implementation in progress, with two reproduced regressions against the unchanged engine:

  • Nested cleanup drains delivered only 3 of 4 callbacks: the suspended outer queue index skipped a registry appended after the inner drain cleared the queue.
  • An actual moving collection rewrote the registry/heap graph but left the native held-value argument stale. The witness detects pointer mismatch before dereferencing the old address.

The candidate uses borrowed, relocation-aware registry/callback/argument roots and a realm-locked shared cursor. Processed registries can be queued again; consumed queue prefixes are compacted before growth and excluded from GC roots. Initial fixed behavior checks pass, including a 64-callback alternating-registry chain with unchanged queue capacity. Expanded validation is still running, so this is not yet a landed or complete claim.

Call admission remains infallible in the drain itself. Per CleanupFinalizationRegistry, a cell is removed before Call and is not restored after user code in a Proxy apply getter or callback has run. #896 checkpoint propagation and #907 mid-GC completion diagnostics remain separate.

9 days ago

Landed and pushed 8d7a3b7a — fix(finalization): retain cleanup roots across reentrant checkpoints (#906). Authored/committed as Chris, conventional subject and measured description, no trailers.

The original witnesses fail on the old engine: nested cleanup delivers 3 rather than 4 callbacks, and an actual nursery move leaves the native held-value argument stale (detected before dereferencing it). Borrowed root frames now retain and rewrite registry/callback/argument slots through direct and nested callbacks. A realm-locked shared cursor preserves reentrant publication; record claiming or empty-entry retirement is atomic with publication. Queue tracing/deduplication use the active suffix, with amortized prefix reuse.

The spec boundary is explicit: drain-owned root admission is infallible; a cell is removed before Call and never replayed after an admitted Proxy accessor/callback fails. Untouched records remain queued. Callback-side unregister removes its selected record, and new ready work remains deliverable.

Measured validation:

  • Full Debug integration on this engine: 2,385 passed, one platform skip, zero failures/leaks (517.5 seconds across 10 shards).
  • 12 finalization and 19 host-checkpoint checks pass ReleaseSafe and unsuppressed TSan.
  • The five strengthened regression tests pass Debug, ReleaseSafe, and unsuppressed TSan: actual failing allocator, zero allocation attempts during drain admission, direct/nested nursery relocation with native-stack scanning disabled, callback unregister/requeue, 64 callbacks without queue-capacity growth, and 128 concurrent ready publications with exact FIFO/once-only delivery.
  • Public C/Home ABI fixtures and audits pass; Home runtime matrix links 392/392 symbols.
  • Full seed-1 mid-GC profile: 900 programs, zero failures, wall-ms 604422.
  • Exact paired Test262: FinalizationRegistry 47 → 47, WeakRef 29 → 29, zero failures; flips 0 cases in those subtrees.

Closing #906. The distinct historical incomplete-sweep observation stays open in #907; this pass does not establish its cause. Private failure reporting/admission remains #896, and the newly identified pending/cached exception-cell GC ownership audit is #908.

Sign in to comment on this issue.