deps: V8: backport wasm wrapper lifetime fix

common.gypi

@@ -42,7 +42,7 @@
 
     # Reset this number to 0 on major V8 upgrades.
     # Increment by one for each non-official patch applied to deps/v8.
-    'v8_embedder_string': '-node.53',
+    'v8_embedder_string': '-node.55',
 
     ##### V8 defaults for Node.js #####
 

deps/v8/src/wasm/wasm-code-manager.cc

@@ -2776,6 +2776,10 @@ void NativeModule::FreeCode(base::Vector<WasmCode* const> codes) {
   // Free the {WasmCode} objects. This will also unregister trap handler data.
   for (WasmCode* code : codes) {
     DCHECK_EQ(1, owned_code_.count(code->instruction_start()));
+    // TODO(407003348): Drop this check if it doesn't trigger in the wild.
+    CHECK_EQ(WasmCode::refcount(
+                 code->ref_count_bitfield_.load(std::memory_order_acquire)),
+             0);
     owned_code_.erase(code->instruction_start());
   }
   // Remove debug side tables for all removed code objects, after releasing our
@@ -2995,6 +2999,16 @@ void WasmCodeRefScope::AddRef(WasmCode* code) {
   code->IncRef();
 }
 
+// static
+WasmCode* WasmCodeRefScope::AddRefIfNotDying(WasmCode* code) {
+  DCHECK_NOT_NULL(code);
+  WasmCodeRefScope* current_scope = current_code_refs_scope;
+  DCHECK_NOT_NULL(current_scope);
+  if (!code->IncRefIfNotDying()) return nullptr;
+  current_scope->code_ptrs_.push_back(code);
+  return code;
+}
+
 void WasmCodeLookupCache::Flush() {
   for (int i = 0; i < kWasmCodeLookupCacheSize; i++)
     cache_[i].pc.store(kNullAddress, std::memory_order_release);

deps/v8/src/wasm/wasm-code-manager.h

@@ -261,40 +261,51 @@ class V8_EXPORT_PRIVATE WasmCode final {
   ~WasmCode();
 
   void IncRef() {
-    [[maybe_unused]] int old_val =
-        ref_count_.fetch_add(1, std::memory_order_acq_rel);
-    DCHECK_LE(1, old_val);
-    DCHECK_GT(kMaxInt, old_val);
+    [[maybe_unused]] uint32_t old_field =
+        ref_count_bitfield_.fetch_add(1, std::memory_order_acq_rel);
+    DCHECK_LE(1, refcount(old_field));
+    DCHECK_GT(kMaxInt, refcount(old_field));
+  }
+
+  // Returns true if the refcount was incremented, false if {this->is_dying()}.
+  bool IncRefIfNotDying() {
+    uint32_t old_field = ref_count_bitfield_.load(std::memory_order_acquire);
+    while (true) {
+      if (is_dying(old_field)) return false;
+      if (ref_count_bitfield_.compare_exchange_weak(
+              old_field, old_field + 1, std::memory_order_acq_rel)) {
+        return true;
+      }
+    }
   }
 
   // Decrement the ref count. Returns whether this code becomes dead and needs
   // to be freed.
   V8_WARN_UNUSED_RESULT bool DecRef() {
-    int old_count = ref_count_.load(std::memory_order_acquire);
+    uint32_t old_field = ref_count_bitfield_.load(std::memory_order_acquire);
     while (true) {
-      DCHECK_LE(1, old_count);
-      if (V8_UNLIKELY(old_count == 1)) {
-        if (is_dying()) {
+      DCHECK_LE(1, refcount(old_field));
+      if (V8_UNLIKELY(refcount(old_field) == 1)) {
+        if (is_dying(old_field)) {
           // The code was already on the path to deletion, only temporary
           // C++ references to it are left. Decrement the refcount, and
           // return true if it drops to zero.
           return DecRefOnDeadCode();
         }
         // Otherwise, the code enters the path to destruction now.
-        mark_as_dying();
-        old_count = ref_count_.load(std::memory_order_acquire);
-        if (V8_LIKELY(old_count == 1)) {
+        if (ref_count_bitfield_.compare_exchange_weak(
+                old_field, old_field | kIsDyingMask,
+                std::memory_order_acq_rel)) {
           // No other thread got in the way. Commit to the decision.
           DecRefOnPotentiallyDeadCode();
           return false;
         }
-        // Another thread managed to increment the refcount again, just
-        // before we set the "dying" bit. So undo that, and resume the
-        // loop to evaluate again what needs to be done.
-        undo_mark_as_dying();
+        // Another thread interfered. Re-evaluate what to do.
+        continue;
       }
-      if (ref_count_.compare_exchange_weak(old_count, old_count - 1,
-                                           std::memory_order_acq_rel)) {
+      DCHECK_LT(1, refcount(old_field));
+      if (ref_count_bitfield_.compare_exchange_weak(
+              old_field, old_field - 1, std::memory_order_acq_rel)) {
         return false;
       }
     }
@@ -303,16 +314,18 @@ 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() {
-    [[maybe_unused]] int old_count =
-        ref_count_.fetch_sub(1, std::memory_order_acq_rel);
-    DCHECK_LE(2, old_count);
+    [[maybe_unused]] uint32_t old_bitfield_value =
+        ref_count_bitfield_.fetch_sub(1, std::memory_order_acq_rel);
+    DCHECK_LE(2, refcount(old_bitfield_value));
   }
 
   // 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.
   V8_WARN_UNUSED_RESULT bool DecRefOnDeadCode() {
-    return ref_count_.fetch_sub(1, std::memory_order_acq_rel) == 1;
+    uint32_t old_bitfield_value =
+        ref_count_bitfield_.fetch_sub(1, std::memory_order_acq_rel);
+    return refcount(old_bitfield_value) == 1;
   }
 
   // Decrement the ref count on a set of {WasmCode} objects, potentially
@@ -321,9 +334,9 @@ class V8_EXPORT_PRIVATE WasmCode final {
 
   // Called by the WasmEngine when it shuts down for code it thinks is
   // probably dead (i.e. is in the "potentially_dead_code_" set). Wrapped
-  // in a method only because {ref_count_} is private.
+  // in a method only because {ref_count_bitfield_} is private.
   void DcheckRefCountIsOne() {
-    DCHECK_EQ(1, ref_count_.load(std::memory_order_acquire));
+    DCHECK_EQ(1, refcount(ref_count_bitfield_.load(std::memory_order_acquire)));
   }
 
   // Returns the last source position before {offset}.
@@ -340,7 +353,15 @@ class V8_EXPORT_PRIVATE WasmCode final {
     return ForDebuggingField::decode(flags_);
   }
 
-  bool is_dying() const { return dying_.load(std::memory_order_acquire); }
+  bool is_dying() const {
+    return is_dying(ref_count_bitfield_.load(std::memory_order_acquire));
+  }
+  static bool is_dying(uint32_t bit_field_value) {
+    return (bit_field_value & kIsDyingMask) != 0;
+  }
+  static uint32_t refcount(uint32_t bit_field_value) {
+    return bit_field_value & ~kIsDyingMask;
+  }
 
   // Returns {true} for Liftoff code that sets up a feedback vector slot in its
   // stack frame.
@@ -427,11 +448,6 @@ class V8_EXPORT_PRIVATE WasmCode final {
   // for consideration in the next Code GC cycle.
   V8_NOINLINE void DecRefOnPotentiallyDeadCode();
 
-  void mark_as_dying() { dying_.store(true, std::memory_order_release); }
-  // This is rarely necessary to mitigate a race condition. See the comment
-  // at its (only) call site.
-  void undo_mark_as_dying() { dying_.store(false, std::memory_order_release); }
-
   NativeModule* const native_module_ = nullptr;
   uint8_t* const instructions_;
   const uint64_t signature_hash_;
@@ -476,10 +492,6 @@ class V8_EXPORT_PRIVATE WasmCode final {
   using ForDebuggingField = ExecutionTierField::Next<ForDebugging, 2>;
   using FrameHasFeedbackSlotField = ForDebuggingField::Next<bool, 1>;
 
-  // Will be set to {true} the first time this code object is considered
-  // "potentially dead" (to be confirmed by the next Wasm Code GC cycle).
-  std::atomic<bool> dying_{false};
-
   // WasmCode is ref counted. Counters are held by:
   //   1) The jump table / code table.
   //   2) {WasmCodeRefScope}s.
@@ -490,7 +502,11 @@ class V8_EXPORT_PRIVATE WasmCode final {
   // it's being used. Once the ref count drops to zero (i.e. after being removed
   // from (3) and all (2)), the code object is deleted and the memory for the
   // machine code is freed.
-  std::atomic<int> ref_count_{1};
+  // The topmost bit is used to indicate that the code is in (3). It is stored
+  // in this same field to avoid race conditions between atomic updates to
+  // that state and the refcount.
+  static constexpr uint32_t kIsDyingMask = 0x8000'0000u;
+  std::atomic<uint32_t> ref_count_bitfield_{1};
 };
 
 WasmCode::Kind GetCodeKind(const WasmCompilationResult& result);
@@ -1237,6 +1253,10 @@ class V8_EXPORT_PRIVATE V8_NODISCARD WasmCodeRefScope {
   // Register a {WasmCode} reference in the current {WasmCodeRefScope}. Fails if
   // there is no current scope.
   static void AddRef(WasmCode*);
+  // Same, but conditional:
+  // - if the {code} is marked as dying, do nothing, return nullptr.
+  // - otherwise add a ref and return {code}.
+  static WasmCode* AddRefIfNotDying(WasmCode* code);
 
  private:
   WasmCodeRefScope* const previous_scope_;

deps/v8/src/wasm/wasm-engine.cc

@@ -1890,17 +1890,11 @@ void WasmEngine::FreeDeadCodeLocked(const DeadCodeMap& dead_code,
     const std::vector<WasmCode*>& code_vec = dead_code_entry.second;
     TRACE_CODE_GC("Freeing %zu code object%s of module %p.\n", code_vec.size(),
                   code_vec.size() == 1 ? "" : "s", native_module);
-#if DEBUG
-    for (WasmCode* code : code_vec) DCHECK(code->is_dying());
-#endif  // DEBUG
     native_module->FreeCode(base::VectorOf(code_vec));
   }
   if (dead_wrappers.size()) {
     TRACE_CODE_GC("Freeing %zu wrapper%s.\n", dead_wrappers.size(),
                   dead_wrappers.size() == 1 ? "" : "s");
-#if DEBUG
-    for (WasmCode* code : dead_wrappers) DCHECK(code->is_dying());
-#endif  // DEBUG
     GetWasmImportWrapperCache()->Free(dead_wrappers);
   }
 }

deps/v8/src/wasm/wasm-import-wrapper-cache.cc

@@ -146,9 +146,7 @@ WasmCode* WasmImportWrapperCache::FindWrapper(WasmCodePointer call_target) {
       GetProcessWideWasmCodePointerTable()->GetEntrypointWithoutSignatureCheck(
           call_target));
   if (iter == codes_.end()) return nullptr;
-  WasmCodeRefScope::AddRef(iter->second);
-  if (iter->second->is_dying()) return nullptr;
-  return iter->second;
+  return WasmCodeRefScope::AddRefIfNotDying(iter->second);
 }
 
 WasmCode* WasmImportWrapperCache::CompileWasmImportCallWrapper(
@@ -165,8 +163,8 @@ WasmCode* WasmImportWrapperCache::CompileWasmImportCallWrapper(
     // again whether another thread has just created the wrapper.
     wasm_code = cache_scope[key];
     if (wasm_code) {
-      WasmCodeRefScope::AddRef(wasm_code);
-      if (!wasm_code->is_dying()) return wasm_code;
+      wasm_code = WasmCodeRefScope::AddRefIfNotDying(wasm_code);
+      if (wasm_code) return wasm_code;
     }
 
     wasm_code = cache_scope.AddWrapper(key, std::move(result),
@@ -218,6 +216,9 @@ void WasmImportWrapperCache::Free(std::vector<WasmCode*>& wrappers) {
   }
   code_allocator_->FreeCode(base::VectorOf(wrappers));
   for (WasmCode* wrapper : wrappers) {
+    // TODO(407003348): Drop this check if it doesn't trigger in the wild.
+    CHECK_EQ(wrapper->ref_count_bitfield_.load(std::memory_order_acquire),
+             WasmCode::kIsDyingMask);
     delete wrapper;
   }
   // Make sure nobody tries to access stale pointers.
@@ -232,9 +233,7 @@ WasmCode* WasmImportWrapperCache::MaybeGet(ImportCallKind kind,
 
   auto it = entry_map_.find({kind, type_index, expected_arity, suspend});
   if (it == entry_map_.end()) return nullptr;
-  WasmCodeRefScope::AddRef(it->second);
-  if (it->second->is_dying()) return nullptr;
-  return it->second;
+  return WasmCodeRefScope::AddRefIfNotDying(it->second);
 }
 
 WasmCode* WasmImportWrapperCache::Lookup(Address pc) const {