Skip to content

test(ffi): remove the toBuffer no-finalizer workaround - #61

Merged
EffortlessSteven merged 1 commit into
claude/ffi-tobuffer-owned-pointerfrom
claude/ffi-tobuffer-owned-pointer-cleanup
Jul 30, 2026
Merged

test(ffi): remove the toBuffer no-finalizer workaround#61
EffortlessSteven merged 1 commit into
claude/ffi-tobuffer-owned-pointerfrom
claude/ffi-tobuffer-owned-pointer-cleanup

Conversation

@EffortlessSteven

@EffortlessSteven EffortlessSteven commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Stacked on oven-sh#31753 for review. Do not merge into the base branch; this gets rebased onto upstream/main and submitted upstream after oven-sh#31753 lands.

What this does

oven-sh#35246 enabled the compiled FFI fixture cross-platform and, in the same hunk, scoped primitives' Buffer and added a Bun.gc(true) after it. That collection segfaults, because toBuffer without a finalizer adopted the caller's pointer as owned, so the test passes getNoopDeallocatorCallback() to avoid it. oven-sh#31753 fixes the path, so the callback now only hides coverage of it.

Fix: drop the callback so the compiled fixture exercises the real default: wrap static native storage, drop the Buffer, force GC, then read the pointer again. getNoopDeallocatorCallback has no other users, so it goes too.

Verification

Red without oven-sh#31753, green with it: on a Bun lacking the fix the fixture takes down the whole test runner with panic(main thread): Segmentation fault at address 0x8420. That is the crash oven-sh#35405 reports, and since oven-sh#35246 the fixture runs on Windows too, so this callback is what currently keeps CI from seeing it.

Check Result
bun bd test ffi/ffi.test.js -t primitives 2 passed, both runner variants
getDeallocatorCallback, getDeallocatorBuffer, getDeallocatorCalledCount kept, still exercised
no remaining getNoopDeallocatorCallback references rg clean across src/ and test/
prettier clean

Review map

  • test/js/bun/ffi/ffi.test.js: primitives calls toBuffer(cptr, 0, 4) and toArrayBuffer(cptr, 0, 4) with no finalizer; symbol descriptor and destructuring entry removed.
  • test/js/bun/ffi/ffi-test.c: getNoopDeallocatorCallback and its noop_deallocator removed.

The FFI runner's `primitives` test passed `getNoopDeallocatorCallback()` to
`toBuffer`/`toArrayBuffer` so it would not hit the no-finalizer path, which
adopted the caller's pointer as owned and freed it on GC. That path is fixed, so
the workaround only hides the coverage: drop the callback and let the compiled
fixture exercise the real default, wrapping static native storage, dropping the
Buffer, forcing GC, then reading the pointer again.

`getNoopDeallocatorCallback` has no other users, so remove it from the symbol
descriptor, the destructuring, and the fixture. The real deallocator-counter
helpers stay.

Without the toBuffer fix this is red: the fixture segfaults the test runner
(`panic(main thread): Segmentation fault`), which is what oven-sh#35405
reports and why `run ffi > primitives` flaked on Windows.
@EffortlessSteven

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Changes

FFI deallocator callback removal

Layer / File(s) Summary
Remove noop deallocator symbol
test/js/bun/ffi/ffi-test.c, test/js/bun/ffi/ffi.test.js
The native noop deallocator export and its JavaScript FFI symbol declarations and destructuring are removed.
Use borrowed pointer storage
test/js/bun/ffi/ffi.test.js
The primitives test converts the borrowed static pointer without a deallocator finalizer and documents that GC must not free it.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change: removing the no-finalizer FFI workaround.
Description check ✅ Passed The description covers the PR purpose and verification clearly; it only uses non-template section headings.

Comment @coderabbitai help to get the list of available commands.

@EffortlessSteven
EffortlessSteven merged commit fabca3d into claude/ffi-tobuffer-owned-pointer Jul 30, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant