From eed401bc6cc222111dc95b112f1edbb6f10a1847 Mon Sep 17 00:00:00 2001
From: Danil Somsikov <[email protected]>
Date: Thu, 30 Apr 2026 04:26:21 -0700
Subject: [PATCH] Avoid holding raw script pointers across potential GC events.

The changes modify how breakpoints are set and removed by:
-   Collecting script IDs instead of raw V8DebuggerScript pointers before iterating.
-   Re-looking up scripts in the `m_scripts` map within loops, especially after calls that might trigger garbage collection and invalidate pointers or iterators.
-   Updating `removeBreakpointImpl` to accept a vector of script IDs instead of script pointers.

Bug: 502784366
Change-Id: I940b27de9b45b176b39d56bbe8abcd6d4e8a35f6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7785931
Auto-Submit: Danil Somsikov <[email protected]>
Commit-Queue: Danil Somsikov <[email protected]>
Reviewed-by: Yang Guo <[email protected]>
Reviewed-by: Simon Zünd <[email protected]>
Cr-Commit-Position: refs/heads/main@{#106998}
---

diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc
index 4233053..b69dad0 100644
--- a/src/debug/debug-interface.cc
+++ b/src/debug/debug-interface.cc
@@ -1350,6 +1350,16 @@
 
 PostponeInterruptsScope::~PostponeInterruptsScope() = default;
 
+DisallowGarbageCollectionScope::DisallowGarbageCollectionScope() {
+  new (internal_) i::DisallowGarbageCollectionInRelease();
+}
+
+DisallowGarbageCollectionScope::~DisallowGarbageCollectionScope() {
+  using i::DisallowGarbageCollectionInRelease;
+  reinterpret_cast<DisallowGarbageCollectionInRelease*>(internal_)
+      ->~DisallowGarbageCollectionInRelease();
+}
+
 DisableBreakScope::DisableBreakScope(v8::Isolate* isolate)
     : scope_(std::make_unique<i::DisableBreak>(
           reinterpret_cast<i::Isolate*>(isolate)->debug())) {}
diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h
index 0677ea0..7891d49 100644
--- a/src/debug/debug-interface.h
+++ b/src/debug/debug-interface.h
@@ -596,6 +596,16 @@
   std::unique_ptr<i::PostponeInterruptsScope> scope_;
 };
 
+class V8_NODISCARD DisallowGarbageCollectionScope {
+ public:
+  DisallowGarbageCollectionScope();
+  ~DisallowGarbageCollectionScope();
+
+ private:
+  alignas(internal::Internals::kDisallowGarbageCollectionAlign) char internal_
+      [internal::Internals::kDisallowGarbageCollectionSize];
+};
+
 class V8_NODISCARD DisableBreakScope {
  public:
   explicit DisableBreakScope(v8::Isolate* isolate);
diff --git a/src/inspector/v8-debugger-agent-impl.cc b/src/inspector/v8-debugger-agent-impl.cc
index f7c9783..50e59b5 100644
--- a/src/inspector/v8-debugger-agent-impl.cc
+++ b/src/inspector/v8-debugger-agent-impl.cc
@@ -688,29 +688,59 @@
         "Breakpoint at specified location already exists.");
   }
 
+  std::vector<String16> allScriptIds;
+  allScriptIds.reserve(m_scripts.size());
+  for (const auto& scriptIter : m_scripts) {
+    allScriptIds.push_back(scriptIter.first);
+  }
+
   std::unique_ptr<protocol::DictionaryValue> hint;
-  for (const auto& script : m_scripts) {
-    if (!matcher.matches(*script.second)) continue;
-    // Make sure the session was not disabled by some re-entrant call
-    // in the script matcher.
-    DCHECK(enabled());
-    int adjustedLineNumber = lineNumber;
-    int adjustedColumnNumber = columnNumber;
-    if (hint) {
-      adjustBreakpointLocation(*script.second, hint.get(), &adjustedLineNumber,
-                               &adjustedColumnNumber);
+  for (const auto& scriptId : allScriptIds) {
+    std::shared_ptr<V8DebuggerScript> script;
+    {
+      v8::debug::DisallowGarbageCollectionScope no_gc;
+      if (getScriptById(scriptId, no_gc)) {
+        script = m_scripts.at(scriptId);
+      }
     }
-    std::unique_ptr<protocol::Debugger::Location> location =
-        setBreakpointImpl(breakpointId, script.first, condition,
-                          adjustedLineNumber, adjustedColumnNumber);
+    if (!script) continue;
+
+    bool isMatch = matcher.matches(*script);
+
+    if (!isMatch) continue;  // Check Match First
+
     if (!enabled()) {
       return Response::ServerError(
           "Debugger domain disabled during setBreakpoint");
     }
+
+    int adjustedLineNumber = lineNumber;
+    int adjustedColumnNumber = columnNumber;
+    if (hint) {
+      adjustBreakpointLocation(*script, hint.get(), &adjustedLineNumber,
+                               &adjustedColumnNumber);
+    }
+    std::unique_ptr<protocol::Debugger::Location> location =
+        setBreakpointImpl(breakpointId, scriptId, condition, adjustedLineNumber,
+                          adjustedColumnNumber);
+    if (!enabled()) {
+      return Response::ServerError(
+          "Debugger domain disabled during setBreakpoint");
+    }
+
+    // We need to look up the script again because setBreakpointImpl might have
+    // triggered GC
     if (location && type != BreakpointType::kByUrlRegex) {
-      hint = breakpointHint(*script.second, lineNumber, columnNumber,
-                            location->getLineNumber(),
-                            location->getColumnNumber(adjustedColumnNumber));
+      bool is_active = false;
+      {
+        v8::debug::DisallowGarbageCollectionScope no_gc;
+        is_active = getScriptById(scriptId, no_gc) != nullptr;
+      }
+      if (is_active) {
+        hint = breakpointHint(*script, lineNumber, columnNumber,
+                              location->getLineNumber(),
+                              location->getColumnNumber(adjustedColumnNumber));
+      }
     }
     if (location) (*locations)->emplace_back(std::move(location));
   }
@@ -832,16 +862,34 @@
   // Get a list of scripts to remove breakpoints.
   // TODO(duongn): we can do better here if from breakpoint id we can tell it is
   // not Wasm breakpoint.
-  std::vector<V8DebuggerScript*> scripts;
+  std::vector<String16> allScriptIds;
+  allScriptIds.reserve(m_scripts.size());
   for (const auto& scriptIter : m_scripts) {
-    const bool scriptSelectorMatch = matcher.matches(*scriptIter.second);
-    // Make sure the session was not disabled by some re-entrant call
-    // in the script matcher.
-    DCHECK(enabled());
-    const bool isInstrumentation =
-        type == BreakpointType::kInstrumentationBreakpoint;
-    if (!scriptSelectorMatch && !isInstrumentation) continue;
-    V8DebuggerScript* script = scriptIter.second.get();
+    allScriptIds.push_back(scriptIter.first);
+  }
+
+  const bool isInstrumentation =
+      type == BreakpointType::kInstrumentationBreakpoint;
+  std::vector<std::shared_ptr<V8DebuggerScript>> scripts;
+  for (const auto& scriptId : allScriptIds) {
+    std::shared_ptr<V8DebuggerScript> script;
+    {
+      v8::debug::DisallowGarbageCollectionScope no_gc;
+      if (getScriptById(scriptId, no_gc)) {
+        script = m_scripts.at(scriptId);
+      }
+    }
+    if (!script) continue;
+
+    bool isMatch = matcher.matches(*script);
+
+    if (!isMatch && !isInstrumentation) continue;  // Check Match First
+
+    if (!enabled()) {
+      return Response::ServerError(
+          "Debugger domain disabled during removeBreakpoint");
+    }
+
     if (script->getLanguage() == V8DebuggerScript::Language::WebAssembly) {
       scripts.push_back(script);
     }
@@ -853,7 +901,7 @@
 
 void V8DebuggerAgentImpl::removeBreakpointImpl(
     const String16& breakpointId,
-    const std::vector<V8DebuggerScript*>& scripts) {
+    const std::vector<std::shared_ptr<V8DebuggerScript>>& scripts) {
   DCHECK(enabled());
   BreakpointIdToDebuggerBreakpointIdsMap::iterator
       debuggerBreakpointIdsIterator =
@@ -864,7 +912,7 @@
   }
   for (const auto& id : debuggerBreakpointIdsIterator->second) {
 #if V8_ENABLE_WEBASSEMBLY
-    for (auto& script : scripts) {