[debug] disable debug breaks in side-effect free debug-evaluate.

hashseed · Commit Bot · commit cc9736a1c0f4 · 2018-03-26T16:27:32.000Z
We don't want to run into the situation of breaking inside of debug-evaluate. That would get even more confusing with throw-on-side-effect. R=kozyatinskiy@chromium.org Bug: v8:7592 Change-Id: I93f5de63d8943792ff000dbf7c6311df655d3793 Reviewed-on: https://chromium-review.googlesource.com/978164 Reviewed-by: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Commit-Queue: Yang Guo <yangguo@chromium.org> Cr-Commit-Position: refs/heads/master@{#52227}
diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc
@@ -24,6 +24,9 @@ namespace internal {
 MaybeHandle<Object> DebugEvaluate::Global(Isolate* isolate,
                                           Handle<String> source,
                                           bool throw_on_side_effect) {
+  // Disable breaks in side-effect free mode.
+  DisableBreak disable_break_scope(isolate->debug(), throw_on_side_effect);
+
   Handle<Context> context = isolate->native_context();
   ScriptOriginOptions origin_options(false, true);
   MaybeHandle<SharedFunctionInfo> maybe_function_info =
diff --git a/src/debug/debug.cc b/src/debug/debug.cc
@@ -2388,7 +2388,7 @@ NoSideEffectScope::~NoSideEffectScope() {
     isolate_->Throw(*isolate_->factory()->NewEvalError(
         MessageTemplate::kNoSideEffectDebugEvaluate));
   }
-  isolate_->set_needs_side_effect_check(old_needs_side_effect_check_);
+  isolate_->set_needs_side_effect_check(false);
   isolate_->debug()->UpdateHookOnFunctionCall();
   isolate_->debug()->side_effect_check_failed_ = false;
 }
diff --git a/src/debug/debug.h b/src/debug/debug.h
@@ -700,9 +700,9 @@ class ReturnValueScope {
 // Stack allocated class for disabling break.
 class DisableBreak BASE_EMBEDDED {
  public:
-  explicit DisableBreak(Debug* debug)
+  explicit DisableBreak(Debug* debug, bool disable = true)
       : debug_(debug), previous_break_disabled_(debug->break_disabled_) {
-    debug_->break_disabled_ = true;
+    debug_->break_disabled_ = disable;
   }
   ~DisableBreak() {
     debug_->break_disabled_ = previous_break_disabled_;
@@ -732,18 +732,17 @@ class SuppressDebug BASE_EMBEDDED {
 class NoSideEffectScope {
  public:
   NoSideEffectScope(Isolate* isolate, bool disallow_side_effects)
-      : isolate_(isolate),
-        old_needs_side_effect_check_(isolate->needs_side_effect_check()) {
-    isolate->set_needs_side_effect_check(old_needs_side_effect_check_ ||
-                                         disallow_side_effects);
+      : isolate_(isolate) {
+    // NoSideEffectScope is not re-entrant if already enabled.
+    CHECK(!isolate->needs_side_effect_check());
+    isolate->set_needs_side_effect_check(disallow_side_effects);
     isolate->debug()->UpdateHookOnFunctionCall();
     isolate->debug()->side_effect_check_failed_ = false;
   }
   ~NoSideEffectScope();
 
  private:
   Isolate* isolate_;
-  bool old_needs_side_effect_check_;
   DISALLOW_COPY_AND_ASSIGN(NoSideEffectScope);
 };
 
diff --git a/test/inspector/runtime/evaluate-without-side-effects-expected.txt b/test/inspector/runtime/evaluate-without-side-effects-expected.txt
@@ -48,3 +48,26 @@ Test expression without side-effect, with throwOnSideEffect: true
         }
     }
 }
+Test that debug break triggers without throwOnSideEffect
+paused
+{
+    id : <messageId>
+    result : {
+        result : {
+            description : 2
+            type : number
+            value : 2
+        }
+    }
+}
+Test that debug break does not trigger with throwOnSideEffect
+{
+    id : <messageId>
+    result : {
+        result : {
+            description : 2
+            type : number
+            value : 2
+        }
+    }
+}
diff --git a/test/inspector/runtime/evaluate-without-side-effects.js b/test/inspector/runtime/evaluate-without-side-effects.js
@@ -4,8 +4,19 @@
 
 let {session, contextGroup, Protocol} = InspectorTest.start("Tests that Runtime.evaluate can run without side effects.");
 
+session.setupScriptMap();
+contextGroup.addScript(`
+    function f() {
+      return 2;
+    } //# sourceURL=test.js`);
 Protocol.Runtime.enable();
 Protocol.Debugger.enable();
+
+Protocol.Debugger.onPaused(message => {
+  InspectorTest.log("paused");
+  Protocol.Debugger.resume();
+});
+
 (async function() {
   InspectorTest.log("Test throwOnSideEffect: false");
   InspectorTest.logMessage(await Protocol.Runtime.evaluate({
@@ -25,5 +36,18 @@ Protocol.Debugger.enable();
     throwOnSideEffect: true
   }));
 
+  InspectorTest.log("Test that debug break triggers without throwOnSideEffect");
+  await Protocol.Debugger.setBreakpointByUrl({ url: 'test.js', lineNumber: 2 });
+  InspectorTest.logMessage(await Protocol.Runtime.evaluate({
+    expression: "f()",
+    throwOnSideEffect: false
+  }));
+
+  InspectorTest.log("Test that debug break does not trigger with throwOnSideEffect");
+  InspectorTest.logMessage(await Protocol.Runtime.evaluate({
+    expression: "f()",
+    throwOnSideEffect: true
+  }));
+
   InspectorTest.completeTest();
 })();

Original file line number	Diff line number	Diff line change
`@@ -2388,7 +2388,7 @@ NoSideEffectScope::~NoSideEffectScope() {`
`2388`	`2388`	`isolate_->Throw(*isolate_->factory()->NewEvalError(`
`2389`	`2389`	`MessageTemplate::kNoSideEffectDebugEvaluate));`
`2390`	`2390`	`}`
`2391`		`- isolate_->set_needs_side_effect_check(old_needs_side_effect_check_);`
	`2391`	`+ isolate_->set_needs_side_effect_check(false);`
`2392`	`2392`	`isolate_->debug()->UpdateHookOnFunctionCall();`
`2393`	`2393`	`isolate_->debug()->side_effect_check_failed_ = false;`
`2394`	`2394`	`}`