[runtime][api] Be stricter about microtasks and termination exceptions

Bug: chromium:1319267
Change-Id: I7956b804246ee2c1fa170bf2eb8f3588b7488b42
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3620285Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80467}

src/api/api-macros.h

@@ -39,7 +39,10 @@
   RCS_SCOPE(i_isolate,                                      \
             i::RuntimeCallCounterId::kAPI_##class_name##_##function_name);
 
-#define ENTER_V8_BASIC(i_isolate) i::VMState<v8::OTHER> __state__((i_isolate))
+#define ENTER_V8_BASIC(i_isolate)                            \
+  /* Embedders should never enter V8 after terminating it */ \
+  DCHECK(!i_isolate->is_execution_terminating());            \
+  i::VMState<v8::OTHER> __state__((i_isolate))
 
 #define ENTER_V8_HELPER_INTERNAL(i_isolate, context, class_name,    \
                                  function_name, bailout_value,      \
@@ -88,15 +91,26 @@
                            bailout_value, HandleScopeClass, false);       \
   i::DisallowJavascriptExecutionDebugOnly __no_script__((i_isolate))
 
-// Lightweight version for APIs that don't require an active context.
-#define DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate)                      \
+#define DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate)       \
   i::DisallowJavascriptExecutionDebugOnly __no_script__((i_isolate)); \
   i::DisallowExceptions __no_exceptions__((i_isolate))
 
+// Lightweight version for APIs that don't require an active context.
+#define DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate)             \
+  /* Embedders should never enter V8 after terminating it */ \
+  DCHECK(!i_isolate->is_execution_terminating());            \
+  DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate)
+
 #define ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate) \
   i::VMState<v8::OTHER> __state__((i_isolate));    \
   DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate)
 
+// Used instead of ENTER_V8_NO_SCRIPT_NO_EXCEPTION where the V8 Api is entered
+// during termination sequences.
+#define ENTER_V8_MAYBE_TEARDOWN(i_isolate)      \
+  i::VMState<v8::OTHER> __state__((i_isolate)); \
+  DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate)
+
 #define ENTER_V8_FOR_NEW_CONTEXT(i_isolate)         \
   DCHECK(!(i_isolate)->is_execution_terminating()); \
   i::VMState<v8::OTHER> __state__((i_isolate));     \
@@ -108,10 +122,14 @@
                            bailout_value, HandleScopeClass, false)
 
 #define DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate)
+#define DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate)
 
 #define ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate) \
   i::VMState<v8::OTHER> __state__((i_isolate));
 
+#define ENTER_V8_MAYBE_TEARDOWN(i_isolate) \
+  i::VMState<v8::OTHER> __state__((i_isolate));
+
 #define ENTER_V8_FOR_NEW_CONTEXT(i_isolate) \
   i::VMState<v8::OTHER> __state__((i_isolate));
 #endif  // DEBUG

src/api/api.cc

@@ -1037,19 +1037,23 @@ bool Data::IsFunctionTemplate() const {
 bool Data::IsContext() const { return Utils::OpenHandle(this)->IsContext(); }
 
 void Context::Enter() {
-  i::Handle<i::Context> env = Utils::OpenHandle(this);
-  i::Isolate* i_isolate = env->GetIsolate();
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  i::DisallowGarbageCollection no_gc;
+  i::Context env = *Utils::OpenHandle(this);
+  i::Isolate* i_isolate = env.GetIsolate();
+  // TODO(cbruni): Use ENTER_V8_NO_SCRIPT_NO_EXCEPTION which also checks
+  // Isolate::is_execution_terminating
+  // ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  ENTER_V8_MAYBE_TEARDOWN(i_isolate);
   i::HandleScopeImplementer* impl = i_isolate->handle_scope_implementer();
-  impl->EnterContext(*env);
+  impl->EnterContext(env);
   impl->SaveContext(i_isolate->context());
-  i_isolate->set_context(*env);
+  i_isolate->set_context(env);
 }
 
 void Context::Exit() {
   i::Handle<i::Context> env = Utils::OpenHandle(this);
   i::Isolate* i_isolate = env->GetIsolate();
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  ENTER_V8_MAYBE_TEARDOWN(i_isolate);
   i::HandleScopeImplementer* impl = i_isolate->handle_scope_implementer();
   if (!Utils::ApiCheck(impl->LastEnteredContextWas(*env), "v8::Context::Exit()",
                        "Cannot exit non-entered context")) {
@@ -1090,7 +1094,7 @@ static i::Handle<i::EmbedderDataArray> EmbedderDataFor(Context* context,
                                                        const char* location) {
   i::Handle<i::Context> env = Utils::OpenHandle(context);
   i::Isolate* i_isolate = env->GetIsolate();
-  DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate);
   bool ok = Utils::ApiCheck(env->IsNativeContext(), location,
                             "Not a native context") &&
             Utils::ApiCheck(index >= 0, location, "Negative index");
@@ -2335,7 +2339,7 @@ Local<Value> Module::GetException() const {
                   "Module status must be kErrored");
   i::Handle<i::Module> self = Utils::OpenHandle(this);
   i::Isolate* i_isolate = self->GetIsolate();
-  DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  ENTER_V8_MAYBE_TEARDOWN(i_isolate);
   return ToApiHandle<Value>(i::handle(self->GetException(), i_isolate));
 }
 
@@ -6519,7 +6523,7 @@ v8::Local<v8::Object> Context::Global() {
 void Context::DetachGlobal() {
   i::Handle<i::Context> context = Utils::OpenHandle(this);
   i::Isolate* i_isolate = context->GetIsolate();
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  ENTER_V8_MAYBE_TEARDOWN(i_isolate);
   i_isolate->DetachGlobal(context);
 }
 
@@ -9613,11 +9617,10 @@ MicrotasksScope::~MicrotasksScope() {
     microtask_queue_->DecrementMicrotasksScopeDepth();
     if (MicrotasksPolicy::kScoped == microtask_queue_->microtasks_policy() &&
         !i_isolate_->has_scheduled_exception()) {
-      DCHECK_IMPLIES(i_isolate_->has_scheduled_exception(),
-                     i_isolate_->scheduled_exception() ==
-                         i::ReadOnlyRoots(i_isolate_).termination_exception());
       microtask_queue_->PerformCheckpoint(
           reinterpret_cast<Isolate*>(i_isolate_));
+      DCHECK_IMPLIES(i_isolate_->has_scheduled_exception(),
+                     i_isolate_->is_execution_terminating());
     }
   }
 #ifdef DEBUG

src/debug/debug-interface.cc

@@ -34,25 +34,38 @@ namespace v8 {
 namespace debug {
 
 void SetContextId(Local<Context> context, int id) {
-  Utils::OpenHandle(*context)->set_debug_context_id(i::Smi::FromInt(id));
+  auto v8_context = Utils::OpenHandle(*context);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION(v8_context->GetIsolate());
+  v8_context->set_debug_context_id(i::Smi::FromInt(id));
 }
 
 int GetContextId(Local<Context> context) {
-  i::Object value = Utils::OpenHandle(*context)->debug_context_id();
+  auto v8_context = Utils::OpenHandle(*context);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(v8_context->GetIsolate());
+  i::Object value = v8_context->debug_context_id();
   return (value.IsSmi()) ? i::Smi::ToInt(value) : 0;
 }
 
 void SetInspector(Isolate* isolate, v8_inspector::V8Inspector* inspector) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  i_isolate->set_inspector(inspector);
+  if (inspector == nullptr) {
+    DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate);
+    i_isolate->set_inspector(nullptr);
+  } else {
+    DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+    i_isolate->set_inspector(inspector);
+  }
 }
 
 v8_inspector::V8Inspector* GetInspector(Isolate* isolate) {
-  return reinterpret_cast<i::Isolate*>(isolate)->inspector();
+  i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(i_isolate);
+  return i_isolate->inspector();
 }
 
 Local<String> GetBigIntDescription(Isolate* isolate, Local<BigInt> bigint) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
   i::Handle<i::BigInt> i_bigint = Utils::OpenHandle(*bigint);
   // For large BigInts computing the decimal string representation
   // can take a long time, so we go with hexadecimal in that case.
@@ -88,16 +101,20 @@ Local<String> GetBigIntDescription(Isolate* isolate, Local<BigInt> bigint) {
 Local<String> GetDateDescription(Local<Date> date) {
   auto receiver = Utils::OpenHandle(*date);
   i::Handle<i::JSDate> jsdate = i::Handle<i::JSDate>::cast(receiver);
-  i::Isolate* isolate = jsdate->GetIsolate();
-  auto buffer = i::ToDateString(jsdate->value().Number(), isolate->date_cache(),
-                                i::ToDateStringMode::kLocalDateAndTime);
-  return Utils::ToLocal(isolate->factory()
+  i::Isolate* i_isolate = jsdate->GetIsolate();
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  auto buffer =
+      i::ToDateString(jsdate->value().Number(), i_isolate->date_cache(),
+                      i::ToDateStringMode::kLocalDateAndTime);
+  return Utils::ToLocal(i_isolate->factory()
                             ->NewStringFromUtf8(base::VectorOf(buffer))
                             .ToHandleChecked());
 }
 
 Local<String> GetFunctionDescription(Local<Function> function) {
   auto receiver = Utils::OpenHandle(*function);
+  auto i_isolate = receiver->GetIsolate();
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
   if (receiver->IsJSBoundFunction()) {
     return Utils::ToLocal(i::JSBoundFunction::ToString(
         i::Handle<i::JSBoundFunction>::cast(receiver)));
@@ -106,18 +123,18 @@ Local<String> GetFunctionDescription(Local<Function> function) {
     auto js_function = i::Handle<i::JSFunction>::cast(receiver);
 #if V8_ENABLE_WEBASSEMBLY
     if (js_function->shared().HasWasmExportedFunctionData()) {
-      auto isolate = js_function->GetIsolate();
+      auto i_isolate = js_function->GetIsolate();
       auto func_index =
           js_function->shared().wasm_exported_function_data().function_index();
       auto instance = i::handle(
           js_function->shared().wasm_exported_function_data().instance(),
-          isolate);
+          i_isolate);
       if (instance->module()->origin == i::wasm::kWasmOrigin) {
         // For asm.js functions, we can still print the source
         // code (hopefully), so don't bother with them here.
         auto debug_name =
-            i::GetWasmFunctionDebugName(isolate, instance, func_index);
-        i::IncrementalStringBuilder builder(isolate);
+            i::GetWasmFunctionDebugName(i_isolate, instance, func_index);
+        i::IncrementalStringBuilder builder(i_isolate);
         builder.AppendCStringLiteral("function ");
         builder.AppendString(debug_name);
         builder.AppendCStringLiteral("() { [native code] }");
@@ -132,13 +149,15 @@ Local<String> GetFunctionDescription(Local<Function> function) {
 }
 
 void SetBreakOnNextFunctionCall(Isolate* isolate) {
-  reinterpret_cast<i::Isolate*>(isolate)->debug()->SetBreakOnNextFunctionCall();
+  auto i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  i_isolate->debug()->SetBreakOnNextFunctionCall();
 }
 
 void ClearBreakOnNextFunctionCall(Isolate* isolate) {
-  reinterpret_cast<i::Isolate*>(isolate)
-      ->debug()
-      ->ClearBreakOnNextFunctionCall();
+  auto i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate);
+  i_isolate->debug()->ClearBreakOnNextFunctionCall();
 }
 
 MaybeLocal<Array> GetInternalProperties(Isolate* v8_isolate,
@@ -158,6 +177,7 @@ void CollectPrivateMethodsAndAccessorsFromContext(
     i::Isolate* isolate, i::Handle<i::Context> context,
     i::IsStaticFlag is_static_flag, std::vector<Local<Value>>* names_out,
     std::vector<Local<Value>>* values_out) {
+  DCHECK_NO_SCRIPT_NO_EXCEPTION(isolate);
   i::Handle<i::ScopeInfo> scope_info(context->scope_info(), isolate);
   for (auto it : i::ScopeInfo::IterateLocalNames(scope_info)) {
     i::Handle<i::String> name(it->name(), isolate);
@@ -294,6 +314,7 @@ MaybeLocal<Context> GetCreationContext(Local<Object> value) {
 
 void ChangeBreakOnException(Isolate* isolate, ExceptionBreakState type) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
+  DCHECK_NO_SCRIPT_NO_EXCEPTION(i_isolate);
   i_isolate->debug()->ChangeBreakOnException(i::BreakException,
                                              type == BreakOnAnyException);
   i_isolate->debug()->ChangeBreakOnException(i::BreakUncaughtException,
@@ -995,8 +1016,13 @@ Local<Function> GetBuiltin(Isolate* v8_isolate, Builtin requested_builtin) {
 
 void SetConsoleDelegate(Isolate* v8_isolate, ConsoleDelegate* delegate) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
-  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
-  isolate->set_console_delegate(delegate);
+  if (delegate == nullptr) {
+    DCHECK_NO_SCRIPT_NO_EXCEPTION_MAYBE_TEARDOWN(isolate);
+    isolate->set_console_delegate(nullptr);
+  } else {
+    DCHECK_NO_SCRIPT_NO_EXCEPTION(isolate);
+    isolate->set_console_delegate(delegate);
+  }
 }
 
 ConsoleCallArguments::ConsoleCallArguments(

src/debug/debug.cc

@@ -2688,8 +2688,7 @@ void Debug::StopSideEffectCheckMode() {
   DCHECK(isolate_->debug_execution_mode() == DebugInfo::kSideEffects);
   if (side_effect_check_failed_) {
     DCHECK(isolate_->has_pending_exception());
-    DCHECK_EQ(ReadOnlyRoots(isolate_).termination_exception(),
-              isolate_->pending_exception());
+    DCHECK(isolate_->is_execution_termination_pending());
     // Convert the termination exception into a regular exception.
     isolate_->CancelTerminateExecution();
     isolate_->Throw(*isolate_->factory()->NewEvalError(

src/execution/isolate-inl.h

@@ -96,6 +96,16 @@ void Isolate::set_scheduled_exception(Object exception) {
   thread_local_top()->scheduled_exception_ = exception;
 }
 
+bool Isolate::is_execution_termination_pending() {
+  return thread_local_top()->pending_exception_ ==
+         i::ReadOnlyRoots(this).termination_exception();
+}
+
+bool Isolate::is_execution_terminating() {
+  return thread_local_top()->scheduled_exception_ ==
+         i::ReadOnlyRoots(this).termination_exception();
+}
+
 #ifdef DEBUG
 Object Isolate::VerifyBuiltinsResult(Object result) {
   if (has_pending_exception()) {
@@ -130,11 +140,6 @@ bool Isolate::is_catchable_by_wasm(Object exception) {
   return !JSReceiver::HasProperty(&it).FromJust();
 }
 
-bool Isolate::is_execution_terminating() {
-  return thread_local_top()->scheduled_exception_ ==
-         i::ReadOnlyRoots(this).termination_exception();
-}
-
 void Isolate::FireBeforeCallEnteredCallback() {
   for (auto& callback : before_call_entered_callbacks_) {
     callback(reinterpret_cast<v8::Isolate*>(this));

src/execution/isolate.cc

@@ -1607,13 +1607,11 @@ void Isolate::CancelTerminateExecution() {
   if (try_catch_handler()) {
     try_catch_handler()->has_terminated_ = false;
   }
-  if (has_pending_exception() &&
-      pending_exception() == ReadOnlyRoots(this).termination_exception()) {
+  if (has_pending_exception() && is_execution_termination_pending()) {
     thread_local_top()->external_caught_exception_ = false;
     clear_pending_exception();
   }
-  if (has_scheduled_exception() &&
-      scheduled_exception() == ReadOnlyRoots(this).termination_exception()) {
+  if (has_scheduled_exception() && is_execution_terminating()) {
     thread_local_top()->external_caught_exception_ = false;
     clear_scheduled_exception();
   }
@@ -2328,12 +2326,10 @@ void Isolate::CancelScheduledExceptionFromTryCatch(v8::TryCatch* handler) {
   DCHECK(has_scheduled_exception());
   if (reinterpret_cast<void*>(scheduled_exception().ptr()) ==
       handler->exception_) {
-    DCHECK_NE(scheduled_exception(),
-              ReadOnlyRoots(heap()).termination_exception());
+    DCHECK(!is_execution_terminating());
     clear_scheduled_exception();
   } else {
-    DCHECK_EQ(scheduled_exception(),
-              ReadOnlyRoots(heap()).termination_exception());
+    DCHECK(is_execution_terminating());
     // Clear termination once we returned from all V8 frames.
     if (thread_local_top()->CallDepthIsZero()) {
       thread_local_top()->external_caught_exception_ = false;
@@ -2611,10 +2607,7 @@ bool Isolate::OptionalRescheduleException(bool clear_exception) {
   PropagatePendingExceptionToExternalTryCatch(
       TopExceptionHandlerType(pending_exception()));
 
-  bool is_termination_exception =
-      pending_exception() == ReadOnlyRoots(this).termination_exception();
-
-  if (is_termination_exception) {
+  if (is_execution_termination_pending()) {
     if (clear_exception) {
       thread_local_top()->external_caught_exception_ = false;
       clear_pending_exception();
@@ -3620,6 +3613,7 @@ void Isolate::InitializeThreadLocal() {
 }
 
 void Isolate::SetTerminationOnExternalTryCatch() {
+  DCHECK(is_execution_termination_pending() || is_execution_terminating());
   if (try_catch_handler() == nullptr) return;
   try_catch_handler()->can_continue_ = false;
   try_catch_handler()->has_terminated_ = true;
@@ -5113,6 +5107,7 @@ void Isolate::OnPromiseAfter(Handle<JSPromise> promise) {
 }
 
 void Isolate::OnTerminationDuringRunMicrotasks() {
+  DCHECK(is_execution_terminating());
   // This performs cleanup for when RunMicrotasks (in
   // builtins-microtask-queue-gen.cc) is aborted via a termination exception.
   // This has to be kept in sync with the code in said file. Currently this

src/execution/isolate.h

@@ -811,6 +811,7 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   inline bool is_catchable_by_javascript(Object exception);
   inline bool is_catchable_by_wasm(Object exception);
   inline bool is_execution_terminating();
+  inline bool is_execution_termination_pending();
 
   // JS execution stack (see frames.h).
   static Address c_entry_fp(ThreadLocalTop* thread) {

src/init/bootstrapper.cc

@@ -6006,8 +6006,7 @@ bool Genesis::InstallExtension(Isolate* isolate,
     // terminating.
     DCHECK(isolate->has_pending_exception() ||
            (isolate->has_scheduled_exception() &&
-            isolate->scheduled_exception() ==
-                ReadOnlyRoots(isolate).termination_exception()));
+            isolate->is_execution_terminating()));
     if (isolate->has_pending_exception()) {
       // We print out the name of the extension that fail to install.
       // When an error is thrown during bootstrapping we automatically print

src/inspector/v8-console.cc

@@ -1011,7 +1011,9 @@ V8Console::CommandLineAPIScope::CommandLineAPIScope(
 }
 
 V8Console::CommandLineAPIScope::~CommandLineAPIScope() {
-  v8::MicrotasksScope microtasksScope(m_context->GetIsolate(),
+  auto isolate = m_context->GetIsolate();
+  if (isolate->IsExecutionTerminating()) return;
+  v8::MicrotasksScope microtasksScope(isolate,
                                       v8::MicrotasksScope::kDoNotRunMicrotasks);
   *static_cast<CommandLineAPIScope**>(
       m_thisReference->GetBackingStore()->Data()) = nullptr;