[wasm][debug] Garbage-collect stepping code

All wasm code has an initial ref count of 1, in the expectation that it
will be added to the code table. When the code is removed from that
table, the ref count will be decremented.
Stepping code (and also other code under special circumstances) will not
be added to the code table though. Hence the ref count will never be
decremented below 1, and the code will never be garbage-collected.

This CL fixes this, by decrementing the ref count if the code is not
added to the code table.
Note that the code will only be collected if no isolate is currently
using it, so it won't be collected while still in use for stepping.

R=thibaudm@chromium.org

Bug: chromium:1168564
Change-Id: I3047753591cbc52689ca019e9548ec58c237b835
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649040
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72354}

src/wasm/wasm-code-manager.cc

@@ -1133,6 +1133,10 @@ WasmCode* NativeModule::PublishCodeLocked(std::unique_ptr<WasmCode> code) {
   // The caller must hold the {allocation_mutex_}, thus we fail to lock it here.
   DCHECK(!allocation_mutex_.TryLock());
 
+  // Add the code to the surrounding code ref scope, so the returned pointer is
+  // guaranteed to be valid.
+  WasmCodeRefScope::AddRef(code.get());
+
   if (!code->IsAnonymous() &&
       code->index() >= module_->num_imported_functions) {
     DCHECK_LT(code->index(), num_functions());
@@ -1169,17 +1173,21 @@ WasmCode* NativeModule::PublishCodeLocked(std::unique_ptr<WasmCode> code) {
         WasmCodeRefScope::AddRef(prior_code);
         // The code is added to the current {WasmCodeRefScope}, hence the ref
         // count cannot drop to zero here.
-        CHECK(!prior_code->DecRef());
+        prior_code->DecRefOnLiveCode();
       }
 
       PatchJumpTablesLocked(slot_idx, code->instruction_start());
+    } else {
+      // The code tables does not hold a reference to the code, hence decrement
+      // the initial ref count of 1. The code was added to the
+      // {WasmCodeRefScope} though, so it cannot die here.
+      code->DecRefOnLiveCode();
     }
     if (!code->for_debugging() && tiering_state_ == kTieredDown &&
         code->tier() == ExecutionTier::kTurbofan) {
       liftoff_bailout_count_.fetch_add(1);
     }
   }
-  WasmCodeRefScope::AddRef(code.get());
   WasmCode* result = code.get();
   owned_code_.emplace(result->instruction_start(), std::move(code));
   return result;

src/wasm/wasm-code-manager.h

@@ -228,6 +228,14 @@ class V8_EXPORT_PRIVATE WasmCode final {
     }
   }
 
+  // Decrement the ref count on code that is known to be in use (i.e. the ref
+  // count cannot drop to zero here).
+  void DecRefOnLiveCode() {
+    int old_count = ref_count_.fetch_sub(1, std::memory_order_acq_rel);
+    DCHECK_LE(2, old_count);
+    USE(old_count);
+  }
+
   // Decrement the ref count on code that is known to be dead, even though there
   // might still be C++ references. Returns whether this drops the last
   // reference and the code needs to be freed.

test/inspector/debugger/wasm-step-a-lot-expected.txt

@@ -0,0 +1,16 @@
+Tests repeated stepping through a large function (should not OOM)
+
+Running test: test
+Setting up global instance variable.
+Got wasm script: wasm://wasm/46f40116
+Setting breakpoint
+Paused 500 and running...
+Paused 1000 and running...
+Paused 1500 and running...
+Paused 2000 and running...
+Paused 2500 and running...
+Paused 3000 and running...
+Paused 3500 and running...
+Paused 4000 and running...
+test function returned.
+Paused 4003 times.

test/inspector/debugger/wasm-step-a-lot.js

@@ -0,0 +1,51 @@
+// Copyright 2021 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.
+
+utils.load('test/inspector/wasm-inspector-test.js');
+
+const {session, contextGroup, Protocol} = InspectorTest.start(
+    'Tests repeated stepping through a large function (should not OOM)');
+session.setupScriptMap();
+
+const builder = new WasmModuleBuilder();
+
+const body = [kExprLocalGet, 0];
+// Stepping through a long function will repeatedly recreate stepping code, with
+// corresponding side tables. This should not run OOM
+// (https://crbug.com/1168564).
+for (let i = 0; i < 2000; ++i) body.push(...wasmI32Const(i), kExprI32Add);
+const func_test =
+    builder.addFunction('test', kSig_i_i).addBody(body).exportFunc();
+const module_bytes = builder.toArray();
+
+let paused = 0;
+Protocol.Debugger.onPaused(msg => {
+  ++paused;
+  if (paused % 500 == 0) InspectorTest.log(`Paused ${paused} and running...`);
+  Protocol.Debugger.stepOver();
+});
+
+InspectorTest.runAsyncTestSuite([
+  async function test() {
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting up global instance variable.');
+    WasmInspectorTest.instantiate(module_bytes);
+    const [, {params: wasmScript}] = await Protocol.Debugger.onceScriptParsed(2);
+
+    InspectorTest.log('Got wasm script: ' + wasmScript.url);
+
+    InspectorTest.log('Setting breakpoint');
+    await Protocol.Debugger.setBreakpoint({
+      location: {
+        scriptId: wasmScript.scriptId,
+        lineNumber: 0,
+        columnNumber: func_test.body_offset
+      }
+    });
+
+    await Protocol.Runtime.evaluate({ expression: 'instance.exports.test()' });
+    InspectorTest.log('test function returned.');
+    InspectorTest.log(`Paused ${paused} times.`);
+  }
+]);