[wasm] WebAssembly.Memory object can be referenced by multiple Instance objects.

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 63 matching lines @@
   kInternalFunctionIndex
 };
 
 // Internal constants for the layout of the module object.
 enum WasmInstanceObjectFields {
   kWasmCompiledModule = 0,
   kWasmMemObject,
   kWasmMemArrayBuffer,
   kWasmGlobalsArrayBuffer,
   kWasmDebugInfo,
+  kWasmMemInstanceWrapper,
   kWasmInstanceInternalFieldCount
 };
 
 byte* raw_buffer_ptr(MaybeHandle<JSArrayBuffer> buffer, int offset) {
   return static_cast<byte*>(buffer.ToHandleChecked()->backing_store()) + offset;
 }
 
 uint32_t GetMinModuleMemSize(const WasmModule* module) {
   return WasmModule::kPageSize * module->min_mem_pages;
 }
@@ skipping 1007 matching lines @@
     table_instances_.reserve(module_->function_tables.size());
     for (int index = 0; index < function_table_count; ++index) {
       table_instances_.push_back({Handle<JSObject>::null(),
                                   Handle<FixedArray>::null(),
                                   Handle<FixedArray>::null()});
     }
 
     //--------------------------------------------------------------------------
     // Process the imports for the module.
     //--------------------------------------------------------------------------
     int num_imported_functions = ProcessImports(code_table, instance);

[Mircea Trofin, 2016/11/09 18:57:09]

the call to ProcessImports may setup a memory instances chain. Between here and
line 1150, we might get GC (if not today, then at some other point in the
future). As a result, the relocation may happen more than once.

In addition, for maintainability, it may be best we deferred linking the memory
in the chain and the relocation for this instance in one place in code. So maybe
ProcessImports just sets memory_, but then you do the SetWasmMemory around line
1150.

[gdeepti, 2016/11/16 05:34:10]

Maybe I'm misunderstanding your point here, but setting up/updating the
instances chain for the memory object does not cause relocations. My rationale
here is that between here and line 1150 below, if we get a GC, depending on how
the wrapper is set up the instances chain will be updated but I do not think
this will trigger multiple relocations. 

Agree with this from a maintainability standpoint, moved to where memory is
updated. 

     if (num_imported_functions < 0) return nothing;
 
     //--------------------------------------------------------------------------
     // Process the initialization for the module's globals.
     //--------------------------------------------------------------------------
     InitGlobals();
 
     //--------------------------------------------------------------------------
     // Set up the memory for the new instance.
     //--------------------------------------------------------------------------
@@ skipping 94 matching lines @@
         }
         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::IsWasmInstance(*instance));
-    Handle<Object> memory_object(instance->GetInternalField(kWasmMemObject),
-                                 isolate_);
-    WasmJs::SetWasmMemoryInstance(isolate_, memory_object, instance);
-
     //--------------------------------------------------------------------------
     // Run the start function if one was specified.
     //--------------------------------------------------------------------------
     if (module_->start_function_index >= 0) {
       HandleScope scope(isolate_);
       int start_index = module_->start_function_index;
       Handle<Code> startup_code =
           code_table->GetValueChecked<Code>(isolate_, start_index);
       FunctionSig* sig = module_->functions[start_index].sig;
       Handle<JSFunction> startup_fct = WrapExportCodeAsJSFunction(
@@ skipping 256 matching lines @@
         }
         case kExternalMemory: {
           Handle<Object> object = result.ToHandleChecked();
           if (!WasmJs::IsWasmMemoryObject(isolate_, object)) {
             ReportFFIError("memory import must be a WebAssembly.Memory object",
                            index, module_name, function_name);
             return -1;
           }
           instance->SetInternalField(kWasmMemObject, *object);
           memory_ = WasmJs::GetWasmMemoryArrayBuffer(isolate_, object);
+          WasmJs::SetWasmMemoryInstance(isolate_, object, instance);
           break;
         }
         case kExternalGlobal: {
           // Global imports are converted to numbers and written into the
           // {globals_} array buffer.
           Handle<Object> object = result.ToHandleChecked();
           MaybeHandle<Object> number = Object::ToNumber(object);
           if (number.is_null()) {
             ReportFFIError("global import could not be converted to number",
                            index, module_name, function_name);
@@ skipping 155 matching lines @@
               instance->GetInternalField(kWasmMemObject), isolate_);
           if (memory_object->IsUndefined(isolate_)) {
             // If there was no imported WebAssembly.Memory object, create one.
             Handle<JSArrayBuffer> buffer(
                 JSArrayBuffer::cast(
                     instance->GetInternalField(kWasmMemArrayBuffer)),
                 isolate_);
             memory_object =
                 WasmJs::CreateWasmMemoryObject(isolate_, buffer, false, 0);
             instance->SetInternalField(kWasmMemObject, *memory_object);
+          } else {
+            DCHECK(WasmJs::IsWasmMemoryObject(isolate_, memory_object));
+            WasmJs::ResetWasmMemoryInstance(isolate_, memory_object);
           }
 
           desc.set_value(memory_object);
           break;
         }
         case kExternalGlobal: {
           // Export the value of the global variable as a number.
           WasmGlobal& global = module_->globals[exp.index];
           double num = 0;
           switch (global.type) {
@@ skipping 373 matching lines @@
                                ModuleOrigin origin) {
   ModuleResult result = DecodeWasmModule(isolate, start, end, false, origin);
   if (result.ok()) {
     DCHECK_NOT_NULL(result.val);
     delete result.val;
     return true;
   }
   return false;
 }
 
+void wasm::SetInstanceMemoryObject(Handle<JSObject> instance,
+                                   Handle<Object> mem_object) {
+  DCHECK(IsWasmInstance(*instance));
+  instance->SetInternalField(kWasmMemObject, *mem_object);
+}
+
 MaybeHandle<JSArrayBuffer> wasm::GetInstanceMemory(Isolate* isolate,
                                                    Handle<JSObject> instance) {
   Object* mem = instance->GetInternalField(kWasmMemArrayBuffer);
   DCHECK(IsWasmInstance(*instance));
   if (mem->IsUndefined(isolate)) return MaybeHandle<JSArrayBuffer>();
   return Handle<JSArrayBuffer>(JSArrayBuffer::cast(mem));
 }
 
 void SetInstanceMemory(Handle<JSObject> instance, JSArrayBuffer* buffer) {
   DisallowHeapAllocation no_gc;
   DCHECK(IsWasmInstance(*instance));
   instance->SetInternalField(kWasmMemArrayBuffer, buffer);
   WasmCompiledModule* compiled_module = GetCompiledModule(*instance);
   compiled_module->set_ptr_to_memory(buffer);
 }
 
 int32_t wasm::GetInstanceMemorySize(Isolate* isolate,
                                     Handle<JSObject> instance) {
+  DCHECK(IsWasmInstance(*instance));
   MaybeHandle<JSArrayBuffer> maybe_mem_buffer =
       GetInstanceMemory(isolate, instance);
   Handle<JSArrayBuffer> buffer;
   if (!maybe_mem_buffer.ToHandle(&buffer)) {
     return 0;
   } else {
     return buffer->byte_length()->Number() / WasmModule::kPageSize;
   }
 }
 
 uint32_t GetMaxInstanceMemorySize(Isolate* isolate, Handle<JSObject> instance) {
   uint32_t max_pages = WasmModule::kV8MaxPages;
   Handle<Object> memory_object(instance->GetInternalField(kWasmMemObject),
                                isolate);
   if (memory_object->IsUndefined(isolate)) return max_pages;
   return WasmJs::GetWasmMemoryMaximumSize(isolate, memory_object);
 }
 
-int32_t wasm::GrowInstanceMemory(Isolate* isolate, Handle<JSObject> instance,
-                                 uint32_t pages) {
-  if (!IsWasmInstance(*instance)) return -1;
-  if (pages == 0) return GetInstanceMemorySize(isolate, instance);
-  uint32_t max_pages = GetMaxInstanceMemorySize(isolate, instance);
-  if (WasmModule::kV8MaxPages < max_pages) return -1;
+Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
+                                       MaybeHandle<JSArrayBuffer> buffer,
+                                       uint32_t pages, uint32_t max_pages) {
+  Handle<JSArrayBuffer> old_buffer;
+  Address old_mem_start = nullptr;
+  uint32_t old_size = 0;
+  if (buffer.ToHandle(&old_buffer) && old_buffer->backing_store() != nullptr) {
+    old_mem_start = static_cast<Address>(old_buffer->backing_store());
+    DCHECK_NOT_NULL(old_mem_start);
+    old_size = old_buffer->byte_length()->Number();
+  }
+  DCHECK(old_size + pages * WasmModule::kPageSize <=
+         std::numeric_limits<uint32_t>::max());
+  uint32_t new_size = old_size + pages * WasmModule::kPageSize;
+  if (new_size <= old_size || max_pages * WasmModule::kPageSize < new_size) {
+    return Handle<JSArrayBuffer>::null();
+  }
+  Handle<JSArrayBuffer> new_buffer = NewArrayBuffer(isolate, new_size);
+  Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
+  if (new_buffer.is_null() || new_mem_start == nullptr) {
+    return Handle<JSArrayBuffer>::null();
+  }
+  if (old_size != 0) memcpy(new_mem_start, old_mem_start, old_size);
+  return new_buffer;
+}
 
-  Address old_mem_start = nullptr;
-  uint32_t old_size = 0, new_size = 0;
-
+int32_t UncheckedUpdateInstanceMemory(Isolate* isolate,
+                                      Handle<JSObject> instance,
+                                      Handle<JSArrayBuffer> buffer) {
   MaybeHandle<JSArrayBuffer> maybe_mem_buffer =
       GetInstanceMemory(isolate, instance);
   Handle<JSArrayBuffer> old_buffer;
-  if (!maybe_mem_buffer.ToHandle(&old_buffer) ||
-      old_buffer->backing_store() == nullptr) {
-    // If module object does not have linear memory associated with it,
-    // Allocate new array buffer of given size.
-    new_size = pages * WasmModule::kPageSize;
-    if (max_pages < pages) return -1;
-  } else {
+  Address old_mem_start = nullptr;
+  uint32_t old_size = 0;
+  if (maybe_mem_buffer.ToHandle(&old_buffer) &&
+      old_buffer->backing_store() != nullptr) {
     old_mem_start = static_cast<Address>(old_buffer->backing_store());
+    DCHECK_NOT_NULL(old_mem_start);
     old_size = old_buffer->byte_length()->Number();
-    // If the old memory was zero-sized, we should have been in the
-    // "undefined" case above.
-    DCHECK_NOT_NULL(old_mem_start);
-    DCHECK(old_size + pages * WasmModule::kPageSize <=
-           std::numeric_limits<uint32_t>::max());
-    new_size = old_size + pages * WasmModule::kPageSize;
   }
-
-  if (new_size <= old_size || max_pages * WasmModule::kPageSize < new_size) {
-    return -1;
-  }
-  Handle<JSArrayBuffer> buffer = NewArrayBuffer(isolate, new_size);
-  if (buffer.is_null()) return -1;
+  uint32_t new_size = buffer->byte_length()->Number();
+  DCHECK(new_size <= std::numeric_limits<uint32_t>::max());
   Address new_mem_start = static_cast<Address>(buffer->backing_store());
-  if (old_size != 0) {
-    memcpy(new_mem_start, old_mem_start, old_size);
-  }
+  DCHECK_NOT_NULL(new_mem_start);
   SetInstanceMemory(instance, *buffer);
   Handle<FixedArray> code_table = GetCompiledModule(*instance)->code_table();
   RelocateMemoryReferencesInCode(code_table, old_mem_start, new_mem_start,
                                  old_size, new_size);
-  Handle<Object> memory_object(instance->GetInternalField(kWasmMemObject),
-                               isolate);
-  if (!memory_object->IsUndefined(isolate)) {
-    WasmJs::SetWasmMemoryArrayBuffer(isolate, memory_object, buffer);
-  }
-
   DCHECK(old_size % WasmModule::kPageSize == 0);
   return (old_size / WasmModule::kPageSize);
 }
 
+int32_t wasm::GrowWebAssemblyMemory(Isolate* isolate,

[Mircea Trofin, 2016/11/09 18:57:10]

please add comment explaining what the return is.

[gdeepti, 2016/11/16 05:34:10]

This is as per Spec for GrowMemory - Return the previous memory size in units of
pages or -1 on failure.

https://github.com/WebAssembly/design/blob/357a311f9322ebb37192dc15429c3c3667...

+                                    Handle<Object> memory_object,
+                                    uint32_t pages) {
+  DCHECK(WasmJs::IsWasmMemoryObject(isolate, memory_object));
+  uint32_t max_pages = WasmJs::GetWasmMemoryMaximumSize(isolate, memory_object);
+  if (WasmModule::kV8MaxPages < max_pages) return -1;

[Mircea Trofin, 2016/11/09 18:57:10]

Should this be a DCHECK rather, and have the caller ensure this is not
happening?

Also, what's the spec say for this case?

[gdeepti, 2016/11/16 05:34:10]

As per the spec, grow_memory should return -1 for failure cases. This is more of
a failure case based on the V8 implementation because V8MaxPages < SpecMaxPages.
So my interpretation is that this should fail gracefully instead of crashing
when trying to allocate a larger number of pages. The Spec says system
allocation for grow_memory may fail, and failure cases return -1. Can change to
DCHECK if others think this should be a DCHECK. 

+  Handle<WasmInstanceWrapper> instance_wrapper =
+      Handle<WasmInstanceWrapper>::cast(
+          WasmJs::GetWasmMemoryInstanceWrapper(isolate, memory_object));
+  DCHECK(WasmInstanceWrapper::IsWasmInstanceWrapper(*instance_wrapper));
+  DCHECK(instance_wrapper->has_instance());
+  Handle<JSObject> instance = instance_wrapper->instance_object();
+  DCHECK(IsWasmInstance(*instance));
+  if (pages == 0) return GetInstanceMemorySize(isolate, instance);

[Mircea Trofin, 2016/11/09 18:57:09]

why support pages == 0? Should this be an early DCHECK rather?

[gdeepti, 2016/11/16 05:34:10]

The Spec says - "Return the previous memory size in units of pages or -1 on
failure."

It does not say anything about the 0 case specifically, but 0 is a valid delta
value, and the previous size in this case will be the current size of the
memory.  

+
+  // Grow memory object buffer and update instances associated with it.
+  MaybeHandle<JSArrayBuffer> old_buffer =
+      WasmJs::GetWasmMemoryArrayBuffer(isolate, memory_object);
+  Handle<JSArrayBuffer> new_buffer =
+      GrowMemoryBuffer(isolate, old_buffer, pages, max_pages);
+  if (new_buffer.is_null()) return -1;
+  DCHECK(!instance_wrapper->has_previous());
+  int32_t ret = UncheckedUpdateInstanceMemory(isolate, instance, new_buffer);
+  while (instance_wrapper->has_next()) {

[Mircea Trofin, 2016/11/09 18:57:09]

nit: perhaps paranoia, but you could do this in a for loop, thus having a better
readability of how you advance through the list.

[gdeepti, 2016/11/16 05:34:10]

I would prefer to leave this as is, as this seems to be more readable to me. Can
change if you feel strongly about it. 

+    instance_wrapper = instance_wrapper->next_wrapper();
+    DCHECK(WasmInstanceWrapper::IsWasmInstanceWrapper(*instance_wrapper));
+    Handle<JSObject> instance = instance_wrapper->instance_object();
+    DCHECK(IsWasmInstance(*instance));
+    CHECK_EQ(ret, UncheckedUpdateInstanceMemory(isolate, instance, new_buffer));
+  }
+  WasmJs::SetWasmMemoryArrayBuffer(isolate, memory_object, new_buffer);
+  return ret;
+}
+
+int32_t wasm::GrowMemory(Isolate* isolate, Handle<JSObject> instance,
+                         uint32_t pages) {
+  if (!IsWasmInstance(*instance)) return -1;
+  if (pages == 0) return GetInstanceMemorySize(isolate, instance);
+  Handle<Object> obj(instance->GetInternalField(kWasmMemObject), isolate);
+  if (obj->IsUndefined(isolate)) {
+    // No other instances to grow, grow just the one.
+    MaybeHandle<JSArrayBuffer> old_buffer =
+        GetInstanceMemory(isolate, instance);
+    Handle<JSArrayBuffer> buffer =
+        GrowMemoryBuffer(isolate, old_buffer, pages, WasmModule::kV8MaxPages);
+    if (buffer.is_null()) return -1;
+    return UncheckedUpdateInstanceMemory(isolate, instance, buffer);
+  } else {
+    return GrowWebAssemblyMemory(isolate, obj, pages);
+  }
+}
+
 void testing::ValidateInstancesChain(Isolate* isolate,
                                      Handle<JSObject> wasm_module,
                                      int instance_count) {
   CHECK_GE(instance_count, 0);
   DisallowHeapAllocation no_gc;
   WasmCompiledModule* compiled_module =
       WasmCompiledModule::cast(wasm_module->GetInternalField(0));
   CHECK_EQ(JSObject::cast(compiled_module->ptr_to_weak_wasm_module()->value()),
            *wasm_module);
   Object* prev = nullptr;
@@ skipping 58 matching lines @@
     CHECK_NOT_NULL(result.val);
     module = const_cast<WasmModule*>(result.val);
   }
 
   Handle<WasmModuleWrapper> module_wrapper =
       WasmModuleWrapper::New(isolate, module);
 
   compiled_module->set_module_wrapper(module_wrapper);
   DCHECK(WasmCompiledModule::IsWasmCompiledModule(*compiled_module));
 }
+
+static void MemoryInstanceFinalizer(const v8::WeakCallbackInfo<void>& data) {

[Mircea Trofin, 2016/11/09 18:57:10]

I'd piggy-back on the existing finalizer registration, and add a "grand
finalizer" that then orchestrates this and the compiled modules one. The grand
finalizer can do the parameter unpacking, too, perhaps. Also, it can ensure
ordering (i.e. first memory, then instances - or vice-versa, whichever), thus
ensuring determinism.

[gdeepti, 2016/11/16 05:34:10]

The MemoryInstanceFinalizer is optional, i.e, only needed if
instances_link/memory_object associated with the link exist - so I've taken the
InstanceFinalizer for the CompiledModules to be the "grand finalizer" and branch
to update the instances chain if needed. 

+  JSObject** p = reinterpret_cast<JSObject**>(data.GetParameter());
+  JSObject* instance = *p;
+  Isolate* isolate = reinterpret_cast<Isolate*>(data.GetIsolate());
+  Handle<WasmInstanceWrapper> instance_wrapper =
+      handle(WasmInstanceWrapper::cast(
+          instance->GetInternalField(kWasmMemInstanceWrapper)));
+
+  DCHECK(WasmInstanceWrapper::IsWasmInstanceWrapper(*instance_wrapper));
+  DCHECK(instance_wrapper->has_instance());
+  bool has_prev = instance_wrapper->has_previous();
+  bool has_next = instance_wrapper->has_next();
+  Handle<Object> memory_object(instance->GetInternalField(kWasmMemObject),
+                               isolate);
+
+  if (!has_prev && !has_next) {
+    if (!memory_object->IsUndefined(isolate)) {
+      WasmJs::ResetWasmMemoryInstance(isolate, memory_object);
+    }
+    GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
+    return;
+  } else {
+    Handle<WasmInstanceWrapper> next_wrapper, prev_wrapper;
+    if (!has_prev) {
+      Handle<WasmInstanceWrapper> next_wrapper =
+          instance_wrapper->next_wrapper();
+      next_wrapper->reset_previous_wrapper();
+      // As this is the first link in the memory object, destroying
+      // without updating memory object would corrupt the instance chain in
+      // the memory object.

[Mircea Trofin, 2016/11/09 18:57:10]

This comment makes me believe even moreso that we need to explicitly control the
ordering of the finalizers

[gdeepti, 2016/11/16 05:34:10]

Agreed, Done.

+      const int kWasmMemObjectInstancesLink = 2;

[Mircea Trofin, 2016/11/09 18:57:10]

please see my earlier comment about fragility. Please move this logic outside,
to avoid relying on "2" being the value here.

[gdeepti, 2016/11/16 05:34:10]

Done.

+      JSObject::cast(*memory_object)
+          ->SetInternalField(kWasmMemObjectInstancesLink, *next_wrapper);
+    } else if (!has_next) {
+      instance_wrapper->previous_wrapper()->reset_next_wrapper();
+    } else {
+      DCHECK(has_next && has_prev);
+      Handle<WasmInstanceWrapper> prev_wrapper =
+          instance_wrapper->previous_wrapper();
+      Handle<WasmInstanceWrapper> next_wrapper =
+          instance_wrapper->next_wrapper();
+      prev_wrapper->set_next_wrapper(*next_wrapper);
+      next_wrapper->set_previous_wrapper(*prev_wrapper);
+    }
+    // Reset to avoid dangling pointers
+    instance_wrapper->reset();
+    GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
+  }
+}
+
+Handle<WasmInstanceWrapper> WasmInstanceWrapper::New(
+    Isolate* isolate, Handle<JSObject> instance) {
+  Handle<FixedArray> array =
+      isolate->factory()->NewFixedArray(kWrapperPropertyCount, TENURED);
+  Handle<WasmInstanceWrapper> instance_wrapper(
+      reinterpret_cast<WasmInstanceWrapper*>(*array), isolate);
+  instance_wrapper->set_instance_object(instance, isolate);
+  return instance_wrapper;
+}
+
+bool WasmInstanceWrapper::IsWasmInstanceWrapper(Object* obj) {
+  if (!obj->IsFixedArray()) return false;
+  FixedArray* array = FixedArray::cast(obj);
+  if (array->length() != kWrapperPropertyCount) return false;
+  if (!array->get(kWrapperInstanceObject)->IsWeakCell()) return false;
+  Isolate* isolate = array->GetIsolate();
+  if (!array->get(kNextInstanceWrapper)->IsUndefined(isolate) &&
+      !array->get(kNextInstanceWrapper)->IsFixedArray())
+    return false;
+  if (!array->get(kPreviousInstanceWrapper)->IsUndefined(isolate) &&
+      !array->get(kPreviousInstanceWrapper)->IsFixedArray())
+    return false;
+  return true;
+}
+
+void WasmInstanceWrapper::set_instance_object(Handle<JSObject> instance,
+                                              Isolate* isolate) {
+  Handle<Object> global_handle = isolate->global_handles()->Create(*instance);
+  Handle<WeakCell> cell = isolate->factory()->NewWeakCell(instance);
+  set(kWrapperInstanceObject, *cell);
+  GlobalHandles::MakeWeak(global_handle.location(), global_handle.location(),
+                          &MemoryInstanceFinalizer,

[bradnelson, 2016/11/09 01:40:11]

You should be able to piggy-back this on the existing instance finalizer.

[gdeepti, 2016/11/16 05:34:10]

Done.

+                          v8::WeakCallbackType::kFinalizer);
+}