[wasm] Fix wasm instantiation flakes

src/wasm/wasm-module.cc

 // Copyright 2015 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.
 
 #include <memory>
 
 #include "src/base/atomic-utils.h"
 #include "src/code-stubs.h"
 
 #include "src/macro-assembler.h"
@@ skipping 1154 matching lines @@
     MaybeHandle<JSObject> nothing;
     HistogramTimerScope wasm_instantiate_module_time_scope(
         isolate_->counters()->wasm_instantiate_module_time());
     Factory* factory = isolate_->factory();
 
     //--------------------------------------------------------------------------
     // Reuse the compiled module (if no owner), otherwise clone.
     //--------------------------------------------------------------------------
     Handle<FixedArray> code_table;
     Handle<FixedArray> old_code_table;
-    Handle<JSObject> owner;
-    // If we don't clone, this will be null(). Otherwise, this will
-    // be a weak link to the original. If we lose the original to GC,
-    // this will be a cleared. We'll link the instances chain last.
-    MaybeHandle<WeakCell> link_to_original;
+    MaybeHandle<JSObject> owner;
 
     TRACE("Starting new module instantiation\n");
     {
-      Handle<WasmCompiledModule> original(
-          WasmCompiledModule::cast(module_object_->GetInternalField(0)),
-          isolate_);
+      // Root the owner, if any, before doing any allocations, which
+      // may trigger GC.
+      // Both owner and original template need to be in sync. Even
+      // after we lose the original template handle, the code
+      // objects we copied from it have data relative to the
+      // instance - such as globals addresses.
+      Handle<WasmCompiledModule> original;
+      {
+        DisallowHeapAllocation no_gc;
+        original = handle(
+            WasmCompiledModule::cast(module_object_->GetInternalField(0)));
+        if (original->has_weak_owning_instance()) {
+          owner =
+              handle(JSObject::cast(original->weak_owning_instance()->value()));
+        }
+      }
+      DCHECK(!original.is_null());
       // Always make a new copy of the code_table, since the old_code_table
       // may still have placeholders for imports.
       old_code_table = original->code_table();
       code_table = factory->CopyFixedArray(old_code_table);
 
       if (original->has_weak_owning_instance()) {
-        WeakCell* tmp = original->ptr_to_weak_owning_instance();
-        DCHECK(!tmp->cleared());
-        // There is already an owner, clone everything.
-        owner = Handle<JSObject>(JSObject::cast(tmp->value()), isolate_);
-        // Insert the latest clone in front.
+        // Clone, but don't insert yet the clone in the instances chain.
+        // We do that last. Since we are holding on to the owner instance,
+        // the owner + original state used for cloning and patching
+        // won't be mutated by possible finalizer runs.
+        DCHECK(!owner.is_null());
         TRACE("Cloning from %d\n", original->instance_id());
         compiled_module_ = WasmCompiledModule::Clone(isolate_, original);
-        // Replace the strong reference to point to the new instance here.
-        // This allows any of the other instances, including the original,
-        // to be collected.
-        module_object_->SetInternalField(0, *compiled_module_);
-        compiled_module_->set_weak_module_object(
-            original->weak_module_object());
-        link_to_original = factory->NewWeakCell(original);
-        // Don't link to original here. We remember the original
-        // as a weak link. If that link isn't clear by the time we finish
-        // instantiating this instance, then we link it at that time.
-        compiled_module_->reset_weak_next_instance();
 
         // Clone the code for WASM functions and exports.
         for (int i = 0; i < code_table->length(); ++i) {
           Handle<Code> orig_code =
               code_table->GetValueChecked<Code>(isolate_, i);
           switch (orig_code->kind()) {
             case Code::WASM_TO_JS_FUNCTION:
               // Imports will be overwritten with newly compiled wrappers.
               break;
             case Code::JS_TO_WASM_FUNCTION:
@@ skipping 33 matching lines @@
     MaybeHandle<JSArrayBuffer> globals;
     uint32_t globals_size = compiled_module_->globals_size();
     if (globals_size > 0) {
       Handle<JSArrayBuffer> global_buffer =
           NewArrayBuffer(isolate_, globals_size);
       globals = global_buffer;
       if (globals.is_null()) {
         thrower_->Error("Out of memory: wasm globals");
         return nothing;
       }
-      Address old_address =
-          owner.is_null() ? nullptr : GetGlobalStartAddressFromCodeTemplate(
-                                          *factory->undefined_value(),
-                                          JSObject::cast(*owner));
+      Address old_address = owner.is_null()
+                                ? nullptr
+                                : GetGlobalStartAddressFromCodeTemplate(
+                                      *factory->undefined_value(),
+                                      JSObject::cast(*owner.ToHandleChecked()));
       RelocateGlobals(instance, old_address,
                       static_cast<Address>(global_buffer->backing_store()));
       instance->SetInternalField(kWasmGlobalsArrayBuffer, *global_buffer);
     }
 
     //--------------------------------------------------------------------------
     // Process the imports for the module.
     //--------------------------------------------------------------------------
     int num_imported_functions = ProcessImports(globals, code_table, instance);
     if (num_imported_functions < 0) return nothing;
@@ skipping 62 matching lines @@
           static_cast<size_t>(code_table->length()));
       for (int i = 0; i < code_table->length(); ++i) {
         functions[i] = code_table->GetValueChecked<Code>(isolate_, i);
       }
 
       if (compiled_module_->has_indirect_function_tables()) {
         Handle<FixedArray> indirect_tables_template =
             compiled_module_->indirect_function_tables();
         Handle<FixedArray> to_replace =
             owner.is_null() ? indirect_tables_template
-                            : handle(FixedArray::cast(owner->GetInternalField(
-                                  kWasmModuleFunctionTable)));
+                            : handle(FixedArray::cast(
+                                  owner.ToHandleChecked()->GetInternalField(
+                                      kWasmModuleFunctionTable)));
         Handle<FixedArray> indirect_tables = SetupIndirectFunctionTable(
             isolate_, code_table, indirect_tables_template, to_replace);
         for (int i = 0; i < indirect_tables->length(); ++i) {
           Handle<FixedArray> metadata =
               indirect_tables->GetValueChecked<FixedArray>(isolate_, i);
           uint32_t size = Smi::cast(metadata->get(kSize))->value();
           Handle<FixedArray> table =
               metadata->GetValueChecked<FixedArray>(isolate_, kTable);
           PopulateFunctionTable(table, size, &functions);
         }
@@ skipping 35 matching lines @@
       MaybeHandle<Object> retval =
           Execution::Call(isolate_, startup_fct, undefined, 0, nullptr);
 
       if (retval.is_null()) {
         thrower_->Error("WASM.instantiateModule(): start function failed");
         return nothing;
       }
     }
 
     {
-      Handle<WeakCell> link_to_owner = factory->NewWeakCell(instance);
-
       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(WasmCompiledModule::cast(
+            owner.ToHandleChecked()->GetInternalField(kWasmCompiledModule)));
+        link_to_original = factory->NewWeakCell(original.ToHandleChecked());
+      }
+      // Publish the new instance to the instances chain.
       {
         DisallowHeapAllocation no_gc;
-        compiled_module_->set_weak_owning_instance(link_to_owner);
-        Handle<WeakCell> next;
-        if (link_to_original.ToHandle(&next) && !next->cleared()) {
-          WasmCompiledModule* original =
-              WasmCompiledModule::cast(next->value());
-          DCHECK(original->has_weak_owning_instance());
-          DCHECK(!original->weak_owning_instance()->cleared());
-          compiled_module_->set_weak_next_instance(next);
-          original->set_weak_prev_instance(link_to_clone);
+        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_module_object(
+              original.ToHandleChecked()->weak_module_object());
         }
-
-        compiled_module_->set_weak_owning_instance(link_to_owner);
+        module_object_->SetInternalField(0, *compiled_module_);
         instance->SetInternalField(kWasmCompiledModule, *compiled_module_);
+        compiled_module_->set_weak_owning_instance(link_to_owning_instance);
         GlobalHandles::MakeWeak(global_handle.location(),
                                 global_handle.location(), &InstanceFinalizer,
                                 v8::WeakCallbackType::kFinalizer);
       }
     }
 
     DCHECK(wasm::IsWasmObject(*instance));
 
     TRACE("Finishing instance %d\n", compiled_module_->instance_id());
     TRACE_CHAIN(WasmCompiledModule::cast(module_object_->GetInternalField(0)));
@@ skipping 815 matching lines @@
   WasmCompiledModule* compiled_module =
       WasmCompiledModule::cast(instance->GetInternalField(kWasmCompiledModule));
   CHECK(compiled_module->has_weak_module_object());
   CHECK(compiled_module->ptr_to_weak_module_object()->cleared());
 }
 
 }  // namespace testing
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8