fix(mesh): ref-count GPU buffers shared by cloneTransformNode to prevent double-free#353
Conversation
…ent double-free cloneMeshNode intentionally shares _gpu (and skeleton/morphTargets/thinInstances) GPU buffers with its source mesh for cheap instancing, but disposeMeshGpu destroyed those buffers unconditionally. Disposing either the source or a clone would free buffers the other was still using (use-after-free), and disposing both would double-free the same GPUBuffer. Add a lazy WeakMap-based ref-count registry (mesh-gpu-refcount.ts), mirroring the existing mesh-scene-registry.ts pattern. cloneMeshNode now shares _gpu by identity (not a shallow copy) and retains an extra ownership claim on every shared GPU resource; disposeMeshGpu releases a claim per resource and only calls .destroy() when it was the last owner. Verified the identity-sharing change is safe against device-lost recovery (engine/device-lost-recovery.ts): rebuildMeshes only ever does whole-object reassignment (`mesh._gpu = uploadRetainedMesh(...)`), never Object.assign on the existing shared object, and only for meshes whose _cpuPositions/_cpuNormals/ _cpuIndices are set — CPU arrays that are always shared by reference between a mesh and its clone, so both or neither independently qualify for reassignment. skeleton/morphTargets recovery legitimately uses Object.assign on the shared object in place, which is correct since both aliases should see the rebuilt buffers. Added a regression test locking in this invariant. Added unit tests covering: disposing the source while a clone is alive does not destroy shared buffers, disposing the clone afterward destroys them exactly once, and the device-lost-recovery reassignment pattern cleanly un-aliases source/clone without cross-corrupting disposal. Ran full unit suite (613 passed), lint (ESLint + tsc across all tsconfigs), full bundle-size ceiling suite (205 passed), and parity tests for all clone-affected scenes (12, 41, 42, 47, 104, 105, 129) plus the device-lost-recovery scene (164) — all pass, including scene42 (physics clone) at MAD=0.000. Rebuilt and committed the per-scene bundle manifests for the affected scenes (small expected growth, all within ceilings). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes GPU buffer lifetime issues when meshes are cloned via cloneTransformNode by introducing ref-counting for GPU-backed resource objects that are intentionally shared between a source mesh and its clones, preventing use-after-free and double-destroy of GPUBuffers.
Changes:
- Added a lazy
WeakMap-based GPU resource refcount registry (retainGpuResource/releaseGpuResource). - Updated
cloneMeshNodeto share_gpuby identity and retain ownership for all shared GPU resource objects. - Updated
disposeMeshGputo only destroy GPU buffers whenreleaseGpuResourceindicates the last owner; added unit tests and refreshed per-scene bundle manifests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/lite/unit/mesh-clone-gpu-dispose.test.ts | Adds unit coverage for shared-buffer disposal ordering and device-lost rebuild reassignment behavior. |
| packages/babylon-lite/src/scene/transform-node.ts | Makes mesh clones share the same _gpu wrapper object and retains shared GPU resources on clone. |
| packages/babylon-lite/src/mesh/mesh-gpu-refcount.ts | Introduces lazy, side-effect-free refcount tracking for shared GPU resource objects. |
| packages/babylon-lite/src/mesh/mesh-dispose.ts | Gates buffer destruction behind releaseGpuResource so shared resources are destroyed exactly once. |
| lab/public/bundle/manifest/scene12.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
| lab/public/bundle/manifest/scene41.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
| lab/public/bundle/manifest/scene42.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
| lab/public/bundle/manifest/scene47.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
| lab/public/bundle/manifest/scene104.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
| lab/public/bundle/manifest/scene105.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
| lab/public/bundle/manifest/scene129.json | Updates measured bundle size/runtime chunk hashes after engine code changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… drops it Addresses Copilot review comment on PR #353 (mesh-dispose.ts:27): the new GPU-buffer ref-counting design assumes every retainGpuResource() call is eventually matched by exactly one releaseGpuResource() call from whichever mesh currently holds the field. attachVat() (vat/vat-baker.ts) violated this by silently doing `mesh.skeleton = null` after reusing the skeleton's buffers into `mesh.vat`, without ever releasing its claim. For a skeleton shared via cloneTransformNode, this permanently pinned the ref-count above zero, leaking the skeleton's GPU buffers forever (no remaining owner could ever be seen as "last"). Fix: attachVat now calls releaseGpuResource(skel) before nulling mesh.skeleton, and destroys skel.boneTexture only if that call reports it was the last owner. jointsBuffer/weightsBuffer/joints1Buffer/weights1Buffer are left untouched since they're reused by `mesh.vat` (VatData) and there is no VAT disposal path yet — matching pre-existing behavior for those fields (not a regression), while boneTexture (VAT-unused) is now correctly freed exactly once with no permanent leak and no double-free when a clone sibling is involved. Documented the general invariant in mesh-gpu-refcount.ts so future code that reassigns a tracked field outside disposeMeshGpu doesn't reintroduce this class of bug. Added a unit test simulating attachVat's release-then-null sequence on a clone sharing a skeleton, verifying: the clone's release doesn't destroy anything prematurely (source still owns it), the source's later disposal correctly destroys the skeleton buffers exactly once, and disposing the already-VAT-baked clone afterwards is a safe no-op. Verified: full unit suite (614 passed), lint (eslint + tsc across all tsconfigs), and parity tests for scene218 (VAT) and scene219 (instanced VAT) — both MAD=0.005, matching pre-existing baseline (no regression). Bundle sizes for scene218/scene219 unchanged (measured via build-bundle-scenes --scenes=218,219); no manifest changes needed.
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
1 similar comment
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
Lab - Static SiteBuild 20260702.5 - merge @ 1f32aaf |
sebavan
left a comment
There was a problem hiding this comment.
2 remarks:
- I am wondering if the counter should be part of the buffer themselves to always use it but no real preferences ?
- Here, it feels limited to the clone part so should it be at the buffer level ?
interesting catch. We can have the counter for more features apart from clone for sure. let me think about this. It will require more changes than this one (probably changing the objects themselves). researching now. |
…itself Per reviewer feedback: replace the WeakMap<object, number> side-table in mesh-gpu-refcount.ts with a plain optional `_refCount` field declared directly on each trackable resource interface (MeshGPU, SkeletonData, MorphTargetData, ThinInstanceData). retainGpuResource/releaseGpuResource now just read/write that field instead of doing a WeakMap lookup. Rationale: measured both variants end-to-end on scene12 (clone-heavy, previously at its bundle-size ceiling) via build-bundle-scenes + summing the actual runtime-loaded chunk bytes. The embedded-field version came out 33 bytes SMALLER (no WeakMap get/set call overhead to minify), and needs zero changes at the 8 resource-creation call sites — `_refCount` stays `undefined` (implicit sole owner) until a clone actually retains it, identical semantics to "no WeakMap entry" today. Field renamed with a `_` prefix per this repo's `underscore-requires-internal` lint rule. Documented a related pitfall discovered while doing this: any code that REPLACES a resource wholesale (e.g. device-lost recovery rebuilding mesh._gpu) must build the replacement as a fresh object literal, never a spread of the old one — spreading would carry over the OLD `_refCount` and corrupt the new object's ownership count. Confirmed the real production call sites (uploadRetainedMesh, and the Object.assign(old, rebuilt) used for skeleton/morphTargets recovery) already follow this safely; only a test I'd written previously used an unrealistic spread pattern, which is now fixed to match the real fresh-literal pattern. Also fixed an unrelated line-number drift in the no-direct-mat4-writes audit allowlist (thin-instance.ts's tracked write shifted from line 119 to 122 after adding the new field. Verified: full unit suite (614 passed, including the fixed device-lost-recovery regression test), lint clean, full bundle-size ceiling suite (205/205 passed — scene12/105/129/218 all shifted by ±0.1 KB, comfortably within ceilings), and parity tests for every clone/VAT-using scene (12, 41, 42, 47, 104, 105, 129, 218, 219) — all matching their pre-existing MAD baselines, no visual regression. EOF )
@sebavan - i pushed a change that moves the count to the objects themselves. it actually doesn't add more size as i suspected. I can always revert this and go back to the clone-only path i had before. |
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
Lab - Static SiteBuild 20260702.9 - merge @ 699a01e |
Love this one, just let me know what you prefer for the clone only part ? |
…nt.ts Address ryantrem's PR review comment: the retain/release helper isn't mesh- or GPU-specific, so move it out of mesh/ and generalize the naming. renames: mesh/mesh-gpu-refcount.ts -> resource/ref-count.ts retainGpuResource/releaseGpuResource -> retain/release Pure rename, no logic changes. Verified: - full lint clean - full unit suite: 614/614 passing - bundle-scenes rebuild for affected scenes (12/41/42/47/104/105/129/218/219): byte-identical, no manifest changes - parity tests for the same 9 scenes: all passing Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
Lab - Static SiteBuild 20260703.7 - merge @ 884f071 |
# Conflicts: # lab/public/bundle/manifest/scene104.json # lab/public/bundle/manifest/scene105.json # lab/public/bundle/manifest/scene47.json
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
… KB) The merge of master into this branch shifted scene42's runtime bundle by ~0.1 KB (still under its 52 KB ceiling). The committed per-scene manifest was from the pre-merge ref-count commit and is now stale, tripping CI's validate:bundle-manifest step (committed 51KB vs rebuilt 52KB rounded). Regenerated via build:bundle-scenes; chunk set unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bundle Size ChangesIncreases
Decreases
Sizes rounded to nearest KB. Run |
1 similar comment
Bundle Size ChangesIncreases
Decreases
Sizes rounded to nearest KB. Run |
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
…easurement) Merging master (incl. #370 'Harden bundle-size measurement') re-measured several scenes by ±0.1 KB. Regenerated via full build:bundle-scenes and committed the freshly-measured per-scene manifests so the committed baseline matches a clean rebuild: scene36, scene41, scene47, scene105, scene115, scene173, scene223 No chunk-set changes (our ref-count code does not land in these bundles); all values remain within their scene-config.json ceilings. Our feature scenes (12/42/129/218) rebuilt byte-identically to HEAD, no change needed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
Lab - Static SiteBuild 20260708.12 - merge @ b3fc6b1 |
# Conflicts: # lab/public/bundle/manifest/scene129.json
Bundle Size ChangesIncreases
Sizes rounded to nearest KB. Run |
Lab - Static SiteBuild 20260708.16 - merge @ 48a793f |
## Scope Bumps **TypeScript `^5.7.0` → `^6.0.3`** across the workspace only. **Vite is unchanged** (stays at its existing `^6.x` pins). This PR was originally TS6 + Vite 8 together, but a split diagnostic showed the two majors carry independent risk, so the Vite 8 (rolldown-vite) work has been moved to its **own separate draft PR (#377, parked as exploration)**. This PR is now a clean, zero-ceiling/zero-golden TypeScript-6-only change and is **the deliverable**. ### Version bumps `typescript ^5.7.0 → ^6.0.3` in: root, `packages/babylon-lite`, `packages/babylon-lite-gl`, `packages/babylon-lite-compat`, `lab`, `playground`. `pnpm-lock.yaml` regenerated (TS-only delta; resolves TS `5.9.3 → 6.0.3`). ## Code changes required by TS 6 - **`packages/babylon-lite/src/material/node/node-geometry-renderable.ts`** — copy the readonly `_vertexBuffers` array (`buffers: [...a._vertexBuffers]`) when passing it as `GPUVertexState.buffers`. TS6 tightened the WebGPU typings so a readonly array is no longer assignable to the mutable `buffers` field. Semantics-preserving (shallow copy only; no runtime/render change). - **`tests/lite/build/public-api-types.test.ts`, `tests/gl/build/public-api.test.ts`** — add `--ignoreConfig` to the standalone `tsc` invocations. TS6 now errors (**TS5112**) when a file is passed on the command line while a `tsconfig.json` exists in cwd; `--ignoreConfig` is the flag the error itself recommends. - **`tests/lite/build/public-api-types.test.ts`** — drop `--types @webgpu/types`. TS6's built-in `dom` lib now bundles the WebGPU declarations, so also loading the `@webgpu/types` package duplicates them and (without `skipLibCheck`, which this test deliberately omits to catch leaking internal types) trips **TS6200/TS2717** conflicts. The native lib fully covers every `GPU*` type the public `build/index.d.ts` references — verified type-checks clean. No tsconfig strictness was loosened; no blanket `any` / `@ts-ignore` was added. ### Bundle manifests All per-scene bundle manifests were regenerated under TS6. Codegen is **byte-identical to the TS5/Rollup baseline for every scene except two** — `scene104.json` and `scene149.json` — which shift by a fraction of a KB (gzip rounding, e.g. `44.5 → 44.6` KB). Those two are therefore the only manifest files this PR changes vs `master`, and both remain within existing size ceilings (**0 overages**). (An earlier revision of this description said "46 manifests"; that reflected a mid-work snapshot before the TS6-only split — the accurate figure vs `master` is 2.) ## Test / build results (TS6 + Vite 6) - `pnpm run lint` (eslint + `tsc --noEmit` across all tsconfigs): ✅ green - `pnpm test:unit` ✅ · `pnpm test:build` ✅ · `pnpm test:unit:gl` ✅ · `pnpm test:build:gl` ✅ - `pnpm build:lib`, `pnpm build`, `pnpm build:gl`, `pnpm build:playground`: ✅ exit 0 - Locked parity + bundle-size on `LAB_TEST_PORT=5279`: **0 bundle-size overages** ✅. Only `scene116` parity fails (see below); `scene144` now **passes** after merging master's widened `0.072` ceiling (#380). ### scene116 is pre-existing local-GPU noise — NOT introduced here `scene116` fails **byte-identically on unmodified `master`** (TS 5.9.3 + Vite 6, this branch's changes reverted): MAD `0.2883984375…` vs threshold `0.01`. It is machine/GPU-specific strict-threshold noise that is **green on CI**, and fails with and without this PR's changes, so it is not a regression from the TS6 upgrade — a reviewer should not be alarmed. `scene144` (`0.07000434…`) previously sat a hair over the old `0.07` threshold but now **passes** under master's widened `0.072` ceiling (#380). No golden references or thresholds were touched. ## Notes - Touches `pnpm-lock.yaml` → shares a conflict surface with sibling dependency PRs (#374 and the parked Vite 8 PR #377); **whichever merges second regenerates the lock** (trivial re-resolve). - Merged current `master` (incl. #374 tooling batch + pnpm 9.15.9, #378 REUSE_BROWSER, #353, #381); all suites re-validated green. - Kept as **draft** for the user's review (drafts-first). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Problem
cloneMeshNode(scene/transform-node.ts) intentionally shares GPU resources with its source mesh for cheap instancing (mirrors BJSMesh.clone()):_gpu(geometry buffers),skeleton,morphTargets, andthinInstanceswere all referenced by both the source and the clone. However,disposeMeshGpudestroyed those buffers unconditionally on every call.Consequences:
.destroy()on the sameGPUBuffertwice (double-free).This affects every scene that uses
cloneTransformNode→cloneMeshNode(e.g. scene12, 41, 42, 47, 104, 105, 129).Fix
Added
packages/babylon-lite/src/mesh/mesh-gpu-refcount.ts— a small, lazily-allocatedWeakMap-based ref-count registry, mirroring the existingmesh-scene-registry.tspattern (no module-level side effects):retainGpuResource(resource)— registers an additional owner.releaseGpuResource(resource)— releases one owner's claim, returnstrueonly when it was the last owner.Changes:
cloneMeshNodenow shares_gpuby identity (not a shallow-copied wrapper) and callsretainGpuResourceon_gpu/skeleton/morphTargets/thinInstanceswhen present.disposeMeshGpucallsreleaseGpuResourceper shared resource and only.destroy()s buffers when it was the last owner.Safety verification (device-lost recovery)
Traced the only other place that reassigns mesh GPU resources:
engine/device-lost-recovery.ts::rebuildMeshes. It doesmesh._gpu = uploadRetainedMesh(engine, mesh)— a whole-object reassignment, neverObject.assignon the existing shared object, so it can't corrupt a sibling's alias. It only fires when_cpuPositions/_cpuNormals/_cpuIndicesare set, and those CPU arrays are always shared by reference between a mesh and its clone (copied at clone time) — so the condition is symmetric: either both meshes get independently rebuilt with fresh buffers (cleanly un-aliasing) or neither is touched (alias stays consistent). This path only runs after an actual WebGPU device-lost event, so any orphaned ref-count entry for the old (dead)_gpuobject is inert and naturally garbage-collected.skeleton/morphTargetsrecovery legitimately usesObject.assign(old, rebuilt)in place on the shared object — correct/desired, since both aliases should see the rebuilt buffers. Thin-instance buffer growth already mutated the sharedthinInstancesobject in place prior to this change (pre-existing, unaffected).Added a unit test (
mesh-clone-gpu-dispose.test.ts, 3rd case) that simulates this exact recovery reassignment pattern with a clone+source pair and confirms independent disposal afterward with no cross-corruption.Testing
tests/lite/unit/mesh-clone-gpu-dispose.test.ts(new, 3 tests): shared-buffer disposal ordering, immediate disposal for never-cloned meshes, device-lost-recovery reassignment safety.tsc --noEmitacross all tsconfigs — clean.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com