[debug] Allow live-editing of top-most frame

This CL extends the live edit mechanism to allow editing the function
that is currently on top of the stack, as long as that call frame is
the only activation of that  function.

The CL changes how we look for functions on the current JS stack:
Instead of starting at thread_local_top we start at the frame we
are currently paused in. This is possible since there can not be any
JavaScript frames above the current "break frame", only C++ frames
which are not relevant for live edit.

If the edited script modifes the top-most function, the inspector
will trigger a restart of that call frame. That is why we check
if we can actually restart the function and only allow the live
edit to go through if that is the case.

Note that this CL also adds a kill switch in the form of a runtime
flag for this feature, in case we need to pull the plug and disable
this feature again via back-merge.

R=jarin@chromium.org

Bug: chromium:1334484
Change-Id: I711913df96c8acc786ad4de28de804d2f90e1847
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3695353Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81097}

src/debug/debug-interface.cc

@@ -671,8 +671,12 @@ bool Script::SetScriptSource(Local<String> newSource, bool preview,
                              LiveEditResult* result) const {
   i::Handle<i::Script> script = Utils::OpenHandle(this);
   i::Isolate* isolate = script->GetIsolate();
+  // TODO(crbug.com/1334484): Pass `allow_top_frame_live_editing` through from
+  // the
+  //                          inspector.
   return isolate->debug()->SetScriptSource(
-      script, Utils::OpenHandle(*newSource), preview, result);
+      script, Utils::OpenHandle(*newSource), preview,
+      /* allow_top_frame_live_editing */ false, result);
 }
 
 bool Script::SetBreakpoint(Local<String> condition, Location* location,

src/debug/debug-interface.h

@@ -162,6 +162,7 @@ struct LiveEditResult {
   bool stack_changed = false;
   // Available only for OK.
   v8::Local<v8::debug::Script> script;
+  bool restart_top_frame_required = false;
   // Fields below are available only for COMPILE_ERROR.
   v8::Local<v8::String> message;
   int line_number = -1;

src/debug/debug-stack-trace-iterator.cc

@@ -154,6 +154,13 @@ v8::Local<v8::Function> DebugStackTraceIterator::GetFunction() const {
   return Utils::ToLocal(frame_inspector_->GetFunction());
 }
 
+Handle<SharedFunctionInfo> DebugStackTraceIterator::GetSharedFunctionInfo()
+    const {
+  DCHECK(!Done());
+  if (!frame_inspector_->IsJavaScript()) return Handle<SharedFunctionInfo>();
+  return handle(frame_inspector_->GetFunction()->shared(), isolate_);
+}
+
 std::unique_ptr<v8::debug::ScopeIterator>
 DebugStackTraceIterator::GetScopeIterator() const {
   DCHECK(!Done());

src/debug/debug-stack-trace-iterator.h

@@ -36,6 +36,8 @@ class DebugStackTraceIterator final : public debug::StackTraceIterator {
                                      bool throw_on_side_effect) override;
   void PrepareRestart();
 
+  Handle<SharedFunctionInfo> GetSharedFunctionInfo() const;
+
  private:
   void UpdateInlineFrameIndexAndResumableFnOnStack();
 

src/debug/debug.cc

@@ -2377,12 +2377,14 @@ bool Debug::CanBreakAtEntry(Handle<SharedFunctionInfo> shared) {
 }
 
 bool Debug::SetScriptSource(Handle<Script> script, Handle<String> source,
-                            bool preview, debug::LiveEditResult* result) {
+                            bool preview, bool allow_top_frame_live_editing,
+                            debug::LiveEditResult* result) {
   RCS_SCOPE(isolate_, RuntimeCallCounterId::kDebugger);
   DebugScope debug_scope(this);
   feature_tracker()->Track(DebugFeatureTracker::kLiveEdit);
   running_live_edit_ = true;
-  LiveEdit::PatchScript(isolate_, script, source, preview, result);
+  LiveEdit::PatchScript(isolate_, script, source, preview,
+                        allow_top_frame_live_editing, result);
   running_live_edit_ = false;
   return result->status == debug::LiveEditResult::OK;
 }

src/debug/debug.h

@@ -330,7 +330,8 @@ class V8_EXPORT_PRIVATE Debug {
   // change. stack_changed is true if after editing script on pause stack is
   // changed and client should request stack trace again.
   bool SetScriptSource(Handle<Script> script, Handle<String> source,
-                       bool preview, debug::LiveEditResult* result);
+                       bool preview, bool allow_top_frame_live_editing,
+                       debug::LiveEditResult* result);
 
   int GetFunctionDebuggingId(Handle<JSFunction> function);
 

src/debug/liveedit.cc

@@ -13,6 +13,7 @@
 #include "src/codegen/source-position-table.h"
 #include "src/common/globals.h"
 #include "src/debug/debug-interface.h"
+#include "src/debug/debug-stack-trace-iterator.h"
 #include "src/debug/debug.h"
 #include "src/execution/frames-inl.h"
 #include "src/execution/isolate-inl.h"
@@ -797,7 +798,7 @@ struct FunctionData {
   // In case of multiple functions with different stack position, the latest
   // one (in the order below) is used, since it is the most restrictive.
   // This is important only for functions to be restarted.
-  enum StackPosition { NOT_ON_STACK, ON_STACK };
+  enum StackPosition { NOT_ON_STACK, ON_TOP_ONLY, ON_STACK };
   StackPosition stack_position;
 };
 
@@ -850,7 +851,7 @@ class FunctionDataMap : public ThreadVisitor {
     }
 
     // Visit the current thread stack.
-    VisitThread(isolate, isolate->thread_local_top());
+    VisitCurrentThread(isolate);
 
     // Visit the stacks of all archived threads.
     isolate->thread_manager()->IterateArchivedThreads(this);
@@ -904,12 +905,33 @@ class FunctionDataMap : public ThreadVisitor {
     }
   }
 
+  void VisitCurrentThread(Isolate* isolate) {
+    // We allow live editing the function that's currently top-of-stack. But
+    // only if no activation of that function is anywhere else on the stack.
+    bool is_top = true;
+    for (DebugStackTraceIterator it(isolate, /* index */ 0); !it.Done();
+         it.Advance(), is_top = false) {
+      auto sfi = it.GetSharedFunctionInfo();
+      if (sfi.is_null()) continue;
+      FunctionData* data = nullptr;
+      if (!Lookup(*sfi, &data)) continue;
+
+      // ON_TOP_ONLY will only be set on the first iteration (and if the frame
+      // can be restarted). Further activations will change the ON_TOP_ONLY to
+      // ON_STACK and prevent the live edit from happening.
+      data->stack_position = is_top && it.CanBeRestarted()
+                                 ? FunctionData::ON_TOP_ONLY
+                                 : FunctionData::ON_STACK;
+    }
+  }
+
   std::map<FuncId, FunctionData> map_;
 };
 
 bool CanPatchScript(const LiteralMap& changed, Handle<Script> script,
                     Handle<Script> new_script,
                     FunctionDataMap& function_data_map,
+                    bool allow_top_frame_live_editing,
                     debug::LiveEditResult* result) {
   for (const auto& mapping : changed) {
     FunctionData* data = nullptr;
@@ -925,6 +947,12 @@ bool CanPatchScript(const LiteralMap& changed, Handle<Script> script,
     } else if (!data->running_generators.empty()) {
       result->status = debug::LiveEditResult::BLOCKED_BY_RUNNING_GENERATOR;
       return false;
+    } else if (data->stack_position == FunctionData::ON_TOP_ONLY) {
+      if (!allow_top_frame_live_editing) {
+        result->status = debug::LiveEditResult::BLOCKED_BY_ACTIVE_FUNCTION;
+        return false;
+      }
+      result->restart_top_frame_required = true;
     }
   }
   return true;
@@ -974,6 +1002,7 @@ void UpdatePositions(Isolate* isolate, Handle<SharedFunctionInfo> sfi,
 
 void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
                            Handle<String> new_source, bool preview,
+                           bool allow_top_frame_live_editing_param,
                            debug::LiveEditResult* result) {
   std::vector<SourceChangeRange> diffs;
   LiveEdit::CompareStrings(isolate,
@@ -1027,7 +1056,10 @@ void LiveEdit::PatchScript(Isolate* isolate, Handle<Script> script,
   }
   function_data_map.Fill(isolate);
 
-  if (!CanPatchScript(changed, script, new_script, function_data_map, result)) {
+  const bool allow_top_frame_live_editing =
+      allow_top_frame_live_editing_param && FLAG_live_edit_top_frame;
+  if (!CanPatchScript(changed, script, new_script, function_data_map,
+                      allow_top_frame_live_editing, result)) {
     return;
   }
 

src/debug/liveedit.h

@@ -63,6 +63,7 @@ class V8_EXPORT_PRIVATE LiveEdit : AllStatic {
                                int position);
   static void PatchScript(Isolate* isolate, Handle<Script> script,
                           Handle<String> source, bool preview,
+                          bool allow_top_frame_live_editing,
                           debug::LiveEditResult* result);
 };
 }  // namespace internal

src/flags/flag-definitions.h

@@ -1628,6 +1628,10 @@ DEFINE_BOOL(hard_abort, true, "abort by crashing")
 DEFINE_BOOL(experimental_async_stack_tagging_api, false,
             "enable experimental async stacks tagging API")
 
+DEFINE_BOOL(
+    live_edit_top_frame, true,
+    "enable support for live-editing the top-most function on the stack")
+
 // disassembler
 DEFINE_BOOL(log_colour, ENABLE_LOG_COLOUR,
             "When logging, try to use coloured output.")

src/runtime/runtime-debug.cc

@@ -894,7 +894,8 @@ RUNTIME_FUNCTION(Runtime_LiveEditPatchScript) {
   Handle<Script> script(Script::cast(script_function->shared().script()),
                         isolate);
   v8::debug::LiveEditResult result;
-  LiveEdit::PatchScript(isolate, script, new_source, false, &result);
+  LiveEdit::PatchScript(isolate, script, new_source, /* preview */ false,
+                        /* allow_top_frame_live_editing */ false, &result);
   switch (result.status) {
     case v8::debug::LiveEditResult::COMPILE_ERROR:
       return isolate->Throw(*isolate->factory()->NewStringFromAsciiChecked(

test/cctest/test-liveedit.cc

@@ -214,7 +214,7 @@ void PatchFunctions(v8::Local<v8::Context> context, const char* source_a,
   if (result) {
     LiveEdit::PatchScript(
         i_isolate, i_script_a,
-        i_isolate->factory()->NewStringFromAsciiChecked(source_b), false,
+        i_isolate->factory()->NewStringFromAsciiChecked(source_b), false, false,
         result);
     if (result->status == v8::debug::LiveEditResult::COMPILE_ERROR) {
       result->message = scope.Escape(result->message);
@@ -223,7 +223,8 @@ void PatchFunctions(v8::Local<v8::Context> context, const char* source_a,
     v8::debug::LiveEditResult r;
     LiveEdit::PatchScript(
         i_isolate, i_script_a,
-        i_isolate->factory()->NewStringFromAsciiChecked(source_b), false, &r);
+        i_isolate->factory()->NewStringFromAsciiChecked(source_b), false, false,
+        &r);
     CHECK_EQ(r.status, v8::debug::LiveEditResult::OK);
   }
 }
@@ -549,7 +550,7 @@ TEST(LiveEditFunctionExpression) {
   LiveEdit::PatchScript(
       i_isolate, i_script,
       i_isolate->factory()->NewStringFromAsciiChecked(updated_source), false,
-      &result);
+      false, &result);
   CHECK_EQ(result.status, debug::LiveEditResult::OK);
   {
     v8::Local<v8::String> result_str =