Fix uncatchable error inside a promise (#810) #811
Conversation
|
It would be good to have a test for this. |
|
How to run all the tests? @saghul I just ran run-test262.exe tests.confResult: 0/29 errors, 3 excluded Cannot run test262.conf. BTW, I should add my test.js to tests directory right? |
|
This wouldn't be a 262 test, so yeah adding it to the tests/ directory is the way. |
|
I just realized that we need an interrupt handler to test it. So I can't do it in pure js. So would need a separate test or modify run-test262.c |
|
Yes it would likely need to be its own C file with the simplified test. |
|
Should I add Makefile and CMakeLists.txt target for this test? @saghul |
|
CMake should be the only requirement, since it shouldn't have EXCLUDE_FROM_ALL |
|
Looking good! Can you pl run the test from the CI files? Building it won't make it run. |
|
Sure! |
|
Updated. |
|
Can you please add it to (some of) the windows targets? |
|
No problem, I am working on it now! |
|
Updated. |
| return *time > MAX_TIME; | ||
| } | ||
|
|
||
| static void sync_call() |
There was a problem hiding this comment.
| static void sync_call() | |
| static void sync_call(void) |
In C before C23, () means a function taking zero or more arguments.
In C23, () means zero arguments - but quickjs is a C11 project so that doesn't apply.
| JS_FreeRuntime(rt); | ||
| } | ||
|
|
||
| static void async_call() |
There was a problem hiding this comment.
| static void async_call() | |
| static void async_call(void) |
| ret2 = JS_Call(ctx, s->resolving_funcs[1], JS_UNDEFINED, | ||
| 1, &error); |
There was a problem hiding this comment.
| ret2 = JS_Call(ctx, s->resolving_funcs[1], JS_UNDEFINED, | |
| 1, &error); | |
| ret2 = JS_Call(ctx, s->resolving_funcs[1], JS_UNDEFINED, | |
| 1, &error); |
Although you can also fit it on one line (fits in 80 columns):
| ret2 = JS_Call(ctx, s->resolving_funcs[1], JS_UNDEFINED, | |
| 1, &error); | |
| ret2 = JS_Call(ctx, s->resolving_funcs[1], JS_UNDEFINED, 1, &error); |
| if (JS_IsUncatchableError(ctx, ctx->rt->current_exception)) { | ||
| is_success = false; | ||
| } else { | ||
| abort(); /* BUG */ |
There was a problem hiding this comment.
Don't abort here. Resolving functions can fail in (at least but not restricted to) out-of-memory or stack overflow conditions, and those are catchable exceptions.
There was a problem hiding this comment.
I think there should be no catchable exception return here.
If the exception can be caught, then it must be handled by the resolving functions.
Which means JS_GetException must be called first in order to pass it to try-catch or .catch().
If JS_GetException is called, there's no reason for it to still return JS_EXCEPTION as the exception is free.
And I tested the stack overflow inside a promise/async, it's caught as expected, abort() is never triggered.
I prefer to treat it as a bug here. But if abort is not the appropriate way to do this, I will remove it.
There was a problem hiding this comment.
Alright, in the specific case of js_async_function_resume it's probably fine.
There was a problem hiding this comment.
Cool, then I will let it be. Will update the code tomorrow.
| if (!js_async_function_resume(ctx, s)) { | ||
| return JS_EXCEPTION; | ||
| } |
There was a problem hiding this comment.
GH seems to have swallowed my last review comment... I was saying that single-statement if statements (ones without an else clause) usually omit the braces:
| if (!js_async_function_resume(ctx, s)) { | |
| return JS_EXCEPTION; | |
| } | |
| if (!js_async_function_resume(ctx, s)) | |
| return JS_EXCEPTION; |
edit: applies here and below
Fix issue #810