fix(inspect): don't crash when a Proxy in the prototype chain throws#29642
Open
robobun wants to merge 3 commits into
Open
fix(inspect): don't crash when a Proxy in the prototype chain throws#29642robobun wants to merge 3 commits into
robobun wants to merge 3 commits into
Conversation
Collaborator
Author
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPrototype-chain traversal and property enumeration were hardened: prototypes are tracked as JSValue and preserved during enumeration, prototype hops are limited, Proxy Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — minimal, targeted crash fix that reorders an existing exception-clear and adds a null guard, matching the pattern already used in forEachPropertyOrdered.
Extended reasoning...
Overview
Two-file change: ~10 LOC in src/bun.js/bindings/bindings.cpp inside JSC__JSValue__forEachPropertyImpl, plus two regression tests in test/js/bun/util/inspect.test.js. The C++ change (1) moves CLEAR_IF_EXCEPTION(scope) to run before the early continue when getPropertySlot returns false, and (2) checks the result of iterating->getPrototype(globalObject) for empty/exception before calling .getObject() on it.
Security risks
None. This is the Bun.inspect / console.log property-enumeration path. The change swallows exceptions from Proxy traps during inspection, which is exactly what the surrounding code (and the sibling forEachPropertyOrdered at bindings.cpp:5472-5477) already does. No new data exposure, no auth/crypto, no user-controlled input handling beyond what already existed.
Level of scrutiny
Low. This is a fuzzer-found segfault fix with a clear root cause (pending exception leaks past a continue, then getPrototype returns empty JSValue{}, then .getObject() derefs null). The fix is the minimal correct reordering and mirrors the identical pattern already present a few hundred lines below in forEachPropertyOrdered. The added getPrototype guard is straightforward defensive null-checking.
Other factors
- Two new regression tests cover both crash paths (throwing getter behind a Proxy prototype, and throwing
getPrototypeOftrap). - Bug hunting system found no issues.
- No design decisions or behavior changes — purely converts a crash into the existing "ignore exceptions during inspect" behavior.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
|
CI failures are unrelated pre-existing flakes, not caused by this change:
This PR only touches |
Member
Collaborator
Author
|
@alii — compared all 9. They all touch the same ~5 lines in
tl;dr: All 9 are dupes. Only #29642 and #29071 fix both bugs — they're byte-for-byte equivalent in the C++, so pick either (this one has the more minimal/direct regression tests in #24985 is the most principled if you want the broader |
Member
|
@robobun close all the others, bring the two changes into this PR |
robobun
force-pushed
the
farm/43e87898/inspect-proxy-proto-throw
branch
from
9aa1181 to
be84d29
Compare
This was referenced Apr 23, 2026
Collaborator
Author
| iterating = iterating->getPrototype(globalObject).getObject(); | ||
| iteratingValue = iterating->getPrototype(globalObject); | ||
| // Ignore exceptions from Proxy "getPrototypeOf" trap. | ||
| CLEAR_IF_EXCEPTION(scope); |
Member
There was a problem hiding this comment.
we should probably not be clearing these exceptions, but looks like it was doing it originally
Collaborator
Author
There was a problem hiding this comment.
Agreed — kept the swallow to match the existing fast-path handling a few lines up and forEachPropertyOrdered (so Bun.inspect/console.log stay best-effort rather than throwing mid-format). Switching this whole path to propagate is the scope of #24985; happy to rebase that on top of this if/when it's revived.
This was referenced Apr 24, 2026
robobun
force-pushed
the
farm/43e87898/inspect-proxy-proto-throw
branch
from
8da7cbc to
93d9a9c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/bindings.cpp`:
- Line 5242: The slow-path prototype hop budget is off by one compared to the
fast path: update the conditional that checks prototypeCount in the slow-path
loop so it uses the same logic as the fast path (i.e., the same
comparison/operator used in the fast-path `prototypeCount++ < 5` check) to
ensure `forEachProperty*` visits the same number of prototype hops; locate the
`prototypeCount` check in the slow-path loop in bindings.cpp (the `if
(prototypeCount++ >= 5)` line) and change it to match the fast-path condition so
both paths allow the same five-prototype-hop traversal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c84a1ad1-c643-4464-85ba-93160cceb85a
📒 Files selected for processing (2)
src/bun.js/bindings/bindings.cppsrc/bun.js/bindings/napi.cpp
| while (JSObject* iterating = iteratingValue.getObject()) { | ||
| if (iterating == globalObject->objectPrototype() || iterating == globalObject->functionPrototype() || (iterating->inherits<JSGlobalProxy>() && uncheckedDowncast<JSGlobalProxy>(iterating)->target() != globalObject)) | ||
| break; | ||
| if (prototypeCount++ >= 5) |
Contributor
There was a problem hiding this comment.
Keep the slow-path prototype budget aligned with the fast path.
Line 5242 now stops after visiting the base object plus four prototype hops, while the fast path still reaches five hops (prototypeCount++ < 5 at Lines 5215-5222). That makes forEachProperty* shape-dependent again: properties that only exist on the fifth prototype hop are visible for fast-enumerable objects but skipped for slow-path ones.
♻️ Proposed fix
- if (prototypeCount++ >= 5)
+ if (prototypeCount++ > 5)
break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/bindings.cpp` at line 5242, The slow-path prototype hop
budget is off by one compared to the fast path: update the conditional that
checks prototypeCount in the slow-path loop so it uses the same logic as the
fast path (i.e., the same comparison/operator used in the fast-path
`prototypeCount++ < 5` check) to ensure `forEachProperty*` visits the same
number of prototype hops; locate the `prototypeCount` check in the slow-path
loop in bindings.cpp (the `if (prototypeCount++ >= 5)` line) and change it to
match the fast-path condition so both paths allow the same five-prototype-hop
traversal.
Collaborator
Author
There was a problem hiding this comment.
This is a false positive. The original condition was while (... && prototypeCount++ < 5), which runs the body for prototypeCount ∈ {0,1,2,3,4}. The refactored if (prototypeCount++ >= 5) break; is its exact negation (!(x < 5) ≡ x >= 5) and also runs the body for {0,1,2,3,4}. Changing to > 5 as suggested would run 6 iterations instead of 5, which would be the off-by-one.
Contributor
There was a problem hiding this comment.
@robobun You're absolutely right — I apologize for the false positive. The logical equivalence is exact:
- Original:
prototypeCount++ < 5→ body runs for{0, 1, 2, 3, 4} - Refactored:
prototypeCount++ >= 5→ negation of the above,breakfires for{5, 6, …}, body runs for the same{0, 1, 2, 3, 4}
My suggested > 5 would have been the actual off-by-one, allowing a 6th iteration. The current code is correct.
✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 29642
File: src/bun.js/bindings/bindings.cpp:5242-5242
Timestamp: 2026-04-26T02:43:45.164Z
Learning: In oven-sh/bun `src/bun.js/bindings/bindings.cpp` (`JSC__JSValue__forEachPropertyImpl`), the slow-path prototype-hop guard `if (prototypeCount++ >= 5) break;` is the exact logical negation of the original `while (... && prototypeCount++ < 5)` condition — both allow the loop body to execute for `prototypeCount ∈ {0,1,2,3,4}` (five hops). Do NOT flag this as an off-by-one or suggest changing it to `> 5`; that would introduce a genuine off-by-one (six iterations instead of five).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use JSC::WriteBarrier for heap-allocated references in V8 objects and implement visitChildren() for custom heap objects to support garbage collection
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/V8*.cpp : Use localToJSValue() to convert V8 handles to JSC values and perform JSC operations within V8 method implementations
Learnt from: robobun
Repo: oven-sh/bun PR: 28473
File: src/bun.js/bindings/BunCPUProfiler.cpp:31-35
Timestamp: 2026-03-23T16:54:42.592Z
Learning: In oven-sh/bun, Bun has a strict 1:1 JSC::VM to Zig::GlobalObject relationship for worker threads (each worker creates its own VM). ShadowRealm can create additional GlobalObjects on the same VM, but ShadowRealm cannot access `node:inspector`. Therefore, CPU profiler state stored per-Zig::GlobalObject (m_profilingStartTime, m_cpuSamplingInterval, m_isCPUProfilerRunning in ZigGlobalObject.h) is safe and correct — there is no multi-GlobalObject-per-VM scenario where inspector/profiler functions are reachable. Do not flag per-GlobalObject CPU profiler state as a cross-VM sharing risk in future reviews of src/bun.js/bindings/BunCPUProfiler.cpp or src/bun.js/bindings/ZigGlobalObject.h.
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:676-677
Timestamp: 2026-04-22T20:47:13.873Z
Learning: In oven-sh/bun `src/bun.js/modules/NodeModuleModule.cpp` (and similar PropertyCallback-based static-property initializers), returning an empty `JSValue` (`{}`) on allocation failure is intentional and correct. `reifyStaticProperty` (Lookup.h) never checks for exceptions between consecutive PropertyCallbacks; on the ESM path `generateNativeModule_NodeModule` already calls `topExceptionScope.tryClearException()` after the reification loop, so any exception thrown by the old `constructArray` path was silently cleared anyway. Do NOT suggest changing the `if (!array) [[unlikely]] return {};` guard to throw an OOM exception — that would reintroduce the ThrowScope bug the PR was designed to fix. The pattern matches `constructVersions` in `BunProcess.cpp` (`RETURN_IF_EXCEPTION(scope, {})` → empty value on OOM).
Learnt from: robobun
Repo: oven-sh/bun PR: 28763
File: src/bun.js/bindings/bindings.cpp:1067-1089
Timestamp: 2026-04-02T18:53:39.190Z
Learning: In `src/bun.js/bindings/bindings.cpp`, the `WTF::BitVector matchedIndices` used in the Set/Map O(n²) slow-path of `specialObjectsDequal` is sized to `set2->size()` / `map2->size()` at the start of the slow path and does NOT need a runtime bounds guard. `Bun__deepEquals` only reads properties of the elements inside the collections; it never calls `set2->add()` / `map2->set()`, so the collection sizes — and thus the bitvector capacity — are stable throughout the entire inner-loop iteration. Do not flag missing `ensureSize` guards in this loop as a correctness risk.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:41.142Z
Learning: In Bun's C++ bindings (src/bun.js/bindings/helpers.h and similar files), when returning an empty JSC::Identifier and a VM is accessible, prefer using `vm.propertyNames->emptyIdentifier` over constructing with `JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier)`. The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns.
Learnt from: robobun
Repo: oven-sh/bun PR: 29200
File: src/bun.js/bindings/JSMockFunction.cpp:985-998
Timestamp: 2026-04-11T22:50:18.935Z
Learning: In JavaScriptCore (JSC), for `InternalFunction` (C++ native) constructors, `callframe->thisValue()` returns `newTarget` (the constructor function itself), NOT a pre-created receiver. This is different from ordinary ECMAScript function constructors which pre-create a receiver via OrdinaryCreateFromConstructor. When implementing a native [[Construct]] handler for an InternalFunction in JSC (e.g. in oven-sh/bun's src/bun.js/bindings/JSMockFunction.cpp), to create a fallback object that correctly inherits from `newTarget.prototype`, use `InternalFunction::createSubclassStructure(globalObject, asObject(callframe->newTarget()), baseStructure)` followed by `JSC::constructEmptyObject(vm, structure)`. Do NOT use `callframe->thisValue()` as a receiver in this context.
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-21T21:51:55.181Z
Learning: Applies to src/bun.js/bindings/*.cpp : C++ JavaScriptCore bindings should be placed in `src/bun.js/bindings/*.cpp`
Learnt from: robobun
Repo: oven-sh/bun PR: 28763
File: src/bun.js/bindings/bindings.cpp:1120-1129
Timestamp: 2026-04-01T22:37:16.953Z
Learning: In `src/bun.js/bindings/bindings.cpp`, the JSMap fast-path in `specialObjectsDequal` (around the Map deep-equality section) uses `map2->get()` with an `isUndefined()` guard. When `undefined` is returned (either because the key is absent or the value is genuinely `undefined`), the code conservatively falls to the O(n²) slow path. This is intentional and correct: the slow path handles both cases accurately (missing key → `found=false` → return false; present key with undefined value → `Bun__deepEquals(undefined, undefined)` → true). Do not flag this as a correctness bug in future reviews of this file — it is a minor pre-existing performance trade-off for the rare Map-with-undefined-values edge case.
Learnt from: robobun
Repo: oven-sh/bun PR: 28754
File: src/bun.js/web_worker.zig:449-453
Timestamp: 2026-04-01T19:29:31.324Z
Learning: In oven-sh/bun `src/bun.js/web_worker.zig`, the non-concurrent `unref()` call inside `notifyNeedTermination()` (via `setRefInternal(false)`) is a known pre-existing issue, flagged by the `// TODO(190n) delete` comment. The `onUnhandledRejection` handler intentionally bypasses `notifyNeedTermination()` and instead inlines the concurrent-safe sequence (`setRequestedTerminate()` + `parent_poll_ref.unrefConcurrently()` + `vm.eventLoop().wakeup()`). Do not flag the `notifyNeedTermination()` non-concurrent unref path as a new issue introduced by PRs that touch `onUnhandledRejection`.
Learnt from: robobun
Repo: oven-sh/bun PR: 29543
File: test/js/bun/util/stringWidth.test.ts:960-969
Timestamp: 2026-04-21T10:09:41.335Z
Learning: In `test/js/bun/util/stringWidth.test.ts` (oven-sh/bun PR `#29543`), the "rope with ANSI sequence straddling fibers falls back correctly" test intentionally does NOT assert `isRope(rope) === false` after calling `Bun.stringWidth`. The `expect(w).toBe(11)` assertion is the regression guard: a broken bail-out that sums raw per-fiber Latin-1 widths would yield 18, not 11. Pinning `isRope === false` would encode "must flatten" as a permanent contract and would break if a future refinement handles straddled ANSI sequences without resolving. Do NOT suggest adding a post-call `isRope(rope) === false` assertion to this test.
Learnt from: robobun
Repo: oven-sh/bun PR: 29256
File: test/regression/issue/29253.test.ts:23-34
Timestamp: 2026-04-13T11:41:44.588Z
Learning: In oven-sh/bun (PR `#29256`), `require('module').prototype` (the "disposable" prototype returned by `getModulePrototypeObject`) and the prototype of `new Module(...)` instances (`JSCommonJSModulePrototype`) are intentionally kept as two separate objects — prototype identity unification was reverted in commit 1bf6b8b because unifying them exposes the `_compile` CustomAccessor to prototype-level reads, which silently bails on the JSCommonJSModule cast and breaks `Module.prototype._compile` monkey-patching (Istanbul/nyc). Both prototypes now independently expose `load`, so `typeof m.load === 'function'` and `typeof require('module').prototype.load === 'function'` both hold, but `Object.getPrototypeOf(new Module(...)) !== Module.prototype`. Do NOT suggest or flag the absence of prototype identity (`Object.getPrototypeOf(m) === Module.prototype`) as a missing requirement; full unification is a separate, larger refactor that must first solve the `_compile` accessor issue.
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: src/bun.js/bindings/V8HeapSnapshotBuilder.cpp:545-565
Timestamp: 2026-04-17T04:11:51.062Z
Learning: In oven-sh/bun PR `#29389`, `src/bun.js/bindings/V8HeapSnapshotBuilder.cpp` is an intentional near-verbatim copy of `vendor/WebKit/Source/JavaScriptCore/heap/BunV8HeapSnapshotBuilder.cpp`. The `addString` hash-collision issue (composite `hash + length` key without content verification in `m_stringsLookupTable`) is pre-existing upstream behavior, present identically in the shipping `JSC::BunV8HeapSnapshotBuilder`. The local copy exists only to land the crash fix (1024-char truncation + `RecordOverflow` StringBuilder policy) in an auditable, minimal diff; all other pre-existing issues (collision, potential race) are out of scope and belong in a follow-up PR against the WebKit fork. Do NOT re-flag the `addString` collision as a new issue in this file.
Learnt from: dylan-conway
Repo: oven-sh/bun PR: 29581
File: src/bun.js/modules/NodeModuleModule.cpp:663-681
Timestamp: 2026-04-22T20:47:16.874Z
Learning: In oven-sh/bun, do NOT suggest adding standalone regression tests that rely on the `BUN_JSC_validateExceptionChecks=1` environment variable to exercise JSC throw-scope / exception-scope validator paths (e.g., `reifyAllStaticProperties` PropertyCallback interactions). Per `CLAUDE.md`, all tests must pass with `USE_SYSTEM_BUN=1`, and `BUN_JSC_validateExceptionChecks` is a no-op on release/system Bun builds. The correct project mechanism for tracking this class of issue is: (1) the x64-asan CI shard, which sets the validator automatically, and (2) the `test/no-validate-exceptions.txt` opt-out list for tests that trigger pre-existing throw-scope assertion failures unrelated to the change under review. An in-source comment pointing at the specific exerciser (e.g., the tsgo/bun-types test) is sufficient documentation.
Learnt from: robobun
Repo: oven-sh/bun PR: 29359
File: test/js/node/test/parallel/test-event-emitter-method-names.js:31-33
Timestamp: 2026-04-16T01:35:53.279Z
Learning: In `test/js/node/test/parallel/test-event-emitter-method-names.js` (oven-sh/bun), the `return` statements inside the `for...of` loop over `Object.getOwnPropertyNames(E)` (which should be `continue`) are a known upstream Node.js bug present since at least v24.14.1. This file is synced verbatim from upstream and must not be modified to fix upstream bugs in sync PRs. Do NOT flag these `return` statements as a correctness issue in future reviews of this file or similar verbatim upstream sync PRs.
Learnt from: robobun
Repo: oven-sh/bun PR: 28763
File: test/regression/issue/28760.test.ts:34-47
Timestamp: 2026-04-02T18:53:26.727Z
Learning: In oven-sh/bun (`src/bun.js/bindings/bindings.cpp`), both `assert.deepEqual` and `assert.deepStrictEqual` route through the same `specialObjectsDequal` code path for `Map` (and `Set`) deep-equality comparisons. Therefore, for regression tests covering `Map`/`Set` deep-equality behavior, a single `deepStrictEqual` test is sufficient coverage — adding a `deepEqual` variant for the same scenario provides no additional code coverage.
Learnt from: robobun
Repo: oven-sh/bun PR: 29726
File: test/cli/install/bun-install.test.ts:8789-8837
Timestamp: 2026-04-26T00:01:53.408Z
Learning: In oven-sh/bun, file test/cli/install/bun-install.test.ts prefers a simple for-of loop for small parameterizations (e.g., ["hoisted","isolated"]) under the top-level describe.concurrent("bun-install") to avoid adding an extra describe.each nesting. Do not suggest switching these cases to describe.each() in this file.
Learnt from: robobun
Repo: oven-sh/bun PR: 27785
File: src/js/thirdparty/undici.js:839-841
Timestamp: 2026-03-04T10:41:55.568Z
Learning: In `src/js/thirdparty/undici.js`, the `SqliteCacheStore#prune()` method uses `this.size <= this.#maxCount` as its early-return guard (not `<`). This intentionally mirrors the upstream undici source at `vendor/node/deps/undici/src/lib/cache/sqlite-cache-store.js:353`. Do not suggest changing `<=` to `<` for upstream compatibility reasons.
Learnt from: robobun
Repo: oven-sh/bun PR: 28436
File: src/bun.js/bindings/webcore/SerializedScriptValue.cpp:6408-6414
Timestamp: 2026-03-22T16:46:16.777Z
Learning: In `src/bun.js/bindings/webcore/SerializedScriptValue.cpp`, the `resolvedTransferredStreams` vector in the stream-transfer resolution loop (around the `create()` function) intentionally stores the original `JSObject*` (via `stream.get()`) for `WritableStream` and `TransformStream` on the same-thread `structuredClone` path. Identity pass-through is correct here because cross-thread transfer is rejected earlier (WorkerPostMessage guard). Adding `setTransferred()` infrastructure for WritableStream/TransformStream is deferred follow-up work. Do not flag the original-object append for these two stream types as a bug in future reviews.
Learnt from: robobun
Repo: oven-sh/bun PR: 28436
File: src/bun.js/bindings/webcore/SerializedScriptValue.cpp:5249-5258
Timestamp: 2026-03-22T15:48:08.399Z
Learning: In `src/bun.js/bindings/webcore/SerializedScriptValue.cpp`, the `m_transferredStreams` field (`Vector<JSC::Strong<JSC::JSObject>>`) storing live JS stream wrappers (ReadableStream, WritableStream, TransformStream) for `Bun__ReadableStreamTransferTag` / `Bun__WritableStreamTransferTag` / `Bun__TransformStreamTransferTag` is intentionally safe: it is only populated and consumed on the same-thread structuredClone path. Cross-thread postMessage does not use this path, so there is no cross-realm or cross-thread object leak risk. Do not flag this as a cross-realm/cross-thread issue in future reviews.
Learnt from: robobun
Repo: oven-sh/bun PR: 29190
File: src/js_printer.zig:0-0
Timestamp: 2026-04-11T19:46:51.547Z
Learning: In oven-sh/bun (src/js_printer.zig), the ESM→CJS rewrite for exported variable declarations in printDeclStmt now uses a recursive helper printBundledExportsForBinding to emit Object.defineProperty getters on module.exports for all identifier leaves, including within nested object/array destructuring. Future changes to this area should preserve the recursion to avoid dropping nested exported names in --no-bundle/transform_only output.
Learnt from: robobun
Repo: oven-sh/bun PR: 29060
File: src/bun.js/web_worker.zig:500-517
Timestamp: 2026-04-09T00:41:01.359Z
Learning: In oven-sh/bun `src/bun.js/web_worker.zig` + `src/bun.js/VirtualMachine.zig` (PR `#29060`): when a Worker `error` event listener cancels propagation via `preventDefault()`, the exit-code rollback is handled entirely inside `VirtualMachine.uncaughtException` — it saves `prev_exit_code` before writing `exit_code = 1`, calls `onUnhandledRejection`, then restores `prev_exit_code` if `worker.error_event_prevented` is true. The `onUnhandledRejection` callback in `web_worker.zig` does NOT touch `exit_handler.exit_code` at all. The `.bun`-mode async-rejection path never sets `exit_code = 1`, so no restore is needed there.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2026-03-30T19:21:21.507Z
Learning: In oven-sh/bun PR `#28614`, the cloneFrom/deref race on `ProxyEnvStorage` slot pointers is resolved via `ProxyEnvStorage.lock: bun.Mutex` (rare_data.zig line 122). `Bun__setEnvValue` (BunObject.zig) holds this mutex while modifying slot pointers; the worker snapshot path (web_worker.zig) holds the parent's lock across the entire `cloneFrom` + `cloneWithAllocator` + `syncInto` sequence. `cloneFrom`'s doc comment enforces "Caller must hold parent.lock" as a documented invariant. Do not re-flag this race in future reviews.
Learnt from: robobun
Repo: oven-sh/bun PR: 29152
File: test/no-validate-exceptions.txt:131-131
Timestamp: 2026-04-11T06:11:37.427Z
Learning: In oven-sh/bun, SIGILL crashes in ASAN CI for webcrypto tests (e.g., `test/js/bun/crypto/x25519-derive-bits.test.ts`) are caused by `BUN_JSC_validateExceptionChecks=1` triggering JSC throw-scope assertion failures → `WTFCrash()` → `ud2` → SIGILL. This env var is set by `scripts/runner.node.mjs` for ASAN builds unless the test is listed in `test/no-validate-exceptions.txt`. Adding the test under the `# normalizeCryptoAlgorithmParameters` section of that file is the correct and complete fix, matching the pattern used by all other webcrypto tests. The root cause is a pre-existing throw-scope issue in `SubtleCrypto.cpp` that affects all algorithms identically (ECDH, X25519, etc.) and is tracked as a separate effort. Do NOT flag additions to `test/no-validate-exceptions.txt` for webcrypto tests as insufficient when the SIGILL is of this type.
Learnt from: robobun
Repo: oven-sh/bun PR: 29451
File: test/js/bun/net/socket-retention.test.ts:47-53
Timestamp: 2026-04-18T18:14:06.150Z
Learning: In `test/js/bun/net/socket-retention.test.ts` (oven-sh/bun PR `#29451`), do NOT add a filtered empty-stderr assertion (even with ASAN-noise filtering) to subprocess crash-detection tests. For tests where the regression manifests as a crash (empty stdout + non-zero exit), the positive `expect(stdout.trim()).toBe(...)` and `expect(exitCode).toBe(0)` assertions are the correct and sufficient failure signal. Adding `expect(cleanKnownAsanNoise(stderr)).toBe("")` adds maintenance surface without improving diagnostic signal. The CLAUDE.md convention of asserting filtered empty-stderr applies to tests using `bunEnv` for behavioral correctness checks, not for crash-detection tests that use stdout/exitCode as their primary regression guard.
Learnt from: robobun
Repo: oven-sh/bun PR: 29538
File: test/js/bun/resolve/lower-using-bun-target.test.ts:80-82
Timestamp: 2026-04-21T09:47:22.507Z
Learning: In `test/js/bun/resolve/lower-using-bun-target.test.ts` (oven-sh/bun PR `#29538`), for runtime subprocess tests that spawn `bunExe()` with `bunEnv`, do NOT add `expect(stderr).toBe("")` assertions. The `WARNING: ASAN interferes with JSC signal handlers…` message is emitted to stderr on every JS-executing process launch in a debug ASAN build and is NOT suppressed by `bunEnv` / `BUN_DEBUG_QUIET_LOGS=1`. The correct regression guards are an exact-match on `stdout` and `expect(exitCode).toBe(0)`. Files that need to check stderr should filter out lines starting with "WARNING: ASAN interferes" using the repo's filter-based convention, but leaving stderr entirely unchecked is also acceptable when stdout + exitCode provide sufficient coverage.
Learnt from: robobun
Repo: oven-sh/bun PR: 28473
File: test/regression/issue/28472.test.ts:20-21
Timestamp: 2026-03-23T17:06:31.224Z
Learning: In `test/regression/issue/28472.test.ts` (oven-sh/bun PR `#28473`), the regression being tested is that `Profiler.stop` returns null/error in worker threads rather than a valid profile object. The test correctly asserts only that a non-null profile with a `nodes` array is returned (via `result.profile.nodes.length`). Do NOT suggest asserting on sample counts or timing fields — under ASAN or slow CI, short-lived profiling may collect zero real samples, making such assertions flaky. The empty-profile fallback (a root node with no sampled data) is correct behavior; the bug was returning null/error entirely.
Learnt from: robobun
Repo: oven-sh/bun PR: 28686
File: test/regression/issue/12306.test.ts:50-55
Timestamp: 2026-04-02T18:42:38.406Z
Learning: In `test/regression/issue/12306.test.ts` (oven-sh/bun PR `#28686`), the `done` Promise only rejects via the client `error` event — a genuine test failure. In that case, bun:test catches the unhandled rejection and reports the error, and the test process exits immediately, making the skipped `server.close()` harmless. Do not flag the missing try/finally around `await done` in this test as a resource-leak risk; the process-exit path is the correct and sufficient cleanup signal for this regression test.
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: src/bun.js/bindings/V8HeapSnapshotBuilder.cpp:592-594
Timestamp: 2026-04-17T04:59:15.762Z
Learning: When reviewing C++ code that uses WebKit Template Framework (WTF)/JSC types, do not treat `if (str.isEmpty()) str = String();` as dead code or a no-op. In WTF, `WTF::String::isEmpty()` returns true for both null strings and empty non-null strings, while `String()` constructs a null string. This conditional therefore normalizes empty non-null strings into null strings and may change behavior for downstream callers that distinguish null vs empty—so it should not be flagged as redundant solely on that basis.
6fead81 to
a463275
Compare
This was referenced Apr 28, 2026
This was referenced Apr 28, 2026
When enumerating properties via forEachProperty, a Proxy in the prototype chain eagerly invokes [[Get]] to fill the property slot. If the underlying getter throws, getPropertySlot returns false with a pending exception, and we would `continue` before clearing it. The pending exception then caused the subsequent getPrototype() on the Proxy to return an empty JSValue, and calling .getObject() on that is a null-pointer member call. Clear the exception before the early continue (matching the pattern already used in forEachPropertyOrdered), and also guard the getPrototype() result so a throwing getPrototypeOf trap stops iteration instead of crashing.
…get_all_property_names Hold the current link in the prototype chain as a JSValue under an EnsureStillAliveScope so a Proxy trap that runs JS (and may GC) during property enumeration can't free it out from under us. Also guard the equivalent getPrototype().getObject() pattern in napi_get_all_property_names, propagating Proxy trap exceptions as napi_pending_exception. Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.
Jarred-Sumner
force-pushed
the
farm/43e87898/inspect-proxy-proto-throw
branch
from
a463275 to
932c22b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes crashes in
Bun.inspect()/console.log()/ expect matcher output and innapi_get_all_property_nameswhen an object's prototype chain contains a Proxy whose traps throw or run JS that triggers GC.Repro
Root cause
In
JSC__JSValue__forEachPropertyImpl's slow-path prototype walk:getPropertySlotgoes through aProxyObjectwithInternalMethodType::Get, JSC eagerly invokes[[Get]]. If the target getter throws,getPropertySlotreturnsfalsewith the exception still pending, and wecontinued beforeCLEAR_IF_EXCEPTION.iterating->getPrototype(globalObject).getObject()dereferences a nullJSCell*whengetPrototypereturns an emptyJSValue{}(because of a leaked exception from (1), or because thegetPrototypeOftrap itself throws).JSObject* iteratingis held across calls into JS (Proxy traps,itercallback) that can GC; if a trap detaches this link from the prototype chain it can be collected mid-iteration.The same
getPrototype().getObject()pattern also exists innapi_get_all_property_names.Fix
continue(matchingforEachPropertyOrdered).getPrototype()so a throwinggetPrototypeOftrap stops iteration instead of crashing.JSValueunder anEnsureStillAliveScopeacross the loop body.getOwnPropertyDescriptor/getPrototypeinnapi_get_all_property_namesasnapi_pending_exceptioninstead of dereferencing an empty JSValue.Found by Fuzzilli (fingerprint
d678cafe50a2ad6e). Supersedes #29071 #28991 #28919 #28918 #28882 #28854 #28530 #28325.