-
Notifications
You must be signed in to change notification settings - Fork 172
Port recent OOM-handling and memory leak fixes from bellard/quickjs #1069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port recent OOM-handling and memory leak fixes from bellard/quickjs #1069
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment, otherwise LGTM.
quickjs.c
Outdated
|
||
if (add_backtrace) | ||
build_backtrace(ctx, obj, JS_UNDEFINED, NULL, 0, 0, 0); | ||
} | ||
if (add_backtrace) | ||
build_backtrace(ctx, obj, JS_UNDEFINED, NULL, 0, 0, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks kind of dubious to me. Ceteris paribus, it's better to have a backtrace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the reason this was part of the upstream commit is that all but one (?) of the reasons JS_NewString
(JS_NewStringLen
) might fail are due to an allocation / OOM situation.
EDIT: And since the case of an invalid message is already handled above:
msg = JS_NewString(ctx, message);
if (JS_IsException(msg))
msg = JS_NewString(ctx, "Invalid error message");
The only case where msg
is still an exception at this point should be if JS_NewStringLen
failed due to allocation failure (OOM). Avoiding the call to build_backtrace in this case seems reasonable (it looks like it tries to allocate plenty of things itself)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is that it's better to try and fail than not try at all, so can you move it outside the block again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is that it's better to try and fail than not try at all, so can you move it outside the block again?
Done. 👍
Of note: Since it has diverged from bellard/quickjs, I did not fully audit build_backtrace
to ensure it properly handles all memory allocation failures internally - would be worth looking at in the future.
Port changes from bellard/quickjs@3dc7ef1 by Fabrice Bellard (except changes to build_backtrace) Co-Authored-By: Fabrice Bellard <[email protected]>
d083b48
to
d221d5a
Compare
Fixes #1066
Ports:
bellard/quickjs@db3d3f0
bellard/quickjs@3dc7ef1 (except changes to build_backtrace)