Skip to content
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.

Sign up for GitHub

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

src: remove ClearFatalExceptionHandlers() #17333

Closed
wants to merge 5 commits into from
Closed

src: remove ClearFatalExceptionHandlers() #17333

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
703f474
src: remove `ClearFatalExceptionHandlers()`
addaleax Nov 27, 2017
9ca5ccb
[squash] TimothyGu comment
addaleax Nov 27, 2017
b41845a
[squash] danbev & bnoordhuis nits
addaleax Nov 27, 2017
64acf54
src: introduce USE() for silencing compiler warnings
addaleax Nov 27, 2017
a2e4472
[squash] TimothyGu nit
addaleax Nov 27, 2017
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
52 changes: 14 additions & 38 deletions src/async_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ RetainedObjectInfo* WrapperInfo(uint16_t class_id, Local<Value> wrapper) {
static void DestroyAsyncIdsCallback(Environment* env, void* data) {
Local<Function> fn = env->async_hooks_destroy_function();

TryCatch try_catch(env->isolate());
FatalTryCatch try_catch(env);

do {
std::vector<double> destroy_async_id_list;
Expand All @@ -153,11 +153,8 @@ static void DestroyAsyncIdsCallback(Environment* env, void* data) {
MaybeLocal<Value> ret = fn->Call(
env->context(), Undefined(env->isolate()), 1, &async_id_value);

if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
UNREACHABLE();
}
if (ret.IsEmpty())
return;
}
} while (!env->destroy_async_id_list()->empty());
}
Expand All @@ -171,14 +168,9 @@ void AsyncWrap::EmitPromiseResolve(Environment* env, double async_id) {

Local<Value> async_id_value = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_promise_resolve_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar = fn->Call(
env->context(), Undefined(env->isolate()), 1, &async_id_value);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
UNREACHABLE();
}
FatalTryCatch try_catch(env);
fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
.ToLocalChecked();
Copy link
Member

Choose a reason for hiding this comment

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

Should ToLocalChecked() be used here? It would make the program hard-crash before the FatalTryCatch got time to terminate the program with error message... right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimothyGu Yeah, you’re right. Updated :)

}


Expand All @@ -205,14 +197,9 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) {

Local<Value> async_id_value = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_before_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar = fn->Call(
env->context(), Undefined(env->isolate()), 1, &async_id_value);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
UNREACHABLE();
}
FatalTryCatch try_catch(env);
fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
.ToLocalChecked();
}


Expand Down Expand Up @@ -241,14 +228,9 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
// end of _fatalException().
Local<Value> async_id_value = Number::New(env->isolate(), async_id);
Local<Function> fn = env->async_hooks_after_function();
TryCatch try_catch(env->isolate());
MaybeLocal<Value> ar = fn->Call(
env->context(), Undefined(env->isolate()), 1, &async_id_value);
if (ar.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
UNREACHABLE();
}
FatalTryCatch try_catch(env);
fn->Call(env->context(), Undefined(env->isolate()), 1, &async_id_value)
.ToLocalChecked();
}

class PromiseWrap : public AsyncWrap {
Expand Down Expand Up @@ -748,14 +730,8 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
object,
};

TryCatch try_catch(env->isolate());
MaybeLocal<Value> ret = init_fn->Call(
env->context(), object, arraysize(argv), argv);

if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(env->isolate(), try_catch);
}
FatalTryCatch try_catch(env);
init_fn->Call(env->context(), object, arraysize(argv), argv).ToLocalChecked();
}


Expand Down
37 changes: 15 additions & 22 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,6 +1471,8 @@ void AppendExceptionLine(Environment* env,
static void ReportException(Environment* env,
Local<Value> er,
Local<Message> message) {
CHECK(!er.IsEmpty());
CHECK(!message.IsEmpty());
HandleScope scope(env->isolate());

AppendExceptionLine(env, er, message, FATAL_ERROR);
Expand Down Expand Up @@ -1540,6 +1542,10 @@ static void ReportException(Environment* env,
}

fflush(stderr);

#if HAVE_INSPECTOR
env->inspector_agent()->FatalException(er, message);
#endif
}


Expand Down Expand Up @@ -2399,6 +2405,15 @@ NO_RETURN void FatalError(const char* location, const char* message) {
}


FatalTryCatch::~FatalTryCatch() {
if (HasCaught()) {
HandleScope scope(env_->isolate());
ReportException(env_, *this);
exit(7);
}
}


void FatalException(Isolate* isolate,
Local<Value> error,
Local<Message> message) {
Expand Down Expand Up @@ -2441,9 +2456,6 @@ void FatalException(Isolate* isolate,
}

if (exit_code) {
#if HAVE_INSPECTOR
env->inspector_agent()->FatalException(error, message);
#endif
exit(exit_code);
}
}
Expand All @@ -2463,25 +2475,6 @@ static void OnMessage(Local<Message> message, Local<Value> error) {
FatalException(Isolate::GetCurrent(), error, message);
}


void ClearFatalExceptionHandlers(Environment* env) {
Local<Object> process = env->process_object();
Local<Value> events =
process->Get(env->context(), env->events_string()).ToLocalChecked();

if (events->IsObject()) {
events.As<Object>()->Set(
env->context(),
OneByteString(env->isolate(), "uncaughtException"),
Undefined(env->isolate())).FromJust();
}

process->Set(
env->context(),
env->domain_string(),
Undefined(env->isolate())).FromJust();
}

// Call process.emitWarning(str), fmt is a snprintf() format string
void ProcessEmitWarning(Environment* env, const char* fmt, ...) {
char warning[1024];
Expand Down
16 changes: 11 additions & 5 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,17 @@ void AppendExceptionLine(Environment* env,

NO_RETURN void FatalError(const char* location, const char* message);

// Like a `TryCatch` but it crashes the process if it did catch an exception.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Perhaps this could rephrased to something like "Like a TryCatch but crashes the process if an exception was caught"?

Copy link
Member

Choose a reason for hiding this comment

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

I'd also write "exits" instead of "crashes" because the latter suggests segfaults and aborts (in my mind at least.)

class FatalTryCatch : public v8::TryCatch {
public:
explicit FatalTryCatch(Environment* env)
: TryCatch(env->isolate()), env_(env) {}
~FatalTryCatch();

private:
Environment* env_;
};

void ProcessEmitWarning(Environment* env, const char* fmt, ...);

void FillStatsArray(double* fields, const uv_stat_t* s);
Expand Down Expand Up @@ -330,11 +341,6 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land.
};

// Clear any domain and/or uncaughtException handlers to force the error's
// propagation and shutdown the process. Use this to force the process to exit
// by clearing all callbacks that could handle the error.
void ClearFatalExceptionHandlers(Environment* env);

namespace Buffer {
v8::MaybeLocal<v8::Object> Copy(Environment* env, const char* data, size_t len);
v8::MaybeLocal<v8::Object> New(Environment* env, size_t size);
Expand Down
7 changes: 1 addition & 6 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2172,7 +2172,7 @@ const Local<Value> URL::ToObject(Environment* env) const {
};
SetArgs(env, argv, &context_);

TryCatch try_catch(isolate);
FatalTryCatch try_catch(env);

// The SetURLConstructor method must have been called already to
// set the constructor function used below. SetURLConstructor is
Expand All @@ -2182,11 +2182,6 @@ const Local<Value> URL::ToObject(Environment* env) const {
env->url_constructor_function()
->Call(env->context(), undef, 9, argv);

if (ret.IsEmpty()) {
ClearFatalExceptionHandlers(env);
FatalException(isolate, try_catch);
}

return ret.ToLocalChecked();
}

Expand Down