Skip to content

Commit

Permalink
Clean up some error handling code
Browse files Browse the repository at this point in the history
  • Loading branch information
@Jarred-Sumner
Jarred-Sumner committed Nov 23, 2024
1 parent f855ae8 commit dbec96c
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 113 deletions.
5 changes: 3 additions & 2 deletions src/bun.js/bindings/bindings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3793,8 +3793,9 @@ JSC__JSValue JSC__JSValue__getIfPropertyExistsImpl(JSC__JSValue JSValue0,

JSC::VM& vm = globalObject->vm();
JSC::JSObject* object = value.getObject();
if (UNLIKELY(!object))
return JSValue::encode({});
if (UNLIKELY(!object)) {
return JSValue::encode(JSValue::decode(JSC::JSValue::ValueDeleted));
}

// Since Identifier might not ref' the string, we need to ensure it doesn't get deref'd until this function returns
const auto propertyString = String(StringImpl::createWithoutCopying({ arg1, arg2 }));
Expand Down
107 changes: 50 additions & 57 deletions src/bun.js/bindings/bindings.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3084,13 +3084,18 @@ pub const JSGlobalObject = opaque {
);

if (possible_errors.OutOfMemory and err == error.OutOfMemory) {
bun.assert(!global.hasException()); // dual exception
global.throwOutOfMemory();
if (!global.hasException()) {
if (comptime bun.Environment.isDebug) bun.Output.panic("attempted to throw OutOfMemory without an exception", .{});
global.throwOutOfMemory();
}
return null_value;
}

if (possible_errors.JSError and err == error.JSError) {
bun.assert(global.hasException()); // Exception was cleared, yet returned.
if (!global.hasException()) {
if (comptime bun.Environment.isDebug) bun.Output.panic("attempted to throw JSError without an exception", .{});
global.throwOutOfMemory();
}
return null_value;
}

Expand Down Expand Up @@ -3736,11 +3741,19 @@ pub const JSValueReprInt = i64;
/// ABI-compatible with EncodedJSValue
/// In the future, this type will exclude `zero`, encoding it as `error.JSError` instead.
pub const JSValue = enum(i64) {
zero = 0,
undefined = 0xa,
null = 0x2,
true = FFI.TrueI64,
false = 0x6,

/// Typically means an exception was thrown.
zero = 0,

/// JSValue::ValueDeleted
///
/// Deleted is a special encoding used in JSC hash map internals used for
/// the null state. It is re-used here for encoding the "not present" state.
property_does_not_exist_on_object = 0x4,
_,

/// When JavaScriptCore throws something, it returns a null cell (0). The
Expand Down Expand Up @@ -5270,17 +5283,25 @@ pub const JSValue = enum(i64) {
// `this` must be known to be an object
// intended to be more lightweight than ZigString.
pub fn fastGet(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) ?JSValue {
if (bun.Environment.allow_assert)
if (bun.Environment.isDebug)
bun.assert(this.isObject());
const result = JSC__JSValue__fastGet(this, global, @intFromEnum(builtin_name)).legacyUnwrap();
if (result == .zero or
// JS APIs treat {}.a as mostly the same as though it was not defined
result == .undefined)
{
return null;
}

return result;
return switch (JSC__JSValue__fastGet(this, global, @intFromEnum(builtin_name))) {
.zero, .undefined, .property_does_not_exist_on_object => null,
else => |val| val,
};
}

pub fn fastGetWithError(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) JSError!?JSValue {
if (bun.Environment.isDebug)
bun.assert(this.isObject());

return switch (JSC__JSValue__fastGet(this, global, @intFromEnum(builtin_name))) {
.zero => error.JSError,
.undefined => null,
.property_does_not_exist_on_object => null,
else => |val| val,
};
}

pub fn fastGetDirect(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) ?JSValue {
Expand All @@ -5292,7 +5313,7 @@ pub const JSValue = enum(i64) {
return result;
}

extern fn JSC__JSValue__fastGet(value: JSValue, global: *JSGlobalObject, builtin_id: u8) GetResult;
extern fn JSC__JSValue__fastGet(value: JSValue, global: *JSGlobalObject, builtin_id: u8) JSValue;
extern fn JSC__JSValue__fastGetOwn(value: JSValue, globalObject: *JSGlobalObject, property: BuiltinName) JSValue;
pub fn fastGetOwn(this: JSValue, global: *JSGlobalObject, builtin_name: BuiltinName) ?JSValue {
const result = JSC__JSValue__fastGetOwn(this, global, builtin_name);
Expand All @@ -5307,42 +5328,7 @@ pub const JSValue = enum(i64) {
return cppFn("fastGetDirect_", .{ this, global, builtin_name });
}

/// Problem: The `get` needs to model !?JSValue
/// - null -> the property does not exist
/// - error -> the get operation threw
/// - any other JSValue -> success. this could be jsNull() or jsUndefined()
///
/// `.zero` is already used for the error state
///
/// Deleted is a special encoding used in JSC hash map internals used for
/// the null state. It is re-used here for encoding the "not present" state.
const GetResult = enum(i64) {
thrown_exception = 0,
does_not_exist = 0x4, // JSC::JSValue::ValueDeleted
_,

fn legacyUnwrap(value: GetResult) ?JSValue {
return switch (value) {
// footgun! caller must check hasException on every `get` or else Bun will crash
.thrown_exception => null,

.does_not_exist => null,
else => @enumFromInt(@intFromEnum(value)),
};
}

fn unwrap(value: GetResult, global: *JSGlobalObject) JSError!?JSValue {
return switch (value) {
.thrown_exception => {
bun.assert(global.hasException());
return error.JSError;
},
.does_not_exist => null,
else => @enumFromInt(@intFromEnum(value)),
};
}
};
extern fn JSC__JSValue__getIfPropertyExistsImpl(target: JSValue, global: *JSGlobalObject, ptr: [*]const u8, len: u32) GetResult;
extern fn JSC__JSValue__getIfPropertyExistsImpl(target: JSValue, global: *JSGlobalObject, ptr: [*]const u8, len: u32) JSValue;

pub fn getIfPropertyExistsFromPath(this: JSValue, global: *JSGlobalObject, path: JSValue) JSValue {
return cppFn("getIfPropertyExistsFromPath", .{ this, global, path });
Expand Down Expand Up @@ -5391,7 +5377,10 @@ pub const JSValue = enum(i64) {
}
}

return JSC__JSValue__getIfPropertyExistsImpl(this, global, property.ptr, @intCast(property.len)).legacyUnwrap();
return switch (JSC__JSValue__getIfPropertyExistsImpl(this, global, property.ptr, @intCast(property.len))) {
.undefined, .zero, .property_does_not_exist_on_object => null,
else => |val| val,
};
}

/// Equivalent to `target[property]`. Calls userland getters/proxies. Can
Expand All @@ -5403,17 +5392,21 @@ pub const JSValue = enum(i64) {
/// marked `inline` to allow Zig to determine if `fastGet` should be used
/// per invocation.
pub inline fn get(target: JSValue, global: *JSGlobalObject, property: anytype) JSError!?JSValue {
if (bun.Environment.allow_assert) bun.assert(target.isObject());
if (bun.Environment.isDebug) bun.assert(target.isObject());
const property_slice: []const u8 = property; // must be a slice!

// This call requires `get2` to be `inline`
if (bun.isComptimeKnown(property_slice)) {
if (comptime BuiltinName.get(property_slice)) |builtin| {
return target.fastGet(global, builtin);
if (comptime BuiltinName.get(property_slice)) |builtin_name| {
return target.fastGetWithError(global, builtin_name);
}
}

return JSC__JSValue__getIfPropertyExistsImpl(target, global, property_slice.ptr, @intCast(property_slice.len)).unwrap(global);
return switch (JSC__JSValue__getIfPropertyExistsImpl(target, global, property_slice.ptr, @intCast(property_slice.len))) {
.zero => error.JSError,
.undefined, .property_does_not_exist_on_object => null,
else => |val| val,
};
}

extern fn JSC__JSValue__getOwn(value: JSValue, globalObject: *JSGlobalObject, propertyName: *const bun.String) JSValue;
Expand Down Expand Up @@ -5463,7 +5456,7 @@ pub const JSValue = enum(i64) {
pub fn getTruthyComptime(this: JSValue, global: *JSGlobalObject, comptime property: []const u8) bun.JSError!?JSValue {
if (comptime bun.ComptimeEnumMap(BuiltinName).has(property)) {
if (fastGet(this, global, @field(BuiltinName, property))) |prop| {
if (prop.isEmptyOrUndefinedOrNull()) return null;
if (!prop.toBoolean()) return null;
return prop;
}

Expand All @@ -5476,7 +5469,7 @@ pub const JSValue = enum(i64) {
// TODO: replace calls to this function with `getOptional`
pub fn getTruthy(this: JSValue, global: *JSGlobalObject, property: []const u8) bun.JSError!?JSValue {
if (try get(this, global, property)) |prop| {
if (prop.isEmptyOrUndefinedOrNull()) return null;
if (!prop.toBoolean()) return null;
return prop;
}

Expand Down
111 changes: 59 additions & 52 deletions src/crash_handler.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1449,66 +1449,73 @@ fn crash() noreturn {

pub var verbose_error_trace = false;

fn handleErrorReturnTraceExtra(err: anyerror, maybe_trace: ?*std.builtin.StackTrace, comptime is_root: bool) void {
if (!builtin.have_error_return_tracing) return;
if (!verbose_error_trace and !is_root) return;

if (maybe_trace) |trace| {
// The format of the panic trace is slightly different in debug
// builds Mainly, we demangle the backtrace immediately instead
// of using a trace string.
//
// To make the release-mode behavior easier to demo, debug mode
// checks for this CLI flag.
const is_debug = bun.Environment.isDebug and check_flag: {
for (bun.argv) |arg| {
if (bun.strings.eqlComptime(arg, "--debug-crash-handler-use-trace-string")) {
break :check_flag false;
}
noinline fn coldHandleErrorReturnTrace(err_int_workaround_for_zig_ccall_bug: std.meta.Int(.unsigned, @bitSizeOf(anyerror)), trace: *std.builtin.StackTrace, comptime is_root: bool) void {
@setCold(true);
const err = @errorFromInt(err_int_workaround_for_zig_ccall_bug);

// The format of the panic trace is slightly different in debug
// builds Mainly, we demangle the backtrace immediately instead
// of using a trace string.
//
// To make the release-mode behavior easier to demo, debug mode
// checks for this CLI flag.
const is_debug = bun.Environment.isDebug and check_flag: {
for (bun.argv) |arg| {
if (bun.strings.eqlComptime(arg, "--debug-crash-handler-use-trace-string")) {
break :check_flag false;
}
break :check_flag true;
};
}
break :check_flag true;
};

if (is_debug) {
if (is_root) {
if (verbose_error_trace) {
Output.note("Release build will not have this trace by default:", .{});
}
} else {
Output.note(
"caught error.{s}:",
.{@errorName(err)},
);
if (is_debug) {
if (is_root) {
if (verbose_error_trace) {
Output.note("Release build will not have this trace by default:", .{});
}
Output.flush();
dumpStackTrace(trace.*);
} else {
const ts = TraceString{
.trace = trace,
.reason = .{ .zig_error = err },
.action = .view_trace,
};
if (is_root) {
Output.prettyErrorln(
\\
\\To send a redacted crash report to Bun's team,
\\please file a GitHub issue using the link below:
\\
\\ <cyan>{}<r>
\\
,
.{ts},
);
} else {
Output.prettyErrorln(
"<cyan>trace<r>: error.{s}: <d>{}<r>",
.{ @errorName(err), ts },
);
}
Output.note(
"caught error.{s}:",
.{@errorName(err)},
);
}
Output.flush();
dumpStackTrace(trace.*);
} else {
const ts = TraceString{
.trace = trace,
.reason = .{ .zig_error = err },
.action = .view_trace,
};
if (is_root) {
Output.prettyErrorln(
\\
\\To send a redacted crash report to Bun's team,
\\please file a GitHub issue using the link below:
\\
\\ <cyan>{}<r>
\\
,
.{ts},
);
} else {
Output.prettyErrorln(
"<cyan>trace<r>: error.{s}: <d>{}<r>",
.{ @errorName(err), ts },
);
}
}
}

inline fn handleErrorReturnTraceExtra(err: anyerror, maybe_trace: ?*std.builtin.StackTrace, comptime is_root: bool) void {
if (!builtin.have_error_return_tracing) return;
if (!verbose_error_trace and !is_root) return;

if (maybe_trace) |trace| {
coldHandleErrorReturnTrace(@intFromError(err), trace, is_root);
}
}

/// In many places we catch errors, the trace for them is absorbed and only a
/// single line (the error name) is printed. When this is set, we will print
/// trace strings for those errors (or full stacks in debug builds).
Expand Down
8 changes: 6 additions & 2 deletions src/string.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,9 @@ pub const String = extern struct {
bun.assert(out.tag != .Dead);
return out;
} else {
bun.assert(globalObject.hasException());
if (comptime bun.Environment.isDebug) {
bun.assert(globalObject.hasException());
}
return error.JSError;
}
}
Expand All @@ -721,7 +723,9 @@ pub const String = extern struct {
if (BunString__fromJSRef(globalObject, value, &out)) {
return out;
} else {
bun.assert(globalObject.hasException());
if (comptime bun.Environment.isDebug) {
bun.assert(globalObject.hasException());
}
return error.JSError;
}
}
Expand Down

0 comments on commit dbec96c

Please sign in to comment.