[Debugger] Add a ReturnValueScope to correctly handle return values in nested debug breaks.

Decouples return value handling from DebugScope when handling nested break
statements. Return values are handled in ReturnValueScope. This would correctly
reset the return_values when exiting the break statements.

BUG=v8:688950

Review-Url: https://codereview.chromium.org/2702343003
Cr-Commit-Position: refs/heads/master@{#43343}

src/debug/debug.cc

@@ -481,8 +481,6 @@ void Debug::Unload() {
 }
 
 void Debug::Break(JavaScriptFrame* frame) {
-  HandleScope scope(isolate_);
-
   // Initialize LiveEdit.
   LiveEdit::InitializeThreadLocal(this);
 
@@ -2206,7 +2204,6 @@ DebugScope::DebugScope(Debug* debug)
   // Store the previous break id, frame id and return value.
   break_id_ = debug_->break_id();
   break_frame_id_ = debug_->break_frame_id();
-  return_value_ = handle(debug_->return_value(), isolate());
 
   // Create the new break info. If there is no proper frames there is no break
   // frame id.
@@ -2232,11 +2229,18 @@ DebugScope::~DebugScope() {
   // Restore to the previous break state.
   debug_->thread_local_.break_frame_id_ = break_frame_id_;
   debug_->thread_local_.break_id_ = break_id_;
-  debug_->thread_local_.return_value_ = *return_value_;
 
   debug_->UpdateState();
 }
 
+ReturnValueScope::ReturnValueScope(Debug* debug) : debug_(debug) {
+  return_value_ = debug_->return_value_handle();
+}
+
+ReturnValueScope::~ReturnValueScope() {
+  debug_->set_return_value(*return_value_);
+}
+
 bool Debug::PerformSideEffectCheck(Handle<JSFunction> function) {
   DCHECK(isolate_->needs_side_effect_check());
   DisallowJavascriptExecution no_js(isolate_);

src/debug/debug.h

@@ -393,6 +393,9 @@ class Debug {
   StackFrame::Id break_frame_id() { return thread_local_.break_frame_id_; }
   int break_id() { return thread_local_.break_id_; }
 
+  Handle<Object> return_value_handle() {
+    return handle(thread_local_.return_value_, isolate_);
+  }
   Object* return_value() { return thread_local_.return_value_; }
   void set_return_value(Object* value) { thread_local_.return_value_ = value; }
 
@@ -692,12 +695,24 @@ class DebugScope BASE_EMBEDDED {
   DebugScope* prev_;               // Previous scope if entered recursively.
   StackFrame::Id break_frame_id_;  // Previous break frame id.
   int break_id_;                   // Previous break id.
-  Handle<Object> return_value_;    // Previous result.
   bool failed_;                    // Did the debug context fail to load?
   SaveContext save_;               // Saves previous context.
   PostponeInterruptsScope no_termination_exceptons_;
 };
 
+// This scope is used to handle return values in nested debug break points.
+// When there are nested debug breaks, we use this to restore the return
+// value to the previous state. This is not merged with DebugScope because
+// return_value_ will not be cleared when we use DebugScope.
+class ReturnValueScope {
+ public:
+  explicit ReturnValueScope(Debug* debug);
+  ~ReturnValueScope();
+
+ private:
+  Debug* debug_;
+  Handle<Object> return_value_;  // Previous result.
+};
 
 // Stack allocated class for disabling break.
 class DisableBreak BASE_EMBEDDED {

src/runtime/runtime-debug.cc

@@ -28,6 +28,8 @@ RUNTIME_FUNCTION(Runtime_DebugBreak) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 0);
+  HandleScope scope(isolate);
+  ReturnValueScope result_scope(isolate->debug());
   isolate->debug()->set_return_value(*value);
 
   // Get the top-most JavaScript frame.
@@ -40,6 +42,8 @@ RUNTIME_FUNCTION(Runtime_DebugBreakOnBytecode) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(1, args.length());
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 0);
+  HandleScope scope(isolate);
+  ReturnValueScope result_scope(isolate->debug());
   isolate->debug()->set_return_value(*value);
 
   // Get the top-most JavaScript frame.