Redirect BytecodeArray pointers on stack when clearing DebugInfo

When clearing a DebugInfo, we need to check whether that function is
currently executing and, if so, update the on-stack BytecodeArray
pointer to refer to the original BytecodeArray. Otherwise, the original
BytecodeArray might get flushed, which can cause problems when
attempting to resume execution of the function.

Bug: v8:9067
Change-Id: Ief28a501294f5a34052e13f618fa084311eaa0b8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1548573Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Seth Brenith <seth.brenith@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#60774}

src/debug/debug-frames.cc

@@ -93,5 +93,31 @@ bool FrameInspector::ParameterIsShadowedByContextLocal(
   return ScopeInfo::ContextSlotIndex(*info, *parameter_name, &mode, &init_flag,
                                      &maybe_assigned_flag) != -1;
 }
+
+RedirectActiveFunctions::RedirectActiveFunctions(SharedFunctionInfo shared,
+                                                 Mode mode)
+    : shared_(shared), mode_(mode) {
+  DCHECK(shared->HasBytecodeArray());
+  if (mode == Mode::kUseDebugBytecode) {
+    DCHECK(shared->HasDebugInfo());
+  }
+}
+
+void RedirectActiveFunctions::VisitThread(Isolate* isolate,
+                                          ThreadLocalTop* top) {
+  for (JavaScriptFrameIterator it(isolate, top); !it.done(); it.Advance()) {
+    JavaScriptFrame* frame = it.frame();
+    JSFunction function = frame->function();
+    if (!frame->is_interpreted()) continue;
+    if (function->shared() != shared_) continue;
+    InterpretedFrame* interpreted_frame =
+        reinterpret_cast<InterpretedFrame*>(frame);
+    BytecodeArray bytecode = mode_ == Mode::kUseDebugBytecode
+                                 ? shared_->GetDebugInfo()->DebugBytecodeArray()
+                                 : shared_->GetBytecodeArray();
+    interpreted_frame->PatchBytecodeArray(bytecode);
+  }
+}
+
 }  // namespace internal
 }  // namespace v8

src/debug/debug-frames.h

@@ -9,6 +9,7 @@
 #include "src/frames.h"
 #include "src/isolate.h"
 #include "src/objects.h"
+#include "src/v8threads.h"
 #include "src/wasm/wasm-interpreter.h"
 
 namespace v8 {
@@ -64,6 +65,24 @@ class FrameInspector {
 
   DISALLOW_COPY_AND_ASSIGN(FrameInspector);
 };
+
+class RedirectActiveFunctions : public ThreadVisitor {
+ public:
+  enum class Mode {
+    kUseOriginalBytecode,
+    kUseDebugBytecode,
+  };
+
+  explicit RedirectActiveFunctions(SharedFunctionInfo shared, Mode mode);
+
+  void VisitThread(Isolate* isolate, ThreadLocalTop* top) override;
+
+ private:
+  SharedFunctionInfo shared_;
+  Mode mode_;
+  DisallowHeapAllocation no_gc_;
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/debug/debug.cc

@@ -1135,31 +1135,6 @@ void Debug::ClearOneShot() {
   }
 }
 
-class RedirectActiveFunctions : public ThreadVisitor {
- public:
-  explicit RedirectActiveFunctions(SharedFunctionInfo shared)
-      : shared_(shared) {
-    DCHECK(shared->HasBytecodeArray());
-  }
-
-  void VisitThread(Isolate* isolate, ThreadLocalTop* top) override {
-    for (JavaScriptFrameIterator it(isolate, top); !it.done(); it.Advance()) {
-      JavaScriptFrame* frame = it.frame();
-      JSFunction function = frame->function();
-      if (!frame->is_interpreted()) continue;
-      if (function->shared() != shared_) continue;
-      InterpretedFrame* interpreted_frame =
-          reinterpret_cast<InterpretedFrame*>(frame);
-      BytecodeArray debug_copy = shared_->GetDebugInfo()->DebugBytecodeArray();
-      interpreted_frame->PatchBytecodeArray(debug_copy);
-    }
-  }
-
- private:
-  SharedFunctionInfo shared_;
-  DisallowHeapAllocation no_gc_;
-};
-
 void Debug::DeoptimizeFunction(Handle<SharedFunctionInfo> shared) {
   // Deoptimize all code compiled from this shared function info including
   // inlining.
@@ -1217,7 +1192,8 @@ void Debug::PrepareFunctionForDebugExecution(
   } else {
     DeoptimizeFunction(shared);
     // Update PCs on the stack to point to recompiled code.
-    RedirectActiveFunctions redirect_visitor(*shared);
+    RedirectActiveFunctions redirect_visitor(
+        *shared, RedirectActiveFunctions::Mode::kUseDebugBytecode);
     redirect_visitor.VisitThread(isolate_, isolate_->thread_local_top());
     isolate_->thread_manager()->IterateArchivedThreads(&redirect_visitor);
   }

src/objects/debug-objects.cc

@@ -32,6 +32,18 @@ void DebugInfo::ClearBreakInfo(Isolate* isolate) {
     // Reset function's bytecode array field to point to the original bytecode
     // array.
     shared()->SetDebugBytecodeArray(OriginalBytecodeArray());
+
+    // If the function is currently running on the stack, we need to update the
+    // bytecode pointers on the stack so they point to the original
+    // BytecodeArray before releasing that BytecodeArray from this DebugInfo.
+    // Otherwise, it could be flushed and cause problems on resume. See v8:9067.
+    {
+      RedirectActiveFunctions redirect_visitor(
+          shared(), RedirectActiveFunctions::Mode::kUseOriginalBytecode);
+      redirect_visitor.VisitThread(isolate, isolate->thread_local_top());
+      isolate->thread_manager()->IterateArchivedThreads(&redirect_visitor);
+    }
+
     set_original_bytecode_array(ReadOnlyRoots(isolate).undefined_value());
     set_debug_bytecode_array(ReadOnlyRoots(isolate).undefined_value());
   }

test/debugger/debug/regress/regress-9067.js

+// Copyright 2019 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-gc --stress-flush-bytecode
+
+var Debug = debug.Debug
+var bp;
+
+Debug.setListener(function (event, exec_state, event_data, data) {
+  if (event == Debug.DebugEvent.Break) {
+    Debug.clearBreakPoint(bp);
+    gc();
+  }
+});
+
+function f() {
+  (function () {})();
+}
+
+bp = Debug.setBreakPoint(f, 0, 0);
+f();