From dbec96ca1d7af095327287eaf6cf34c946a0da22 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sat, 23 Nov 2024 04:26:00 -0800 Subject: [PATCH] Clean up some error handling code --- src/bun.js/bindings/bindings.cpp | 5 +- src/bun.js/bindings/bindings.zig | 107 ++++++++++++++--------------- src/crash_handler.zig | 111 ++++++++++++++++--------------- src/string.zig | 8 ++- 4 files changed, 118 insertions(+), 113 deletions(-) diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index f80dd361480077..b813c614cfec43 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -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 })); diff --git a/src/bun.js/bindings/bindings.zig b/src/bun.js/bindings/bindings.zig index af62ff0b897e48..1b7e8acbcbffa3 100644 --- a/src/bun.js/bindings/bindings.zig +++ b/src/bun.js/bindings/bindings.zig @@ -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; } @@ -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 @@ -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 { @@ -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); @@ -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 }); @@ -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 @@ -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; @@ -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; } @@ -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; } diff --git a/src/crash_handler.zig b/src/crash_handler.zig index d9cb08f9899240..c0ea3aafaf60cf 100644 --- a/src/crash_handler.zig +++ b/src/crash_handler.zig @@ -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: - \\ - \\ {} - \\ - , - .{ts}, - ); - } else { - Output.prettyErrorln( - "trace: error.{s}: {}", - .{ @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: + \\ + \\ {} + \\ + , + .{ts}, + ); + } else { + Output.prettyErrorln( + "trace: error.{s}: {}", + .{ @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). diff --git a/src/string.zig b/src/string.zig index d47cc49cf0a9ef..3526f67f03989b 100644 --- a/src/string.zig +++ b/src/string.zig @@ -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; } } @@ -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; } }