[wasm] Keep NativeModule alive in BackgroundCompileScope

We need to ensure that the NativeModule stays alive while any
{BackgroundCompileScope} exists, because during that time we hold
shared ownership of the mutex in the {BackgroundCompileToken}. If the
{NativeModule} dies during that period, we would need to get exclusive
ownership of the mutex and deadlock.

This change requires holding a {std::weak_ptr<NativeModule>} in the
BackgroundCompileToken instead of a raw pointer, hence it can only be
initialized after the NativeModule was created. This is done via a
separate {InitCompilationState} method.

R=ahaas@chromium.org

Bug: v8:8979
Change-Id: Ia14bd272ea0bc47aec547024da6020608418c9d2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1518178
Auto-Submit: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60203}

src/wasm/compilation-environment.h

@@ -126,8 +126,11 @@ class CompilationState {
   friend class WasmCompilationUnit;
   CompilationState() = delete;
 
-  static std::unique_ptr<CompilationState> New(NativeModule*,
-                                               std::shared_ptr<Counters>);
+  // The CompilationState keeps a {std::weak_ptr} back to the {NativeModule}
+  // such that it can keep it alive (by regaining a {std::shared_ptr}) in
+  // certain scopes.
+  static std::unique_ptr<CompilationState> New(
+      const std::shared_ptr<NativeModule>&, std::shared_ptr<Counters>);
 };
 
 }  // namespace wasm

src/wasm/module-compiler.cc

@@ -61,22 +61,23 @@ enum class CompileMode : uint8_t { kRegular, kTiering };
 // on background compile jobs.
 class BackgroundCompileToken {
  public:
-  explicit BackgroundCompileToken(NativeModule* native_module)
+  explicit BackgroundCompileToken(
+      const std::shared_ptr<NativeModule>& native_module)
       : native_module_(native_module) {}
 
   void Cancel() {
     base::SharedMutexGuard<base::kExclusive> mutex_guard(&mutex_);
-    native_module_ = nullptr;
+    native_module_.reset();
   }
 
  private:
   friend class BackgroundCompileScope;
   base::SharedMutex mutex_;
-  NativeModule* native_module_;
+  std::weak_ptr<NativeModule> native_module_;
 
-  NativeModule* StartScope() {
+  std::shared_ptr<NativeModule> StartScope() {
     mutex_.LockShared();
-    return native_module_;
+    return native_module_.lock();
   }
 
   void ExitScope() { mutex_.UnlockShared(); }
@@ -99,14 +100,15 @@ class BackgroundCompileScope {
 
   NativeModule* native_module() {
     DCHECK(!cancelled());
-    return native_module_;
+    return native_module_.get();
   }
 
   inline CompilationStateImpl* compilation_state();
 
  private:
   BackgroundCompileToken* const token_;
-  NativeModule* const native_module_;
+  // Keep the native module alive while in this scope.
+  std::shared_ptr<NativeModule> const native_module_;
 };
 
 // The {CompilationStateImpl} keeps track of the compilation state of the
@@ -116,7 +118,8 @@ class BackgroundCompileScope {
 // It's public interface {CompilationState} lives in compilation-environment.h.
 class CompilationStateImpl {
  public:
-  CompilationStateImpl(NativeModule*, std::shared_ptr<Counters> async_counters);
+  CompilationStateImpl(const std::shared_ptr<NativeModule>& native_module,
+                       std::shared_ptr<Counters> async_counters);
   ~CompilationStateImpl();
 
   // Cancel all background compilation and wait for all tasks to finish. Call
@@ -310,7 +313,8 @@ void CompilationState::OnFinishedUnit(ExecutionTier tier, WasmCode* code) {
 
 // static
 std::unique_ptr<CompilationState> CompilationState::New(
-    NativeModule* native_module, std::shared_ptr<Counters> async_counters) {
+    const std::shared_ptr<NativeModule>& native_module,
+    std::shared_ptr<Counters> async_counters) {
   return std::unique_ptr<CompilationState>(reinterpret_cast<CompilationState*>(
       new CompilationStateImpl(native_module, std::move(async_counters))));
 }
@@ -726,7 +730,7 @@ class BackgroundCompileTask : public CancelableTask {
 
 }  // namespace
 
-std::unique_ptr<NativeModule> CompileToNativeModule(
+std::shared_ptr<NativeModule> CompileToNativeModule(
     Isolate* isolate, const WasmFeatures& enabled, ErrorThrower* thrower,
     std::shared_ptr<const WasmModule> module, const ModuleWireBytes& wire_bytes,
     Handle<FixedArray>* export_wrappers_out) {
@@ -1485,8 +1489,9 @@ bool AsyncStreamingProcessor::Deserialize(Vector<const uint8_t> module_bytes,
 }
 
 CompilationStateImpl::CompilationStateImpl(
-    NativeModule* native_module, std::shared_ptr<Counters> async_counters)
-    : native_module_(native_module),
+    const std::shared_ptr<NativeModule>& native_module,
+    std::shared_ptr<Counters> async_counters)
+    : native_module_(native_module.get()),
       background_compile_token_(
           std::make_shared<BackgroundCompileToken>(native_module)),
       compile_mode_(FLAG_wasm_tier_up &&

src/wasm/module-compiler.h

@@ -37,7 +37,7 @@ class NativeModule;
 class WasmCode;
 struct WasmModule;
 
-std::unique_ptr<NativeModule> CompileToNativeModule(
+std::shared_ptr<NativeModule> CompileToNativeModule(
     Isolate* isolate, const WasmFeatures& enabled, ErrorThrower* thrower,
     std::shared_ptr<const WasmModule> module, const ModuleWireBytes& wire_bytes,
     Handle<FixedArray>* export_wrappers_out);

src/wasm/wasm-code-manager.cc

@@ -368,11 +368,10 @@ WasmCode::~WasmCode() {
 NativeModule::NativeModule(WasmEngine* engine, const WasmFeatures& enabled,
                            bool can_request_more, VirtualMemory code_space,
                            std::shared_ptr<const WasmModule> module,
-                           std::shared_ptr<Counters> async_counters)
+                           std::shared_ptr<Counters> async_counters,
+                           std::shared_ptr<NativeModule>* shared_this)
     : enabled_features_(enabled),
       module_(std::move(module)),
-      compilation_state_(
-          CompilationState::New(this, std::move(async_counters))),
       import_wrapper_cache_(std::unique_ptr<WasmImportWrapperCache>(
           new WasmImportWrapperCache(this))),
       free_code_space_(code_space.region()),
@@ -380,6 +379,13 @@ NativeModule::NativeModule(WasmEngine* engine, const WasmFeatures& enabled,
       can_request_more_memory_(can_request_more),
       use_trap_handler_(trap_handler::IsTrapHandlerEnabled() ? kUseTrapHandler
                                                              : kNoTrapHandler) {
+  // We receive a pointer to an empty {std::shared_ptr}, and install ourselve
+  // there.
+  DCHECK_NOT_NULL(shared_this);
+  DCHECK_NULL(*shared_this);
+  shared_this->reset(this);
+  compilation_state_ =
+      CompilationState::New(*shared_this, std::move(async_counters));
   DCHECK_NOT_NULL(module_);
   owned_code_space_.emplace_back(std::move(code_space));
   owned_code_.reserve(num_functions());
@@ -1075,7 +1081,7 @@ size_t WasmCodeManager::EstimateNativeModuleNonCodeSize(
   return wasm_module_estimate + native_module_estimate;
 }
 
-std::unique_ptr<NativeModule> WasmCodeManager::NewNativeModule(
+std::shared_ptr<NativeModule> WasmCodeManager::NewNativeModule(
     WasmEngine* engine, Isolate* isolate, const WasmFeatures& enabled,
     size_t code_size_estimate, bool can_request_more,
     std::shared_ptr<const WasmModule> module) {
@@ -1111,9 +1117,11 @@ std::unique_ptr<NativeModule> WasmCodeManager::NewNativeModule(
   Address start = code_space.address();
   size_t size = code_space.size();
   Address end = code_space.end();
-  std::unique_ptr<NativeModule> ret(
+  std::shared_ptr<NativeModule> ret;
   new NativeModule(engine, enabled, can_request_more, std::move(code_space),
-                       std::move(module), isolate->async_counters()));
+                   std::move(module), isolate->async_counters(), &ret);
+  // The constructor initialized the shared_ptr.
+  DCHECK_NOT_NULL(ret);
   TRACE_HEAP("New NativeModule %p: Mem: %" PRIuPTR ",+%zu\n", ret.get(), start,
              size);
   base::MutexGuard lock(&native_modules_mutex_);

src/wasm/wasm-code-manager.h

@@ -371,10 +371,12 @@ class V8_EXPORT_PRIVATE NativeModule final {
   friend class WasmCodeManager;
   friend class NativeModuleModificationScope;
 
+  // Private constructor, called via {WasmCodeManager::NewNativeModule()}.
   NativeModule(WasmEngine* engine, const WasmFeatures& enabled_features,
                bool can_request_more, VirtualMemory code_space,
                std::shared_ptr<const WasmModule> module,
-               std::shared_ptr<Counters> async_counters);
+               std::shared_ptr<Counters> async_counters,
+               std::shared_ptr<NativeModule>* shared_this);
 
   WasmCode* AddAnonymousCode(Handle<Code>, WasmCode::Kind kind,
                              const char* name = nullptr);
@@ -518,7 +520,7 @@ class V8_EXPORT_PRIVATE WasmCodeManager final {
   friend class NativeModule;
   friend class WasmEngine;
 
-  std::unique_ptr<NativeModule> NewNativeModule(
+  std::shared_ptr<NativeModule> NewNativeModule(
       WasmEngine* engine, Isolate* isolate,
       const WasmFeatures& enabled_features, size_t code_size_estimate,
       bool can_request_more, std::shared_ptr<const WasmModule> module);

src/wasm/wasm-engine.cc

@@ -140,7 +140,7 @@ MaybeHandle<AsmWasmData> WasmEngine::SyncCompileTranslatedAsmJs(
   // Transfer ownership of the WasmModule to the {Managed<WasmModule>} generated
   // in {CompileToNativeModule}.
   Handle<FixedArray> export_wrappers;
-  std::unique_ptr<NativeModule> native_module =
+  std::shared_ptr<NativeModule> native_module =
       CompileToNativeModule(isolate, kAsmjsWasmFeatures, thrower,
                             std::move(result).value(), bytes, &export_wrappers);
   if (!native_module) return {};
@@ -188,7 +188,7 @@ MaybeHandle<WasmModuleObject> WasmEngine::SyncCompile(
   // Transfer ownership of the WasmModule to the {Managed<WasmModule>} generated
   // in {CompileToModuleObject}.
   Handle<FixedArray> export_wrappers;
-  std::unique_ptr<NativeModule> native_module =
+  std::shared_ptr<NativeModule> native_module =
       CompileToNativeModule(isolate, enabled, thrower,
                             std::move(result).value(), bytes, &export_wrappers);
   if (!native_module) return {};
@@ -487,10 +487,10 @@ void WasmEngine::EnableCodeLogging(Isolate* isolate) {
   it->second->log_codes = true;
 }
 
-std::unique_ptr<NativeModule> WasmEngine::NewNativeModule(
+std::shared_ptr<NativeModule> WasmEngine::NewNativeModule(
     Isolate* isolate, const WasmFeatures& enabled, size_t code_size_estimate,
     bool can_request_more, std::shared_ptr<const WasmModule> module) {
-  std::unique_ptr<NativeModule> native_module =
+  std::shared_ptr<NativeModule> native_module =
       code_manager_.NewNativeModule(this, isolate, enabled, code_size_estimate,
                                     can_request_more, std::move(module));
   base::MutexGuard lock(&mutex_);

src/wasm/wasm-engine.h

@@ -169,7 +169,7 @@ class V8_EXPORT_PRIVATE WasmEngine {
   // is determined with a heuristic based on the total size of wasm
   // code. The native module may later request more memory.
   // TODO(titzer): isolate is only required here for CompilationState.
-  std::unique_ptr<NativeModule> NewNativeModule(
+  std::shared_ptr<NativeModule> NewNativeModule(
       Isolate* isolate, const WasmFeatures& enabled_features,
       size_t code_size_estimate, bool can_request_more,
       std::shared_ptr<const WasmModule> module);

test/cctest/compiler/test-multiple-return.cc

@@ -118,7 +118,7 @@ Node* ToInt32(RawMachineAssembler& m, MachineType type, Node* a) {
   }
 }
 
-std::unique_ptr<wasm::NativeModule> AllocateNativeModule(Isolate* isolate,
+std::shared_ptr<wasm::NativeModule> AllocateNativeModule(Isolate* isolate,
                                                          size_t code_size) {
   std::shared_ptr<wasm::WasmModule> module(new wasm::WasmModule());
   module->num_declared_functions = 1;
@@ -183,7 +183,7 @@ void TestReturnMultipleValues(MachineType type) {
         if (i % 4 == 0) sign = -sign;
       }
 
-      std::unique_ptr<wasm::NativeModule> module = AllocateNativeModule(
+      std::shared_ptr<wasm::NativeModule> module = AllocateNativeModule(
           handles.main_isolate(), code->raw_instruction_size());
       byte* code_start =
           module->AddCodeForTesting(code)->instructions().start();
@@ -272,7 +272,7 @@ void ReturnLastValue(MachineType type) {
             AssemblerOptions::Default(handles.main_isolate()), m.Export())
             .ToHandleChecked();
 
-    std::unique_ptr<wasm::NativeModule> module = AllocateNativeModule(
+    std::shared_ptr<wasm::NativeModule> module = AllocateNativeModule(
         handles.main_isolate(), code->raw_instruction_size());
     byte* code_start = module->AddCodeForTesting(code)->instructions().start();
 
@@ -333,7 +333,7 @@ void ReturnSumOfReturns(MachineType type) {
             AssemblerOptions::Default(handles.main_isolate()), m.Export())
             .ToHandleChecked();
 
-    std::unique_ptr<wasm::NativeModule> module = AllocateNativeModule(
+    std::shared_ptr<wasm::NativeModule> module = AllocateNativeModule(
         handles.main_isolate(), code->raw_instruction_size());
     byte* code_start = module->AddCodeForTesting(code)->instructions().start();
 

test/cctest/wasm/test-wasm-import-wrapper-cache.cc

@@ -17,7 +17,7 @@ namespace internal {
 namespace wasm {
 namespace test_wasm_import_wrapper_cache {
 
-std::unique_ptr<NativeModule> NewModule(Isolate* isolate) {
+std::shared_ptr<NativeModule> NewModule(Isolate* isolate) {
   std::shared_ptr<WasmModule> module(new WasmModule);
   bool can_request_more = false;
   size_t size = 16384;

test/fuzzer/multi-return.cc

@@ -134,7 +134,7 @@ CallDescriptor* CreateRandomCallDescriptor(Zone* zone, size_t return_count,
   return compiler::GetWasmCallDescriptor(zone, builder.Build());
 }
 
-std::unique_ptr<wasm::NativeModule> AllocateNativeModule(i::Isolate* isolate,
+std::shared_ptr<wasm::NativeModule> AllocateNativeModule(i::Isolate* isolate,
                                                          size_t code_size) {
   std::shared_ptr<wasm::WasmModule> module(new wasm::WasmModule);
   module->num_declared_functions = 1;
@@ -243,7 +243,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
                           AssemblerOptions::Default(i_isolate), callee.Export())
                           .ToHandleChecked();
 
-  std::unique_ptr<wasm::NativeModule> module =
+  std::shared_ptr<wasm::NativeModule> module =
       AllocateNativeModule(i_isolate, code->raw_instruction_size());
   byte* code_start = module->AddCodeForTesting(code)->instructions().start();
   // Generate wrapper.

test/unittests/wasm/wasm-code-manager-unittest.cc

@@ -159,7 +159,7 @@ class WasmCodeManagerTest : public TestWithContext,
   static constexpr uint32_t kJumpTableSize = RoundUp<kCodeAlignment>(
       JumpTableAssembler::SizeForNumberOfSlots(kNumFunctions));
 
-  using NativeModulePtr = std::unique_ptr<NativeModule>;
+  using NativeModulePtr = std::shared_ptr<NativeModule>;
 
   NativeModulePtr AllocModule(size_t size, ModuleStyle style) {
     std::shared_ptr<WasmModule> module(new WasmModule);