[debugger] debug-evaluate should not not modify local values.

Debug evaluate no longer writes back changes to the replicated
context chain to the original after execution. Changes to the
global object or script contexts still stick. Calling functions
that bind to the original context chain also have their expected
side effects.

As far as I can tell, DevTools is not interested in modifying
local variable values. Modifying global variable values still
works as expected. However, I have not yet removed the old
implementation, but merely keep it behind a flag.

R=mstarzinger@chromium.org, rossberg@chromium.org

Review URL: https://codereview.chromium.org/1513183003

Cr-Commit-Position: refs/heads/master@{#32841}

src/debug/debug-evaluate.cc

@@ -68,16 +68,19 @@ MaybeHandle<Object> DebugEvaluate::Local(Isolate* isolate,
   // variables accessible by the function we are evaluating from are
   // materialized and included on top of the native context. Changes to
   // the materialized object are written back afterwards.
+  // Note that the native context is taken from the original context chain,
+  // which may not be the current native context of the isolate.
   ContextBuilder context_builder(isolate, frame, inlined_jsframe_index);
   if (isolate->has_pending_exception()) return MaybeHandle<Object>();
 
-  Handle<Context> context = isolate->native_context();
+  Handle<Context> context = context_builder.native_context();
   Handle<JSObject> receiver(context->global_proxy());
-  Handle<SharedFunctionInfo> outer_info(context->closure()->shared(), isolate);
   MaybeHandle<Object> maybe_result = Evaluate(
       isolate, context_builder.outer_info(),
       context_builder.innermost_context(), context_extension, receiver, source);
-  if (!maybe_result.is_null()) context_builder.UpdateValues();
+  if (!maybe_result.is_null() && !FLAG_debug_eval_readonly_locals) {
+    context_builder.UpdateValues();
+  }
   return maybe_result;
 }
 
@@ -127,8 +130,8 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
   Handle<JSFunction> local_function =
       handle(JSFunction::cast(frame_inspector.GetFunction()));
   Handle<Context> outer_context(local_function->context());
-  Handle<Context> native_context = isolate->native_context();
-  Handle<JSFunction> global_function(native_context->closure());
+  native_context_ = Handle<Context>(outer_context->native_context());
+  Handle<JSFunction> global_function(native_context_->closure());
   outer_info_ = handle(global_function->shared());
   Handle<Context> inner_context;
 
@@ -166,7 +169,7 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
       // The "this" binding, if any, can't be bound via "with".  If we need
       // to, add another node onto the outer context to bind "this".
       Handle<Context> receiver_context =
-          MaterializeReceiver(native_context, local_context, local_function,
+          MaterializeReceiver(native_context_, local_context, local_function,
                               global_function, it.ThisIsNonLocal());
 
       Handle<JSObject> materialized_function = NewJSObjectWithNullProto();
@@ -309,7 +312,7 @@ void DebugEvaluate::ContextBuilder::MaterializeArgumentsObject(
 
 
 MaybeHandle<Object> DebugEvaluate::ContextBuilder::LoadFromContext(
-    Handle<Context> context, Handle<String> name) {
+    Handle<Context> context, Handle<String> name, bool* global) {
   static const ContextLookupFlags flags = FOLLOW_CONTEXT_CHAIN;
   int index;
   PropertyAttributes attributes;
@@ -320,9 +323,13 @@ MaybeHandle<Object> DebugEvaluate::ContextBuilder::LoadFromContext(
   Handle<Object> value;
   if (index != Context::kNotFound) {  // Found on context.
     Handle<Context> context = Handle<Context>::cast(holder);
+    // Do not shadow variables on the script context.
+    *global = context->IsScriptContext();
     return Handle<Object>(context->get(index), isolate_);
   } else {  // Found on object.
     Handle<JSReceiver> object = Handle<JSReceiver>::cast(holder);
+    // Do not shadow properties on the global object.
+    *global = object->IsJSGlobalObject();
     return JSReceiver::GetDataProperty(object, name);
   }
 }
@@ -333,7 +340,13 @@ void DebugEvaluate::ContextBuilder::MaterializeContextChain(
   for (const Handle<String>& name : non_locals_) {
     HandleScope scope(isolate_);
     Handle<Object> value;
-    if (!LoadFromContext(context, name).ToHandle(&value)) continue;
+    bool global;
+    if (!LoadFromContext(context, name, &global).ToHandle(&value) || global) {
+      // If resolving the variable fails, skip it. If it resolves to a global
+      // variable, skip it as well since it's not read-only and can be resolved
+      // within debug-evaluate.
+      continue;
+    }
     JSObject::SetOwnPropertyIgnoreAttributes(target, name, value, NONE).Check();
   }
 }
@@ -381,7 +394,8 @@ Handle<Context> DebugEvaluate::ContextBuilder::MaterializeReceiver(
   Handle<Object> receiver = isolate_->factory()->undefined_value();
   Handle<String> this_string = isolate_->factory()->this_string();
   if (this_is_non_local) {
-    LoadFromContext(lookup_context, this_string).ToHandle(&receiver);
+    bool global;
+    LoadFromContext(lookup_context, this_string, &global).ToHandle(&receiver);
   } else if (local_function->shared()->scope_info()->HasReceiver()) {
     receiver = handle(frame_->receiver(), isolate_);
   }

src/debug/debug-evaluate.h

@@ -54,6 +54,7 @@ class DebugEvaluate : public AllStatic {
     void UpdateValues();
 
     Handle<Context> innermost_context() const { return innermost_context_; }
+    Handle<Context> native_context() const { return native_context_; }
     Handle<SharedFunctionInfo> outer_info() const { return outer_info_; }
 
    private:
@@ -87,13 +88,14 @@ class DebugEvaluate : public AllStatic {
                                         bool this_is_non_local);
 
     MaybeHandle<Object> LoadFromContext(Handle<Context> context,
-                                        Handle<String> name);
+                                        Handle<String> name, bool* global);
 
     void StoreToContext(Handle<Context> context, Handle<String> name,
                         Handle<Object> value);
 
     Handle<SharedFunctionInfo> outer_info_;
     Handle<Context> innermost_context_;
+    Handle<Context> native_context_;
     List<ContextChainElement> context_chain_;
     List<Handle<String> > non_locals_;
     Isolate* isolate_;

src/flag-definitions.h

@@ -597,8 +597,7 @@ DEFINE_BOOL(cache_prototype_transitions, true, "cache prototype transitions")
 DEFINE_INT(cpu_profiler_sampling_interval, 1000,
            "CPU profiler sampling interval in microseconds")
 
-// debug.cc
-DEFINE_BOOL(trace_debug_json, false, "trace debugging JSON request/response")
+// Array abuse tracing
 DEFINE_BOOL(trace_js_array_abuse, false,
             "trace out-of-bounds accesses to JS arrays")
 DEFINE_BOOL(trace_external_array_abuse, false,
@@ -607,6 +606,11 @@ DEFINE_BOOL(trace_array_abuse, false,
             "trace out-of-bounds accesses to all arrays")
 DEFINE_IMPLICATION(trace_array_abuse, trace_js_array_abuse)
 DEFINE_IMPLICATION(trace_array_abuse, trace_external_array_abuse)
+
+// debugger
+DEFINE_BOOL(debug_eval_readonly_locals, true,
+            "do not update locals after debug-evaluate")
+DEFINE_BOOL(trace_debug_json, false, "trace debugging JSON request/response")
 DEFINE_BOOL(enable_liveedit, true, "enable liveedit experimental feature")
 DEFINE_BOOL(hard_abort, true, "abort by crashing")
 

test/mjsunit/debug-evaluate-closure.js

@@ -26,6 +26,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 // Flags: --expose-debug-as debug --allow-natives-syntax
+// Flags: --debug-eval-readonly-locals
 
 Debug = debug.Debug;
 var listened = false;
@@ -34,20 +35,18 @@ function listener(event, exec_state, event_data, data) {
   if (event != Debug.DebugEvent.Break) return;
   try {
     assertEquals("goo", exec_state.frame(0).evaluate("goo").value());
-    exec_state.frame(0).evaluate("goo = 'goo foo'");
+    exec_state.frame(0).evaluate("goo = 'goo foo'");  // no effect
     assertEquals("bar return", exec_state.frame(0).evaluate("bar()").value());
-    // Check that calling bar() has no effect to context-allocated variables.
-    // TODO(yangguo): reevaluate this if we no longer update context from copy.
-    assertEquals("inner", exec_state.frame(0).evaluate("inner").value());
-    assertEquals("outer", exec_state.frame(0).evaluate("outer").value());
+    assertEquals("inner bar", exec_state.frame(0).evaluate("inner").value());
+    assertEquals("outer bar", exec_state.frame(0).evaluate("outer").value());
 
     assertEquals("baz inner", exec_state.frame(0).evaluate("baz").value());
     assertEquals("baz outer", exec_state.frame(1).evaluate("baz").value());
     exec_state.frame(0).evaluate("w = 'w foo'");
-    exec_state.frame(0).evaluate("inner = 'inner foo'");
-    exec_state.frame(0).evaluate("outer = 'outer foo'");
-    exec_state.frame(0).evaluate("baz = 'baz inner foo'");
-    exec_state.frame(1).evaluate("baz = 'baz outer foo'");
+    exec_state.frame(0).evaluate("inner = 'inner foo'");  // no effect
+    exec_state.frame(0).evaluate("outer = 'outer foo'");  // has effect
+    exec_state.frame(0).evaluate("baz = 'baz inner foo'");  // no effect
+    exec_state.frame(1).evaluate("baz = 'baz outer foo'");  // has effect
     listened = true;
   } catch (e) {
     print(e);
@@ -69,9 +68,9 @@ function foo() {
 
   with (withv) {
     var bar = function bar() {
-      assertEquals("goo foo", goo);
-      inner = "inner bar";  // this has no effect, when called from debug-eval
-      outer = "outer bar";  // this has no effect, when called from debug-eval
+      assertEquals("goo", goo);
+      inner = "inner bar";
+      outer = "outer bar";
       v = "v bar";
       return "bar return";
     };
@@ -81,8 +80,8 @@ function foo() {
     debugger;
   }
 
-  assertEquals("inner foo", inner);
-  assertEquals("baz inner foo", baz);
+  assertEquals("inner bar", inner);
+  assertEquals("baz inner", baz);
   assertEquals("w foo", withw.w);
   assertEquals("v bar", withv.v);
 }

test/mjsunit/debug-evaluate-const.js

@@ -25,7 +25,7 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 
 Debug = debug.Debug
 
@@ -50,10 +50,8 @@ function f() {
 
     debugger;  // Break point.
 
-    assertEquals(30, var0);
-    // TODO(yangguo): debug evaluate should not be able to alter
-    //                stack-allocated const values
-    // assertEquals(0, const0);
+    assertEquals(undefined, var0);
+    assertEquals(0, const0);
     assertEquals(undefined, const1);
     assertEquals(undefined, const2);
     var var0 = 20;
@@ -66,7 +64,7 @@ function f() {
 
   g();
 
-  assertEquals(31, var1);
+  assertEquals(21, var1);
   assertEquals(3, const3);
 }
 

test/mjsunit/debug-evaluate-locals-capturing.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 
 Debug = debug.Debug
 var exception = null;
@@ -15,8 +15,8 @@ function listener(event, exec_state, event_data, data) {
     if (event == Debug.DebugEvent.Break) {
       var frameMirror = exec_state.frame(0);
 
-      f = frameMirror.evaluate('f = function() { i = 5; }, f(), f').value();
-      print(f);
+      var i = frameMirror.evaluate('f = function() { i = 5; }, f(), i').value();
+      assertEquals(5, i);
     }
   } catch(e) {
     exception = e;
@@ -35,7 +35,7 @@ Debug.setListener(listener);
   } catch (e) {
     assertEquals(0, i);
     debugger;
-    assertEquals(5, i);
+    assertEquals(0, i);
   }
 }());
 

test/mjsunit/debug-evaluate-locals.js

@@ -25,7 +25,7 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 // Get the Debug object exposed from the debug context global object.
 Debug = debug.Debug
 
@@ -127,6 +127,7 @@ function listener(event, exec_state, event_data, data) {
       assertEquals(6, exec_state.frame(2).evaluate('b').value());
       assertEquals("function",
                    typeof exec_state.frame(2).evaluate('eval').value());
+      // Assignments to local variables only have temporary effect.
       assertEquals("foo",
                    exec_state.frame(0).evaluate('a = "foo"').value());
       assertEquals("bar",
@@ -145,7 +146,7 @@ Debug.setListener(listener);
 
 var f_result = f();
 
-assertEquals('foobar', f_result);
+assertEquals(4, f_result);
 
 // Make sure that the debug event listener was invoked.
 assertFalse(exception, "exception in listener")

test/mjsunit/debug-evaluate-modify-catch-block-scope.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 
 Debug = debug.Debug
 
@@ -26,12 +26,12 @@ Debug.setListener(listener);
   } catch (e) {
     let a = 1;
     function bar() {
-      a = 2;  // this has no effect, when called from debug-eval
-      e = 2;  // this has no effect, when called from debug-eval.
+      a = 2;
+      e = 2;
     }
     debugger;
-    assertEquals(1, a);
-    assertEquals(1, e);
+    assertEquals(2, a);
+    assertEquals(2, e);
   }
 })();
 

test/mjsunit/debug-evaluate-shadowed-context.js

@@ -29,7 +29,8 @@ function listener(event, exec_state, event_data, data) {
     }
     assertEquals("[object global]",
                  String(exec_state.frame(0).evaluate("this").value()));
-    exec_state.frame(0).evaluate("y = 'Y'");
+    assertEquals("y", exec_state.frame(0).evaluate("y").value());
+    assertEquals("a", exec_state.frame(0).evaluate("a").value());
     exec_state.frame(0).evaluate("a = 'A'");
     assertThrows(() => exec_state.frame(0).evaluate("z"), ReferenceError);
   } catch (e) {
@@ -41,7 +42,7 @@ function listener(event, exec_state, event_data, data) {
 Debug.setListener(listener);
 
 var a = "a";
-assertEquals("Y", (function() {
+(function() {
   var x = 1;     // context allocate x
   (() => x);
   var y = "y";
@@ -54,12 +55,12 @@ assertEquals("Y", (function() {
     })();        // 2
   })();          // 1
   return y;
-})());
+})();
 
 assertEquals("A", a);
 a = "a";
 
-assertEquals("Y", (function() {
+(function() {
   var x = 1;     // context allocate x
   (() => x);
   var y = "y";
@@ -74,7 +75,7 @@ assertEquals("Y", (function() {
     })();        // 2
   })();          // 1
   return y;
-})());
+})();
 
 assertEquals("A", a);
 

test/mjsunit/es6/debug-blockscopes.js

@@ -26,6 +26,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 // Flags: --expose-debug-as debug --allow-natives-syntax
+// Flags: --debug-eval-readonly-locals
 // The functions used for testing backtraces. They are at the top to make the
 // testing of source line/column easier.
 
@@ -525,15 +526,12 @@ function shadowing_1() {
   {
     let i = 5;
     debugger;
-    assertEqualsUnlessOptimized(27, i, shadowing_1);
   }
   assertEquals(0, i);
-  debugger;
-  assertEqualsUnlessOptimized(27, i, shadowing_1);
 }
 
 listener_delegate = function (exec_state) {
-  exec_state.frame(0).evaluate("i = 27");
+  assertEqualsUnlessOptimized(5, exec_state.frame(0).evaluate("i").value());
 }
 shadowing_1();
 EndTest();
@@ -546,13 +544,12 @@ function shadowing_2() {
   {
     let j = 5;
     debugger;
-    assertEqualsUnlessOptimized(27, j, shadowing_2);
   }
-  assertEqualsUnlessOptimized(0, i, shadowing_2);
 }
 
 listener_delegate = function (exec_state) {
-  exec_state.frame(0).evaluate("j = 27");
+  assertEqualsUnlessOptimized(0, exec_state.frame(0).evaluate("i").value());
+  assertEqualsUnlessOptimized(5, exec_state.frame(0).evaluate("j").value());
 }
 shadowing_2();
 EndTest();

test/mjsunit/regress-3225.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 
 Debug = debug.Debug
 
@@ -15,12 +15,13 @@ function listener(event, exec_state, event_data, data) {
     if (debug_step == 0) {
       assertEquals(1, exec_state.frame(0).evaluate('a').value());
       assertEquals(3, exec_state.frame(0).evaluate('b').value());
-      exec_state.frame(0).evaluate("a = 4").value();
+      exec_state.frame(0).evaluate("a = 4").value();  // no effect.
       debug_step++;
     } else {
-      assertEquals(4, exec_state.frame(0).evaluate('a').value());
+      assertEquals(1, exec_state.frame(0).evaluate('a').value());
       assertEquals(3, exec_state.frame(0).evaluate('b').value());
-      exec_state.frame(0).evaluate("b = 5").value();
+      exec_state.frame(0).evaluate("set_a_to_5()");
+      exec_state.frame(0).evaluate("b = 5").value();  // no effect.
     }
   } catch (e) {
     failure = e;
@@ -30,19 +31,22 @@ function listener(event, exec_state, event_data, data) {
 Debug.setListener(listener);
 
 function* generator(a, b) {
+  function set_a_to_5() { a = 5 }
   var b = 3;  // Shadows a parameter.
   debugger;
   yield a;
   yield b;
   debugger;
+  yield a;
   return b;
 }
 
 var foo = generator(1, 2);
 
-assertEquals(4, foo.next().value);
+assertEquals(1, foo.next().value);
 assertEquals(3, foo.next().value);
 assertEquals(5, foo.next().value);
+assertEquals(3, foo.next().value);
 assertNull(failure);
 
 Debug.setListener(null);

test/mjsunit/regress/regress-325676.js

@@ -25,7 +25,7 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 
 // If a function parameter is forced to be context allocated,
 // debug evaluate need to resolve it to a context slot instead of
@@ -40,7 +40,7 @@ function listener(event, exec_state, event_data, data) {
   if (event != Debug.DebugEvent.Break) return;
   try {
     assertEquals(expected, exec_state.frame(0).evaluate('arg').value());
-    exec_state.frame(0).evaluate('arg = "evaluated";');
+    exec_state.frame(0).evaluate('arg = "evaluated";');  // no effect
   } catch (e) {
     exception = e;
   }
@@ -51,12 +51,12 @@ Debug.setListener(listener);
 function f(arg) {
   expected = arg;
   debugger;
-  assertEquals("evaluated", arg);
+  assertEquals(expected, arg);
 
   arg = "value";
   expected = arg;
   debugger;
-  assertEquals("evaluated", arg);
+  assertEquals(expected, arg);
 
   // Forces arg to be context allocated even though a parameter.
   function g() { arg; }

test/mjsunit/regress/regress-crbug-323936.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --expose-debug-as debug
+// Flags: --expose-debug-as debug --debug-eval-readonly-locals
 
 Debug = debug.Debug;
 
@@ -14,11 +14,11 @@ function listener(event, exec_state, event_data, data) {
   try {
     if (step == 0) {
       assertEquals("error", exec_state.frame(0).evaluate("e").value());
-      exec_state.frame(0).evaluate("e = 'foo'");
-      exec_state.frame(0).evaluate("x = 'modified'");
+      exec_state.frame(0).evaluate("write_0('foo')");
+      exec_state.frame(0).evaluate("write_1('modified')");
     } else {
-      assertEquals("argument", exec_state.frame(0).evaluate("e").value());
-      exec_state.frame(0).evaluate("e = 'bar'");
+      assertEquals("foo", exec_state.frame(0).evaluate("e").value());
+      exec_state.frame(0).evaluate("write_2('bar')");
     }
     step++;
   } catch (e) {
@@ -33,9 +33,13 @@ function f(e, x) {
   try {
     throw "error";
   } catch(e) {
+    // 'e' and 'x' bind to the argument due to hoisting
+    function write_0(v) { e = v }
+    function write_1(v) { x = v }
     debugger;
-    assertEquals("foo", e);
+    assertEquals("error", e);
   }
+  function write_2(v) { e = v }
   debugger;
   assertEquals("bar", e);
   assertEquals("modified", x);