Skip to content

Commit fa8e9d4

Browse files
author
Ralph Kuepper
committed
fix(web): address CodeRabbit review on the Jolt bridge + FFI validator
- constraintSixDofLockedTranslation now destroys its temporary SixDOFConstraintSettings on every exit (including the API-absent path where the constructor itself throws) and releases the body read-locks exactly once via an idempotent finally, instead of leaking the settings and double-releasing if registerConstraint threw. - shapeHeightfield destroys its HeightFieldShapeSettings after Create(). - destroyWorld evicts this world's cached query filters (queryFilters()) and frees the native bp/obj/body/shape objects — previously one filter set per (world, layer) queried leaked on every world teardown. - validate-ffi.js strips block + line comments before scraping web FFI signatures, so a commented-out `// pub fn bloom_old(...)` can't be read as a real export and a trailing `x: f64 // note` can't break the all-f64 arity check. Validator still: 0 failures, 0 warnings.
1 parent 050a69f commit fa8e9d4

2 files changed

Lines changed: 32 additions & 5 deletions

File tree

native/web/jolt_bridge.js

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,16 @@ export function destroyWorld(h) {
225225
for (const [ch, c] of state.constraints) {
226226
if (c.world === h) { w.system.RemoveConstraint(c.constraint); state.constraints.delete(ch); }
227227
}
228+
// Evict this world's cached query filters (queryFilters()) and free the
229+
// native filter objects — otherwise repeated world create/destroy leaks
230+
// one bp/obj/body/shape set per (world, layer) that was ever queried.
231+
for (const [k, f] of queryFilterCache) {
232+
if (k.startsWith(h + ':')) {
233+
JoltModule.destroy(f.bp); JoltModule.destroy(f.obj);
234+
JoltModule.destroy(f.body); JoltModule.destroy(f.shape);
235+
queryFilterCache.delete(k);
236+
}
237+
}
228238
JoltModule.destroy(w.jolt);
229239
state.worlds.delete(h);
230240
}
@@ -924,8 +934,14 @@ export function constraintSixDofLockedTranslation(
924934
if (warnUninit('constraintSixDofLockedTranslation')) return 0;
925935
const ctx = resolveConstraintBodies(bodyA, bodyB); if (!ctx) return 0;
926936
const J = JoltModule;
937+
// Release the body read-locks exactly once (idempotent), and destroy the
938+
// temporary settings object on every exit — including the SixDOF-API-absent
939+
// path, where `new SixDOFConstraintSettings()` itself throws.
940+
let settings = null;
941+
let released = false;
942+
const release = () => { if (!released) { released = true; ctx.release(); } };
927943
try {
928-
const settings = new J.SixDOFConstraintSettings();
944+
settings = new J.SixDOFConstraintSettings();
929945
settings.mSpace = worldSpace ? J.EConstraintSpace_WorldSpace : J.EConstraintSpace_LocalToBodyCOM;
930946
settings.mPosition1 = rvec3(ax, ay, az);
931947
settings.mPosition2 = rvec3(bx, by, bz);
@@ -942,12 +958,14 @@ export function constraintSixDofLockedTranslation(
942958
else settings.SetLimitedAxis(axis, lo, hi);
943959
}
944960
const c = settings.Create(ctx.body1, ctx.body2);
945-
ctx.release();
961+
release(); // AddConstraint below does not need the body locks
946962
return registerConstraint(state.bodies.get(bodyA).world, c);
947963
} catch (e) {
948-
ctx.release();
949964
console.warn('[jolt_bridge] SixDOF constraint unavailable:', e);
950965
return 0;
966+
} finally {
967+
release();
968+
if (settings) J.destroy(settings);
951969
}
952970
}
953971

@@ -1054,7 +1072,9 @@ export function shapeHeightfield(sampleCount, ox, oy, oz, sx, sy, sz, blockSize)
10541072
);
10551073
for (let i = 0; i < need; i++) heap[i] = state.scratchF32[i];
10561074
const result = settings.Create();
1057-
return result.IsValid() ? registerShape(result.Get()) : 0;
1075+
const shapeH = result.IsValid() ? registerShape(result.Get()) : 0;
1076+
J.destroy(settings);
1077+
return shapeH;
10581078
}
10591079

10601080
// --- Compound builder ---

tools/validate-ffi.js

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,14 @@ for (const platform of PLATFORMS) {
166166
// _floats designs) are skipped: only signatures that are pure f64 mirrors
167167
// are compared, which is precisely where drift is a bug.
168168
{
169-
const webSrc = readDirRust(path.join(ROOT, 'native/web/src'));
169+
// Strip comments before parsing signatures: a commented-out
170+
// `// pub fn bloom_old(...)` must not be scraped as a real export, and a
171+
// trailing `x: f64 // note` inside a param list must not break the all-f64
172+
// arity check. Block comments then line comments; string literals in this
173+
// Rust source never contain `pub fn bloom_`, so this is safe.
174+
const webSrc = readDirRust(path.join(ROOT, 'native/web/src'))
175+
.replace(/\/\*[\s\S]*?\*\//g, '')
176+
.replace(/\/\/[^\n]*/g, '');
170177
const names = new Set();
171178
// name -> {arity, allF64} for the Rust exports (the jolt bridge's JS
172179
// functions are name-only; physics arity is covered by its own manifest

0 commit comments

Comments
 (0)