[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 1193 matching lines @@
   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;
 
   TRACE("Starting new module instantiation\n");
   {
     Handle<WasmCompiledModule> original(
         WasmCompiledModule::cast(module_object->GetInternalField(0)), isolate);
-    // 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();

[titzer, 2016/10/07 12:40:32]

Why do we have a raw pointer here?

[Mircea Trofin, 2016/10/07 14:44:23]

Not sure, I think this is from 0xC; it's immaterial, though, it seems
to be used for a check only and then we fetch the owner, neither are
GC-ing.

       DCHECK(!tmp->cleared());
       // There is already an owner, clone everything.
       owner = Handle<JSObject>(JSObject::cast(tmp->value()), isolate);
+
+      old_code_table = original->code_table();
+      code_table = factory->CopyFixedArray(old_code_table);

[titzer, 2016/10/07 12:40:32]

I don't understand how this fixes anything. As far as I can tell, it just moves
an allocation until after the check. 

[Mircea Trofin, 2016/10/07 14:44:23]

Consider the old code. Suppose the allocation on line 1217 triggers GC for
{original}'s owner. As a result, the weak link to the owner is reset by the
InstanceFinalizer. We want to do that because that is how we then determine if a
compiled module is owned or not. 

{owner} will be uninitialized, because we're not cloning.

Now, when looking to relocate globals, we will believe the old globals address
is nullptr (incorrect), and fail to patch the code.

The change ensures we catch the owner before any of this happens.

As I mentioned, I'm working on a more involved change, to avoid such
inconsistencies "once and for all".

[titzer, 2016/10/07 15:37:42]

Ok, until then can we have a switch to turn off this optimization until it's
fully stabilized? It's caused lots of gc-stress failures and resisted many
attempts to fix it so far.

+
       // Insert the latest clone in front.
       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
@@ skipping 13 matching lines @@
             Handle<Code> code = factory->CopyCode(orig_code);
             code_table->set(i, *code);
             break;
           }
           default:
             UNREACHABLE();
         }
       }
       RecordStats(isolate, code_table);
     } else {
+      // 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);
+
       // There was no owner, so we can reuse the original.
       compiled_module = original;
       TRACE("Reusing existing instance %d\n", compiled_module->instance_id());
     }
     compiled_module->set_code_table(code_table);
   }
 
   //--------------------------------------------------------------------------
   // Allocate the instance object.
   //--------------------------------------------------------------------------
@@ skipping 646 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