Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -319,9 +319,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 @@ -331,9 +328,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
8 changes: 5 additions & 3 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5596,10 +5596,12 @@ 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 found = object->getPropertySlot(globalObject, property, slot);
// Ignore exceptions from "Get" proxy traps and throwing lazy
// property initializers (which report the slot as not found).
Comment on lines +5600 to +5601

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

CLEAR_IF_EXCEPTION(scope);
if (!found)
continue;

if ((slot.attributes() & PropertyAttribute::DontEnum) != 0) {
if (property == propertyNames->underscoreProto
Expand Down
51 changes: 51 additions & 0 deletions test/js/bun/util/inspect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -928,3 +928,54 @@
expect(exitCode).toBe(0);
});
});

it("skips properties whose lazy initializer throws instead of leaving the exception pending", async () => {
// Overwriting the global Symbol breaks the lazily-initialized Bun.$ and
// Bun.sql properties (their module-scope code calls Symbol). Inspecting the
// Bun object reifies every property; a throwing initializer has to be
// skipped, not left as a pending exception, which aborts assert-enabled
// builds.
const code = `
Symbol++;
const out = Bun.inspect(Bun);
if (typeof out !== "string" || out.length === 0) throw new Error("empty inspect output");
console.log("ok");
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);

Check warning on line 950 in test/js/bun/util/inspect.test.js

View check run for this annotation

Claude / Claude Code Review

New subprocess tests pipe stderr but never drain it

Both new subprocess tests set `stderr: "pipe"` but never drain it — only `proc.stdout.text()` and `proc.exited` are awaited. REVIEW.md's "Subprocess tests: drain pipes concurrently" rule and the neighboring tests in this file both call for `Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited])`; add `proc.stderr.text()` here and at line 978, or drop `stderr: "pipe"`.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
expect(stdout).toBe("ok\n");
expect(exitCode).toBe(0);
});

it("building an invalid-argument error message survives lazy initializers that throw", async () => {
// Same walk reached through ERR_INVALID_ARG_VALUE rendering the received
// value: new CompressionStream(globalThis) inspects globalThis for its
// error message while the global Symbol is broken. Getters that report
// their failure can set a nonzero exit code, so only assert that execution
// gets past the constructor instead of aborting.
const code = `
Symbol++;
let caught;
try {
new CompressionStream(globalThis);
} catch (e) {
caught = e;
}
if (!caught) throw new Error("expected CompressionStream to throw");
console.log("ok");
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout] = await Promise.all([proc.stdout.text(), proc.exited]);
expect(stdout).toBe("ok\n");
expect(proc.signalCode).toBeNull();
});