Make termination exception more consistent.

Termination exceptions tear down V8 to the bottom-most V8 call. If there is a
v8::TryCatch scope around that call, it returns true for HasTerminated() and
HasCaught(). However, Isolate::IsExecutionTerminating() returns false and we
can call into V8 from still inside the v8::TryCatch scope.

Changes that this patch introduces:
 - You need to leave the v8::TryCatch scope around the bottom-most call to
   reset the termination state, in order to resume.
 - Explicitly check for termination exception and reporting it through the
   DevTools protocol after Runtime.evaluate and Debugger.evaluateOnCallFrame.

Bug: v8:8455
Change-Id: I1f36f7a365985469813c2619bf16f18ee69aa4b8
Reviewed-on: https://chromium-review.googlesource.com/c/1337582
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57963}

src/api.cc

@@ -273,8 +273,6 @@ class CallDepthScope {
                                      ? i::InterruptsScope::kRunInterrupts
                                      : i::InterruptsScope::kPostponeInterrupts)
                               : i::InterruptsScope::kNoop) {
-    // TODO(dcarney): remove this when blink stops crashing.
-    DCHECK(!isolate_->external_caught_exception());
     isolate_->handle_scope_implementer()->IncrementCallDepth();
     isolate_->set_next_v8_call_is_safe_for_termination(false);
     if (!context.IsEmpty()) {
@@ -309,8 +307,10 @@ class CallDepthScope {
     escaped_ = true;
     auto handle_scope_implementer = isolate_->handle_scope_implementer();
     handle_scope_implementer->DecrementCallDepth();
-    bool call_depth_is_zero = handle_scope_implementer->CallDepthIsZero();
-    isolate_->OptionalRescheduleException(call_depth_is_zero);
+    bool clear_exception =
+        handle_scope_implementer->CallDepthIsZero() &&
+        isolate_->thread_local_top()->try_catch_handler() == nullptr;
+    isolate_->OptionalRescheduleException(clear_exception);
   }
 
  private:
@@ -8994,7 +8994,6 @@ MicrotasksScope::~MicrotasksScope() {
 
 void MicrotasksScope::PerformCheckpoint(Isolate* v8Isolate) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8Isolate);
-  if (IsExecutionTerminatingCheck(isolate)) return;
   auto handle_scope_implementer = isolate->handle_scope_implementer();
   if (!handle_scope_implementer->GetMicrotasksScopeDepth() &&
       !handle_scope_implementer->HasMicrotasksSuppressions()) {

src/inspector/injected-script.h

@@ -129,6 +129,7 @@ class InjectedScript final {
     v8::Local<v8::Context> context() const { return m_context; }
     InjectedScript* injectedScript() const { return m_injectedScript; }
     const v8::TryCatch& tryCatch() const { return m_tryCatch; }
+    V8InspectorImpl* inspector() const { return m_inspector; }
 
    protected:
     explicit Scope(V8InspectorSessionImpl*);

src/inspector/v8-debugger-agent-impl.cc

@@ -1064,7 +1064,7 @@ Response V8DebuggerAgentImpl::evaluateOnCallFrame(
 
   v8::MaybeLocal<v8::Value> maybeResultValue;
   {
-    V8InspectorImpl::EvaluateScope evaluateScope(m_isolate);
+    V8InspectorImpl::EvaluateScope evaluateScope(scope);
     if (timeout.isJust()) {
       response = evaluateScope.setTimeout(timeout.fromJust() / 1000.0);
       if (!response.isSuccess()) return response;

src/inspector/v8-debugger.cc

@@ -305,19 +305,22 @@ void V8Debugger::terminateExecution(
   m_isolate->TerminateExecution();
 }
 
+void V8Debugger::reportTermination() {
+  if (!m_terminateExecutionCallback) return;
+  m_isolate->RemoveCallCompletedCallback(
+      &V8Debugger::terminateExecutionCompletedCallback);
+  m_isolate->RemoveMicrotasksCompletedCallback(
+      &V8Debugger::terminateExecutionCompletedCallback);
+  m_isolate->CancelTerminateExecution();
+  m_terminateExecutionCallback->sendSuccess();
+  m_terminateExecutionCallback.reset();
+}
+
 void V8Debugger::terminateExecutionCompletedCallback(v8::Isolate* isolate) {
-  isolate->RemoveCallCompletedCallback(
-      &V8Debugger::terminateExecutionCompletedCallback);
-  isolate->RemoveMicrotasksCompletedCallback(
-      &V8Debugger::terminateExecutionCompletedCallback);
   V8InspectorImpl* inspector =
       static_cast<V8InspectorImpl*>(v8::debug::GetInspector(isolate));
   V8Debugger* debugger = inspector->debugger();
-  debugger->m_isolate->CancelTerminateExecution();
-  if (debugger->m_terminateExecutionCallback) {
-    debugger->m_terminateExecutionCallback->sendSuccess();
-    debugger->m_terminateExecutionCallback.reset();
-  }
+  debugger->reportTermination();
 }
 
 Response V8Debugger::continueToLocation(

src/inspector/v8-debugger.h

@@ -138,6 +138,8 @@ class V8Debugger : public v8::debug::DebugDelegate,
   V8InternalValueType getInternalType(v8::Local<v8::Context> context,
                                       v8::Local<v8::Object> object);
 
+  void reportTermination();
+
  private:
   void clearContinueToLocation();
   bool shouldContinueToCurrentLocation();
@@ -145,7 +147,6 @@ class V8Debugger : public v8::debug::DebugDelegate,
   static size_t nearHeapLimitCallback(void* data, size_t current_heap_limit,
                                       size_t initial_heap_limit);
   static void terminateExecutionCompletedCallback(v8::Isolate* isolate);
-
   void handleProgramBreak(
       v8::Local<v8::Context> pausedContext, v8::Local<v8::Value> exception,
       const std::vector<v8::debug::BreakpointId>& hitBreakpoints,
@@ -203,6 +204,7 @@ class V8Debugger : public v8::debug::DebugDelegate,
   v8::Isolate* m_isolate;
   V8InspectorImpl* m_inspector;
   int m_enableCount;
+
   int m_breakpointsActiveCount = 0;
   int m_ignoreScriptParsedEventsCounter;
   size_t m_originalHeapLimit = 0;

src/inspector/v8-inspector-impl.cc

@@ -399,8 +399,11 @@ void V8InspectorImpl::forEachSession(
   }
 }
 
-V8InspectorImpl::EvaluateScope::EvaluateScope(v8::Isolate* isolate)
-    : m_isolate(isolate), m_safeForTerminationScope(isolate) {}
+V8InspectorImpl::EvaluateScope::EvaluateScope(
+    const InjectedScript::Scope& scope)
+    : m_scope(scope),
+      m_isolate(scope.inspector()->isolate()),
+      m_safeForTerminationScope(m_isolate) {}
 
 struct V8InspectorImpl::EvaluateScope::CancelToken {
   v8::base::Mutex m_mutex;
@@ -408,6 +411,9 @@ struct V8InspectorImpl::EvaluateScope::CancelToken {
 };
 
 V8InspectorImpl::EvaluateScope::~EvaluateScope() {
+  if (m_scope.tryCatch().HasTerminated()) {
+    m_scope.inspector()->debugger()->reportTermination();
+  }
   if (m_cancelToken) {
     v8::base::MutexGuard lock(&m_cancelToken->m_mutex);
     m_cancelToken->m_canceled = true;

src/inspector/v8-inspector-impl.h

@@ -37,6 +37,7 @@
 
 #include "src/base/macros.h"
 #include "src/base/platform/mutex.h"
+#include "src/inspector/injected-script.h"
 #include "src/inspector/protocol/Protocol.h"
 
 #include "include/v8-inspector.h"
@@ -127,15 +128,17 @@ class V8InspectorImpl : public V8Inspector {
 
   class EvaluateScope {
    public:
-    explicit EvaluateScope(v8::Isolate* isolate);
+    explicit EvaluateScope(const InjectedScript::Scope& scope);
     ~EvaluateScope();
 
     protocol::Response setTimeout(double timeout);
 
    private:
-    v8::Isolate* m_isolate;
     class TerminateTask;
     struct CancelToken;
+
+    const InjectedScript::Scope& m_scope;
+    v8::Isolate* m_isolate;
     std::shared_ptr<CancelToken> m_cancelToken;
     v8::Isolate::SafeForTerminationScope m_safeForTerminationScope;
   };

src/inspector/v8-runtime-agent-impl.cc

@@ -261,7 +261,7 @@ void V8RuntimeAgentImpl::evaluate(
   scope.allowCodeGenerationFromStrings();
   v8::MaybeLocal<v8::Value> maybeResultValue;
   {
-    V8InspectorImpl::EvaluateScope evaluateScope(m_inspector->isolate());
+    V8InspectorImpl::EvaluateScope evaluateScope(scope);
     if (timeout.isJust()) {
       response = evaluateScope.setTimeout(timeout.fromJust() / 1000.0);
       if (!response.isSuccess()) {

src/isolate.cc

@@ -1889,9 +1889,17 @@ void Isolate::RestorePendingMessageFromTryCatch(v8::TryCatch* handler) {
 void Isolate::CancelScheduledExceptionFromTryCatch(v8::TryCatch* handler) {
   DCHECK(has_scheduled_exception());
   if (scheduled_exception() == handler->exception_) {
-    DCHECK(scheduled_exception() !=
+    DCHECK_NE(scheduled_exception(),
               ReadOnlyRoots(heap()).termination_exception());
     clear_scheduled_exception();
+  } else {
+    DCHECK_EQ(scheduled_exception(),
+              ReadOnlyRoots(heap()).termination_exception());
+    // Clear termination once we returned from all V8 frames.
+    if (handle_scope_implementer()->CallDepthIsZero()) {
+      thread_local_top()->external_caught_exception_ = false;
+      clear_scheduled_exception();
+    }
   }
   if (thread_local_top_.pending_message_obj_ == handler->message_obj_) {
     clear_pending_message();
@@ -2258,19 +2266,15 @@ MessageLocation Isolate::GetMessageLocation() {
   return MessageLocation();
 }
 
-
-bool Isolate::OptionalRescheduleException(bool is_bottom_call) {
+bool Isolate::OptionalRescheduleException(bool clear_exception) {
   DCHECK(has_pending_exception());
   PropagatePendingExceptionToExternalTryCatch();
 
   bool is_termination_exception =
       pending_exception() == ReadOnlyRoots(this).termination_exception();
 
-  // Do not reschedule the exception if this is the bottom call.
-  bool clear_exception = is_bottom_call;
-
   if (is_termination_exception) {
-    if (is_bottom_call) {
+    if (clear_exception) {
       thread_local_top()->external_caught_exception_ = false;
       clear_pending_exception();
       return false;

src/isolate.h

@@ -764,7 +764,7 @@ class Isolate final : private HiddenFactory {
   // exceptions.  If an exception was thrown and not handled by an external
   // handler the exception is scheduled to be rethrown when we return to running
   // JavaScript code.  If an exception is scheduled true is returned.
-  V8_EXPORT_PRIVATE bool OptionalRescheduleException(bool is_bottom_call);
+  V8_EXPORT_PRIVATE bool OptionalRescheduleException(bool clear_exception);
 
   // Push and pop a promise and the current try-catch handler.
   void PushPromise(Handle<JSObject> promise);

test/cctest/test-thread-termination.cc

@@ -315,6 +315,7 @@ void ReenterAfterTermination(const v8::FunctionCallbackInfo<v8::Value>& args) {
   CHECK(try_catch.Exception()->IsNull());
   CHECK(try_catch.Message().IsEmpty());
   CHECK(!try_catch.CanContinue());
+  CHECK(try_catch.HasTerminated());
   CHECK(isolate->IsExecutionTerminating());
   script = v8::Local<v8::String>::New(isolate, reenter_script_2);
   v8::MaybeLocal<v8::Script> compiled_script =
@@ -357,6 +358,45 @@ TEST(TerminateAndReenterFromThreadItself) {
   reenter_script_2.Reset();
 }
 
+TEST(TerminateAndReenterFromThreadItselfWithOuterTryCatch) {
+  v8::Isolate* isolate = CcTest::isolate();
+  v8::HandleScope scope(isolate);
+  v8::Local<v8::ObjectTemplate> global = CreateGlobalTemplate(
+      isolate, TerminateCurrentThread, ReenterAfterTermination);
+  v8::Local<v8::Context> context = v8::Context::New(isolate, nullptr, global);
+  v8::Context::Scope context_scope(context);
+  CHECK(!v8::Isolate::GetCurrent()->IsExecutionTerminating());
+  // Create script strings upfront as it won't work when terminating.
+  reenter_script_1.Reset(isolate, v8_str("function f() {"
+                                         "  var term = true;"
+                                         "  try {"
+                                         "    while(true) {"
+                                         "      if (term) terminate();"
+                                         "      term = false;"
+                                         "    }"
+                                         "    fail();"
+                                         "  } catch(e) {"
+                                         "    fail();"
+                                         "  }"
+                                         "}"
+                                         "f()"));
+  reenter_script_2.Reset(isolate, v8_str("function f() { fail(); } f()"));
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("try { loop(); fail(); } catch(e) { fail(); }");
+    CHECK(try_catch.HasCaught());
+    CHECK(try_catch.Exception()->IsNull());
+    CHECK(try_catch.Message().IsEmpty());
+    CHECK(!try_catch.CanContinue());
+    CHECK(try_catch.HasTerminated());
+    CHECK(isolate->IsExecutionTerminating());
+  }
+  CHECK(!isolate->IsExecutionTerminating());
+  // Check we can run JS again after termination.
+  CHECK(CompileRun("function f() { return true; } f()")->IsTrue());
+  reenter_script_1.Reset();
+  reenter_script_2.Reset();
+}
 
 void DoLoopCancelTerminate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::TryCatch try_catch(args.GetIsolate());
@@ -746,6 +786,7 @@ TEST(TerminateAndTryCall) {
   v8::Local<v8::Context> context = v8::Context::New(isolate, nullptr, global);
   v8::Context::Scope context_scope(context);
   CHECK(!isolate->IsExecutionTerminating());
+  {
     v8::TryCatch try_catch(isolate);
     CHECK(!isolate->IsExecutionTerminating());
     // Terminate execution has been triggered inside TryCall, but re-requested
@@ -760,7 +801,8 @@ TEST(TerminateAndTryCall) {
     CHECK(value->IsFunction());
     // The first stack check after terminate has been re-requested fails.
     CHECK(CompileRun("1 + 1").IsEmpty());
-  CHECK(!isolate->IsExecutionTerminating());
+    CHECK(isolate->IsExecutionTerminating());
+  }
   // V8 then recovers.
   v8::Maybe<int32_t> result = CompileRun("2 + 2")->Int32Value(
       v8::Isolate::GetCurrent()->GetCurrentContext());
@@ -791,7 +833,7 @@ TEST(TerminateConsole) {
   CHECK(!isolate->IsExecutionTerminating());
   CHECK(CompileRun("terminate(); console.log(); fail();").IsEmpty());
   CHECK(try_catch.HasCaught());
-  CHECK(!isolate->IsExecutionTerminating());
+  CHECK(isolate->IsExecutionTerminating());
 }
 
 class TerminatorSleeperThread : public v8::base::Thread {
@@ -830,7 +872,7 @@ TEST(TerminateRegExp) {
   CHECK(CompileRun("re.test('xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'); fail();")
             .IsEmpty());
   CHECK(try_catch.HasCaught());
-  CHECK(!isolate->IsExecutionTerminating());
+  CHECK(isolate->IsExecutionTerminating());
 #endif  // V8_INTERPRETED_REGEXP
 }
 

test/inspector/debugger/terminate-execution-on-pause-expected.txt

@@ -3,3 +3,58 @@ Tests Runtime.terminateExecution on pause
 Running test: testTerminateOnDebugger
 
 Running test: testTerminateAtBreakpoint
+{
+    error : {
+        code : -32000
+        message : Execution was terminated
+    }
+    id : <messageId>
+}
+
+Running test: testTerminateRuntimeEvaluate
+{
+    id : <messageId>
+    result : {
+    }
+}
+{
+    error : {
+        code : -32000
+        message : Execution was terminated
+    }
+    id : <messageId>
+}
+{
+    description : 42
+    type : number
+    value : 42
+}
+
+Running test: testTerminateRuntimeEvaluateOnCallFrame
+{
+    id : <messageId>
+    result : {
+        result : {
+            description : 1
+            type : number
+            value : 1
+        }
+    }
+}
+{
+    id : <messageId>
+    result : {
+    }
+}
+{
+    error : {
+        code : -32000
+        message : Execution was terminated
+    }
+    id : <messageId>
+}
+{
+    description : 43
+    type : number
+    value : 43
+}

test/inspector/debugger/terminate-execution-on-pause.js

@@ -22,22 +22,63 @@ callback();
 //# sourceURL=test.js`});
     await Protocol.Debugger.oncePaused();
     const terminated = Protocol.Runtime.terminateExecution();
-    Protocol.Debugger.resume();
+    await Protocol.Debugger.resume();
     await terminated;
   },
 
   async function testTerminateAtBreakpoint() {
     Protocol.Debugger.setBreakpointByUrl({url: 'test.js', lineNumber: 2});
-    Protocol.Runtime.evaluate({expression: `
+    const result = Protocol.Runtime.evaluate({expression: `
 function callback() {
   console.log(42);
   setTimeout(callback, 0);
 }
 callback();
-//# sourceURL=test.js`});
+//# sourceURL=test.js`}).then(InspectorTest.logMessage);
     await Protocol.Debugger.oncePaused();
     const terminated = Protocol.Runtime.terminateExecution();
-    Protocol.Debugger.resume();
+    await Protocol.Debugger.resume();
     await terminated;
-  }
+    await result;
+  },
+
+  async function testTerminateRuntimeEvaluate() {
+    Protocol.Runtime.evaluate({expression: `
+function callback() {
+  debugger;
+  console.log(42);
+  debugger;
+}
+callback();
+//# sourceURL=test.js`});
+    await Protocol.Debugger.oncePaused();
+    await Promise.all([
+      Protocol.Runtime.terminateExecution().then(InspectorTest.logMessage),
+      Protocol.Runtime.evaluate({expression: 'console.log(42)'}).then(InspectorTest.logMessage)
+    ]);
+    await Protocol.Debugger.resume();
+    await Protocol.Debugger.oncePaused();
+    await Protocol.Debugger.resume();
+  },
+
+  async function testTerminateRuntimeEvaluateOnCallFrame() {
+    Protocol.Runtime.evaluate({expression: `
+function callback() {
+  let a = 1;
+  debugger;
+  console.log(43);
+}
+callback();
+//# sourceURL=test.js`});
+    let message = await Protocol.Debugger.oncePaused();
+    let topFrameId = message.params.callFrames[0].callFrameId;
+    await Protocol.Debugger.evaluateOnCallFrame({callFrameId: topFrameId, expression: "a"})
+    .then(InspectorTest.logMessage)
+    await Promise.all([
+      Protocol.Runtime.terminateExecution().then(InspectorTest.logMessage),
+      Protocol.Debugger.evaluateOnCallFrame({callFrameId: topFrameId, expression: "a"})
+          .then(InspectorTest.logMessage)
+    ]);
+    await Protocol.Debugger.resume();
+  },
 ]);