[debugger] Allow termination-on-resume when paused at a breakpoint

This CL implements functionality to allow an embedder to mark a
debug scope as terminate-on-resume. This results in a termination
exception when that debug scope is left and execution is resumed.
Execution of JavaScript remains possible after a debug scope is
marked as terminate-on-resume (but before execution of the paused
code resumes).
This is used by blink to correctly prevent resuming JavaScript
execution upon reload while being paused at a breakpoint.

This is important for handling reloads while paused at a breakpoint
in blink. The resume command terminates blink's nested message loop
that is used while to keep the frame responsive while the debugger
is paused. But if a reload is triggered while execution is paused
on a breakpoint, but before execution is actually resumed from the
 breakpoint (that means before returning into the V8 JavaScript
frames that are paused on the stack below the C++ frames that belong
to the nested message loop), we re-enter V8 to do tear-down actions
of the old frame. In this case Runtime.terminateExecution() cannot be
used before Debugger.resume(), because the tear-down actions that
re-enter V8 would trigger the termination exception and crash the
browser (because the browser expected the tear-down to succeed).

Hence we introduce this flag on V8 that says: It is OK if someone
re-enters V8 (to execute JS), but upon resuming from the breakpoint
(i.e. returning to the paused frames that are on the stack below),
generate a termination exception.

We deliberated adding a corresponding logic on the blink side (instead
of V8) but we think this is the simplest solution.

More details in the design doc:

https://docs.google.com/document/d/1aO9v0YhoKNqKleqfACGUpwrBUayLFGqktz9ltdgKHMk

Bug: chromium:1004038, chromium:1014415

Change-Id: I896692d4c21cb0acae89c1d783d37ce45b73c113
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1924366
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66084}

include/js_protocol.pdl

@@ -273,6 +273,13 @@ domain Debugger
 
   # Resumes JavaScript execution.
   command resume
+    parameters
+      # Set to true to terminate execution upon resuming execution. In contrast
+      # to Runtime.terminateExecution, this will allows to execute further
+      # JavaScript (i.e. via evaluation) until execution of the paused code
+      # is actually resumed, at which point termination is triggered.
+      # If execution is currently not paused, this parameter has no effect.
+      optional boolean terminateOnResume
 
   # Searches for given string in script content.
   command searchInContent

include/v8-inspector.h

@@ -145,7 +145,7 @@ class V8_EXPORT V8InspectorSession {
   virtual void breakProgram(const StringView& breakReason,
                             const StringView& breakDetails) = 0;
   virtual void setSkipAllPauses(bool) = 0;
-  virtual void resume() = 0;
+  virtual void resume(bool setTerminateOnResume = false) = 0;
   virtual void stepOver() = 0;
   virtual std::vector<std::unique_ptr<protocol::Debugger::API::SearchMatch>>
   searchInTextByLines(const StringView& text, const StringView& query,

src/api/api.cc

@@ -9443,6 +9443,12 @@ void debug::BreakRightNow(Isolate* v8_isolate) {
   isolate->debug()->HandleDebugBreak(i::kIgnoreIfAllFramesBlackboxed);
 }
 
+void debug::SetTerminateOnResume(Isolate* v8_isolate) {
+  i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
+  isolate->debug()->SetTerminateOnResume();
+}
+
 bool debug::AllFramesOnStackAreBlackboxed(Isolate* v8_isolate) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
   ENTER_V8_DO_NOT_USE(isolate);

src/debug/debug-interface.h

@@ -101,6 +101,13 @@ void PrepareStep(Isolate* isolate, StepAction action);
 void ClearStepping(Isolate* isolate);
 V8_EXPORT_PRIVATE void BreakRightNow(Isolate* isolate);
 
+// Use `SetTerminateOnResume` to indicate that an TerminateExecution interrupt
+// should be set shortly before resuming, i.e. shortly before returning into
+// the JavaScript stack frames on the stack. In contrast to setting the
+// interrupt with `RequestTerminateExecution` directly, this flag allows
+// the isolate to be entered for further JavaScript execution.
+V8_EXPORT_PRIVATE void SetTerminateOnResume(Isolate* isolate);
+
 bool AllFramesOnStackAreBlackboxed(Isolate* isolate);
 
 class Script;

src/debug/debug.cc

@@ -1718,8 +1718,8 @@ Handle<FixedArray> Debug::GetLoadedScripts() {
   return FixedArray::ShrinkOrEmpty(isolate_, results, length);
 }
 
-void Debug::OnThrow(Handle<Object> exception) {
-  if (in_debug_scope() || ignore_events()) return;
+base::Optional<Object> Debug::OnThrow(Handle<Object> exception) {
+  if (in_debug_scope() || ignore_events()) return {};
   // Temporarily clear any scheduled_exception to allow evaluating
   // JavaScript from the debug event handler.
   HandleScope scope(isolate_);
@@ -1736,6 +1736,14 @@ void Debug::OnThrow(Handle<Object> exception) {
     isolate_->thread_local_top()->scheduled_exception_ = *scheduled_exception;
   }
   PrepareStepOnThrow();
+  // If the OnException handler requested termination, then indicated this to
+  // our caller Isolate::Throw so it can deal with it immediatelly instead of
+  // throwing the original exception.
+  if (isolate_->stack_guard()->CheckTerminateExecution()) {
+    isolate_->stack_guard()->ClearTerminateExecution();
+    return isolate_->TerminateExecution();
+  }
+  return {};
 }
 
 void Debug::OnPromiseReject(Handle<Object> promise, Handle<Object> value) {
@@ -2092,7 +2100,6 @@ DebugScope::DebugScope(Debug* debug)
   // Link recursive debugger entry.
   base::Relaxed_Store(&debug_->thread_local_.current_debug_scope_,
                       reinterpret_cast<base::AtomicWord>(this));
-
   // Store the previous frame id and return value.
   break_frame_id_ = debug_->break_frame_id();
 
@@ -2106,8 +2113,18 @@ DebugScope::DebugScope(Debug* debug)
   debug_->UpdateState();
 }
 
+void DebugScope::set_terminate_on_resume() { terminate_on_resume_ = true; }
 
 DebugScope::~DebugScope() {
+  // Terminate on resume must have been handled by retrieving it, if this is
+  // the outer scope.
+  if (terminate_on_resume_) {
+    if (!prev_) {
+      debug_->isolate_->stack_guard()->RequestTerminateExecution();
+    } else {
+      prev_->set_terminate_on_resume();
+    }
+  }
   // Leaving this debugger entry.
   base::Relaxed_Store(&debug_->thread_local_.current_debug_scope_,
                       reinterpret_cast<base::AtomicWord>(prev_));
@@ -2147,6 +2164,13 @@ void Debug::UpdateDebugInfosForExecutionMode() {
   }
 }
 
+void Debug::SetTerminateOnResume() {
+  DebugScope* scope = reinterpret_cast<DebugScope*>(
+      base::Acquire_Load(&thread_local_.current_debug_scope_));
+  CHECK_NOT_NULL(scope);
+  scope->set_terminate_on_resume();
+}
+
 void Debug::StartSideEffectCheckMode() {
   DCHECK(isolate_->debug_execution_mode() != DebugInfo::kSideEffects);
   isolate_->set_debug_execution_mode(DebugInfo::kSideEffects);

src/debug/debug.h

@@ -217,7 +217,8 @@ class V8_EXPORT_PRIVATE Debug {
   // Debug event triggers.
   void OnDebugBreak(Handle<FixedArray> break_points_hit);
 
-  void OnThrow(Handle<Object> exception);
+  base::Optional<Object> OnThrow(Handle<Object> exception)
+      V8_WARN_UNUSED_RESULT;
   void OnPromiseReject(Handle<Object> promise, Handle<Object> value);
   void OnCompileError(Handle<Script> script);
   void OnAfterCompile(Handle<Script> script);
@@ -238,6 +239,8 @@ class V8_EXPORT_PRIVATE Debug {
   void ChangeBreakOnException(ExceptionBreakType type, bool enable);
   bool IsBreakOnException(ExceptionBreakType type);
 
+  void SetTerminateOnResume();
+
   bool SetBreakPointForScript(Handle<Script> script, Handle<String> condition,
                               int* source_position, int* id);
   bool SetBreakpointForFunction(Handle<SharedFunctionInfo> shared,
@@ -564,6 +567,8 @@ class DebugScope {
   explicit DebugScope(Debug* debug);
   ~DebugScope();
 
+  void set_terminate_on_resume();
+
  private:
   Isolate* isolate() { return debug_->isolate_; }
 
@@ -571,6 +576,8 @@ class DebugScope {
   DebugScope* prev_;               // Previous scope if entered recursively.
   StackFrameId break_frame_id_;    // Previous break frame id.
   PostponeInterruptsScope no_interrupts_;
+  // This is used as a boolean.
+  bool terminate_on_resume_ = false;
 };
 
 // This scope is used to handle return values in nested debug break points.

src/execution/isolate.cc

@@ -1523,7 +1523,10 @@ Object Isolate::Throw(Object raw_exception, MessageLocation* location) {
 
   // Notify debugger of exception.
   if (is_catchable_by_javascript(raw_exception)) {
-    debug()->OnThrow(exception);
+    base::Optional<Object> maybe_exception = debug()->OnThrow(exception);
+    if (maybe_exception.has_value()) {
+      return *maybe_exception;
+    }
   }
 
   // Generate the message if required.

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

@@ -1034,10 +1034,11 @@ Response V8DebuggerAgentImpl::pause() {
   return Response::OK();
 }
 
-Response V8DebuggerAgentImpl::resume() {
+Response V8DebuggerAgentImpl::resume(Maybe<bool> terminateOnResume) {
   if (!isPaused()) return Response::Error(kDebuggerNotPaused);
   m_session->releaseObjectGroup(kBacktraceObjectGroup);
-  m_debugger->continueProgram(m_session->contextGroupId());
+  m_debugger->continueProgram(m_session->contextGroupId(),
+                              terminateOnResume.fromMaybe(false));
   return Response::OK();
 }
 

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

@@ -98,7 +98,7 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   Response getWasmBytecode(const String16& scriptId,
                            protocol::Binary* bytecode) override;
   Response pause() override;
-  Response resume() override;
+  Response resume(Maybe<bool> terminateOnResume) override;
   Response stepOver() override;
   Response stepInto(Maybe<bool> inBreakOnAsyncCall) override;
   Response stepOut() override;

src/inspector/v8-debugger.cc

@@ -245,9 +245,15 @@ void V8Debugger::interruptAndBreak(int targetContextGroupId) {
       nullptr);
 }
 
-void V8Debugger::continueProgram(int targetContextGroupId) {
+void V8Debugger::continueProgram(int targetContextGroupId,
+                                 bool terminateOnResume) {
   if (m_pausedContextGroupId != targetContextGroupId) return;
-  if (isPaused()) m_inspector->client()->quitMessageLoopOnPause();
+  if (isPaused()) {
+    if (terminateOnResume) {
+      v8::debug::SetTerminateOnResume(m_isolate);
+    }
+    m_inspector->client()->quitMessageLoopOnPause();
+  }
 }
 
 void V8Debugger::breakProgramOnAssert(int targetContextGroupId) {

src/inspector/v8-debugger.h

@@ -77,7 +77,8 @@ class V8Debugger : public v8::debug::DebugDelegate,
   bool canBreakProgram();
   void breakProgram(int targetContextGroupId);
   void interruptAndBreak(int targetContextGroupId);
-  void continueProgram(int targetContextGroupId);
+  void continueProgram(int targetContextGroupId,
+                       bool terminateOnResume = false);
   void breakProgramOnAssert(int targetContextGroupId);
 
   void setPauseOnNextCall(bool, int targetContextGroupId);

src/inspector/v8-inspector-session-impl.cc

@@ -448,7 +448,9 @@ void V8InspectorSessionImpl::setSkipAllPauses(bool skip) {
   m_debuggerAgent->setSkipAllPauses(skip);
 }
 
-void V8InspectorSessionImpl::resume() { m_debuggerAgent->resume(); }
+void V8InspectorSessionImpl::resume(bool terminateOnResume) {
+  m_debuggerAgent->resume(terminateOnResume);
+}
 
 void V8InspectorSessionImpl::stepOver() { m_debuggerAgent->stepOver(); }
 

src/inspector/v8-inspector-session-impl.h

@@ -76,7 +76,7 @@ class V8InspectorSessionImpl : public V8InspectorSession,
   void breakProgram(const StringView& breakReason,
                     const StringView& breakDetails) override;
   void setSkipAllPauses(bool) override;
-  void resume() override;
+  void resume(bool terminateOnResume = false) override;
   void stepOver() override;
   std::vector<std::unique_ptr<protocol::Debugger::API::SearchMatch>>
   searchInTextByLines(const StringView& text, const StringView& query,