Revert of Debugger: use FrameInspector in ScopeIterator to find context....

Revert of Debugger: use FrameInspector in ScopeIterator to find context. (patchset #3 id:40001 of https://codereview.chromium.org/1239033002/)

Reason for revert:
breaks roll: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/87292/steps/browser_tests%20%28with%20patch%29/logs/DevToolsSanityTest.TestPauseWhenScriptIsRunning

Original issue's description:
> Debugger: use FrameInspector in ScopeIterator to find context.
>
> In optimized code, it's not guaranteed that the current context
> is stored in its frame slot.
>
> R=bmeurer@chromium.org
> BUG=v8:4309
> LOG=N
>
> Committed: https://crrev.com/3a0ee39cbde6a9778cfc4e2a6a0a8ff68933ff38
> Cr-Commit-Position: refs/heads/master@{#29697}

TBR=bmeurer@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4309

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

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

src/debug.cc

@@ -2735,8 +2735,19 @@ void Debug::HandleDebugBreak() {
   bool debug_command_only = isolate_->stack_guard()->CheckDebugCommand() &&
                             !isolate_->stack_guard()->CheckDebugBreak();
 
+  bool is_debugger_statement = !isolate_->stack_guard()->CheckDebugCommand() &&
+                               !isolate_->stack_guard()->CheckDebugBreak();
+
   isolate_->stack_guard()->ClearDebugBreak();
 
+  if (is_debugger_statement) {
+    // If we have been called via 'debugger' Javascript statement,
+    // we might not be prepared for breakpoints.
+    // TODO(dslomov,yangguo): CheckDebugBreak may race with RequestDebugBreak.
+    // Revisit this to clean-up.
+    HandleScope handle_scope(isolate_);
+    PrepareForBreakPoints();
+  }
   ProcessDebugMessages(debug_command_only);
 }
 

src/runtime/runtime-debug.cc

@@ -529,7 +529,6 @@ class FrameInspector {
   Object* GetContext() {
     return is_optimized_ ? deoptimized_frame_->GetContext() : frame_->context();
   }
-  JavaScriptFrame* GetArgumentsFrame() { return frame_; }
 
   // To inspect all the provided arguments the frame might need to be
   // replaced with the arguments frame.
@@ -960,7 +959,9 @@ static void UpdateStackLocalsFromMaterializedObject(
     Isolate* isolate, Handle<JSObject> target, Handle<ScopeInfo> scope_info,
     JavaScriptFrame* frame, int inlined_jsframe_index) {
   if (inlined_jsframe_index != 0 || frame->is_optimized()) {
-    // Optimized frames are not supported. Simply give up.
+    // Optimized frames are not supported.
+    // TODO(yangguo): make sure all code deoptimized when debugger is active
+    //                and assert that this cannot happen.
     return;
   }
 
@@ -993,7 +994,7 @@ static void UpdateStackLocalsFromMaterializedObject(
 
 MUST_USE_RESULT static MaybeHandle<JSObject> MaterializeLocalContext(
     Isolate* isolate, Handle<JSObject> target, Handle<JSFunction> function,
-    Handle<Context> frame_context) {
+    JavaScriptFrame* frame) {
   HandleScope scope(isolate);
   Handle<SharedFunctionInfo> shared(function->shared());
   Handle<ScopeInfo> scope_info(shared->scope_info());
@@ -1001,6 +1002,7 @@ MUST_USE_RESULT static MaybeHandle<JSObject> MaterializeLocalContext(
   if (!scope_info->HasContext()) return target;
 
   // Third fill all context locals.
+  Handle<Context> frame_context(Context::cast(frame->context()));
   Handle<Context> function_context(frame_context->declaration_context());
   ScopeInfo::CopyContextLocalsToScopeObject(scope_info, function_context,
                                             target);
@@ -1056,17 +1058,16 @@ MUST_USE_RESULT static MaybeHandle<JSObject> MaterializeScriptScope(
 
 
 MUST_USE_RESULT static MaybeHandle<JSObject> MaterializeLocalScope(
-    Isolate* isolate, FrameInspector* frame_inspector) {
-  Handle<JSFunction> function(JSFunction::cast(frame_inspector->GetFunction()));
+    Isolate* isolate, JavaScriptFrame* frame, int inlined_jsframe_index) {
+  FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
+  Handle<JSFunction> function(JSFunction::cast(frame_inspector.GetFunction()));
 
   Handle<JSObject> local_scope =
       isolate->factory()->NewJSObject(isolate->object_function());
   MaterializeStackLocalsWithFrameInspector(isolate, local_scope, function,
-                                           frame_inspector);
+                                           &frame_inspector);
 
-  Handle<Context> frame_context(Context::cast(frame_inspector->GetContext()));
-
-  return MaterializeLocalContext(isolate, local_scope, function, frame_context);
+  return MaterializeLocalContext(isolate, local_scope, function, frame);
 }
 
 
@@ -1095,10 +1096,13 @@ static bool SetContextLocalValue(Isolate* isolate, Handle<ScopeInfo> scope_info,
 
 
 static bool SetLocalVariableValue(Isolate* isolate, JavaScriptFrame* frame,
+                                  int inlined_jsframe_index,
                                   Handle<String> variable_name,
                                   Handle<Object> new_value) {
-  // Optimized frames are not supported.
-  if (frame->is_optimized()) return false;
+  if (inlined_jsframe_index != 0 || frame->is_optimized()) {
+    // Optimized frames are not supported.
+    return false;
+  }
 
   Handle<JSFunction> function(frame->function());
   Handle<SharedFunctionInfo> shared(function->shared());
@@ -1307,13 +1311,15 @@ static bool SetCatchVariableValue(Isolate* isolate, Handle<Context> context,
 static Handle<JSObject> MaterializeBlockScope(Isolate* isolate,
                                               Handle<ScopeInfo> scope_info,
                                               Handle<Context> context,
-                                              FrameInspector* frame_inspector) {
+                                              JavaScriptFrame* frame,
+                                              int inlined_jsframe_index) {
   Handle<JSObject> block_scope =
       isolate->factory()->NewJSObject(isolate->object_function());
 
-  if (frame_inspector != nullptr) {
+  if (frame != nullptr) {
+    FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
     MaterializeStackLocalsWithFrameInspector(isolate, block_scope, scope_info,
-                                             frame_inspector);
+                                             &frame_inspector);
   }
 
   if (!context.is_null()) {
@@ -1364,19 +1370,21 @@ class ScopeIterator {
     ScopeTypeModule
   };
 
-  ScopeIterator(Isolate* isolate, FrameInspector* frame_inspector,
-                bool ignore_nested_scopes = false)
+  ScopeIterator(Isolate* isolate, JavaScriptFrame* frame,
+                int inlined_jsframe_index, bool ignore_nested_scopes = false)
       : isolate_(isolate),
-        frame_inspector_(frame_inspector),
-        context_(Context::cast(frame_inspector->GetContext())),
+        frame_(frame),
+        inlined_jsframe_index_(inlined_jsframe_index),
+        function_(frame->function()),
+        context_(Context::cast(frame->context())),
         nested_scope_chain_(4),
         seen_script_scope_(false),
         failed_(false) {
     // Catch the case when the debugger stops in an internal function.
-    Handle<SharedFunctionInfo> shared_info(function()->shared());
+    Handle<SharedFunctionInfo> shared_info(function_->shared());
     Handle<ScopeInfo> scope_info(shared_info->scope_info());
     if (shared_info->script() == isolate->heap()->undefined_value()) {
-      while (context_->closure() == function()) {
+      while (context_->closure() == *function_) {
         context_ = Handle<Context>(context_->previous(), isolate_);
       }
       return;
@@ -1400,7 +1408,7 @@ class ScopeIterator {
 
       // PC points to the instruction after the current one, possibly a break
       // location as well. So the "- 1" to exclude it from the search.
-      Address call_pc = frame()->pc() - 1;
+      Address call_pc = frame->pc() - 1;
 
       // Find the break point where execution has stopped.
       BreakLocation location =
@@ -1413,7 +1421,7 @@ class ScopeIterator {
       if (scope_info->HasContext()) {
         context_ = Handle<Context>(context_->declaration_context(), isolate_);
       } else {
-        while (context_->closure() == function()) {
+        while (context_->closure() == *function_) {
           context_ = Handle<Context>(context_->previous(), isolate_);
         }
       }
@@ -1438,7 +1446,7 @@ class ScopeIterator {
         } else {
           DCHECK(scope_info->scope_type() == EVAL_SCOPE);
           info.set_eval();
-          info.set_context(Handle<Context>(function()->context()));
+          info.set_context(Handle<Context>(function_->context()));
         }
         if (Parser::ParseStatic(&info) && Scope::Analyze(&info)) {
           scope = info.function()->scope();
@@ -1446,7 +1454,7 @@ class ScopeIterator {
         RetrieveScopeChain(scope, shared_info);
       } else {
         // Function code
-        ParseInfo info(&zone, Handle<JSFunction>(function()));
+        ParseInfo info(&zone, function_);
         if (Parser::ParseStatic(&info) && Scope::Analyze(&info)) {
           scope = info.function()->scope();
         }
@@ -1457,11 +1465,15 @@ class ScopeIterator {
 
   ScopeIterator(Isolate* isolate, Handle<JSFunction> function)
       : isolate_(isolate),
-        frame_inspector_(NULL),
+        frame_(NULL),
+        inlined_jsframe_index_(0),
+        function_(function),
         context_(function->context()),
         seen_script_scope_(false),
         failed_(false) {
-    if (function->IsBuiltin()) context_ = Handle<Context>();
+    if (function->IsBuiltin()) {
+      context_ = Handle<Context>();
+    }
   }
 
   // More scopes?
@@ -1572,7 +1584,7 @@ class ScopeIterator {
       case ScopeIterator::ScopeTypeLocal:
         // Materialize the content of the local scope into a JSObject.
         DCHECK(nested_scope_chain_.length() == 1);
-        return MaterializeLocalScope(isolate_, frame_inspector_);
+        return MaterializeLocalScope(isolate_, frame_, inlined_jsframe_index_);
       case ScopeIterator::ScopeTypeWith:
         // Return the with object.
         return Handle<JSObject>(JSObject::cast(CurrentContext()->extension()));
@@ -1588,11 +1600,11 @@ class ScopeIterator {
           Handle<Context> context = scope_info->HasContext()
                                         ? CurrentContext()
                                         : Handle<Context>::null();
-          return MaterializeBlockScope(isolate_, scope_info, context,
-                                       frame_inspector_);
+          return MaterializeBlockScope(isolate_, scope_info, context, frame_,
+                                       inlined_jsframe_index_);
         } else {
           return MaterializeBlockScope(isolate_, Handle<ScopeInfo>::null(),
-                                       CurrentContext(), nullptr);
+                                       CurrentContext(), nullptr, 0);
         }
       }
       case ScopeIterator::ScopeTypeModule:
@@ -1619,8 +1631,8 @@ class ScopeIterator {
       case ScopeIterator::ScopeTypeGlobal:
         break;
       case ScopeIterator::ScopeTypeLocal:
-        return SetLocalVariableValue(isolate_, frame(), variable_name,
-                                     new_value);
+        return SetLocalVariableValue(isolate_, frame_, inlined_jsframe_index_,
+                                     variable_name, new_value);
       case ScopeIterator::ScopeTypeWith:
         break;
       case ScopeIterator::ScopeTypeCatch:
@@ -1635,7 +1647,7 @@ class ScopeIterator {
       case ScopeIterator::ScopeTypeBlock:
         return SetBlockVariableValue(
             isolate_, HasContext() ? CurrentContext() : Handle<Context>::null(),
-            CurrentScopeInfo(), frame(), variable_name, new_value);
+            CurrentScopeInfo(), frame_, variable_name, new_value);
       case ScopeIterator::ScopeTypeModule:
         // TODO(2399): should we implement it?
         break;
@@ -1682,7 +1694,7 @@ class ScopeIterator {
 
       case ScopeIterator::ScopeTypeLocal: {
         os << "Local:\n";
-        function()->shared()->scope_info()->Print();
+        function_->shared()->scope_info()->Print();
         if (!CurrentContext().is_null()) {
           CurrentContext()->Print(os);
           if (CurrentContext()->has_extension()) {
@@ -1735,24 +1747,18 @@ class ScopeIterator {
 
  private:
   Isolate* isolate_;
-  FrameInspector* const frame_inspector_;
+  JavaScriptFrame* frame_;
+  int inlined_jsframe_index_;
+  Handle<JSFunction> function_;
   Handle<Context> context_;
   List<Handle<ScopeInfo> > nested_scope_chain_;
   bool seen_script_scope_;
   bool failed_;
 
-  inline JavaScriptFrame* frame() {
-    return frame_inspector_->GetArgumentsFrame();
-  }
-
-  inline JSFunction* function() {
-    return JSFunction::cast(frame_inspector_->GetFunction());
-  }
-
   void RetrieveScopeChain(Scope* scope,
                           Handle<SharedFunctionInfo> shared_info) {
     if (scope != NULL) {
-      int source_position = frame_inspector_->GetSourcePosition();
+      int source_position = shared_info->code()->SourcePosition(frame_->pc());
       scope->GetNestedScopeChain(isolate_, &nested_scope_chain_,
                                  source_position);
     } else {
@@ -1783,11 +1789,10 @@ RUNTIME_FUNCTION(Runtime_GetScopeCount) {
   StackFrame::Id id = UnwrapFrameId(wrapped_id);
   JavaScriptFrameIterator it(isolate, id);
   JavaScriptFrame* frame = it.frame();
-  FrameInspector frame_inspector(frame, 0, isolate);
 
   // Count the visible scopes.
   int n = 0;
-  for (ScopeIterator it(isolate, &frame_inspector); !it.Done(); it.Next()) {
+  for (ScopeIterator it(isolate, frame, 0); !it.Done(); it.Next()) {
     n++;
   }
 
@@ -1912,11 +1917,10 @@ RUNTIME_FUNCTION(Runtime_GetScopeDetails) {
   StackFrame::Id id = UnwrapFrameId(wrapped_id);
   JavaScriptFrameIterator frame_it(isolate, id);
   JavaScriptFrame* frame = frame_it.frame();
-  FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
 
   // Find the requested scope.
   int n = 0;
-  ScopeIterator it(isolate, &frame_inspector);
+  ScopeIterator it(isolate, frame, inlined_jsframe_index);
   for (; !it.Done() && n < index; it.Next()) {
     n++;
   }
@@ -1958,10 +1962,9 @@ RUNTIME_FUNCTION(Runtime_GetAllScopesDetails) {
   StackFrame::Id id = UnwrapFrameId(wrapped_id);
   JavaScriptFrameIterator frame_it(isolate, id);
   JavaScriptFrame* frame = frame_it.frame();
-  FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
 
   List<Handle<JSObject> > result(4);
-  ScopeIterator it(isolate, &frame_inspector, ignore_nested_scopes);
+  ScopeIterator it(isolate, frame, inlined_jsframe_index, ignore_nested_scopes);
   for (; !it.Done(); it.Next()) {
     Handle<JSObject> details;
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, details,
@@ -2062,9 +2065,8 @@ RUNTIME_FUNCTION(Runtime_SetScopeVariableValue) {
     StackFrame::Id id = UnwrapFrameId(wrapped_id);
     JavaScriptFrameIterator frame_it(isolate, id);
     JavaScriptFrame* frame = frame_it.frame();
-    FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate);
 
-    ScopeIterator it(isolate, &frame_inspector);
+    ScopeIterator it(isolate, frame, inlined_jsframe_index);
     res = SetScopeVariableValue(&it, index, variable_name, new_value);
   } else {
     CONVERT_ARG_HANDLE_CHECKED(JSFunction, fun, 0);
@@ -2084,9 +2086,7 @@ RUNTIME_FUNCTION(Runtime_DebugPrintScopes) {
   // Print the scopes for the top frame.
   StackFrameLocator locator(isolate);
   JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
-  FrameInspector frame_inspector(frame, 0, isolate);
-
-  for (ScopeIterator it(isolate, &frame_inspector); !it.Done(); it.Next()) {
+  for (ScopeIterator it(isolate, frame, 0); !it.Done(); it.Next()) {
     it.DebugPrint();
   }
 #endif
@@ -2479,7 +2479,7 @@ class EvaluationContextBuilder {
     Handle<Context> inner_context;
 
     bool stop = false;
-    for (ScopeIterator it(isolate, &frame_inspector);
+    for (ScopeIterator it(isolate, frame, inlined_jsframe_index);
          !it.Failed() && !it.Done() && !stop; it.Next()) {
       ScopeIterator::ScopeType scope_type = it.Type();
 

test/mjsunit/es6/debug-blockscopes.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 --allow-natives-syntax
+// Flags: --expose-debug-as debug
 // The functions used for testing backtraces. They are at the top to make the
 // testing of source line/column easier.
 
@@ -187,14 +187,6 @@ function CheckScopeContent(content, number, exec_state) {
 }
 
 
-function assertEqualsUnlessOptimized(expected, value, f) {
-  try {
-    assertEquals(expected, value);
-  } catch (e) {
-    assertOptimized(f);
-  }
-}
-
 // Simple empty block scope in local scope.
 BeginTest("Local block 1");
 
@@ -525,11 +517,11 @@ function shadowing_1() {
   {
     let i = 5;
     debugger;
-    assertEqualsUnlessOptimized(27, i, shadowing_1);
+    assertEquals(27, i);
   }
   assertEquals(0, i);
   debugger;
-  assertEqualsUnlessOptimized(27, i, shadowing_1);
+  assertEquals(27, i);
 }
 
 listener_delegate = function (exec_state) {
@@ -546,9 +538,9 @@ function shadowing_2() {
   {
     let j = 5;
     debugger;
-    assertEqualsUnlessOptimized(27, j, shadowing_2);
+    assertEquals(27, j);
   }
-  assertEqualsUnlessOptimized(0, i, shadowing_2);
+  assertEquals(0, i);
 }
 
 listener_delegate = function (exec_state) {

test/mjsunit/regress/regress-4309-1.js

-// Copyright 2015 the V8 project authors. All rights reserved.
-// 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 --allow-natives-syntax
-
-var Debug = debug.Debug;
-
-var exception = null;
-
-function listener(event, exec_state, event_data, data) {
-  if (event != Debug.DebugEvent.Break) return;
-  try {
-    var scopes = exec_state.frame().allScopes();
-    assertEquals(3, scopes.length);
-    assertEquals(debug.ScopeType.Local, scopes[0].scopeType());
-    assertEquals(debug.ScopeType.Script, scopes[1].scopeType());
-    assertEquals(debug.ScopeType.Global, scopes[2].scopeType());
-  } catch (e) {
-    exception = e;
-  }
-}
-
-function f() {
-  eval('');
-  debugger;
-}
-
-f();
-f();
-
-%OptimizeFunctionOnNextCall(f);
-Debug.setListener(listener);
-
-f();
-
-assertNull(exception);

test/mjsunit/regress/regress-4309-2.js

-// Copyright 2015 the V8 project authors. All rights reserved.
-// 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 --allow-natives-syntax
-
-var Debug = debug.Debug;
-
-var exception = null;
-
-function listener(event, exec_state, event_data, data) {
-  if (event != Debug.DebugEvent.Break) return;
-  try {
-    var scope = exec_state.frame().scope(0);
-    assertEquals(5, scope.scopeObject().property("i").value().value());
-  } catch (e) {
-    exception = e;
-  }
-}
-
-function f() {
-  eval('var i = 5');
-  debugger;
-}
-
-f();
-f();
-
-%OptimizeFunctionOnNextCall(f);
-Debug.setListener(listener);
-
-f();
-
-assertNull(exception);

test/mjsunit/regress/regress-4309-3.js

-// Copyright 2015 the V8 project authors. All rights reserved.
-// 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 --allow-natives-syntax
-
-var Debug = debug.Debug;
-
-var exception = null;
-
-function listener(event, exec_state, event_data, data) {
-  if (event != Debug.DebugEvent.Break) return;
-  try {
-    var scopes = exec_state.frame().allScopes();
-    assertEquals(4, scopes.length);
-    assertEquals(debug.ScopeType.With, scopes[0].scopeType());
-    assertEquals(debug.ScopeType.Local, scopes[1].scopeType());
-    assertEquals(debug.ScopeType.Script, scopes[2].scopeType());
-    assertEquals(debug.ScopeType.Global, scopes[3].scopeType());
-  } catch (e) {
-    exception = e;
-  }
-}
-
-function f() {
-  with({}) {
-    debugger;
-  }
-}
-
-f();
-f();
-
-%OptimizeFunctionOnNextCall(f);
-Debug.setListener(listener);
-
-f();
-
-assertNull(exception);