[inspector] Don't hold on to Script objects strongly for caching.

Previously we'd hold on to Script objects strongly after they are
considered unreachable by V8 itself, and keep them around for the
V8DebuggerAgent cache (whose upper limit can be controlled with a
parameter to `Debugger.enable`).

This CL changes that to instead copy out the script source and the
WebAssembly bytecode (depending on whether it's JavaScript or Wasm)
to the C++ heap and keep it cached there.

Fixed: chromium:1295659
Bug: chromium:1246884
Change-Id: Idfcd7172715eafca6b011826ae03a573d58803f2
Doc: https://bit.ly/v8-inspector-script-caching
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3472082
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79217}

src/inspector/v8-debugger-agent-impl.cc

@@ -435,7 +435,7 @@ Response V8DebuggerAgentImpl::disable() {
   resetBlackboxedStateCache();
   m_skipList.clear();
   m_scripts.clear();
-  m_cachedScriptIds.clear();
+  m_cachedScripts.clear();
   m_cachedScriptSize = 0;
   for (const auto& it : m_debuggerBreakpointIdToBreakpointId) {
     v8::debug::RemoveBreakpoint(m_isolate, it.first);
@@ -1072,8 +1072,20 @@ Response V8DebuggerAgentImpl::getScriptSource(
     Maybe<protocol::Binary>* bytecode) {
   if (!enabled()) return Response::ServerError(kDebuggerNotEnabled);
   ScriptsMap::iterator it = m_scripts.find(scriptId);
-  if (it == m_scripts.end())
+  if (it == m_scripts.end()) {
+    auto cachedScriptIt =
+        std::find_if(m_cachedScripts.begin(), m_cachedScripts.end(),
+                     [&scriptId](const CachedScript& cachedScript) {
+                       return cachedScript.scriptId == scriptId;
+                     });
+    if (cachedScriptIt != m_cachedScripts.end()) {
+      *scriptSource = cachedScriptIt->source;
+      *bytecode = protocol::Binary::fromSpan(cachedScriptIt->bytecode.data(),
+                                             cachedScriptIt->bytecode.size());
+      return Response::Success();
+    }
     return Response::ServerError("No script for id: " + scriptId.utf8());
+  }
   *scriptSource = it->second->source(0);
 #if V8_ENABLE_WEBASSEMBLY
   v8::MemorySpan<const uint8_t> span;
@@ -1949,23 +1961,31 @@ void V8DebuggerAgentImpl::reset() {
   resetBlackboxedStateCache();
   m_skipList.clear();
   m_scripts.clear();
-  m_cachedScriptIds.clear();
+  m_cachedScripts.clear();
   m_cachedScriptSize = 0;
 }
 
 void V8DebuggerAgentImpl::ScriptCollected(const V8DebuggerScript* script) {
   DCHECK_NE(m_scripts.find(script->scriptId()), m_scripts.end());
-  m_cachedScriptIds.push_back(script->scriptId());
-  // TODO(alph): Properly calculate size when sources are one-byte strings.
-  m_cachedScriptSize += script->length() * sizeof(uint16_t);
+  std::vector<uint8_t> bytecode;
+#if V8_ENABLE_WEBASSEMBLY
+  v8::MemorySpan<const uint8_t> span;
+  if (script->wasmBytecode().To(&span)) {
+    bytecode.reserve(span.size());
+    bytecode.insert(bytecode.begin(), span.data(), span.data() + span.size());
+  }
+#endif
+  CachedScript cachedScript{script->scriptId(), script->source(0),
+                            std::move(bytecode)};
+  m_cachedScriptSize += cachedScript.size();
+  m_cachedScripts.push_back(std::move(cachedScript));
+  m_scripts.erase(script->scriptId());
 
   while (m_cachedScriptSize > m_maxScriptCacheSize) {
-    const String16& scriptId = m_cachedScriptIds.front();
-    size_t scriptSize = m_scripts[scriptId]->length() * sizeof(uint16_t);
-    DCHECK_GE(m_cachedScriptSize, scriptSize);
-    m_cachedScriptSize -= scriptSize;
-    m_scripts.erase(scriptId);
-    m_cachedScriptIds.pop_front();
+    const CachedScript& cachedScript = m_cachedScripts.front();
+    DCHECK_GE(m_cachedScriptSize, cachedScript.size());
+    m_cachedScriptSize -= cachedScript.size();
+    m_cachedScripts.pop_front();
   }
 }
 

src/inspector/v8-debugger-agent-impl.h

@@ -226,7 +226,16 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
 
   size_t m_maxScriptCacheSize = 0;
   size_t m_cachedScriptSize = 0;
-  std::deque<String16> m_cachedScriptIds;
+  struct CachedScript {
+    String16 scriptId;
+    String16 source;
+    std::vector<uint8_t> bytecode;
+
+    size_t size() const {
+      return source.length() * sizeof(UChar) + bytecode.size();
+    }
+  };
+  std::deque<CachedScript> m_cachedScripts;
 
   using BreakReason =
       std::pair<String16, std::unique_ptr<protocol::DictionaryValue>>;

test/inspector/debugger/get-possible-breakpoints-after-gc-expected.txt

@@ -1,7 +1,5 @@
 Checks if we keep alive breakpoint information for top-level functions when calling getPossibleBreakpoints.
-Result of get possible breakpoints in topLevel.js
-[{"scriptId":"3","lineNumber":0,"columnNumber":0},{"scriptId":"3","lineNumber":0,"columnNumber":8,"type":"call"},{"scriptId":"3","lineNumber":0,"columnNumber":43,"type":"return"}]
 Result of get possible breakpoints in moduleFunc.js
-[{"scriptId":"5","lineNumber":0,"columnNumber":22},{"scriptId":"5","lineNumber":0,"columnNumber":30,"type":"call"},{"scriptId":"5","lineNumber":0,"columnNumber":63,"type":"return"},{"scriptId":"5","lineNumber":0,"columnNumber":64,"type":"return"}]
+[{"scriptId":"3","lineNumber":0,"columnNumber":22},{"scriptId":"3","lineNumber":0,"columnNumber":30,"type":"call"},{"scriptId":"3","lineNumber":0,"columnNumber":63,"type":"return"},{"scriptId":"3","lineNumber":0,"columnNumber":64,"type":"return"}]
 Result of get possible breakpoints in mixedFunctions.js
-[{"scriptId":"7","lineNumber":0,"columnNumber":15,"type":"return"},{"scriptId":"7","lineNumber":1,"columnNumber":2},{"scriptId":"7","lineNumber":1,"columnNumber":10,"type":"call"},{"scriptId":"7","lineNumber":2,"columnNumber":0,"type":"return"}]
+[{"scriptId":"5","lineNumber":0,"columnNumber":15,"type":"return"},{"scriptId":"5","lineNumber":1,"columnNumber":2},{"scriptId":"5","lineNumber":1,"columnNumber":10,"type":"call"},{"scriptId":"5","lineNumber":2,"columnNumber":0,"type":"return"}]

test/inspector/debugger/get-possible-breakpoints-after-gc.js

@@ -17,7 +17,6 @@ const callGarbageCollector = `
   %CollectGarbage("");
 `;
 
-const topLevelFunction = `console.log('This is a top level function')`;
 const moduleFunction =
     `function testFunc() { console.log('This is a module function') }`;
 let mixedFunctions = ` function A() {}
@@ -33,8 +32,6 @@ function onDebuggerEnabled() {
 
 async function onExecutionContextCreated(messageObject) {
   executionContextId = messageObject.params.context.id;
-  await testGetPossibleBreakpoints(
-      executionContextId, topLevelFunction, 'topLevel.js');
   await testGetPossibleBreakpoints(
       executionContextId, moduleFunction, 'moduleFunc.js');
   await testGetPossibleBreakpoints(

test/inspector/debugger/set-breakpoint-after-gc-expected.txt

@@ -1,5 +1,5 @@
 Checks if we keep alive breakpoint information for top-level functions.
 Result of setting breakpoint in topLevel.js
-[{"scriptId":"3","lineNumber":0,"columnNumber":0}]
+[]
 Result of setting breakpoint in moduleFunc.js
-[{"scriptId":"5","lineNumber":0,"columnNumber":22}]
+[{"scriptId":"5","lineNumber":0,"columnNumber":22}]