Fix crash in MessagePort.postMessage when context is destroyed #23496
Conversation
Fixes a null pointer dereference in MessagePort::postMessage when the ScriptExecutionContext is destroyed during high-frequency message passing between workers using Comlink. The issue occurred when postMessage() was called on a MessagePort after its associated ScriptExecutionContext had been destroyed. The code was directly dereferencing protectedScriptExecutionContext() without checking if it returned null, leading to a segfault at the address stored in the RefPtr (typically 0x18-0x48). This fix checks if the context is valid before dereferencing it, and returns early if it's null, preventing the crash. This is consistent with other parts of the codebase that check for context validity before use. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Updated 5:18 PM PT - Oct 12th, 2025
❌ @Jarred-Sumner, your commit df3b61d has 3 failures in
🧪 To try this PR locally: bunx bun-pr 23496That installs a local version of the PR into your bun-23496 --bun |
robobun
changed the title
robobun
changed the title
WalkthroughAdds a null-check in MessagePort::postMessage to avoid dereferencing a null protected script execution context; adds a regression test and a Comlink-like helper used by the test to verify worker postMessage does not crash after the script execution context is destroyed. Changes
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (8)test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/regression/issue/**📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/**/*.{ts,tsx,js,jsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/regression/issue/*.test.ts📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/regression/**📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (12)📚 Learning: 2025-10-12T02:22:34.349ZApplied to files:
📚 Learning: 2025-10-04T09:51:30.294ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.349ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.349ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.349ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-10-04T09:51:30.294ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.349ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.349ZApplied to files:
📚 Learning: 2025-10-04T09:51:30.294ZApplied to files:
📚 Learning: 2025-09-02T05:33:37.517ZApplied to files:
🧬 Code graph analysis (1)test/regression/issue/23194.test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/webcore/MessagePort.cpp (1)
168-168: Remove or relax the assertion for consistency.The
ASSERT(scriptExecutionContext())at line 168 is now inconsistent with the null check introduced at line 187. Since the context can legitimately be null (as the fix demonstrates), this assertion may fire in debug builds despite the null check below.Apply this diff to remove the stale assertion:
if (!isEntangled()) return {}; - ASSERT(scriptExecutionContext());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/bindings/webcore/MessagePort.cpp(1 hunks)test/regression/issue/23194.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/bindings/webcore/MessagePort.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/bindings/webcore/MessagePort.cpp
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: When a JS class has a public constructor, implement Foo, FooPrototype, and FooConstructor (JSDestructibleObject, JSNonFinalObject, InternalFunction)
Define class properties using HashTableValue arrays in C++ bindings
Add iso subspaces for classes with C++ fields
Cache structures in ZigGlobalObject for C++-backed classes
Files:
src/bun.js/bindings/webcore/MessagePort.cpp
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/23194.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/23194.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/23194.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue numberPlace regression tests under test/regression/issue/ (one per bug fix)
Files:
test/regression/issue/23194.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/regression/issue/23194.test.ts
🧬 Code graph analysis (1)
test/regression/issue/23194.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
src/bun.js/bindings/webcore/MessagePort.cpp (1)
186-190: LGTM! Critical segfault fix.The null check correctly guards against a race where
postMessage()is invoked after theScriptExecutionContextis destroyed. Returning an emptyExceptionOr<void>is consistent with the early return at line 167 for!isEntangled().test/regression/issue/23194.test.ts (3)
18-34: LGTM! Worker setup correctly reproduces the issue.The worker script uses Comlink to expose a
startfunction that sends high-frequency callbacks (every 1ms for 3 seconds), which effectively reproduces the race condition wherepostMessageis called after context destruction.
40-62: LGTM! Main script correctly orchestrates the test.The main script properly sets up Comlink communication, waits for the worker to complete, and terminates cleanly. The "SUCCESS" output provides clear verification that the test passed without crashing.
95-101: LGTM! Assertions comprehensively verify the fix.The test correctly asserts:
- Exit code 0 (no crash)
- Presence of "SUCCESS" in stdout (test completed)
- Absence of "Segmentation fault" or "panic" in stderr (no crash indicators)
These assertions effectively validate that the MessagePort null-check fix prevents the segfault.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/regression/issue/23194.test.ts (1)
10-13: UsetempDirWithFilesfrom harness for temporary directories.This concern was already raised in a previous review comment. The coding guidelines recommend using
tempDirortempDirWithFilesfrom the harness module instead of manualtmpdir()andmkdirSync()calls.Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/23194.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/23194.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/23194.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/23194.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue numberPlace regression tests under test/regression/issue/ (one per bug fix)
Files:
test/regression/issue/23194.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/regression/issue/23194.test.ts
🧠 Learnings (7)
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Applied to files:
test/regression/issue/23194.test.ts
🧬 Code graph analysis (1)
test/regression/issue/23194.test.ts (1)
test/harness.ts (1)
bunExe(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/23194.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/23194.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/23194.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/23194.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue numberPlace regression tests under test/regression/issue/ (one per bug fix)
Files:
test/regression/issue/23194.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/regression/issue/23194.test.ts
🧠 Learnings (6)
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/23194.test.ts
🧬 Code graph analysis (1)
test/regression/issue/23194.test.ts (1)
test/harness.ts (2)
tempDir(277-284)bunExe(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
🔇 Additional comments (4)
test/regression/issue/23194.test.ts (4)
1-6: LGTM! Proper use of harness utilities.The imports correctly use
bunExe(),bunEnv, andtempDirfrom the test harness, and the comments clearly explain the test's purpose.
7-55: LGTM! Correct use ofusingwithtempDirfor resource management.The test properly uses
using testDir = tempDir(...)for automatic cleanup, and the file structure correctly reproduces the high-frequency messaging scenario that triggered the original crash. The 1ms interval in the worker is intentionally aggressive to exercise the race condition.
57-66: LGTM! Proper dependency installation pattern.The code correctly uses
Bun.spawnwithbunExe()andbunEnvfrom the harness, and properly awaits the installation process before proceeding.
68-78: LGTM! Correct use ofawait usingfor process cleanup.The code properly uses
await usingwithBun.spawnfor automatic resource cleanup and efficiently collects stdout, stderr, and exit code in parallel.
- Removes bun install step from test, making it faster - Copies comlink.js (13KB) to test directory - Test now runs in ~6.5s instead of requiring npm install
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/regression/issue/23194.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/regression/issue/23194.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/regression/issue/23194.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/regression/issue/23194.test.ts
test/regression/issue/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Place regression tests under
test/regression/issue/and organize by issue numberPlace regression tests under test/regression/issue/ (one per bug fix)
Files:
test/regression/issue/23194.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/regression/issue/23194.test.ts
🧠 Learnings (9)
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Use `tempDir`/`tempDirWithFiles` from `harness` for temporary files and directories in tests
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer running tests via bun bd test <file> and use provided harness utilities (bunEnv, bunExe, tempDir)
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Import common test utilities from `harness` (e.g., `bunExe`, `bunEnv`, `tempDirWithFiles`, `tmpdirSync`, platform checks, GC helpers)
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Use shared utilities from test/harness.ts where applicable
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/cli/**/*.{js,ts} : When testing Bun as a CLI, use spawn with bunExe() and bunEnv from harness, and capture stdout/stderr via pipes
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-10-04T09:51:30.294Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-04T09:51:30.294Z
Learning: Applies to test/**/*.test.{ts,tsx} : Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : In error tests, assert non-zero exit codes for failing processes and use `toThrow` for synchronous errors
Applied to files:
test/regression/issue/23194.test.ts
📚 Learning: 2025-09-03T17:10:13.486Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-09-03T17:10:13.486Z
Learning: Applies to test/**/*.test.ts : Do not set explicit test timeouts; rely on Bun’s built-in timeouts
Applied to files:
test/regression/issue/23194.test.ts
🧬 Code graph analysis (1)
test/regression/issue/23194.test.ts (1)
test/harness.ts (2)
tempDir(277-284)bunExe(102-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Format
test/regression/issue/23194.test.ts
Outdated
| const installProc = Bun.spawn({ | ||
| cmd: [bunExe(), "install"], | ||
| cwd: String(testDir), | ||
| env: bunEnv, | ||
| stdout: "ignore", | ||
| stderr: "ignore", | ||
| }); | ||
| await installProc.exited; | ||
| expect(installProc.exitCode).toBe(0); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use await using when spawning Bun processes
The coding guidelines ask us to wrap Bun.spawn handles in using/await using so they’re disposed even if the test fails. Please mirror the pattern you already use for proc by making the install step an await using declaration.
- const installProc = Bun.spawn({
+ await using installProc = Bun.spawn({
cmd: [bunExe(), "install"],
cwd: String(testDir),
env: bunEnv,
stdout: "ignore",
stderr: "ignore",
});
await installProc.exited;As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const installProc = Bun.spawn({ | |
| cmd: [bunExe(), "install"], | |
| cwd: String(testDir), | |
| env: bunEnv, | |
| stdout: "ignore", | |
| stderr: "ignore", | |
| }); | |
| await installProc.exited; | |
| expect(installProc.exitCode).toBe(0); | |
| await using installProc = Bun.spawn({ | |
| cmd: [bunExe(), "install"], | |
| cwd: String(testDir), | |
| env: bunEnv, | |
| stdout: "ignore", | |
| stderr: "ignore", | |
| }); | |
| await installProc.exited; | |
| expect(installProc.exitCode).toBe(0); |
🤖 Prompt for AI Agents
In test/regression/issue/23194.test.ts around lines 58 to 66, the Bun.spawn
handle for the install step is created as a plain const and not disposed on test
failure; change it to use the same await using pattern used for proc (declare
the spawn handle with "await using" so it is automatically disposed), keep the
cwd/env/stdout/stderr settings the same, await its .exited promise as before,
and then assert the exitCode—this ensures the spawned process is cleaned up even
if the test throws.
- Linter was removing import attributes - Use readFileSync with __dirname to reliably load comlink.js - Include comlink source directly in tempDir files - Test now passes reliably on all platforms
Summary
Fixes #23194 - Resolves a null pointer dereference in
MessagePort::postMessagethat causes segfaults during high-frequency message passing between workers using Comlink.Problem
The crash occurred at
MessagePort.cpp:186whenpostMessage()was called on a MessagePort after its associatedScriptExecutionContexthad been destroyed. The code was directly dereferencingprotectedScriptExecutionContext()without checking if it returned null:This resulted in a segfault at addresses like 0x18, 0x28, 0x30, 0x40, 0x48 (small offsets from null pointer).
Solution
Added a null check before dereferencing the protected context:
When the context is null, we return early with success (empty ExceptionOr), which is consistent with the early return for
!isEntangled()on line 167.Testing
test/regression/issue/23194.test.tsthat reproduces the original crash scenario🤖 Generated with Claude Code