From a8e68c8de874eea0568f660d6353a399c868823f Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 16 Oct 2026 03:35:34 +0000 Subject: [PATCH 1/3] spyOn: throw a TypeError when the object refuses the spy spyOn() and mockRestore() wrote with putDirect(), putDirectIndex() and putDirectAccessor() and read none of the results. So: - spyOn() on a WebAssembly GC struct or array aborted the process. Its Structure never transitions. - An index key on a frozen array or object returned a spy that was not installed. - A name that a module does not export also returned a spy that was not installed. - A restore that the object refused left the spy installed, forgot it, and stopped restoreAllMocks() before the other spies. Install and restore now go through one writer, putOwnPropertyStorage(). A Proxy or a WebAssembly GC object that is not extensible decides through its own defineOwnProperty(). Every other object keeps the direct store, so a spy on a frozen object with a named key, and on read-only builtins such as Bun.fetch, works as before. A refused store throws a TypeError. clearSpy() writes first and forgets the spy only when the write succeeded. restoreAllMocks() restores every spy it can, keeps the ones that were refused, and rethrows the first error. The single writer with a defineOwnProperty() arm and the restore loop follow the design in #44833. Co-authored-by: Jarred Sumner --- docs/test/mocks.mdx | 2 + src/jsc/bindings/JSMockFunction.cpp | 189 +++++++++++++--------- test/js/bun/test/mock-fn.test.js | 181 +++++++++++++++++++++ test/js/bun/test/mock/mock-module.test.ts | 8 + 4 files changed, 307 insertions(+), 73 deletions(-) diff --git a/docs/test/mocks.mdx b/docs/test/mocks.mdx index ed4fd45349b1..c41d0bc5b523 100644 --- a/docs/test/mocks.mdx +++ b/docs/test/mocks.mdx @@ -186,6 +186,8 @@ test("spyon", () => { }); ``` +`spyOn()` throws a `TypeError` when the object cannot take the spy: an index of a frozen array, a `Proxy` whose target is frozen, a WebAssembly GC object, or a name that a module does not export. If the object refuses the original value at `mockRestore()`, that call throws and the spy stays installed. + ### Advanced Spy Usage ```ts title="test.ts" icon="/icons/typescript.svg" diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index 5c09121b9372..51437a23e991 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -224,6 +224,37 @@ enum class CallbackKind : uint8_t { const ClassInfo JSMockImplementation::s_info = { "MockImpl"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSMockImplementation) }; +// Only a Proxy and a WebAssembly GC object answer isExtensible() themselves. When they say no, their own defineOwnProperty() decides the store. +static bool storesThroughDefineOwnProperty(JSGlobalObject* globalObject, JSObject* target) +{ + if (!target->structure()->typeInfo().overridesIsExtensible()) [[likely]] + return false; + return !target->isExtensible(globalObject); +} + +// Returns false when the target refuses the store without throwing. +static bool putOwnPropertyStorage(JSGlobalObject* globalObject, JSObject* target, PropertyName key, JSValue storage, unsigned attributes, bool throughDefineOwnProperty) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (throughDefineOwnProperty) [[unlikely]] { + PropertyDescriptor descriptor; + descriptor.setDescriptor(storage, attributes); + RELEASE_AND_RETURN(scope, target->methodTable()->defineOwnProperty(target, globalObject, key, descriptor, true)); + } + if (attributes & PropertyAttribute::Accessor) + RELEASE_AND_RETURN(scope, target->putDirectAccessor(globalObject, key, uncheckedDowncast(storage), attributes)); + if (auto index = parseIndex(key)) + RELEASE_AND_RETURN(scope, target->putDirectIndex(globalObject, *index, storage, attributes, PutDirectIndexLikePutDirect)); + return target->putDirect(vm, key, storage, attributes); +} + +static void throwStoreRefused(JSGlobalObject* globalObject, JSC::ThrowScope& scope, ASCIILiteral action, PropertyName key, ASCIILiteral reason) +{ + throwTypeError(globalObject, scope, makeString("Cannot "_s, action, " the `"_s, String(key.uid()), "` property because "_s, reason)); +} + class JSMockFunction : public JSC::InternalFunction { public: using Base = JSC::InternalFunction; @@ -271,6 +302,7 @@ class JSMockFunction : public JSC::InternalFunction { unsigned spyAttributes = 0; static constexpr unsigned SpyAttributeESModuleNamespace = 1 << 30; + static constexpr unsigned SpyAttributeDefineOwnProperty = 1 << 29; JSString* jsName() { @@ -357,38 +389,34 @@ class JSMockFunction : public JSC::InternalFunction { this->tail.clear(); } - // Puts the original value back on the spied-on object; that put can throw (a module namespace - // export, an indexed slot). + // Puts the original value back on the spied-on object. If the object refuses, the spy stays installed and can be restored again. void clearSpy(JSGlobalObject* globalObject) { auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); - this->reset(); - auto* target = this->spyTarget.get(); - JSValue implValue = this->spyOriginal.get(); - if (!implValue) { - implValue = jsUndefined(); + if (auto* target = this->spyTarget.get()) { + JSValue original = this->spyOriginal.get(); + if (!original) + original = jsUndefined(); + unsigned attributes = this->spyAttributes; + bool isExport = attributes & SpyAttributeESModuleNamespace; + + bool restored = true; + if (!isExport) + restored = putOwnPropertyStorage(globalObject, target, this->spyIdentifier, original, attributes & ~SpyAttributeDefineOwnProperty, attributes & SpyAttributeDefineOwnProperty); + else if (auto* moduleNamespaceObject = tryJSDynamicCast(target)) + restored = moduleNamespaceObject->overrideExportValue(globalObject, this->spyIdentifier, original); + RETURN_IF_EXCEPTION(scope, ); + if (!restored) [[unlikely]] { + throwStoreRefused(globalObject, scope, "restore"_s, this->spyIdentifier, isExport ? "the module does not export it"_s : "the object does not allow it to be changed"_s); + return; + } } - JSC::Identifier identifier = this->spyIdentifier; - unsigned attributes = this->spyAttributes; + + this->reset(); this->spyTarget.clear(); this->spyIdentifier = JSC::Identifier(); this->spyAttributes = 0; - if (!target) - return; - - if (attributes & SpyAttributeESModuleNamespace) { - if (auto* moduleNamespaceObject = tryJSDynamicCast(target)) { - moduleNamespaceObject->overrideExportValue(globalObject, identifier, implValue); - RETURN_IF_EXCEPTION(scope, ); - } - } else if (auto index = parseIndex(identifier)) { - // Use putDirectIndex for numeric property keys (e.g., spyOn(arr, 0)) - target->putDirectIndex(globalObject, *index, implValue, attributes, PutDirectIndexLikePutDirect); - RETURN_IF_EXCEPTION(scope, ); - } else { - target->putDirect(this->vm(), identifier, implValue, attributes); - } } JSArray* getCalls() const @@ -632,21 +660,41 @@ static void forEachMockInSet(JSC::WriteBarrier& mockSet, const Fun } } +static void addToMockSet(Zig::GlobalObject* globalObject, JSC::WriteBarrier& mockSet, JSMockFunction* mock) +{ + auto& vm = JSC::getVM(globalObject); + if (!mockSet) + mockSet.set(vm, globalObject, ActiveSpySet::create(vm, globalObject->mockModule.activeSpySetStructure.getInitializedOnMainThread(globalObject))); + uncheckedDowncast(mockSet.get())->add(vm, mock, mock); +} + +// Restores every spy it can, keeps the ones whose object refused, and rethrows the first error. static void restoreAllMocks(Zig::GlobalObject* globalObject) { auto scope = DECLARE_THROW_SCOPE(globalObject->vm()); - JSValue spies = globalObject->mockModule.activeSpies.get(); - if (!spies) + auto& activeSpies = globalObject->mockModule.activeSpies; + if (!activeSpies) return; MarkedArgumentBuffer active; - uncheckedDowncast(spies)->takeSnapshot(active); + uncheckedDowncast(activeSpies.get())->takeSnapshot(active); + activeSpies.clear(); + + JSC::Exception* firstException = nullptr; for (size_t i = 0; i < active.size(); ++i) { - if (auto* spy = dynamicDowncast(active.at(i))) { - spy->clearSpy(globalObject); - RETURN_IF_EXCEPTION(scope, ); + auto* spy = dynamicDowncast(active.at(i)); + if (!spy) + continue; + spy->clearSpy(globalObject); + if (auto* exception = scope.exception()) [[unlikely]] { + if (!scope.tryClearException()) + return; + addToMockSet(globalObject, activeSpies, spy); + if (!firstException) + firstException = exception; } } - globalObject->mockModule.activeSpies.clear(); + if (firstException) + throwException(globalObject, scope, firstException); } extern "C" void JSMock__clearAllMocks(Zig::GlobalObject* globalObject) @@ -1497,6 +1545,38 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsResetAllMocks, (JSC::JSGlobalObject * globalO return JSValue::encode(jsUndefined()); } +// Replaces target[key] with the spy: the mock itself, or a getter/setter pair of it when `attributes` has Accessor. +static void installSpy(Zig::GlobalObject* globalObject, JSMockFunction* mock, JSC::JSObject* target, JSC::PropertyName key, unsigned attributes) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (auto* moduleNamespaceObject = tryJSDynamicCast(target)) { + bool overridden = moduleNamespaceObject->overrideExportValue(globalObject, key, mock); + RETURN_IF_EXCEPTION(scope, ); + if (!overridden) [[unlikely]] { + throwStoreRefused(globalObject, scope, "spy on"_s, key, "the module does not export it"_s); + return; + } + mock->spyAttributes |= JSMockFunction::SpyAttributeESModuleNamespace; + return; + } + + bool throughDefineOwnProperty = storesThroughDefineOwnProperty(globalObject, target); + RETURN_IF_EXCEPTION(scope, ); + JSValue storage = mock; + if (attributes & PropertyAttribute::Accessor) + storage = JSC::GetterSetter::create(vm, globalObject, mock, mock); + bool stored = putOwnPropertyStorage(globalObject, target, key, storage, attributes, throughDefineOwnProperty); + RETURN_IF_EXCEPTION(scope, ); + if (!stored) [[unlikely]] { + throwStoreRefused(globalObject, scope, "spy on"_s, key, "the object does not allow it to be changed"_s); + return; + } + if (throughDefineOwnProperty) + mock->spyAttributes |= JSMockFunction::SpyAttributeDefineOwnProperty; +} + BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalObject, JSC::CallFrame* callframe)) { auto& vm = JSC::getVM(lexicalGlobalObject); @@ -1563,16 +1643,7 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb mock->copyNameAndLength(vm, globalObject, value); RETURN_IF_EXCEPTION(scope, {}); - if (JSModuleNamespaceObject* moduleNamespaceObject = tryJSDynamicCast(object)) { - moduleNamespaceObject->overrideExportValue(globalObject, propertyKey, mock); - mock->spyAttributes |= JSMockFunction::SpyAttributeESModuleNamespace; - } else if (auto index = parseIndex(propertyKey)) { - // Use putDirectIndex for numeric property keys (e.g., spyOn(arr, 0)) - object->putDirectIndex(globalObject, *index, mock, attributes, PutDirectIndexLikePutDirect); - } else { - object->putDirect(vm, propertyKey, mock, attributes); - } - + installSpy(globalObject, mock, object, propertyKey, attributes); RETURN_IF_EXCEPTION(scope, {}); pushImpl(mock, globalObject, JSMockImplementation::Kind::Call, value); @@ -1589,13 +1660,7 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb attributes |= PropertyAttribute::Accessor; - if (JSModuleNamespaceObject* moduleNamespaceObject = tryJSDynamicCast(object)) { - moduleNamespaceObject->overrideExportValue(globalObject, propertyKey, mock); - mock->spyAttributes |= JSMockFunction::SpyAttributeESModuleNamespace; - } else { - object->putDirectAccessor(globalObject, propertyKey, JSC::GetterSetter::create(vm, globalObject, mock, mock), attributes); - } - + installSpy(globalObject, mock, object, propertyKey, attributes); // mock->setName(propertyKey.publicName()); RETURN_IF_EXCEPTION(scope, {}); @@ -1603,24 +1668,8 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb } mock->spyOriginal.set(vm, mock, value); - - { - if (!globalObject->mockModule.activeSpies) { - ActiveSpySet* activeSpies = ActiveSpySet::create(vm, globalObject->mockModule.activeSpySetStructure.getInitializedOnMainThread(globalObject)); - globalObject->mockModule.activeSpies.set(vm, globalObject, activeSpies); - } - ActiveSpySet* activeSpies = uncheckedDowncast(globalObject->mockModule.activeSpies.get()); - activeSpies->add(vm, mock, mock); - } - - { - if (!globalObject->mockModule.activeMocks) { - ActiveSpySet* activeMocks = ActiveSpySet::create(vm, globalObject->mockModule.activeSpySetStructure.getInitializedOnMainThread(globalObject)); - globalObject->mockModule.activeMocks.set(vm, globalObject, activeMocks); - } - ActiveSpySet* activeMocks = uncheckedDowncast(globalObject->mockModule.activeMocks.get()); - activeMocks->add(vm, mock, mock); - } + addToMockSet(globalObject, globalObject->mockModule.activeSpies, mock); + addToMockSet(globalObject, globalObject->mockModule.activeMocks, mock); return JSValue::encode(mock); } @@ -1661,13 +1710,7 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsMockFn, (JSC::JSGlobalObject * lexicalGlobalO thisObject->setName("mockConstructor"_s); } - if (!globalObject->mockModule.activeMocks) { - ActiveSpySet* activeMocks = ActiveSpySet::create(vm, globalObject->mockModule.activeSpySetStructure.getInitializedOnMainThread(globalObject)); - globalObject->mockModule.activeMocks.set(vm, globalObject, activeMocks); - } - - ActiveSpySet* activeMocks = uncheckedDowncast(globalObject->mockModule.activeMocks.get()); - activeMocks->add(vm, thisObject, thisObject); + addToMockSet(globalObject, globalObject->mockModule.activeMocks, thisObject); return JSValue::encode(thisObject); } diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 8773618a30a1..16fa8f88f432 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -1098,6 +1098,187 @@ describe("spyOn", () => { obj.original; expect(fn).not.toHaveBeenCalled(); }); + + test("throws when the object refuses an index key, and returns no spy that is not installed", () => { + const original = () => 1; + for (const target of [Object.freeze([original]), Object.freeze({ 0: original })]) { + expect(() => spyOn(target, 0)).toThrow(TypeError); + expect(() => spyOn(target, 0)).toThrow( + "Cannot spy on the `0` property because the object does not allow it to be changed", + ); + expect(target[0]).toBe(original); + } + + const numbers = Object.freeze([7]); + expect(() => spyOn(numbers, 0)).toThrow(TypeError); + expect(() => spyOn(numbers, 5)).toThrow( + "Cannot spy on the `5` property because the object does not allow it to be changed", + ); + expect(numbers).toEqual([7]); + }); + + test("still spies on a named key of a frozen, sealed or non-extensible object", () => { + const original = () => 1; + const locks = [ + [Object.freeze, Object.isFrozen], + [Object.seal, Object.isSealed], + [Object.preventExtensions, target => !Object.isExtensible(target)], + ]; + for (const [lock, isLocked] of locks) { + const target = lock({ fn: original }); + const spy = spyOn(target, "fn").mockReturnValue(2); + expect(target.fn()).toBe(2); + spy.mockRestore(); + expect(target.fn).toBe(original); + expect(isLocked(target)).toBe(true); + } + }); + + test("still spies on builtins that are read-only or not configurable", () => { + const builtins = [ + [Bun, "fetch"], + [Bun, "file"], + [Bun, "env"], + [Bun, "version"], + [Bun, "argv"], + [Bun, "stdout"], + [Response, "json"], + [Math, "PI"], + ]; + for (const [target, key] of builtins) { + const before = Object.getOwnPropertyDescriptor(target, key); + expect(before.configurable).toBe(false); + const spy = spyOn(target, key); + let seen; + try { + spy.mockReturnValue("spied"); + seen = typeof before.value === "function" ? target[key]() : target[key]; + } finally { + spy.mockRestore(); + } + expect(seen).toBe("spied"); + expect(Object.getOwnPropertyDescriptor(target, key)).toEqual(before); + } + }); + + test("a Proxy that is not extensible decides the store itself", () => { + const original = () => 1; + const frozen = new Proxy(Object.freeze({ fn: original }), {}); + expect(() => spyOn(frozen, "fn")).toThrow(TypeError); + expect(frozen.fn).toBe(original); + + const sealed = Object.seal({ fn: original }); + const spy = spyOn(new Proxy(sealed, {}), "fn"); + expect(sealed.fn).toBe(spy); + spy.mockRestore(); + expect(sealed.fn).toBe(original); + expect(Object.isSealed(sealed)).toBe(true); + }); + + // The handler refuses a definition while `refuse` is true, so a test can undo the refusal. + function refusingProxy(original) { + const state = { refuse: false, target: Object.seal({ fn: original }) }; + state.proxy = new Proxy(state.target, { + defineProperty: (target, key, descriptor) => !state.refuse && Reflect.defineProperty(target, key, descriptor), + }); + return state; + } + + test("a refused restore throws, and the spy stays installed and can be restored again", () => { + const original = () => 1; + const state = refusingProxy(original); + const spy = spyOn(state.proxy, "fn"); + expect(state.target.fn).toBe(spy); + + state.refuse = true; + expect(() => spy.mockRestore()).toThrow(TypeError); + expect(state.target.fn).toBe(spy); + expect(state.target.fn()).toBe(1); + expect(spy).toHaveBeenCalledTimes(1); + + state.refuse = false; + spy.mockRestore(); + expect(state.target.fn).toBe(original); + }); + + test("restoreAllMocks restores the other spies when one object refuses", () => { + const original = () => 1; + const first = { fn: original }; + const last = { fn: original }; + const state = refusingProxy(original); + spyOn(first, "fn"); + const refused = spyOn(state.proxy, "fn"); + spyOn(last, "fn"); + + state.refuse = true; + expect(() => jest.restoreAllMocks()).toThrow(TypeError); + expect(first.fn).toBe(original); + expect(last.fn).toBe(original); + expect(state.target.fn).toBe(refused); + expect(() => jest.restoreAllMocks()).toThrow(TypeError); + + state.refuse = false; + jest.restoreAllMocks(); + expect(state.target.fn).toBe(original); + }); + + // Both cases need their own process: the first one aborted it, and the second leaves a spy that no restore can remove. + test("throws on a WebAssembly GC object, and keeps an index spy that a frozen array refuses to restore", async () => { + const { bunEnv, bunExe } = require("harness"); + const src = ` + const { spyOn, jest } = require("bun:test"); + const lines = []; + const attempt = (label, fn) => { + try { + fn(); + lines.push(label + ": no error"); + } catch (e) { + lines.push(label + ": " + e.constructor.name + ": " + e.message); + } + }; + + // (type $s (struct (field (mut i32)))) (type $a (array (mut i32))) + // (func (export "s") (result (ref null $s)) struct.new_default $s) + // (func (export "a") (result (ref null $a)) i32.const 2 array.new_default $a) + const bytes = new Uint8Array([ + 0, 0x61, 0x73, 0x6d, 1, 0, 0, 0, + 1, 18, 4, 0x5f, 1, 0x7f, 1, 0x5e, 0x7f, 1, 0x60, 0, 1, 0x63, 0, 0x60, 0, 1, 0x63, 1, + 3, 3, 2, 2, 3, + 7, 9, 2, 1, 0x73, 0, 0, 1, 0x61, 0, 1, + 10, 15, 2, 5, 0, 0xfb, 1, 0, 0x0b, 7, 0, 0x41, 2, 0xfb, 7, 1, 0x0b, + ]); + const { s, a } = new WebAssembly.Instance(new WebAssembly.Module(bytes)).exports; + for (const [name, target] of [["struct", s()], ["array", a()]]) { + for (const key of ["x", Symbol("x"), 0]) attempt(name + " " + String(key), () => spyOn(target, key)); + } + + const original = () => 1; + const list = [original]; + const spy = spyOn(list, 0); + Object.freeze(list); + attempt("mockRestore", () => spy.mockRestore()); + attempt("restoreAllMocks", () => jest.restoreAllMocks()); + lines.push("still installed: " + (list[0] === spy && list[0]() === 1)); + console.log(lines.join("\\n")); + `; + const proc = Bun.spawn({ cmd: [bunExe(), "-e", src], env: bunEnv, stdout: "pipe", stderr: "pipe" }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + const refusedByWasm = "TypeError: Cannot define property for WebAssembly GC object"; + const refusedByArray = + "TypeError: Cannot restore the `0` property because the object does not allow it to be changed"; + expect(stdout.trim().split("\n"), stderr).toEqual([ + `struct x: ${refusedByWasm}`, + `struct Symbol(x): ${refusedByWasm}`, + `struct 0: ${refusedByWasm}`, + `array x: ${refusedByWasm}`, + `array Symbol(x): ${refusedByWasm}`, + `array 0: ${refusedByWasm}`, + `mockRestore: ${refusedByArray}`, + `restoreAllMocks: ${refusedByArray}`, + "still installed: true", + ]); + expect(exitCode).toBe(0); + }); } test("spyOn twice works", () => { diff --git a/test/js/bun/test/mock/mock-module.test.ts b/test/js/bun/test/mock/mock-module.test.ts index aabd736b575f..4f0ee21b79b8 100644 --- a/test/js/bun/test/mock/mock-module.test.ts +++ b/test/js/bun/test/mock/mock-module.test.ts @@ -40,6 +40,14 @@ test("spyOn", () => { expect(spyFixture.iSpy).toHaveBeenCalled(); }); +test("spyOn throws for a name that the module does not export", () => { + // @ts-expect-error + const spyOnMissingExport = () => spyOn(spyFixture, "notExported"); + expect(spyOnMissingExport).toThrow(TypeError); + expect(spyOnMissingExport).toThrow("Cannot spy on the `notExported` property because the module does not export it"); + expect(Object.keys(spyFixture)).toEqual(["iSpy"]); +}); + test("mocking a module that points to a file which does not resolve successfully still works", async () => { mock.module("i-never-existed-and-i-never-will", () => { return { From 67bb72131407401b8627b27a82ec0542a0707495 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 16 Oct 2026 04:51:02 +0000 Subject: [PATCH 2/3] spyOn: restore what the target of a non-extensible Proxy held A Proxy can answer the lookup of spyOn() from its `has` and `get` traps. Then the slot has no attributes, and the value is what the trap returned. The store through defineOwnProperty() used both, so a spy on such a Proxy made a non-enumerable method enumerable, and the restore wrote the value from the trap. On that path, read the own descriptor of the target. Install and restore use its attributes, and the restore writes its value. An own accessor property gets the same error as on an ordinary object. --- src/jsc/bindings/JSMockFunction.cpp | 17 +++++++++++++++- test/js/bun/test/mock-fn.test.js | 30 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index 51437a23e991..80982c07a8f4 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -1564,6 +1564,21 @@ static void installSpy(Zig::GlobalObject* globalObject, JSMockFunction* mock, JS bool throughDefineOwnProperty = storesThroughDefineOwnProperty(globalObject, target); RETURN_IF_EXCEPTION(scope, ); + if (throughDefineOwnProperty) [[unlikely]] { + // A Proxy can answer the lookup of spyOn() from its `has` and `get` traps. The restore needs what the target holds. + PropertyDescriptor own; + bool hasOwn = target->getOwnPropertyDescriptor(globalObject, key, own); + RETURN_IF_EXCEPTION(scope, ); + if (hasOwn && !own.isDataDescriptor()) { + throwVMError(globalObject, scope, "spyOn(target, prop) does not support accessor properties yet"_s); + return; + } + if (hasOwn) { + mock->spyAttributes = own.attributes() & (PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum | PropertyAttribute::DontDelete); + mock->spyOriginal.set(vm, mock, own.value()); + attributes = mock->spyAttributes | (attributes & PropertyAttribute::Accessor); + } + } JSValue storage = mock; if (attributes & PropertyAttribute::Accessor) storage = JSC::GetterSetter::create(vm, globalObject, mock, mock); @@ -1634,6 +1649,7 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb mock->spyTarget = JSC::Weak(object, &weakValueHandleOwner(), nullptr); mock->spyIdentifier = propertyKey.isSymbol() ? Identifier::fromUid(vm, propertyKey.uid()) : Identifier::fromString(vm, propertyKey.publicName()); mock->spyAttributes = hasValue ? slot.attributes() : 0; + mock->spyOriginal.set(vm, mock, value); unsigned attributes = 0; if (hasValue && ((slot.attributes() & PropertyAttribute::Function) != 0 || (value.isCell() && value.isCallable()))) { @@ -1667,7 +1683,6 @@ BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalOb pushImpl(mock, globalObject, JSMockImplementation::Kind::ReturnValue, value); } - mock->spyOriginal.set(vm, mock, value); addToMockSet(globalObject, globalObject->mockModule.activeSpies, mock); addToMockSet(globalObject, globalObject->mockModule.activeMocks, mock); diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 16fa8f88f432..1be7e6659f5b 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -1175,6 +1175,36 @@ describe("spyOn", () => { expect(Object.isSealed(sealed)).toBe(true); }); + test("a Proxy that is not extensible gets back the attributes and the value that its target held", () => { + const original = () => 1; + const wrapper = () => original() + 1; + const target = Object.preventExtensions( + Object.defineProperty({}, "fn", { value: original, writable: true, enumerable: false, configurable: true }), + ); + const before = Object.getOwnPropertyDescriptor(target, "fn"); + // The `has` trap hides the attributes from the lookup, and the `get` trap returns another function. + const proxy = new Proxy(target, { + has: (object, key) => key in object, + get: (object, key) => (key === "fn" ? wrapper : object[key]), + }); + + const spy = spyOn(proxy, "fn"); + expect(target.fn).toBe(spy); + expect(target.fn()).toBe(2); + expect(Object.getOwnPropertyDescriptor(target, "fn").enumerable).toBe(false); + spy.mockRestore(); + expect(Object.getOwnPropertyDescriptor(target, "fn")).toEqual(before); + + const accessor = Object.preventExtensions({ + get x() { + return 1; + }, + }); + expect(() => spyOn(new Proxy(accessor, { has: () => true }), "x")).toThrow( + "spyOn(target, prop) does not support accessor properties yet", + ); + }); + // The handler refuses a definition while `refuse` is true, so a test can undo the refusal. function refusingProxy(original) { const state = { refuse: false, target: Object.seal({ fn: original }) }; From f40e1c9c6bb22f138396b51d9c2c66a49293cd44 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 16 Oct 2026 05:39:33 +0000 Subject: [PATCH 3/3] spyOn: keep a Proxy alive while its target holds the spy When defineOwnProperty() of a Proxy stores the spy, the spy is in the target of the Proxy. The mock held the Proxy weakly. If nothing else kept the Proxy alive, a collection cleared that reference, and mockRestore() and restoreAllMocks() then did nothing and left the spy on the target. The mock now holds such a target strongly until the restore. The same reference replaces the flag bit that said which path stored the spy. --- src/jsc/bindings/JSMockFunction.cpp | 9 ++++++--- test/js/bun/test/mock-fn.test.js | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/jsc/bindings/JSMockFunction.cpp b/src/jsc/bindings/JSMockFunction.cpp index 80982c07a8f4..a6c5b7227062 100644 --- a/src/jsc/bindings/JSMockFunction.cpp +++ b/src/jsc/bindings/JSMockFunction.cpp @@ -298,11 +298,12 @@ class JSMockFunction : public JSC::InternalFunction { mutable JSC::WriteBarrier returnValues; JSC::Weak spyTarget; + // Set when defineOwnProperty() of spyTarget stored the spy. The spy is then in another object, which can outlive spyTarget. + mutable JSC::WriteBarrier spyTargetForDefine; JSC::Identifier spyIdentifier; unsigned spyAttributes = 0; static constexpr unsigned SpyAttributeESModuleNamespace = 1 << 30; - static constexpr unsigned SpyAttributeDefineOwnProperty = 1 << 29; JSString* jsName() { @@ -403,7 +404,7 @@ class JSMockFunction : public JSC::InternalFunction { bool restored = true; if (!isExport) - restored = putOwnPropertyStorage(globalObject, target, this->spyIdentifier, original, attributes & ~SpyAttributeDefineOwnProperty, attributes & SpyAttributeDefineOwnProperty); + restored = putOwnPropertyStorage(globalObject, target, this->spyIdentifier, original, attributes, !!this->spyTargetForDefine); else if (auto* moduleNamespaceObject = tryJSDynamicCast(target)) restored = moduleNamespaceObject->overrideExportValue(globalObject, this->spyIdentifier, original); RETURN_IF_EXCEPTION(scope, ); @@ -415,6 +416,7 @@ class JSMockFunction : public JSC::InternalFunction { this->reset(); this->spyTarget.clear(); + this->spyTargetForDefine.clear(); this->spyIdentifier = JSC::Identifier(); this->spyAttributes = 0; } @@ -505,6 +507,7 @@ void JSMockFunction::visitAdditionalChildrenInGCThread(Visitor& visitor) visitor.append(fn->returnValues); visitor.append(fn->invocationCallOrder); visitor.append(fn->spyOriginal); + visitor.append(fn->spyTargetForDefine); fn->mock.visit(visitor); } @@ -1589,7 +1592,7 @@ static void installSpy(Zig::GlobalObject* globalObject, JSMockFunction* mock, JS return; } if (throughDefineOwnProperty) - mock->spyAttributes |= JSMockFunction::SpyAttributeDefineOwnProperty; + mock->spyTargetForDefine.set(vm, mock, target); } BUN_DEFINE_HOST_FUNCTION(JSMock__jsSpyOn, (JSC::JSGlobalObject * lexicalGlobalObject, JSC::CallFrame* callframe)) diff --git a/test/js/bun/test/mock-fn.test.js b/test/js/bun/test/mock-fn.test.js index 1be7e6659f5b..1d4f38199751 100644 --- a/test/js/bun/test/mock-fn.test.js +++ b/test/js/bun/test/mock-fn.test.js @@ -1175,6 +1175,21 @@ describe("spyOn", () => { expect(Object.isSealed(sealed)).toBe(true); }); + test("restores through a Proxy that nothing else keeps alive", () => { + const original = () => 1; + const first = Object.seal({ fn: original }); + const second = Object.seal({ fn: original }); + const spy = (() => spyOn(new Proxy(first, {}), "fn"))(); + (() => spyOn(new Proxy(second, {}), "fn"))(); + Bun.gc(true); + expect(first.fn).toBe(spy); + + spy.mockRestore(); + expect(first.fn).toBe(original); + jest.restoreAllMocks(); + expect(second.fn).toBe(original); + }); + test("a Proxy that is not extensible gets back the attributes and the value that its target held", () => { const original = () => 1; const wrapper = () => original() + 1;