[wasm] harden inter-WasmCompiledModule links

This both simplifies the code, and brings it in-line with
what we need for wasm on the native heap.

Bug: v8:6876
Change-Id: Ia48aed86ddcd94e85c0aecc053c327dfacb795d6
Reviewed-on: https://chromium-review.googlesource.com/744593
Commit-Queue: Mircea Trofin <mtrofin@chromium.org>
Commit-Queue: Brad Nelson <bradnelson@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49048}

src/wasm/module-compiler.cc

@@ -426,56 +426,32 @@ static void InstanceFinalizer(const v8::WeakCallbackInfo<void>& data) {
   }
 
   // weak_wasm_module may have been cleared, meaning the module object
-  // was GC-ed. In that case, there won't be any new instances created,
-  // and we don't need to maintain the links between instances.
+  // was GC-ed. We still want to maintain the links between instances, to
+  // release the WasmCompiledModule corresponding to the WasmModuleInstance
+  // being finalized here.
+  WasmModuleObject* wasm_module = nullptr;
   if (!weak_wasm_module->cleared()) {
-    WasmModuleObject* wasm_module =
-        WasmModuleObject::cast(weak_wasm_module->value());
+    wasm_module = WasmModuleObject::cast(weak_wasm_module->value());
     WasmCompiledModule* current_template = wasm_module->compiled_module();
 
     TRACE("chain before {\n");
     TRACE_CHAIN(current_template);
     TRACE("}\n");
 
-    DCHECK(!current_template->has_weak_prev_instance());
-    WeakCell* next = compiled_module->maybe_ptr_to_weak_next_instance();
-    WeakCell* prev = compiled_module->maybe_ptr_to_weak_prev_instance();
-
+    DCHECK(!current_template->has_prev_instance());
     if (current_template == compiled_module) {
-      if (next == nullptr) {
+      if (!compiled_module->has_next_instance()) {
         WasmCompiledModule::Reset(isolate, compiled_module);
       } else {
-        WasmCompiledModule* next_compiled_module =
-            WasmCompiledModule::cast(next->value());
         WasmModuleObject::cast(wasm_module)
-            ->set_compiled_module(next_compiled_module);
-        DCHECK_NULL(prev);
-        next_compiled_module->reset_weak_prev_instance();
-      }
-    } else {
-      DCHECK(!(prev == nullptr && next == nullptr));
-      // the only reason prev or next would be cleared is if the
-      // respective objects got collected, but if that happened,
-      // we would have relinked the list.
-      if (prev != nullptr) {
-        DCHECK(!prev->cleared());
-        if (next == nullptr) {
-          WasmCompiledModule::cast(prev->value())->reset_weak_next_instance();
-        } else {
-          WasmCompiledModule::cast(prev->value())
-              ->set_ptr_to_weak_next_instance(next);
-        }
-      }
-      if (next != nullptr) {
-        DCHECK(!next->cleared());
-        if (prev == nullptr) {
-          WasmCompiledModule::cast(next->value())->reset_weak_prev_instance();
-        } else {
-          WasmCompiledModule::cast(next->value())
-              ->set_ptr_to_weak_prev_instance(prev);
-        }
+            ->set_compiled_module(compiled_module->ptr_to_next_instance());
       }
     }
+  }
+
+  compiled_module->RemoveFromChain();
+
+  if (wasm_module != nullptr) {
     TRACE("chain after {\n");
     TRACE_CHAIN(wasm_module->compiled_module());
     TRACE("}\n");
@@ -1930,35 +1906,17 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
   {
     Handle<Object> global_handle =
         isolate_->global_handles()->Create(*instance);
-    Handle<WeakCell> link_to_clone = factory->NewWeakCell(compiled_module_);
     Handle<WeakCell> link_to_owning_instance = factory->NewWeakCell(instance);
-    MaybeHandle<WeakCell> link_to_original;
-    MaybeHandle<WasmCompiledModule> original;
     if (!owner.is_null()) {
-      // prepare the data needed for publishing in a chain, but don't link
-      // just yet, because
-      // we want all the publishing to happen free from GC interruptions, and
-      // so we do it in
-      // one GC-free scope afterwards.
-      original = handle(owner.ToHandleChecked()->compiled_module());
-      link_to_original = factory->NewWeakCell(original.ToHandleChecked());
-    }
-    // Publish the new instance to the instances chain.
-    {
+      // Publish the new instance to the instances chain.
       DisallowHeapAllocation no_gc;
-      if (!link_to_original.is_null()) {
-        compiled_module_->set_weak_next_instance(
-            link_to_original.ToHandleChecked());
-        original.ToHandleChecked()->set_weak_prev_instance(link_to_clone);
-        compiled_module_->set_weak_wasm_module(
-            original.ToHandleChecked()->weak_wasm_module());
-      }
-      module_object_->set_compiled_module(*compiled_module_);
-      compiled_module_->set_weak_owning_instance(link_to_owning_instance);
-      GlobalHandles::MakeWeak(
-          global_handle.location(), global_handle.location(),
-          instance_finalizer_callback_, v8::WeakCallbackType::kFinalizer);
+      compiled_module_->InsertInChain(*module_object_);
     }
+    module_object_->set_compiled_module(*compiled_module_);
+    compiled_module_->set_weak_owning_instance(link_to_owning_instance);
+    GlobalHandles::MakeWeak(global_handle.location(), global_handle.location(),
+                            instance_finalizer_callback_,
+                            v8::WeakCallbackType::kFinalizer);
   }
 
   //--------------------------------------------------------------------------

src/wasm/wasm-objects.cc

@@ -62,25 +62,17 @@ class CompiledModulesIterator
   void Advance() {
     DCHECK(!current_.is_null());
     if (!is_backwards_) {
-      if (current_->has_weak_next_instance()) {
-        WeakCell* weak_next = current_->ptr_to_weak_next_instance();
-        if (!weak_next->cleared()) {
-          current_ =
-              handle(WasmCompiledModule::cast(weak_next->value()), isolate_);
-          return;
-        }
+      if (current_->has_next_instance()) {
+        current_ = current_->next_instance();
+        return;
       }
       // No more modules in next-links, now try the previous-links.
       is_backwards_ = true;
       current_ = start_module_;
     }
-    if (current_->has_weak_prev_instance()) {
-      WeakCell* weak_prev = current_->ptr_to_weak_prev_instance();
-      if (!weak_prev->cleared()) {
-        current_ =
-            handle(WasmCompiledModule::cast(weak_prev->value()), isolate_);
-        return;
-      }
+    if (current_->has_prev_instance()) {
+      current_ = current_->prev_instance();
+      return;
     }
     current_ = Handle<WasmCompiledModule>::null();
   }
@@ -179,8 +171,8 @@ void WasmModuleObject::ValidateStateForTesting(
   WasmCompiledModule* compiled_module = module_obj->compiled_module();
   CHECK(compiled_module->has_weak_wasm_module());
   CHECK_EQ(compiled_module->ptr_to_weak_wasm_module()->value(), *module_obj);
-  CHECK(!compiled_module->has_weak_prev_instance());
-  CHECK(!compiled_module->has_weak_next_instance());
+  CHECK(!compiled_module->has_prev_instance());
+  CHECK(!compiled_module->has_next_instance());
   CHECK(!compiled_module->has_weak_owning_instance());
 }
 
@@ -589,16 +581,16 @@ void WasmInstanceObject::ValidateInstancesChainForTesting(
   Object* prev = nullptr;
   int found_instances = compiled_module->has_weak_owning_instance() ? 1 : 0;
   WasmCompiledModule* current_instance = compiled_module;
-  while (current_instance->has_weak_next_instance()) {
-    CHECK((prev == nullptr && !current_instance->has_weak_prev_instance()) ||
-          current_instance->ptr_to_weak_prev_instance()->value() == prev);
+  while (current_instance->has_next_instance()) {
+    CHECK((prev == nullptr && !current_instance->has_prev_instance()) ||
+          current_instance->ptr_to_prev_instance() == prev);
     CHECK_EQ(current_instance->ptr_to_weak_wasm_module()->value(), *module_obj);
     CHECK(current_instance->ptr_to_weak_owning_instance()
               ->value()
               ->IsWasmInstanceObject());
     prev = current_instance;
-    current_instance = WasmCompiledModule::cast(
-        current_instance->ptr_to_weak_next_instance()->value());
+    current_instance =
+        WasmCompiledModule::cast(current_instance->ptr_to_next_instance());
     ++found_instances;
     CHECK_LE(found_instances, instance_count);
   }
@@ -998,8 +990,8 @@ Handle<WasmCompiledModule> WasmCompiledModule::Clone(
   ret->InitId();
   ret->set_code_table(code_copy);
   ret->reset_weak_owning_instance();
-  ret->reset_weak_next_instance();
-  ret->reset_weak_prev_instance();
+  ret->reset_next_instance();
+  ret->reset_prev_instance();
   ret->reset_weak_exported_functions();
   return ret;
 }
@@ -1169,15 +1161,36 @@ void WasmCompiledModule::PrintInstancesChain() {
   if (!FLAG_trace_wasm_instances) return;
   for (WasmCompiledModule* current = this; current != nullptr;) {
     PrintF("->%d", current->instance_id());
-    if (!current->has_weak_next_instance()) break;
-    DCHECK(!current->ptr_to_weak_next_instance()->cleared());
-    current =
-        WasmCompiledModule::cast(current->ptr_to_weak_next_instance()->value());
+    if (!current->has_next_instance()) break;
+    current = current->ptr_to_next_instance();
   }
   PrintF("\n");
 #endif
 }
 
+void WasmCompiledModule::InsertInChain(WasmModuleObject* module) {
+  DisallowHeapAllocation no_gc;
+  WasmCompiledModule* original = module->compiled_module();
+  set_ptr_to_next_instance(original);
+  original->set_ptr_to_prev_instance(this);
+  set_weak_wasm_module(original->weak_wasm_module());
+}
+
+void WasmCompiledModule::RemoveFromChain() {
+  DisallowHeapAllocation no_gc;
+  Isolate* isolate = GetIsolate();
+
+  Object* next = get(kID_next_instance);
+  Object* prev = get(kID_prev_instance);
+
+  if (!prev->IsUndefined(isolate)) {
+    WasmCompiledModule::cast(prev)->set(kID_next_instance, next);
+  }
+  if (!next->IsUndefined(isolate)) {
+    WasmCompiledModule::cast(next)->set(kID_prev_instance, prev);
+  }
+}
+
 void WasmCompiledModule::ReinitializeAfterDeserialization(
     Isolate* isolate, Handle<WasmCompiledModule> compiled_module) {
   // This method must only be called immediately after deserialization.

src/wasm/wasm-objects.h

@@ -393,8 +393,8 @@ class WasmCompiledModule : public FixedArray {
   MACRO(CONST_OBJECT, FixedArray, empty_function_tables)      \
   MACRO(CONST_OBJECT, FixedArray, empty_signature_tables)     \
   MACRO(SMALL_CONST_NUMBER, uint32_t, initial_pages)          \
-  MACRO(WEAK_LINK, WasmCompiledModule, next_instance)         \
-  MACRO(WEAK_LINK, WasmCompiledModule, prev_instance)         \
+  MACRO(WASM_OBJECT, WasmCompiledModule, next_instance)       \
+  MACRO(WASM_OBJECT, WasmCompiledModule, prev_instance)       \
   MACRO(WEAK_LINK, JSObject, owning_instance)                 \
   MACRO(WEAK_LINK, WasmModuleObject, wasm_module)
 
@@ -430,6 +430,8 @@ class WasmCompiledModule : public FixedArray {
   static void Reset(Isolate* isolate, WasmCompiledModule* module);
 
   uint32_t default_mem_size() const;
+  void InsertInChain(WasmModuleObject*);
+  void RemoveFromChain();
 
 #define DECLARATION(KIND, TYPE, NAME) WCM_##KIND(TYPE, NAME)
   WCM_PROPERTY_TABLE(DECLARATION)