[wasm] Fix wasm instantiation flakes

src/wasm/wasm-module.cc

Index: src/wasm/wasm-module.cc
diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc
index f4cf505f5a910761dda7ba846dcc2f4ee630be89..dd3768be2d03f5df2a7edd73bd5e05829b4aa81a 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -1211,16 +1211,15 @@ MaybeHandle<JSObject> WasmModule::Instantiate(Isolate* isolate,
   {
     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);
@@ -1254,6 +1253,11 @@ MaybeHandle<JSObject> WasmModule::Instantiate(Isolate* isolate,
       }
       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());