Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/jsc/bindings/BunObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -320,9 +320,6 @@ static JSValue defaultBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, vm.propertyNames->defaultKeyword));
}
Expand All @@ -332,9 +329,6 @@ static JSValue constructBunSQLObject(VM& vm, JSObject* bunObject)
auto scope = DECLARE_THROW_SCOPE(vm);
auto* globalObject = defaultGlobalObject(bunObject->globalObject());
JSValue sqlValue = globalObject->internalModuleRegistry()->requireId(globalObject, vm, InternalModuleRegistry::BunSql);
#if BUN_DEBUG
if (scope.exception()) globalObject->reportUncaughtExceptionAtEventLoop(globalObject, scope.exception());
#endif
RETURN_IF_EXCEPTION(scope, {});
auto clientData = WebCore::clientData(vm);
RELEASE_AND_RETURN(scope, sqlValue.getObject()->get(globalObject, clientData->builtinNames().SQLPublicName()));
Expand Down
14 changes: 10 additions & 4 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5643,10 +5643,11 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
}

JSC::PropertySlot slot(object, PropertySlot::InternalMethodType::Get);
if (!object->getPropertySlot(globalObject, property, slot))
continue;
// Ignore exceptions from "Get" proxy traps.
bool hasProperty = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps and lazy initializers; they also report the property as not found.
CLEAR_IF_EXCEPTION(scope);
if (!hasProperty)
continue;

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
Expand Down Expand Up @@ -5718,7 +5719,12 @@ static void JSC__JSValue__forEachPropertyImpl(JSC::EncodedJSValue JSValue0, JSC:
break;
if (iterating == globalObject)
break;
iterating = iterating->getPrototype(globalObject).getObject();
JSValue prototype = iterating->getPrototype(globalObject);
// Ignore exceptions from Proxy "getPrototypeOf" trap.
CLEAR_IF_EXCEPTION(scope);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably not be clearing these exceptions, but looks like it was doing it originally

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!prototype)
break;
iterating = prototype.getObject();
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/jsc/bindings/napi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2077,14 +2077,18 @@ extern "C" napi_status napi_get_all_property_names(
if (key_mode == napi_key_include_prototypes) {
// Climb up the prototype chain to find inherited properties
while (!owner->getOwnPropertyDescriptor(globalObject, propKey, desc)) {
JSObject* proto = owner->getPrototype(globalObject).getObject();
NAPI_RETURN_IF_EXCEPTION(env);
JSValue protoValue = owner->getPrototype(globalObject);
NAPI_RETURN_IF_EXCEPTION(env);
JSObject* proto = protoValue ? protoValue.getObject() : nullptr;
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
if (!proto) {
break;
}
owner = proto;
}
} else {
owner->getOwnPropertyDescriptor(globalObject, propKey, desc);
NAPI_RETURN_IF_EXCEPTION(env);
}

// V8 never applies ONLY_WRITABLE/ONLY_CONFIGURABLE to Proxy keys
Expand Down
42 changes: 42 additions & 0 deletions test/js/bun/util/BunObject.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { env } from "bun";
import { hasNonReifiedStatic } from "bun:internal-for-testing";
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";
test("hasNonReifiedStatic", () => {
expect(hasNonReifiedStatic(Bun), "do not eagerly initialize the Bun object. This will make Bun much slower.").toBe(
true,
Expand Down Expand Up @@ -33,3 +34,44 @@ test("await import('bun')", async () => {
}
expect(BunESM.default).toBe(Bun);
});

test("a lazy property whose builtin fails to load throws from the read", async () => {
// The shell builtin ($) and the sql module body (sql, SQL, postgres) call Symbol(), so
// breaking it makes each builder throw. The read must throw that error (debug builds used to
// report the still-pending exception from inside the sql builders and abort) and the slot
// must stay unreified so a later read runs the builder again.
//
// process.env is read first because the shell builtin reads it before calling Symbol(), and
// building it on Windows reifies another property of the Bun object; doing that in the middle
// of the throwing read trips a separate structure assertion in debug builds.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
`process.env;
globalThis.Symbol = NaN;
const results = {};
for (const name of ["$", "sql", "SQL", "postgres"]) {
results[name] = [];
for (let i = 0; i < 2; i++) {
try { Bun[name]; results[name].push("no throw"); } catch (e) { results[name].push(e.constructor.name); }
}
}
console.log(JSON.stringify(results));`,
],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({
stdout: JSON.stringify({
$: ["TypeError", "TypeError"],
sql: ["TypeError", "TypeError"],
SQL: ["TypeError", "TypeError"],
postgres: ["TypeError", "TypeError"],
}),
stderr: "",
exitCode: 0,
});
});
94 changes: 93 additions & 1 deletion test/js/bun/util/inspect.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from "bun:test";
import { bunEnv, bunExe, isASAN, isWindows, normalizeBunSnapshot, tmpdirSync } from "harness";
import { bunEnv, bunExe, isASAN, isWindows, normalizeBunSnapshot, tempDir, tmpdirSync } from "harness";
import { join } from "path";
import util from "util";
it("prototype", () => {
Expand Down Expand Up @@ -579,6 +579,98 @@ it("Bun.inspect huge sparse array summarizes holes without iterating them", asyn
});
});

// A property lookup that throws while an object is being formatted (a Proxy trap in the
// prototype chain, a lazily initialized property whose initializer throws, a module namespace
// export that is still in its temporal dead zone) used to leave the exception pending: the
// lookups of the following properties failed and were dropped from the output or the formatter
// rethrew the exception from the next property, debug builds asserted, and moving on to the
// next prototype dereferenced the empty value returned by the throwing getPrototype. Each case
// runs in a child so a regression fails the test instead of taking down the runner.
describe.concurrent("Bun.inspect when a property lookup throws", () => {
async function runChild(args, cwd) {
await using proc = Bun.spawn({
cmd: [bunExe(), ...args],
env: bunEnv,
cwd,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
}
const inspectInChild = code => runChild(["-e", code]);

it("skips a prototype property whose Proxy get trap throws and keeps the rest", async () => {
const result = await inspectInChild(`
const proto = new Proxy({ a: 1, b: 2, c: 3 }, {
get(target, key, receiver) {
if (key === "b") throw new Error("get trap");
return Reflect.get(target, key, receiver);
},
});
const obj = Object.create(proto);
obj.own = 0;
console.log(Bun.inspect(obj));
`);
expect(result).toEqual({ stdout: "{\n own: 0,\n a: 1,\n c: 3,\n}\n", stderr: "", exitCode: 0 });
});

it("skips a prototype getter that throws behind a Proxy and keeps the rest", async () => {
const result = await inspectInChild(`
const proto = new Proxy({ a: 1, get b() { throw new Error("getter"); }, c: 3 }, {});
console.log(Bun.inspect(Object.create(proto)));
`);
expect(result).toEqual({ stdout: "{\n a: 1,\n c: 3,\n}\n", stderr: "", exitCode: 0 });
});

it("stops walking the prototype chain at a Proxy whose getPrototypeOf trap throws", async () => {
const result = await inspectInChild(`
const proto = new Proxy({ a: 1 }, {
getPrototypeOf() {
throw new Error("getPrototypeOf trap");
},
});
const obj = Object.create(proto);
obj.own = 0;
console.log(Bun.inspect(obj));
`);
expect(result).toEqual({ stdout: "{\n own: 0,\n a: 1,\n}\n", stderr: "", exitCode: 0 });
});

it("skips a lazily initialized Bun property whose initializer throws and keeps the rest", async () => {
// Bun.$ is the first property of the Bun object and is built by a builtin that calls
// Symbol(), as are Bun.sql and Bun.SQL further down, so breaking Symbol makes those
// initializers throw while Bun is formatted. Custom inspect functions (Bun.env has one on
// Windows) load node:util the first time one runs, which also needs Symbol, so load it first.
const result = await inspectInChild(`
Bun.inspect({ [Bun.inspect.custom]() { return ""; } });
globalThis.Symbol = 0;
const out = Bun.inspect(Bun);
console.log(JSON.stringify(["$", "Archive", "version"].map(key => out.includes("\\n " + key + ": "))));
`);
expect(result).toEqual({ stdout: "[false,true,true]\n", stderr: "", exitCode: 0 });
});

it("skips a module namespace export that is in its temporal dead zone and keeps the rest", async () => {
// b.mjs runs while a.mjs is still evaluating, so reading `later` off the namespace throws a
// ReferenceError. console.log used to rethrow it; util.inspect prints such an export as
// `<uninitialized>`, this formatter leaves it out.
using dir = tempDir("inspect-tdz-namespace", {
"a.mjs": `
import "./b.mjs";
export const later = 1;
export function hoisted() {}
`,
"b.mjs": `
import * as a from "./a.mjs";
console.log(a);
`,
});
const result = await runChild(["a.mjs"], String(dir));
expect(result).toEqual({ stdout: "Module {\n hoisted: [Function: hoisted],\n}\n", stderr: "", exitCode: 0 });
});
});

describe("console.logging function displays async and generator names", async () => {
const cases = [
function () {},
Expand Down
14 changes: 13 additions & 1 deletion test/napi/napi-app/js_test_helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,8 @@ static napi_value create_latin1_string(const Napi::CallbackInfo &info) {
}

// get_all_property_names(object, key_mode, key_filter, key_conversion)
// returns { status, keys }
// returns { status, keys, exception }; a pending exception is cleared and
// returned as `exception` so callers can observe `status` alongside it.
static napi_value get_all_property_names(const Napi::CallbackInfo &info) {
napi_env env = info.Env();
napi_value object = info[0];
Expand All @@ -391,6 +392,15 @@ static napi_value get_all_property_names(const Napi::CallbackInfo &info) {
static_cast<napi_key_filter>(key_filter),
static_cast<napi_key_conversion>(key_conversion), &keys);

bool is_pending = false;
NODE_API_CALL(env, napi_is_exception_pending(env, &is_pending));
napi_value exception;
if (is_pending) {
NODE_API_CALL(env, napi_get_and_clear_last_exception(env, &exception));
} else {
NODE_API_CALL(env, napi_get_undefined(env, &exception));
}

napi_value result;
NODE_API_CALL(env, napi_create_object(env, &result));
napi_value status_val;
Expand All @@ -400,6 +410,8 @@ static napi_value get_all_property_names(const Napi::CallbackInfo &info) {
NODE_API_CALL(env, napi_get_undefined(env, &keys));
}
NODE_API_CALL(env, napi_set_named_property(env, result, "keys", keys));
NODE_API_CALL(env,
napi_set_named_property(env, result, "exception", exception));
return result;
}

Expand Down
69 changes: 69 additions & 0 deletions test/napi/napi-app/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,75 @@ nativeTests.test_get_all_property_names_proxy_and_string_wrapper = () => {
show("frozen writable:", apn(Object.freeze({ a: 1, b: 2 }), napi_key_writable));
};

nativeTests.test_get_all_property_names_throwing_proxy_traps = () => {
const napi_key_include_prototypes = 0;
const napi_key_own_only = 1;
const napi_key_enumerable = 1 << 1;
const napi_key_keep_numbers = 0;

const show = (label, { status, keys, exception }) =>
console.log(label, `status=${status}`, `keys=${JSON.stringify(keys)}`, `exception=${exception?.message}`);

// ownKeys succeeds so key collection completes; the per-key descriptor walk
// required by napi_key_enumerable is what invokes the throwing trap.
const throwingDescriptor = new Proxy(
{},
{
ownKeys: () => ["a"],
getOwnPropertyDescriptor() {
throw new Error("gopd trap");
},
},
);
show(
"own_only gopd throws:",
nativeTests.get_all_property_names(
throwingDescriptor,
napi_key_own_only,
napi_key_enumerable,
napi_key_keep_numbers,
),
);
show(
"include_prototypes gopd throws on prototype:",
nativeTests.get_all_property_names(
Object.create(throwingDescriptor),
napi_key_include_prototypes,
napi_key_enumerable,
napi_key_keep_numbers,
),
);

};

nativeTests.test_get_all_property_names_get_prototype_throws_in_descriptor_walk = () => {
const napi_key_include_prototypes = 0;
const napi_key_enumerable = 1 << 1;
const napi_key_keep_numbers = 0;

// Key collection asks the proxy for its prototype once and must succeed so
// the descriptor walk is reached; the walk asks again (the target does not
// own "a") and that second call throws.
let calls = 0;
const proxy = new Proxy(
{},
{
ownKeys: () => ["a"],
getPrototypeOf() {
if (calls++ > 0) throw new Error("getPrototypeOf trap");
return null;
},
},
);
const { status, keys, exception } = nativeTests.get_all_property_names(
Object.create(proxy),
napi_key_include_prototypes,
napi_key_enumerable,
napi_key_keep_numbers,
);
console.log(`status=${status} keys=${JSON.stringify(keys)} exception=${exception?.message} calls=${calls}`);
};

nativeTests.test_set_property = () => {
const objects = [
{},
Expand Down
16 changes: 16 additions & 0 deletions test/napi/napi.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,22 @@ describe.concurrent.skipIf(!canBuildNodeAddons())("napi", () => {
it("handles accessor properties when filtering by napi_key_writable", async () => {
await checkSameOutput("test_get_all_property_names_accessor", []);
});
it("returns napi_pending_exception when a Proxy trap throws during the descriptor walk", async () => {
const output = await checkSameOutput("test_get_all_property_names_throwing_proxy_traps", []);
expect(output).toContain("own_only gopd throws: status=10 keys=undefined exception=gopd trap");
expect(output).toContain(
"include_prototypes gopd throws on prototype: status=10 keys=undefined exception=gopd trap",
);
});
it("returns napi_pending_exception when getPrototypeOf throws during the descriptor walk", async () => {
// Not checkSameOutput: V8 filters proxy keys while collecting them and
// only calls getPrototypeOf once, so a trap that throws on the second
// call never throws under Node.
const output = (await runOn(bunExe(), "test_get_all_property_names_get_prototype_throws_in_descriptor_walk", []))
.replaceAll(/^\[\w+\].+$/gm, "")
.trim();
expect(output).toBe("status=10 keys=undefined exception=getPrototypeOf trap calls=2");
});
it("matches Node for Proxy and String wrapper with napi_key_writable/napi_key_configurable", async () => {
const output = await checkSameOutput("test_get_all_property_names_proxy_and_string_wrapper", []);
expect(output).toContain(`proxy own_only writable: status=0 keys=["x","y"]`);
Expand Down
Loading