Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor Error.stackTraceLimit #874

Merged
merged 3 commits into from
Feb 3, 2025
Merged

Refactor Error.stackTraceLimit #874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fc7734
Refactor Error.stackTraceLimit
saghul Feb 1, 2025
ac6c462
fixup!
saghul Feb 3, 2025
9f02332
fixup!
saghul Feb 3, 2025
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
58 changes: 34 additions & 24 deletions quickjs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ struct JSRuntime {
JSValue current_exception;
/* true if inside an out of memory error, to avoid recursing */
bool in_out_of_memory;
/* and likewise if inside Error.prepareStackTrace() */
bool in_prepare_stack_trace;
/* true if inside build_backtrace, to avoid recursing */
bool in_build_stack_trace;
/* true if inside JS_FreeRuntime */
bool in_free;

Expand Down Expand Up @@ -396,7 +396,7 @@ struct JSContext {
JSValue error_ctor;
JSValue error_back_trace;
JSValue error_prepare_stack;
int error_stack_trace_limit;
JSValue error_stack_trace_limit;
JSValue iterator_ctor;
JSValue iterator_proto;
JSValue async_iterator_proto;
Expand Down Expand Up @@ -2311,7 +2311,7 @@ JSContext *JS_NewContextRaw(JSRuntime *rt)
ctx->error_ctor = JS_NULL;
ctx->error_back_trace = JS_UNDEFINED;
ctx->error_prepare_stack = JS_UNDEFINED;
ctx->error_stack_trace_limit = 10;
ctx->error_stack_trace_limit = js_int32(10);
init_list_head(&ctx->loaded_modules);

JS_AddIntrinsicBasicObjects(ctx);
Expand Down Expand Up @@ -2431,6 +2431,7 @@ static void JS_MarkContext(JSRuntime *rt, JSContext *ctx,
JS_MarkValue(rt, ctx->error_ctor, mark_func);
JS_MarkValue(rt, ctx->error_back_trace, mark_func);
JS_MarkValue(rt, ctx->error_prepare_stack, mark_func);
JS_MarkValue(rt, ctx->error_stack_trace_limit, mark_func);
for(i = 0; i < rt->class_count; i++) {
JS_MarkValue(rt, ctx->class_proto[i], mark_func);
}
Expand Down Expand Up @@ -2499,6 +2500,7 @@ void JS_FreeContext(JSContext *ctx)
JS_FreeValue(ctx, ctx->error_ctor);
JS_FreeValue(ctx, ctx->error_back_trace);
JS_FreeValue(ctx, ctx->error_prepare_stack);
JS_FreeValue(ctx, ctx->error_stack_trace_limit);
for(i = 0; i < rt->class_count; i++) {
JS_FreeValue(ctx, ctx->class_proto[i]);
}
Expand Down Expand Up @@ -6673,26 +6675,43 @@ static void build_backtrace(JSContext *ctx, JSValue error_val, JSValue filter_fu
JSRuntime *rt;
JSCallSiteData csd[64];
uint32_t i;
double d;
int stack_trace_limit;

rt = ctx->rt;
if (rt->in_build_stack_trace)
return;
rt->in_build_stack_trace = true;

// Save exception because conversion to double may fail.
saved_exception = JS_GetException(ctx);

// Extract stack trace limit.
JS_ToFloat64(ctx, &d, ctx->error_stack_trace_limit);
if (isnan(d) || d < 0.0)
stack_trace_limit = 0;
else if (d > INT32_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to cast to double (but it doesn't hurt either, it's just a no-op)

The isinf(d) && d < 0.0 two lines up looks superfluous. You check d < 0.0 first and -Infinity < 0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Does the same apply to checking for positive infinity? It will be larger than INT32_MAX I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Does the same apply to checking for positive infinity? It will be larger than INT32_MAX I guess.

Good point. By an infinite amount, according to mathematicians.

stack_trace_limit = INT32_MAX;
else
stack_trace_limit = fabs(d);

// Restore current exception.
JS_Throw(ctx, saved_exception);
saved_exception = JS_UNINITIALIZED;
stack_trace_limit = ctx->error_stack_trace_limit;

stack_trace_limit = min_int(stack_trace_limit, countof(csd));
stack_trace_limit = max_int(stack_trace_limit, 0);
rt = ctx->rt;
has_prepare = false;
has_filter_func = backtrace_flags & JS_BACKTRACE_FLAG_FILTER_FUNC;
i = 0;

if (!rt->in_prepare_stack_trace && !JS_IsNull(ctx->error_ctor)) {
if (!JS_IsNull(ctx->error_ctor)) {
prepare = js_dup(ctx->error_prepare_stack);
has_prepare = JS_IsFunction(ctx, prepare);
rt->in_prepare_stack_trace = true;
}

if (has_prepare) {
saved_exception = rt->current_exception;
rt->current_exception = JS_UNINITIALIZED;
saved_exception = JS_GetException(ctx);
if (stack_trace_limit == 0)
goto done;
if (filename)
Expand Down Expand Up @@ -6816,8 +6835,7 @@ static void build_backtrace(JSContext *ctx, JSValue error_val, JSValue filter_fu
else
stack = stack2;
JS_FreeValue(ctx, prepare);
JS_FreeValue(ctx, rt->current_exception);
rt->current_exception = saved_exception;
JS_Throw(ctx, saved_exception);
} else {
if (dbuf_error(&dbuf))
stack = JS_NULL;
Expand All @@ -6826,7 +6844,6 @@ static void build_backtrace(JSContext *ctx, JSValue error_val, JSValue filter_fu
dbuf_free(&dbuf);
}

rt->in_prepare_stack_trace = false;
if (JS_IsUndefined(ctx->error_back_trace))
ctx->error_back_trace = js_dup(stack);
if (has_filter_func || can_add_backtrace(error_val)) {
Expand All @@ -6835,6 +6852,8 @@ static void build_backtrace(JSContext *ctx, JSValue error_val, JSValue filter_fu
} else {
JS_FreeValue(ctx, stack);
}

rt->in_build_stack_trace = false;
}

JSValue JS_NewError(JSContext *ctx)
Expand Down Expand Up @@ -38041,23 +38060,14 @@ static JSValue js_error_get_stackTraceLimit(JSContext *ctx, JSValue this_val)
if (JS_IsException(val))
return val;
JS_FreeValue(ctx, val);
return js_int32(ctx->error_stack_trace_limit);
return js_dup(ctx->error_stack_trace_limit);
}

static JSValue js_error_set_stackTraceLimit(JSContext *ctx, JSValue this_val, JSValue value)
{
if (JS_IsUndefined(this_val) || JS_IsNull(this_val))
return JS_ThrowTypeErrorNotAnObject(ctx);
double limit = 0;
if (JS_ToFloat64(ctx, &limit, value) < 0)
return JS_EXCEPTION;
if (isfinite(limit)) {
ctx->error_stack_trace_limit = limit;
} else if (isnan(limit) || limit == -INFINITY) {
ctx->error_stack_trace_limit = 0;
} else {
ctx->error_stack_trace_limit = INT32_MAX;
}
ctx->error_stack_trace_limit = js_dup(value);
return JS_UNDEFINED;
}

Expand Down
18 changes: 17 additions & 1 deletion tests/bug858.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
import {assert} from "./assert.js";

Error.stackTraceLimit = Infinity;
assert(Error.stackTraceLimit === Infinity);
assert(new Error("error").stack.includes("bug858.js")); // stack should not be empty

Error.stackTraceLimit = -Infinity;
assert(Error.stackTraceLimit === -Infinity);
assert(!new Error("error").stack.includes("bug858.js")); // stack should be empty

Error.stackTraceLimit = NaN;
assert(!new Error("error").stack.includes("bug858.js")); // stack should be empty
assert(Number.isNaN(Error.stackTraceLimit));
assert(!new Error("error").stack.includes("bug858.js")); // stack should be empty

Error.stackTraceLimit = -0;
assert(Object.is(Error.stackTraceLimit, -0));
assert(!new Error("error").stack.includes("bug858.js")); // stack should be empty

const obj = { valueOf() { throw "evil" } };
Error.stackTraceLimit = obj;
assert(Error.stackTraceLimit === obj);
try {
throw new Error("fail")
} catch (e) {
assert(e.message.includes("fail"));
}