-
Notifications
You must be signed in to change notification settings - Fork 5k
ai slop #29200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ai slop #29200
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,6 +86,7 @@ | |
| } | ||
|
|
||
| JSC_DECLARE_HOST_FUNCTION(jsMockFunctionCall); | ||
| JSC_DECLARE_HOST_FUNCTION(jsMockFunctionConstruct); | ||
| JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_protoImpl); | ||
| JSC_DECLARE_CUSTOM_GETTER(jsMockFunctionGetter_mock); | ||
| JSC_DECLARE_HOST_FUNCTION(jsMockFunctionGetter_mockGetLastCall); | ||
|
|
@@ -462,7 +463,7 @@ | |
| } | ||
|
|
||
| JSMockFunction(JSC::VM& vm, JSC::Structure* structure, CallbackKind wrapKind) | ||
| : Base(vm, structure, jsMockFunctionCall, jsMockFunctionCall) | ||
| : Base(vm, structure, jsMockFunctionCall, jsMockFunctionConstruct) | ||
| { | ||
| initMock(); | ||
| } | ||
|
|
@@ -978,9 +979,21 @@ | |
| } | ||
|
|
||
| setReturnValue(createMockResult(vm, globalObject, "return"_s, jsUndefined())); | ||
| return JSValue::encode(jsUndefined()); | ||
| } | ||
|
|
||
| JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) | ||
| { | ||
| auto& vm = JSC::getVM(lexicalGlobalObject); | ||
| auto scope = DECLARE_THROW_SCOPE(vm); | ||
| JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); | ||
| RETURN_IF_EXCEPTION(scope, {}); | ||
| JSValue result = JSValue::decode(encodedResult); | ||
| if (!result.isObject()) | ||
| return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)); | ||
| return encodedResult; | ||
| } | ||
|
Check warning on line 995 in src/bun.js/bindings/JSMockFunction.cpp
|
||
|
claude[bot] marked this conversation as resolved.
|
||
|
|
||
| void JSMockFunctionPrototype::finishCreation(JSC::VM& vm, JSC::JSGlobalObject* globalObject) | ||
| { | ||
|
Comment on lines
982
to
1001
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 When jsMockFunctionConstruct delegates to jsMockFunctionCall, the thisValue captured inside jsMockFunctionCall is callframe->thisValue(), which for JSC InternalFunction native constructors equals newTarget() (the constructor function itself). This causes two observable deviations from Jest semantics: (1) mock().mockReturnThis() used as a constructor via Reflect.construct returns the mock function itself as the result (since result.isObject() is true for functions); (2) mock.mock.contexts records the mock constructor function rather than the constructed instance for every construct invocation. Neither issue is covered by the new mock-construct.test.ts suite. Extended reasoning...What the bug is and how it manifests When jsMockFunctionConstruct delegates to jsMockFunctionCall by passing the same callframe, the line 'JSValue thisValue = callframe->thisValue()' inside jsMockFunctionCall captures newTarget() rather than a pre-created instance. This is an inherent property of JSC::InternalFunction native constructors, confirmed explicitly in the PR timeline by the author: 'For InternalFunction native constructors in JSC, callframe->thisValue() is newTarget() — there is no pre-created receiver.' Two separate misbehaviors follow. Bug 1 — mockReturnThis() in construct context returns the mock constructor itself When mockReturnThis() is active, jsMockFunctionCall executes 'case ReturnThis: return JSValue::encode(thisValue)', returning the mock constructor function (which equals newTarget()). Back in jsMockFunctionConstruct, the check 'if (result.isObject()) return encodedResult' passes — because functions are objects — so the constructor returns the mock function itself as the constructed result. Concretely: const m = mock().mockReturnThis(); const r = Reflect.construct(m, []); r === m // true. In Jest, mockReturnThis() in a constructor would return the pre-created this (the new instance), not the constructor function. Bug 2 — mock.mock.contexts records newTarget instead of the constructed instance In jsMockFunctionCall, the contexts array is populated with thisValue (which equals the mock constructor function). The actual constructed instance is only synthesized later, inside jsMockFunctionConstruct, after jsMockFunctionCall has already pushed to contexts. So mock.mock.contexts[0] === m for construct calls, instead of the newly allocated instance. Jest's mock.mock.contexts[i] for a constructor call holds the newly created this-object. Why existing code does not prevent it Before this PR, Reflect.construct(mock(), []) crashed (assertion in executeConstruct due to the native handler returning jsUndefined()), so neither behavior was observable. The PR correctly introduces jsMockFunctionConstruct to fix the crash, and correctly uses InternalFunction::createSubclassStructure for the primitive-return fallback path. However, the two paths that explicitly return thisValue (the ReturnThis implementation case) and record thisValue (contexts) remain affected, since thisValue equals newTarget — not an instance — in the construct callframe. Addressing the refutations The refutations correctly note that thisValue == newTarget is a pre-existing JSC limitation inherent to InternalFunction, and that a full fix would require architectural changes. However, the observable misbehaviors are new: before this PR, any Reflect.construct call on a mock would crash, making these bugs moot. After this PR enables construct, they become observable for the first time. The bugs exist in newly reachable code paths introduced by this PR. Step-by-step proof for Bug 1
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both behaviors are pre-existing on the const m = mock().mockReturnThis();
new m() === m; // true (pre-existing)
m.mock.contexts[0] === m; // true (pre-existing)This PR's scope is fixing the crash when the construct path is entered via |
||
| Base::finishCreation(vm); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,12 +133,21 @@ extern "C" void WebWorker__setRef( | |
|
|
||
| void Worker::setKeepAlive(bool keepAlive) | ||
| { | ||
| WebWorker__setRef(impl_, keepAlive); | ||
| Locker locker { m_implLock }; | ||
| if (impl_) | ||
| WebWorker__setRef(impl_, keepAlive); | ||
| } | ||
|
|
||
| void Worker::clearZigImpl() | ||
| { | ||
| Locker locker { m_implLock }; | ||
| impl_ = nullptr; | ||
| } | ||
|
Comment on lines
+141
to
145
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Worker::updatePtr call sites =="
rg -nP '\bupdatePtr\s*\(' src --glob '*.{cpp,h,zig}'
echo
echo "== All impl_ accesses in Worker.* =="
rg -nP '\bimpl_\b' src/bun.js/bindings/webcore/Worker.cpp src/bun.js/bindings/webcore/Worker.h
echo
echo "== WebWorker__create declarations/implementations =="
rg -nP 'WebWorker__create\s*\(' src --glob '*.{cpp,h,zig}'
echo
echo "== WebWorker__dispatchExit declarations/implementations =="
rg -nP 'WebWorker__dispatchExit\s*\(' src --glob '*.{cpp,h,zig}'Repository: oven-sh/bun Length of output: 1545 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Worker.cpp lines 145-155 (updatePtr function context) =="
sed -n '145,155p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Worker.cpp lines 195-235 (WebWorker__create and impl_ assignment) =="
sed -n '195,235p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Worker::create full signature =="
rg -B5 'void\* WebWorker__create' src/bun.js/bindings/webcore/Worker.cpp | head -20Repository: oven-sh/bun Length of output: 2193 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== JSWorker.cpp lines 340-360 (updatePtr call context) =="
sed -n '340,360p' src/bun.js/bindings/webcore/JSWorker.cpp
echo
echo "== Worker.cpp lines 130-155 (full context of locking changes) =="
sed -n '130,155p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Worker.cpp lines 270-285 (terminate path) =="
sed -n '270,285p' src/bun.js/bindings/webcore/Worker.cppRepository: oven-sh/bun Length of output: 2139 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Worker constructor/create full flow =="
rg -B10 -A15 'static ExceptionOr<RefPtr<Worker>> create' src/bun.js/bindings/webcore/Worker.cpp | head -60
echo
echo "== Check if m_implLock is reentrant =="
rg -B5 -A5 'class Worker' src/bun.js/bindings/webcore/Worker.h | grep -A30 'm_implLock'
echo
echo "== Verify: does WebWorker__updatePtr check for null? =="
rg -A10 'WebWorker__updatePtr' src/bun.js/web_worker.zig | head -30Repository: oven-sh/bun Length of output: 96 🏁 Script executed: #!/bin/bash
set -euo pipeflag
echo "== Worker.h - class definition and members =="
sed -n '50,130p' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Check m_implLock type =="
rg -B2 -A2 'm_implLock' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Worker::create implementation start =="
sed -n '160,210p' src/bun.js/bindings/webcore/Worker.cppRepository: oven-sh/bun Length of output: 108 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Worker.h - class definition and members =="
sed -n '50,130p' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Check m_implLock type =="
rg -B2 -A2 'm_implLock' src/bun.js/bindings/webcore/Worker.h
echo
echo "== Worker::create implementation start =="
sed -n '160,210p' src/bun.js/bindings/webcore/Worker.cppRepository: oven-sh/bun Length of output: 5782 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== WebWorker__updatePtr in web_worker.zig =="
rg -B5 -A15 'fn WebWorker__updatePtr' src/bun.js/web_worker.zig
echo
echo "== Verify: is there a null check in WebWorker__updatePtr? =="
rg -A20 'export fn WebWorker__updatePtr' src/bun.js/web_worker.zigRepository: oven-sh/bun Length of output: 1186 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== WebWorker__dispatchExit call from Zig worker thread =="
rg -B10 -A5 'WebWorker__dispatchExit' src/bun.js/web_worker.zig
echo
echo "== Full context: when does clearZigImpl happen? =="
sed -n '475,490p' src/bun.js/bindings/webcore/Worker.cpp
echo
echo "== Check: is Worker::create and updatePtr in same synchronous call? =="
sed -n '348,360p' src/bun.js/bindings/webcore/JSWorker.cppRepository: oven-sh/bun Length of output: 2492 Lock The new lock protections in 🤖 Prompt for AI Agents
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| bool Worker::updatePtr() | ||
| { | ||
| if (!WebWorker__updatePtr(impl_, this)) { | ||
| clearZigImpl(); | ||
| m_onlineClosingFlags = ClosingFlag; | ||
| m_terminationFlags.fetch_or(TerminatedFlag); | ||
| return false; | ||
|
|
@@ -263,7 +272,9 @@ void Worker::terminate() | |
| { | ||
| // m_contextProxy.terminateWorkerGlobalScope(); | ||
| m_terminationFlags.fetch_or(TerminateRequestedFlag); | ||
| WebWorker__notifyNeedTermination(impl_); | ||
| Locker locker { m_implLock }; | ||
| if (impl_) | ||
| WebWorker__notifyNeedTermination(impl_); | ||
| } | ||
|
|
||
| // const char* Worker::activeDOMObjectName() const | ||
|
|
@@ -468,6 +479,8 @@ void Worker::forEachWorker(const Function<Function<void(ScriptExecutionContext&) | |
| extern "C" void WebWorker__dispatchExit(Zig::GlobalObject* globalObject, Worker* worker, int32_t exitCode) | ||
| { | ||
| worker->dispatchExit(exitCode); | ||
| // The Zig WebWorker is about to be freed; prevent terminate()/ref()/unref() from touching it. | ||
| worker->clearZigImpl(); | ||
| // no longer referenced by Zig | ||
| worker->deref(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import { describe, expect, mock, test } from "bun:test"; | ||
|
|
||
| describe("mock() used as a constructor", () => { | ||
| test("Reflect.construct with no implementation returns an object", () => { | ||
| const m = mock(); | ||
| const r = Reflect.construct(m, []); | ||
| expect(typeof r).toBe("object"); | ||
| expect(r).not.toBeNull(); | ||
| }); | ||
|
|
||
| test("Reflect.construct with a different newTarget returns an object", () => { | ||
| const m = mock(); | ||
| const r = Reflect.construct(m, [], function () {}); | ||
| expect(typeof r).toBe("object"); | ||
| expect(r).not.toBeNull(); | ||
| }); | ||
|
|
||
| test("Reflect.construct with implementation returning a primitive returns an object", () => { | ||
| const m = mock(() => 42); | ||
| const r = Reflect.construct(m, []); | ||
| expect(typeof r).toBe("object"); | ||
| expect(r).not.toBeNull(); | ||
| }); | ||
|
|
||
| test("Reflect.construct with mockReturnValue(primitive) returns an object", () => { | ||
| const m = mock().mockReturnValue(7); | ||
| const r = Reflect.construct(m, []); | ||
| expect(typeof r).toBe("object"); | ||
| expect(r).not.toBeNull(); | ||
| }); | ||
|
|
||
| test("Reflect.construct preserves object return values", () => { | ||
| const obj = { hello: "world" }; | ||
| const m = mock(() => obj); | ||
| const r = Reflect.construct(m, []); | ||
| expect(r).toBe(obj); | ||
| }); | ||
|
|
||
| test("calling without new still returns the implementation's value", () => { | ||
| const m1 = mock(); | ||
| expect(m1()).toBeUndefined(); | ||
| const m2 = mock(() => 42); | ||
| expect(m2()).toBe(42); | ||
| const m3 = mock().mockReturnValue("x"); | ||
| expect(m3()).toBe("x"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
|
|
||
| describe("Worker", () => { | ||
| test("terminate()/ref()/unref() after the worker has exited does not crash", async () => { | ||
| const url = URL.createObjectURL(new Blob(["/* exits immediately */"], { type: "application/javascript" })); | ||
| try { | ||
| const workers: Worker[] = []; | ||
| for (let i = 0; i < 4; i++) { | ||
| const w = new Worker(url); | ||
| workers.push(w); | ||
| } | ||
|
|
||
| await Promise.all( | ||
| workers.map(w => { | ||
| const { promise, resolve } = Promise.withResolvers<void>(); | ||
| w.addEventListener("close", () => resolve(), { once: true }); | ||
| return promise; | ||
| }), | ||
| ); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| Bun.gc(true); | ||
|
|
||
| for (const w of workers) { | ||
| w.terminate(); | ||
| w.ref(); | ||
| w.unref(); | ||
| w.terminate(); | ||
| } | ||
| } finally { | ||
| URL.revokeObjectURL(url); | ||
| } | ||
|
|
||
| expect(true).toBe(true); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
In ECMAScript, when Reflect.construct(target, args, newTarget) calls a constructor that returns a primitive, should the result be the original receiver created from newTarget.prototype or a fresh empty object? For JavaScriptCore native constructor host functions, is CallFrame::thisValue() that pre-created receiver?💡 Result:
In ECMAScript, Reflect.construct(target, args, newTarget) invokes the [[Construct]] internal method of target with arguments args and newTarget. For ordinary ECMAScript function objects, [[Construct]] (10.2.2) creates thisArgument via OrdinaryCreateFromConstructor(newTarget, "%Object.prototype%"), which uses newTarget.prototype (or fallback) as [[Prototype]], then calls the function body with this bound to that object. After execution, if the constructor returns a primitive (including undefined), it ignores the return value and returns thisArgument—the pre-created object from newTarget.prototype—for base constructors. For derived constructors, it throws if non-object/non-undefined is returned. Thus, the result is always the original receiver created from newTarget.prototype. For JavaScriptCore native constructor host functions (built-in functions), CallFrame::thisValue holds the pre-created receiver. Per CallFrame.h comment: "Under the constructor implemented in C++, thisValue holds the newTarget instead of the automatically constructed value. The result of this function is only effective under the "construct" context. JSValue newTarget const { return thisValue; }". However, this indicates newTarget is accessible via thisValue in C++ host constructors, but the actual receiver (this binding) is the pre-created object from OrdinaryCreateFromConstructor or equivalent, passed as thisArgument to the host function per spec BuiltinCallOrConstruct (10.3.3). The comment clarifies special handling for C++ implemented constructors where thisValue slot serves dual purpose, but standard behavior provides the pre-created empty object as this.
Citations:
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 23495
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 16827
🏁 Script executed:
Repository: oven-sh/bun
Length of output: 1546
Return the constructor receiver instead of allocating a fresh object.
The current implementation violates ECMAScript [[Construct]] semantics. When the mock implementation returns a primitive,
constructEmptyObject()creates a fresh plain object, discarding the receiver that was pre-created fromnewTarget.prototype. Per the spec, [[Construct]] must return the pre-created receiver. The fix is to usecallframe->thisValue()(the JSC equivalent of the spec's pre-created receiver) with a fallback toconstructEmptyObject()for defensive safety.💡 Proposed fix
JSC_DEFINE_HOST_FUNCTION(jsMockFunctionConstruct, (JSGlobalObject * lexicalGlobalObject, CallFrame* callframe)) { auto& vm = JSC::getVM(lexicalGlobalObject); auto scope = DECLARE_THROW_SCOPE(vm); JSC::EncodedJSValue encodedResult = jsMockFunctionCall(lexicalGlobalObject, callframe); RETURN_IF_EXCEPTION(scope, {}); JSValue result = JSValue::decode(encodedResult); - if (!result.isObject()) - return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)); + if (!result.isObject()) { + JSValue thisValue = callframe->thisValue(); + if (thisValue.isObject()) + return JSValue::encode(thisValue); + return JSValue::encode(JSC::constructEmptyObject(lexicalGlobalObject)); + } return encodedResult; }📝 Committable suggestion
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For
InternalFunctionnative constructors in JSC,callframe->thisValue()isnewTarget()(see the comment inCallFrame.hyou quoted) — there is no pre-created receiver. ReturningthisValue()would return the newTarget constructor itself, not an instance.Updated in 98a869b to use
InternalFunction::createSubclassStructureso the fallback object inherits fromnewTarget.prototype, matching other native constructors in the codebase. Added a test forinstanceof.