Fix assertion failure when spyOn is used on non-callable indexed property#29481
Fix assertion failure when spyOn is used on non-callable indexed property#29481robobun wants to merge 1 commit into
Conversation
|
Updated 3:51 AM PT - May 6th, 2026
❌ @robobun, your commit 22eaff9 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29481That installs a local version of the PR into your bun-29481 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughModified JSMockFunction.cpp to clear the PropertyAttribute::Accessor bit when installing mocks on indexed properties via putDirectIndex. Added test case verifying spyOn behavior on numeric property keys for non-callable values, including proper restoration of original values. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal one-line fix that correctly masks out PropertyAttribute::Accessor before calling putDirectIndex, with a solid regression test.
Extended reasoning...
Overview
The PR touches two files: (one-line change) and (regression test). It targets the non-callable indexed property branch of JSMock__jsSpyOn.
Security risks
None. This is purely in the test mock/spy infrastructure and involves no auth, crypto, or permissions concerns.
Level of scrutiny
Low. The bug is a clear invariant violation: JSC's PropertySlot::setValue asserts that plain values must not carry PropertyAttribute::Accessor. The existing code set that bit even when storing a plain mock value (not a GetterSetter) via putDirectIndex. The fix — attributes & ~PropertyAttribute::Accessor — is the minimal and correct remediation. The callable path already used putDirect without the Accessor bit; this makes the non-callable path consistent.
Other factors
No bugs were found by the bug hunting system. The regression test covers missing and existing non-callable indexed properties, Object.getOwnPropertyDescriptor, and mockRestore. The PR is Fuzzilli-found, well-described, and self-contained.
|
CI summary for build #46435:
No failures are related to this change. |
robobun
force-pushed
the
farm/2e6b20d2/spyon-indexed-accessor-attribute
branch
from
9c2d0bc to
6d6cfe4
Compare
There was a problem hiding this comment.
LGTM — minimal, correct fix masking out the Accessor bit for the indexed-property putDirectIndex path, with a regression test.
Extended reasoning...
Overview
This PR changes a single line in src/jsc/bindings/JSMockFunction.cpp inside JSMock__jsSpyOn, in the branch that handles spying on a non-callable value at a numeric index. The branch already stores the mock as a plain value via putDirectIndex (not wrapped in a GetterSetter), but it was passing attributes that had just had PropertyAttribute::Accessor OR'd in. The fix masks that bit back out (attributes & ~PropertyAttribute::Accessor) so the indexed storage doesn't carry an Accessor attribute on a plain value, which is what tripped the PropertySlot::setValue debug assertion. A regression test is added to test/js/bun/test/mock-fn.test.js.
Security risks
None. This is test-runner mock infrastructure (spyOn), not auth/crypto/network code. The change only narrows attribute bits passed to an internal JSC put; it cannot expose data or change permissions.
Level of scrutiny
Low. The diff is one expression change whose intent is fully explained by the surrounding code: the sibling else branch uses putDirectAccessor with a real GetterSetter (where Accessor is appropriate), while this branch stores a plain value (where Accessor is invalid). mock->spyAttributes is captured before attributes |= PropertyAttribute::Accessor, so the clearSpy() restore path is unaffected and the new mockRestore test confirms it.
Other factors
CI passed across all platforms (the only failures were unrelated darwin-aarch64 agent expirations and an autofix.ci formatting issue on an untouched docs file). No CODEOWNERS apply to these paths, no outstanding reviewer comments, and the bug-hunting system found nothing. The duplicate-PR bot flagged two earlier PRs targeting the same assertion, but that's a maintainer triage concern rather than a correctness concern for this change.
|
CI summary for build #51953 (rebased commit 6d6cfe4): None of the failures involve
The added regression test ( |
robobun
force-pushed
the
farm/2e6b20d2/spyon-indexed-accessor-attribute
branch
from
6d6cfe4 to
1ca98f8
Compare
…erty When spyOn(obj, 1) was called on an object where obj[1] was missing or a non-callable value, the mock was stored via putDirectIndex with PropertyAttribute::Accessor set, even though the stored value was a plain JSMockFunction and not a GetterSetter. Any subsequent property lookup on that index hit the PropertySlot::setValue assertion that forbids the Accessor attribute on plain values. Mask out the Accessor attribute when storing the mock at a numeric index, since the indexed path intentionally does not wrap the mock in a GetterSetter.
robobun
force-pushed
the
farm/2e6b20d2/spyon-indexed-accessor-attribute
branch
from
1ca98f8 to
22eaff9
Compare
There was a problem hiding this comment.
LGTM — minimal, well-scoped fix that strips the invalid Accessor bit for the putDirectIndex path, with a regression test.
Extended reasoning...
Overview
This PR changes a single line in src/jsc/bindings/JSMockFunction.cpp (JSMock__jsSpyOn) and adds a regression test in test/js/bun/test/mock-fn.test.js. In the non-callable indexed-property branch, the code unconditionally OR'd PropertyAttribute::Accessor into attributes (intended for the putDirectAccessor + GetterSetter path), but then passed those same attributes to putDirectIndex, which stores the mock as a plain value. Storing a plain value with the Accessor attribute violates JSC's PropertySlot::setValue invariant and trips a debug assertion on the next read. The fix masks out PropertyAttribute::Accessor for the putDirectIndex call only.
Security risks
None. This is purely a correctness fix in test-runner mock infrastructure (bun:test spyOn). It does not touch auth, crypto, networking, parsing, or any user-input boundary. The change strictly removes an attribute bit that was incorrect in this branch and cannot widen capabilities.
Level of scrutiny
Low. The diff is one expression (attributes → attributes & ~PropertyAttribute::Accessor), the rationale is mechanically verifiable from the surrounding code (the Accessor bit was added two lines above for the sibling putDirectAccessor branch and is meaningless/incorrect for a plain-value putDirectIndex), and mock->spyAttributes (used by clearSpy() for restoration) was captured before the bit was OR'd in, so restore behavior is unaffected. The added test exercises both the missing-index and existing-non-callable-index cases plus getOwnPropertyDescriptor and mockRestore.
Other factors
Earlier CI runs (#46435, #51953) passed on all platforms with only unrelated/flaky failures, and the new regression test passed everywhere. The most recent build (#52039) shows infrastructure-wide build-zig failures in scripts/build/ci.ts across every target, which are unrelated to this C++/test-only change. The duplicate-PR bot flagged #28540/#28335 as overlapping; that's a triage concern for maintainers but doesn't affect correctness here. No human reviewer comments are outstanding.
|
Closing in favor of #31619, which fixes the same assertion with the correct semantics. This PR strips the The non-indexed |
1 participant
What does this PR do?
Fixes a debug assertion in
PropertySlot::setValuetriggered after callingspyOn(obj, <numeric index>)on a non-callable (or missing) value.When spying on a non-callable value at a numeric index,
spyOnstores the mock directly viaputDirectIndexrather than wrapping it in aGetterSetter. However it was still passingPropertyAttribute::Accessorin the attributes. This left the indexed storage with a plain value carrying theAccessorattribute, so the next lookup on that index hit thePropertySlot::setValueassertion that forbidsAccessoron plain values.The fix masks out
PropertyAttribute::Accessorfor theputDirectIndexcall.Found by Fuzzilli. Fingerprint:
PropertySlot.h(219)How did you verify your code works?
Added a regression test in
test/js/bun/test/mock-fn.test.jscoveringspyOnon missing and existing non-callable numeric indices, including subsequent reads andObject.getOwnPropertyDescriptor. The fullmock-fn.test.jssuite (73 tests) passes.