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

Jump to bottom

lib: remove manual exception handling in queueMicrotask #33859

Closed
wants to merge 2 commits into from
Closed

lib: remove manual exception handling in queueMicrotask #33859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce4d184
deps: V8: cherry-pick 767e65f945e7
devsnek Jun 12, 2020
b9f5307
lib: remove manual exception handling in queueMicrotask
devsnek Jun 12, 2020
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
2 changes: 1 addition & 1 deletion common.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.19',
'v8_embedder_string': '-node.20',

##### V8 defaults for Node.js #####

Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/builtins/builtins-microtask-queue-gen.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ void MicrotaskQueueBuiltinsAssembler::RunSingleMicrotask(
BIND(&if_exception);
{
// Report unhandled exceptions from microtasks.
CallRuntime(Runtime::kReportMessage, current_context,
CallRuntime(Runtime::kReportMessageFromMicrotask, current_context,
var_exception.value());
RewindEnteredContext(saved_entered_context_count);
SetCurrentContext(current_context);
Expand Down
14 changes: 2 additions & 12 deletions deps/v8/src/builtins/promise-reaction-job.tq
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@

#include 'src/builtins/builtins-promise-gen.h'

namespace runtime {
extern transitioning runtime
ReportMessage(implicit context: Context)(JSAny): JSAny;
}

namespace promise {

transitioning
Expand All @@ -30,13 +25,8 @@ namespace promise {
case (capability: PromiseCapability): {
// In the general case we need to call the (user provided)
// promiseCapability.[[Reject]] function.
try {
const reject = UnsafeCast<Callable>(capability.reject);
return Call(context, reject, Undefined, reason);
} catch (e) {
// Swallow the exception here.
return runtime::ReportMessage(e);
}
const reject = UnsafeCast<Callable>(capability.reject);
return Call(context, reject, Undefined, reason);
}
}
} else {
Expand Down
170 changes: 55 additions & 115 deletions deps/v8/src/execution/isolate.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1412,6 +1412,8 @@ void Isolate::InvokeApiInterruptCallbacks() {
}
}

namespace {

void ReportBootstrappingException(Handle<Object> exception,
MessageLocation* location) {
base::OS::PrintError("Exception thrown during bootstrapping\n");
Expand Down Expand Up @@ -1467,6 +1469,36 @@ void ReportBootstrappingException(Handle<Object> exception,
#endif
}

} // anonymous namespace

Handle<JSMessageObject> Isolate::CreateMessageOrAbort(
Handle<Object> exception, MessageLocation* location) {
Handle<JSMessageObject> message_obj = CreateMessage(exception, location);

// If the abort-on-uncaught-exception flag is specified, and if the
// embedder didn't specify a custom uncaught exception callback,
// or if the custom callback determined that V8 should abort, then
// abort.
if (FLAG_abort_on_uncaught_exception) {
CatchType prediction = PredictExceptionCatcher();
if ((prediction == NOT_CAUGHT || prediction == CAUGHT_BY_EXTERNAL) &&
(!abort_on_uncaught_exception_callback_ ||
abort_on_uncaught_exception_callback_(
reinterpret_cast<v8::Isolate*>(this)))) {
// Prevent endless recursion.
FLAG_abort_on_uncaught_exception = false;
// This flag is intended for use by JavaScript developers, so
// print a user-friendly stack trace (not an internal one).
PrintF(stderr, "%s\n\nFROM\n",
MessageHandler::GetLocalizedMessage(this, message_obj).get());
PrintCurrentStackTrace(stderr);
base::OS::Abort();
}
}

return message_obj;
}

Object Isolate::Throw(Object raw_exception, MessageLocation* location) {
DCHECK(!has_pending_exception());

Expand Down Expand Up @@ -1538,38 +1570,14 @@ Object Isolate::Throw(Object raw_exception, MessageLocation* location) {
if (location == nullptr && ComputeLocation(&computed_location)) {
location = &computed_location;
}

if (bootstrapper()->IsActive()) {
// It's not safe to try to make message objects or collect stack traces
// while the bootstrapper is active since the infrastructure may not have
// been properly initialized.
ReportBootstrappingException(exception, location);
} else {
Handle<Object> message_obj = CreateMessage(exception, location);
Handle<Object> message_obj = CreateMessageOrAbort(exception, location);
thread_local_top()->pending_message_obj_ = *message_obj;

// For any exception not caught by JavaScript, even when an external
// handler is present:
// If the abort-on-uncaught-exception flag is specified, and if the
// embedder didn't specify a custom uncaught exception callback,
// or if the custom callback determined that V8 should abort, then
// abort.
if (FLAG_abort_on_uncaught_exception) {
CatchType prediction = PredictExceptionCatcher();
if ((prediction == NOT_CAUGHT || prediction == CAUGHT_BY_EXTERNAL) &&
(!abort_on_uncaught_exception_callback_ ||
abort_on_uncaught_exception_callback_(
reinterpret_cast<v8::Isolate*>(this)))) {
// Prevent endless recursion.
FLAG_abort_on_uncaught_exception = false;
// This flag is intended for use by JavaScript developers, so
// print a user-friendly stack trace (not an internal one).
PrintF(stderr, "%s\n\nFROM\n",
MessageHandler::GetLocalizedMessage(this, message_obj).get());
PrintCurrentStackTrace(stderr);
base::OS::Abort();
}
}
}
}

Expand Down Expand Up @@ -2106,7 +2114,7 @@ bool Isolate::ComputeLocationFromStackTrace(MessageLocation* target,
bool is_at_number_conversion =
elements->IsAsmJsWasmFrame(i) &&
elements->Flags(i).value() & FrameArray::kAsmJsAtNumberConversion;
if (elements->IsWasmCompiledFrame(i)) {
if (elements->IsWasmCompiledFrame(i) || elements->IsAsmJsWasmFrame(i)) {
// WasmCode* held alive by the {GlobalWasmCodeRef}.
wasm::WasmCode* code =
Managed<wasm::GlobalWasmCodeRef>::cast(elements->WasmCodeObject(i))
Expand Down Expand Up @@ -2230,9 +2238,28 @@ bool Isolate::IsExternalHandlerOnTop(Object exception) {
return (entry_handler > external_handler);
}

void Isolate::ReportPendingMessagesImpl(bool report_externally) {
std::vector<MemoryRange>* Isolate::GetCodePages() const {
return code_pages_.load(std::memory_order_acquire);
}

void Isolate::SetCodePages(std::vector<MemoryRange>* new_code_pages) {
code_pages_.store(new_code_pages, std::memory_order_release);
}

void Isolate::ReportPendingMessages() {
DCHECK(AllowExceptions::IsAllowed(this));

// The embedder might run script in response to an exception.
AllowJavascriptExecutionDebugOnly allow_script(this);

Object exception_obj = pending_exception();

// Try to propagate the exception to an external v8::TryCatch handler. If
// propagation was unsuccessful, then we will get another chance at reporting
// the pending message if the exception is re-thrown.
bool has_been_propagated = PropagatePendingExceptionToExternalTryCatch();
if (!has_been_propagated) return;

// Clear the pending message object early to avoid endless recursion.
Object message_obj = thread_local_top()->pending_message_obj_;
clear_pending_message();
Expand All @@ -2245,7 +2272,7 @@ void Isolate::ReportPendingMessagesImpl(bool report_externally) {
// depending on whether and external v8::TryCatch or an internal JavaScript
// handler is on top.
bool should_report_exception;
if (report_externally) {
if (IsExternalHandlerOnTop(exception_obj)) {
// Only report the exception if the external handler is verbose.
should_report_exception = try_catch_handler()->is_verbose_;
} else {
Expand All @@ -2271,93 +2298,6 @@ void Isolate::ReportPendingMessagesImpl(bool report_externally) {
}
}

std::vector<MemoryRange>* Isolate::GetCodePages() const {
return code_pages_.load(std::memory_order_acquire);
}

void Isolate::SetCodePages(std::vector<MemoryRange>* new_code_pages) {
code_pages_.store(new_code_pages, std::memory_order_release);
}

void Isolate::ReportPendingMessages() {
DCHECK(AllowExceptions::IsAllowed(this));

// The embedder might run script in response to an exception.
AllowJavascriptExecutionDebugOnly allow_script(this);

Object exception = pending_exception();

// Try to propagate the exception to an external v8::TryCatch handler. If
// propagation was unsuccessful, then we will get another chance at reporting
// the pending message if the exception is re-thrown.
bool has_been_propagated = PropagatePendingExceptionToExternalTryCatch();
if (!has_been_propagated) return;

ReportPendingMessagesImpl(IsExternalHandlerOnTop(exception));
}

void Isolate::ReportPendingMessagesFromJavaScript() {
DCHECK(AllowExceptions::IsAllowed(this));

auto IsHandledByJavaScript = [=]() {
// In this situation, the exception is always a non-terminating exception.

// Get the top-most JS_ENTRY handler, cannot be on top if it doesn't exist.
Address entry_handler = Isolate::handler(thread_local_top());
DCHECK_NE(entry_handler, kNullAddress);
entry_handler = StackHandler::FromAddress(entry_handler)->next_address();

// Get the address of the external handler so we can compare the address to
// determine which one is closer to the top of the stack.
Address external_handler = thread_local_top()->try_catch_handler_address();
if (external_handler == kNullAddress) return true;

return (entry_handler < external_handler);
};

auto IsHandledExternally = [=]() {
Address external_handler = thread_local_top()->try_catch_handler_address();
if (external_handler == kNullAddress) return false;

// Get the top-most JS_ENTRY handler, cannot be on top if it doesn't exist.
Address entry_handler = Isolate::handler(thread_local_top());
DCHECK_NE(entry_handler, kNullAddress);
entry_handler = StackHandler::FromAddress(entry_handler)->next_address();
return (entry_handler > external_handler);
};

auto PropagateToExternalHandler = [=]() {
if (IsHandledByJavaScript()) {
thread_local_top()->external_caught_exception_ = false;
return false;
}

if (!IsHandledExternally()) {
thread_local_top()->external_caught_exception_ = false;
return true;
}

thread_local_top()->external_caught_exception_ = true;
v8::TryCatch* handler = try_catch_handler();
DCHECK(thread_local_top()->pending_message_obj_.IsJSMessageObject() ||
thread_local_top()->pending_message_obj_.IsTheHole(this));
handler->can_continue_ = true;
handler->has_terminated_ = false;
handler->exception_ = reinterpret_cast<void*>(pending_exception().ptr());
// Propagate to the external try-catch only if we got an actual message.
if (thread_local_top()->pending_message_obj_.IsTheHole(this)) return true;

handler->message_obj_ =
reinterpret_cast<void*>(thread_local_top()->pending_message_obj_.ptr());
return true;
};

// Try to propagate to an external v8::TryCatch handler.
if (!PropagateToExternalHandler()) return;

ReportPendingMessagesImpl(true);
}

bool Isolate::OptionalRescheduleException(bool clear_exception) {
DCHECK(has_pending_exception());
PropagatePendingExceptionToExternalTryCatch();
Expand Down
6 changes: 2 additions & 4 deletions deps/v8/src/execution/isolate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,10 +822,6 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
// Un-schedule an exception that was caught by a TryCatch handler.
void CancelScheduledExceptionFromTryCatch(v8::TryCatch* handler);
void ReportPendingMessages();
void ReportPendingMessagesFromJavaScript();

// Implements code shared between the two above methods
void ReportPendingMessagesImpl(bool report_externally);

// Promote a scheduled exception to pending. Asserts has_scheduled_exception.
Object PromoteScheduledException();
Expand All @@ -842,6 +838,8 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {

Handle<JSMessageObject> CreateMessage(Handle<Object> exception,
MessageLocation* location);
Handle<JSMessageObject> CreateMessageOrAbort(Handle<Object> exception,
MessageLocation* location);

// Out of resource exception helpers.
Object StackOverflow();
Expand Down
16 changes: 9 additions & 7 deletions deps/v8/src/runtime/runtime-internal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,19 +578,21 @@ RUNTIME_FUNCTION(Runtime_GetTemplateObject) {
isolate, native_context, description, shared_info, slot_id);
}

RUNTIME_FUNCTION(Runtime_ReportMessage) {
RUNTIME_FUNCTION(Runtime_ReportMessageFromMicrotask) {
// Helper to report messages and continue JS execution. This is intended to
// behave similarly to reporting exceptions which reach the top-level in
// Execution.cc, but allow the JS code to continue. This is useful for
// implementing algorithms such as RunMicrotasks in JS.
// behave similarly to reporting exceptions which reach the top-level, but
// allow the JS code to continue.
HandleScope scope(isolate);
DCHECK_EQ(1, args.length());

CONVERT_ARG_HANDLE_CHECKED(Object, message_obj, 0);
CONVERT_ARG_HANDLE_CHECKED(Object, exception, 0);

DCHECK(!isolate->has_pending_exception());
isolate->set_pending_exception(*message_obj);
isolate->ReportPendingMessagesFromJavaScript();
isolate->set_pending_exception(*exception);
MessageLocation* no_location = nullptr;
Handle<JSMessageObject> message =
isolate->CreateMessageOrAbort(exception, no_location);
MessageHandler::ReportMessage(isolate, no_location, message);
isolate->clear_pending_exception();
return ReadOnlyRoots(isolate).undefined_value();
}
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/runtime/runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ namespace internal {
F(NewTypeError, 2, 1) \
F(OrdinaryHasInstance, 2, 1) \
F(PromoteScheduledException, 0, 1) \
F(ReportMessage, 1, 1) \
F(ReportMessageFromMicrotask, 1, 1) \
F(ReThrow, 1, 1) \
F(RunMicrotaskCallback, 2, 1) \
F(PerformMicrotaskCheckpoint, 0, 1) \
Expand Down
20 changes: 20 additions & 0 deletions deps/v8/test/cctest/test-api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19834,11 +19834,30 @@ static void MicrotaskExceptionTwo(
v8::Exception::Error(v8_str("second")));
}

int handler_call_count = 0;
static void MicrotaskExceptionHandler(Local<Message> message,
Local<Value> exception) {
CHECK(exception->IsNativeError());
Local<Context> context = message->GetIsolate()->GetCurrentContext();
Local<String> str = exception->ToString(context).ToLocalChecked();
switch (handler_call_count++) {
case 0:
CHECK(str->StrictEquals(v8_str("Error: first")));
break;
case 1:
CHECK(str->StrictEquals(v8_str("Error: second")));
break;
default:
UNREACHABLE();
}
}

TEST(RunMicrotasksIgnoresThrownExceptions) {
LocalContext env;
v8::Isolate* isolate = env->GetIsolate();
v8::HandleScope scope(isolate);
isolate->AddMessageListenerWithErrorLevel(MicrotaskExceptionHandler,
v8::Isolate::kMessageAll);
CompileRun(
"var exception1Calls = 0;"
"var exception2Calls = 0;");
Expand All @@ -19849,6 +19868,7 @@ TEST(RunMicrotasksIgnoresThrownExceptions) {
TryCatch try_catch(isolate);
CompileRun("1+1;");
CHECK(!try_catch.HasCaught());
CHECK_EQ(handler_call_count, 2);
CHECK_EQ(1,
CompileRun("exception1Calls")->Int32Value(env.local()).FromJust());
CHECK_EQ(1,
Expand Down
8 changes: 0 additions & 8 deletions lib/internal/process/task_queues.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ const {
enqueueMicrotask
} = internalBinding('task_queue');

const {
triggerUncaughtException
} = internalBinding('errors');

const {
setHasRejectionToWarn,
hasRejectionToWarn,
Expand Down Expand Up @@ -151,10 +147,6 @@ function runMicrotask() {
const callback = this.callback;
try {
callback();
} catch (error) {
// runInAsyncScope() swallows the error so we need to catch
// it and handle it here.
triggerUncaughtException(error, false /* fromPromise */);
} finally {
this.emitDestroy();
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a check to verify that this emitDestroy() is still reached?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, test/parallel/test-queue-microtask-uncaught-asynchooks.js

}
Expand Down