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

Revert of [debugger] debug-evaluate should not not modify local values. (patchset #2 id:20001 of https://codereview.chromium.org/1513183003/ )

Reason for revert:
[Sheriff] Layout test changes.

Original issue's description:
> [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
>
> Committed: https://crrev.com/92caa9b85eefffbef51c67428397951bd2e2c330
> Cr-Commit-Position: refs/heads/master@{#32841}

TBR=mstarzinger@chromium.org,rossberg@chromium.org,yangguo@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

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

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

src/debug/debug-evaluate.cc

@@ -68,19 +68,16 @@ 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 = context_builder.native_context();
+  Handle<Context> context = isolate->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() && !FLAG_debug_eval_readonly_locals) {
-    context_builder.UpdateValues();
-  }
+  if (!maybe_result.is_null()) context_builder.UpdateValues();
   return maybe_result;
 }
 
@@ -130,8 +127,8 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
   Handle<JSFunction> local_function =
       handle(JSFunction::cast(frame_inspector.GetFunction()));
   Handle<Context> outer_context(local_function->context());
-  native_context_ = Handle<Context>(outer_context->native_context());
-  Handle<JSFunction> global_function(native_context_->closure());
+  Handle<Context> native_context = isolate->native_context();
+  Handle<JSFunction> global_function(native_context->closure());
   outer_info_ = handle(global_function->shared());
   Handle<Context> inner_context;
 
@@ -169,7 +166,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();
@@ -312,7 +309,7 @@ void DebugEvaluate::ContextBuilder::MaterializeArgumentsObject(
 
 
 MaybeHandle<Object> DebugEvaluate::ContextBuilder::LoadFromContext(
-    Handle<Context> context, Handle<String> name, bool* global) {
+    Handle<Context> context, Handle<String> name) {
   static const ContextLookupFlags flags = FOLLOW_CONTEXT_CHAIN;
   int index;
   PropertyAttributes attributes;
@@ -323,13 +320,9 @@ 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);
   }
 }
@@ -340,13 +333,7 @@ void DebugEvaluate::ContextBuilder::MaterializeContextChain(
   for (const Handle<String>& name : non_locals_) {
     HandleScope scope(isolate_);
     Handle<Object> value;
-    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;
-    }
+    if (!LoadFromContext(context, name).ToHandle(&value)) continue;
     JSObject::SetOwnPropertyIgnoreAttributes(target, name, value, NONE).Check();
   }
 }
@@ -394,8 +381,7 @@ 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) {
-    bool global;
-    LoadFromContext(lookup_context, this_string, &global).ToHandle(&receiver);
+    LoadFromContext(lookup_context, this_string).ToHandle(&receiver);
   } else if (local_function->shared()->scope_info()->HasReceiver()) {
     receiver = handle(frame_->receiver(), isolate_);
   }

src/debug/debug-evaluate.h

@@ -54,7 +54,6 @@ 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:
@@ -88,14 +87,13 @@ class DebugEvaluate : public AllStatic {
                                         bool this_is_non_local);
 
     MaybeHandle<Object> LoadFromContext(Handle<Context> context,
-                                        Handle<String> name, bool* global);
+                                        Handle<String> name);
 
     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,7 +597,8 @@ DEFINE_BOOL(cache_prototype_transitions, true, "cache prototype transitions")
 DEFINE_INT(cpu_profiler_sampling_interval, 1000,
            "CPU profiler sampling interval in microseconds")
 
-// Array abuse tracing
+// debug.cc
+DEFINE_BOOL(trace_debug_json, false, "trace debugging JSON request/response")
 DEFINE_BOOL(trace_js_array_abuse, false,
             "trace out-of-bounds accesses to JS arrays")
 DEFINE_BOOL(trace_external_array_abuse, false,
@@ -606,11 +607,6 @@ 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,7 +26,6 @@
 // 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;
@@ -35,18 +34,20 @@ 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'");  // no effect
+    exec_state.frame(0).evaluate("goo = 'goo foo'");
     assertEquals("bar return", exec_state.frame(0).evaluate("bar()").value());
-    assertEquals("inner bar", exec_state.frame(0).evaluate("inner").value());
-    assertEquals("outer bar", exec_state.frame(0).evaluate("outer").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("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'");  // 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
+    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'");
     listened = true;
   } catch (e) {
     print(e);
@@ -68,9 +69,9 @@ function foo() {
 
   with (withv) {
     var bar = function bar() {
-      assertEquals("goo", goo);
-      inner = "inner bar";
-      outer = "outer 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
       v = "v bar";
       return "bar return";
     };
@@ -80,8 +81,8 @@ function foo() {
     debugger;
   }
 
-  assertEquals("inner bar", inner);
-  assertEquals("baz inner", baz);
+  assertEquals("inner foo", inner);
+  assertEquals("baz inner foo", 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 
 Debug = debug.Debug
 
@@ -50,8 +50,10 @@ function f() {
 
     debugger;  // Break point.
 
-    assertEquals(undefined, var0);
-    assertEquals(0, const0);
+    assertEquals(30, var0);
+    // TODO(yangguo): debug evaluate should not be able to alter
+    //                stack-allocated const values
+    // assertEquals(0, const0);
     assertEquals(undefined, const1);
     assertEquals(undefined, const2);
     var var0 = 20;
@@ -64,7 +66,7 @@ function f() {
 
   g();
 
-  assertEquals(21, var1);
+  assertEquals(31, 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 
 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);
 
-      var i = frameMirror.evaluate('f = function() { i = 5; }, f(), i').value();
-      assertEquals(5, i);
+      f = frameMirror.evaluate('f = function() { i = 5; }, f(), f').value();
+      print(f);
     }
   } catch(e) {
     exception = e;
@@ -35,7 +35,7 @@ Debug.setListener(listener);
   } catch (e) {
     assertEquals(0, i);
     debugger;
-    assertEquals(0, i);
+    assertEquals(5, 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 // Get the Debug object exposed from the debug context global object.
 Debug = debug.Debug
 
@@ -127,7 +127,6 @@ 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",
@@ -146,7 +145,7 @@ Debug.setListener(listener);
 
 var f_result = f();
 
-assertEquals(4, f_result);
+assertEquals('foobar', 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 
 Debug = debug.Debug
 
@@ -26,12 +26,12 @@ Debug.setListener(listener);
   } catch (e) {
     let a = 1;
     function bar() {
-      a = 2;
-      e = 2;
+      a = 2;  // this has no effect, when called from debug-eval
+      e = 2;  // this has no effect, when called from debug-eval.
     }
     debugger;
-    assertEquals(2, a);
-    assertEquals(2, e);
+    assertEquals(1, a);
+    assertEquals(1, e);
   }
 })();
 

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

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

test/mjsunit/es6/debug-blockscopes.js

@@ -26,7 +26,6 @@
 // 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.
 
@@ -526,12 +525,15 @@ 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) {
-  assertEqualsUnlessOptimized(5, exec_state.frame(0).evaluate("i").value());
+  exec_state.frame(0).evaluate("i = 27");
 }
 shadowing_1();
 EndTest();
@@ -544,12 +546,13 @@ function shadowing_2() {
   {
     let j = 5;
     debugger;
+    assertEqualsUnlessOptimized(27, j, shadowing_2);
   }
+  assertEqualsUnlessOptimized(0, i, shadowing_2);
 }
 
 listener_delegate = function (exec_state) {
-  assertEqualsUnlessOptimized(0, exec_state.frame(0).evaluate("i").value());
-  assertEqualsUnlessOptimized(5, exec_state.frame(0).evaluate("j").value());
+  exec_state.frame(0).evaluate("j = 27");
 }
 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 
 Debug = debug.Debug
 
@@ -15,13 +15,12 @@ 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();  // no effect.
+      exec_state.frame(0).evaluate("a = 4").value();
       debug_step++;
     } else {
-      assertEquals(1, exec_state.frame(0).evaluate('a').value());
+      assertEquals(4, exec_state.frame(0).evaluate('a').value());
       assertEquals(3, exec_state.frame(0).evaluate('b').value());
-      exec_state.frame(0).evaluate("set_a_to_5()");
-      exec_state.frame(0).evaluate("b = 5").value();  // no effect.
+      exec_state.frame(0).evaluate("b = 5").value();
     }
   } catch (e) {
     failure = e;
@@ -31,22 +30,19 @@ 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(1, foo.next().value);
+assertEquals(4, 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 
 // 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";');  // no effect
+    exec_state.frame(0).evaluate('arg = "evaluated";');
   } catch (e) {
     exception = e;
   }
@@ -51,12 +51,12 @@ Debug.setListener(listener);
 function f(arg) {
   expected = arg;
   debugger;
-  assertEquals(expected, arg);
+  assertEquals("evaluated", arg);
 
   arg = "value";
   expected = arg;
   debugger;
-  assertEquals(expected, arg);
+  assertEquals("evaluated", 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 --debug-eval-readonly-locals
+// Flags: --expose-debug-as debug
 
 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("write_0('foo')");
-      exec_state.frame(0).evaluate("write_1('modified')");
+      exec_state.frame(0).evaluate("e = 'foo'");
+      exec_state.frame(0).evaluate("x = 'modified'");
     } else {
-      assertEquals("foo", exec_state.frame(0).evaluate("e").value());
-      exec_state.frame(0).evaluate("write_2('bar')");
+      assertEquals("argument", exec_state.frame(0).evaluate("e").value());
+      exec_state.frame(0).evaluate("e = 'bar'");
     }
     step++;
   } catch (e) {
@@ -33,13 +33,9 @@ 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("error", e);
+    assertEquals("foo", e);
   }
-  function write_2(v) { e = v }
   debugger;
   assertEquals("bar", e);
   assertEquals("modified", x);