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

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 aa385f824e6917b38815aadf8d047c4fa5964ea4..4f6dbac5d2855832052ec923119592565d8541bd 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -81,6 +81,7 @@ enum WasmInstanceObjectFields {
   kWasmMemArrayBuffer,
   kWasmGlobalsArrayBuffer,
   kWasmDebugInfo,
+  kWasmMemInstanceWrapper,
   kWasmInstanceInternalFieldCount
 };
 
@@ -1223,11 +1224,6 @@ class WasmInstanceBuilder {
       }
     }
 
-    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.
     //--------------------------------------------------------------------------
@@ -1504,6 +1500,7 @@ class WasmInstanceBuilder {
           }
           instance->SetInternalField(kWasmMemObject, *object);
           memory_ = WasmJs::GetWasmMemoryArrayBuffer(isolate_, object);
+          WasmJs::SetWasmMemoryInstance(isolate_, object, instance);
           break;
         }
         case kExternalGlobal: {
@@ -1679,6 +1676,9 @@ class WasmInstanceBuilder {
             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);
@@ -2072,6 +2072,12 @@ bool wasm::ValidateModuleBytes(Isolate* isolate, const byte* start,
   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);
@@ -2090,6 +2096,7 @@ void SetInstanceMemory(Handle<JSObject> instance, JSArrayBuffer* buffer) {
 
 int32_t wasm::GetInstanceMemorySize(Isolate* isolate,
                                     Handle<JSObject> instance) {
+  DCHECK(IsWasmInstance(*instance));
   MaybeHandle<JSArrayBuffer> maybe_mem_buffer =
       GetInstanceMemory(isolate, instance);
   Handle<JSArrayBuffer> buffer;
@@ -2108,59 +2115,110 @@ uint32_t GetMaxInstanceMemorySize(Isolate* isolate, Handle<JSObject> instance) {
   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, new_size = 0;
+  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;
+}
 
+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());
-    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;
+    old_size = old_buffer->byte_length()->Number();
   }
-  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) {
@@ -2239,3 +2297,88 @@ void WasmCompiledModule::RecreateModuleWrapper(Isolate* isolate,
   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);
+}