Fix Wasm reporting to multiple inspectors

Separate creating Wasm translations from reporting them to an agent.
This is done in order to support multiple connected sessions.

Previously connecting more than one agent would fail assertion in debug
mode and overwrite translation objects over and over
(and potentially do something worse) in release mode.

Bug: v8:9725
Change-Id: I13fde5ebf6e64e7268eb6870f9c21ac9a5bed81e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1807273Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Ingvar Stepanyan <rreverser@google.com>
Cr-Commit-Position: refs/heads/master@{#63867}

src/inspector/wasm-translation.cc

@@ -67,15 +67,20 @@ class WasmTranslation::TranslatorImpl {
           column(column) {}
   };
 
-  TranslatorImpl(v8::Isolate* isolate, v8::Local<v8::debug::WasmScript> script)
+  TranslatorImpl(v8::Isolate* isolate, WasmTranslation* translation,
+                 v8::Local<v8::debug::WasmScript> script)
       : script_(isolate, script) {
     script_.AnnotateStrongRetainer(kGlobalScriptHandleLabel);
+
+    ForEachFunction(script, [this, translation](String16& script_id,
+                                                int func_idx) {
+      translation->AddFakeScript(GetFakeScriptId(script_id, func_idx), this);
+    });
   }
 
-  void Init(v8::Isolate* isolate, WasmTranslation* translation,
-            V8DebuggerAgentImpl* agent) {
-    // Register fake scripts for each function in this wasm module/script.
-    v8::Local<v8::debug::WasmScript> script = script_.Get(isolate);
+  template <typename Callback>
+  void ForEachFunction(v8::Local<v8::debug::WasmScript> script,
+                       Callback callback) {
     int num_functions = script->NumFunctions();
     int num_imported_functions = script->NumImportedFunctions();
     DCHECK_LE(0, num_imported_functions);
@@ -84,10 +89,18 @@ class WasmTranslation::TranslatorImpl {
     String16 script_id = String16::fromInteger(script->Id());
     for (int func_idx = num_imported_functions; func_idx < num_functions;
          ++func_idx) {
-      AddFakeScript(isolate, script_id, func_idx, translation, agent);
+      callback(script_id, func_idx);
     }
   }
 
+  void ReportFakeScripts(v8::Isolate* isolate, WasmTranslation* translation,
+                         V8DebuggerAgentImpl* agent) {
+    ForEachFunction(
+        script_.Get(isolate), [=](String16& script_id, int func_idx) {
+          ReportFakeScript(isolate, script_id, func_idx, translation, agent);
+        });
+  }
+
   void Translate(TransLocation* loc) {
     const OffsetTable& offset_table = GetOffsetTable(loc);
     DCHECK(!offset_table.empty());
@@ -212,8 +225,9 @@ class WasmTranslation::TranslatorImpl {
     return GetFakeScriptId(loc->script_id, loc->line);
   }
 
-  void AddFakeScript(v8::Isolate* isolate, const String16& underlyingScriptId,
-                     int func_idx, WasmTranslation* translation,
+  void ReportFakeScript(v8::Isolate* isolate,
+                        const String16& underlyingScriptId, int func_idx,
+                        WasmTranslation* translation,
                         V8DebuggerAgentImpl* agent) {
     String16 fake_script_id = GetFakeScriptId(underlyingScriptId, func_idx);
     String16 fake_script_url = GetFakeScriptUrl(isolate, func_idx);
@@ -223,7 +237,6 @@ class WasmTranslation::TranslatorImpl {
                                      fake_script_id, std::move(fake_script_url),
                                      func_idx);
 
-    translation->AddFakeScript(fake_script->scriptId(), this);
     agent->didParseSource(std::move(fake_script), true);
   }
 
@@ -254,6 +267,9 @@ class WasmTranslation::TranslatorImpl {
   // We assume to only disassemble a subset of the functions, so store them in a
   // map instead of an array.
   std::unordered_map<int, WasmSourceInformation> source_informations_;
+
+  // Disallow copies, because our pointer is registered in translation.
+  DISALLOW_COPY_AND_ASSIGN(TranslatorImpl);
 };
 
 constexpr char WasmTranslation::TranslatorImpl::kGlobalScriptHandleLabel[];
@@ -264,15 +280,11 @@ WasmTranslation::~WasmTranslation() { Clear(); }
 
 void WasmTranslation::AddScript(v8::Local<v8::debug::WasmScript> script,
                                 V8DebuggerAgentImpl* agent) {
-  std::unique_ptr<TranslatorImpl> impl;
-  impl.reset(new TranslatorImpl(isolate_, script));
-  DCHECK(impl);
-  auto inserted =
-      wasm_translators_.insert(std::make_pair(script->Id(), std::move(impl)));
-  // Check that no mapping for this script id existed before.
-  DCHECK(inserted.second);
-  // impl has been moved, use the returned iterator to call Init.
-  inserted.first->second->Init(isolate_, this, agent);
+  auto& impl = wasm_translators_[script->Id()];
+  if (impl == nullptr) {
+    impl = std::make_unique<TranslatorImpl>(isolate_, this, script);
+  }
+  impl->ReportFakeScripts(isolate_, this, agent);
 }
 
 void WasmTranslation::Clear() {

test/inspector/debugger/wasm-scripts-expected.txt

 Tests how wasm scripts are reported
-Check that inspector gets two wasm scripts at module creation time.
-Script #0 parsed. URL: v8://test/testFunction
-Script #1 parsed. URL: 
-Script #2 parsed. URL: v8://test/runTestRunction
-Script #3 parsed. URL: wasm://wasm/wasm-7b04570e/wasm-7b04570e-0
-Script #4 parsed. URL: wasm://wasm/wasm-7b04570e/wasm-7b04570e-1
-Source for wasm://wasm/wasm-7b04570e/wasm-7b04570e-0:
+Check that each inspector gets two wasm scripts at module creation time.
+Session #1: Script #0 parsed. URL: wasm://wasm/wasm-7b04570e/wasm-7b04570e-0
+Session #1: Script #1 parsed. URL: wasm://wasm/wasm-7b04570e/wasm-7b04570e-1
+Session #2: Script #0 parsed. URL: wasm://wasm/wasm-7b04570e/wasm-7b04570e-0
+Session #2: Script #1 parsed. URL: wasm://wasm/wasm-7b04570e/wasm-7b04570e-1
+Session #1: Source for wasm://wasm/wasm-7b04570e/wasm-7b04570e-0:
 func $nopFunction
   nop
 end
 
-Source for wasm://wasm/wasm-7b04570e/wasm-7b04570e-1:
+Session #1: Source for wasm://wasm/wasm-7b04570e/wasm-7b04570e-1:
 func $main
   block
     i32.const 2
@@ -18,3 +17,15 @@ func $main
   end
 end
 
+Session #2: Source for wasm://wasm/wasm-7b04570e/wasm-7b04570e-0:
+func $nopFunction
+  nop
+end
+
+Session #2: Source for wasm://wasm/wasm-7b04570e/wasm-7b04570e-1:
+func $main
+  block
+    i32.const 2
+    drop
+  end
+end

test/inspector/debugger/wasm-scripts.js

@@ -4,7 +4,16 @@
 
 // Flags: --expose-wasm
 
-let {session, contextGroup, Protocol} = InspectorTest.start('Tests how wasm scripts are reported');
+InspectorTest.log("Tests how wasm scripts are reported");
+
+let contextGroup = new InspectorTest.ContextGroup();
+let sessions = [
+  // Main session.
+  trackScripts(),
+  // Extra session to verify that all inspectors get same messages.
+  // See https://bugs.chromium.org/p/v8/issues/detail?id=9725.
+  trackScripts(),
+];
 
 utils.load('test/mjsunit/wasm/wasm-module-builder.js');
 
@@ -18,54 +27,55 @@ builder.addFunction('main', kSig_v_v)
 var module_bytes = builder.toArray();
 
 function testFunction(bytes) {
-  var buffer = new ArrayBuffer(bytes.length);
-  var view = new Uint8Array(buffer);
-  for (var i = 0; i < bytes.length; i++) {
-    view[i] = bytes[i] | 0;
-  }
-
   // Compilation triggers registration of wasm scripts.
-  new WebAssembly.Module(buffer);
+  new WebAssembly.Module(new Uint8Array(bytes));
 }
 
 contextGroup.addScript(testFunction.toString(), 0, 0, 'v8://test/testFunction');
 contextGroup.addScript('var module_bytes = ' + JSON.stringify(module_bytes));
 
-Protocol.Debugger.enable();
-Protocol.Debugger.onScriptParsed(handleScriptParsed);
 InspectorTest.log(
-    'Check that inspector gets two wasm scripts at module creation time.');
-Protocol.Runtime
+    'Check that each inspector gets two wasm scripts at module creation time.');
+
+sessions[0].Protocol.Runtime
     .evaluate({
       'expression': '//# sourceURL=v8://test/runTestRunction\n' +
           'testFunction(module_bytes)'
     })
-    .then(checkFinished);
-
-var num_scripts = 0;
-var missing_sources = 0;
+    .then(() => (
+      // At this point all scripts were parsed.
+      // Stop tracking and wait for script sources in each session.
+      Promise.all(sessions.map(session => session.getScripts()))
+    ))
+    .catch(err => {
+      InspectorTest.log("FAIL: " + err.message);
+    })
+    .then(() => InspectorTest.completeTest());
 
-function checkFinished() {
-  if (missing_sources == 0)
-    InspectorTest.completeTest();
-}
+function trackScripts() {
+  let {id: sessionId, Protocol} = contextGroup.connect();
+  let scripts = [];
 
-function handleScriptParsed(messageObject)
-{
-  var url = messageObject.params.url;
-  InspectorTest.log("Script #" + num_scripts + " parsed. URL: " + url);
-  ++num_scripts;
+  Protocol.Debugger.enable();
+  Protocol.Debugger.onScriptParsed(handleScriptParsed);
 
-  if (url.startsWith("wasm://")) {
-    ++missing_sources;
-    function dumpScriptSource(message) {
-      InspectorTest.log("Source for " + url + ":");
-      InspectorTest.log(message.result.scriptSource);
-      --missing_sources;
+  async function getScript({url, scriptId}) {
+    let {result: {scriptSource}} = await Protocol.Debugger.getScriptSource({scriptId});
+    InspectorTest.log(`Session #${sessionId}: Source for ${url}:`);
+    InspectorTest.log(scriptSource);
+    return {url, scriptSource};
   }
 
-    Protocol.Debugger.getScriptSource({scriptId: messageObject.params.scriptId})
-        .then(dumpScriptSource.bind(null))
-        .then(checkFinished);
+  function handleScriptParsed({params}) {
+    let {url} = params;
+    if (!url.startsWith("wasm://")) return;
+
+    InspectorTest.log(`Session #${sessionId}: Script #${scripts.length} parsed. URL: ${url}`);
+    scripts.push(getScript(params));
   }
+
+  return {
+    Protocol,
+    getScripts: () => Promise.all(scripts),
+  };
 }