From ce4d184fbd2883492f835898d822b48c69a5a7af Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 12 Jun 2020 14:45:40 -0500 Subject: [PATCH 1/2] deps: V8: cherry-pick 767e65f945e7 Original commit message: [API] Fix microtask message reporting RunSingleMicrotask calls Runtime::ReportMessage, but the implementation of ReportMessage would unconditionally discard these exceptions. This CL removes all of the intermediate logic and directly calls MessageHandler::ReportMessage, restoring the ability of RunSingleMicrotask to report exceptions that occur in microtasks. Bug: v8:8326 Change-Id: I493de74383b2ab191d786611fb9eba9d27e7a243 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2162121 Commit-Queue: Gus Caplan Reviewed-by: Jakob Gruber Cr-Commit-Position: refs/heads/master@{#67630} Refs: https://github.com/v8/v8/commit/767e65f945e7caa4becc2bd4b5e9ea8562da0ca4 --- common.gypi | 2 +- .../builtins/builtins-microtask-queue-gen.cc | 2 +- deps/v8/src/builtins/promise-reaction-job.tq | 14 +- deps/v8/src/execution/isolate.cc | 170 ++++++------------ deps/v8/src/execution/isolate.h | 6 +- deps/v8/src/runtime/runtime-internal.cc | 16 +- deps/v8/src/runtime/runtime.h | 2 +- deps/v8/test/cctest/test-api.cc | 20 +++ 8 files changed, 91 insertions(+), 141 deletions(-) diff --git a/common.gypi b/common.gypi index 2c32f9e6c3c3ea..e2e7eaf00db3db 100644 --- a/common.gypi +++ b/common.gypi @@ -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 ##### diff --git a/deps/v8/src/builtins/builtins-microtask-queue-gen.cc b/deps/v8/src/builtins/builtins-microtask-queue-gen.cc index 917255f9bb3e54..5a033a102f0070 100644 --- a/deps/v8/src/builtins/builtins-microtask-queue-gen.cc +++ b/deps/v8/src/builtins/builtins-microtask-queue-gen.cc @@ -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); diff --git a/deps/v8/src/builtins/promise-reaction-job.tq b/deps/v8/src/builtins/promise-reaction-job.tq index 1d20d22efbbacc..f17886c0d18643 100644 --- a/deps/v8/src/builtins/promise-reaction-job.tq +++ b/deps/v8/src/builtins/promise-reaction-job.tq @@ -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 @@ -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(capability.reject); - return Call(context, reject, Undefined, reason); - } catch (e) { - // Swallow the exception here. - return runtime::ReportMessage(e); - } + const reject = UnsafeCast(capability.reject); + return Call(context, reject, Undefined, reason); } } } else { diff --git a/deps/v8/src/execution/isolate.cc b/deps/v8/src/execution/isolate.cc index 5c21b0982e9315..033d23d85b824c 100644 --- a/deps/v8/src/execution/isolate.cc +++ b/deps/v8/src/execution/isolate.cc @@ -1412,6 +1412,8 @@ void Isolate::InvokeApiInterruptCallbacks() { } } +namespace { + void ReportBootstrappingException(Handle exception, MessageLocation* location) { base::OS::PrintError("Exception thrown during bootstrapping\n"); @@ -1467,6 +1469,36 @@ void ReportBootstrappingException(Handle exception, #endif } +} // anonymous namespace + +Handle Isolate::CreateMessageOrAbort( + Handle exception, MessageLocation* location) { + Handle 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(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()); @@ -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 message_obj = CreateMessage(exception, location); + Handle 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(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(); - } - } } } @@ -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::cast(elements->WasmCodeObject(i)) @@ -2230,9 +2238,28 @@ bool Isolate::IsExternalHandlerOnTop(Object exception) { return (entry_handler > external_handler); } -void Isolate::ReportPendingMessagesImpl(bool report_externally) { +std::vector* Isolate::GetCodePages() const { + return code_pages_.load(std::memory_order_acquire); +} + +void Isolate::SetCodePages(std::vector* 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(); @@ -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 { @@ -2271,93 +2298,6 @@ void Isolate::ReportPendingMessagesImpl(bool report_externally) { } } -std::vector* Isolate::GetCodePages() const { - return code_pages_.load(std::memory_order_acquire); -} - -void Isolate::SetCodePages(std::vector* 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(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(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(); diff --git a/deps/v8/src/execution/isolate.h b/deps/v8/src/execution/isolate.h index 037e6ea7f59d0b..edf9a1a95ad9d8 100644 --- a/deps/v8/src/execution/isolate.h +++ b/deps/v8/src/execution/isolate.h @@ -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(); @@ -842,6 +838,8 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory { Handle CreateMessage(Handle exception, MessageLocation* location); + Handle CreateMessageOrAbort(Handle exception, + MessageLocation* location); // Out of resource exception helpers. Object StackOverflow(); diff --git a/deps/v8/src/runtime/runtime-internal.cc b/deps/v8/src/runtime/runtime-internal.cc index b3d9f26ee54b71..4806922a970f6b 100644 --- a/deps/v8/src/runtime/runtime-internal.cc +++ b/deps/v8/src/runtime/runtime-internal.cc @@ -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 message = + isolate->CreateMessageOrAbort(exception, no_location); + MessageHandler::ReportMessage(isolate, no_location, message); isolate->clear_pending_exception(); return ReadOnlyRoots(isolate).undefined_value(); } diff --git a/deps/v8/src/runtime/runtime.h b/deps/v8/src/runtime/runtime.h index 57500be5107790..c9ee6d88ac1917 100644 --- a/deps/v8/src/runtime/runtime.h +++ b/deps/v8/src/runtime/runtime.h @@ -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) \ diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index 6b7ea85452d2b0..db68750c723e3e 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -19834,11 +19834,30 @@ static void MicrotaskExceptionTwo( v8::Exception::Error(v8_str("second"))); } +int handler_call_count = 0; +static void MicrotaskExceptionHandler(Local message, + Local exception) { + CHECK(exception->IsNativeError()); + Local context = message->GetIsolate()->GetCurrentContext(); + Local 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;"); @@ -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, From b9f5307ce2d5290470ad9df00c6fff5d143776c6 Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 12 Jun 2020 16:12:17 -0500 Subject: [PATCH 2/2] lib: remove manual exception handling in queueMicrotask --- lib/internal/process/task_queues.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/internal/process/task_queues.js b/lib/internal/process/task_queues.js index c07942587ca9c2..3cf07601a23aa4 100644 --- a/lib/internal/process/task_queues.js +++ b/lib/internal/process/task_queues.js @@ -15,10 +15,6 @@ const { enqueueMicrotask } = internalBinding('task_queue'); -const { - triggerUncaughtException -} = internalBinding('errors'); - const { setHasRejectionToWarn, hasRejectionToWarn, @@ -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(); }