[wasm] Memory buffer should be detached after Memory.Grow

src/wasm/wasm-js.cc

Index: src/wasm/wasm-js.cc
diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc
index 20c6d9bb69910cb15ae93ccad99dc0b3a21b5be4..cbd2c321dced2969aa17db81f16fa506d1674e7b 100644
--- a/src/wasm/wasm-js.cc
+++ b/src/wasm/wasm-js.cc
@@ -732,6 +732,30 @@ void WebAssemblyMemoryGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
     isolate->ThrowException(e);
     return;
   }
+
+  if (delta_size != 0) {

[titzer, 2017/01/25 09:28:57]

There was a discussion about whether a delta_size of 0 should detach here:
https://github.com/WebAssembly/design/issues/966

It's not resolved, but IMO it makes more intuitive sense to always detach if the
operation doesn't fail.

[gdeepti, 2017/01/25 21:05:24]

I opened that issue because I was not able to get a conclusive answer as to
which way that should work and figured it might be good to have it clarified in
the spec, fixed to always detach. 

+    // Detach the old buffer
+    const bool has_guard_regions =

[titzer, 2017/01/25 09:28:56]

I think it also makes sense to move the detachment of the old bugger into the
GrowWebAssemblyMemory operation, since it might get lucky and reuse the
underlying memory, so it would detach the old buffer itself.

[gdeepti, 2017/01/25 21:05:24]

Done.

+        (!old_buffer.is_null() && old_buffer->has_guard_region()) ? true
+                                                                  : false;

[Eric Holk, 2017/01/25 18:16:57]

nit: Is the ? operator needed here? `(!old_buffer.is_null() &&
old_buffer->has_guard_region())` is already a boolean.

[gdeepti, 2017/01/25 18:59:19]

Done.

+    DCHECK(!old_buffer->is_neuterable());
+    void* backing_store = old_buffer->backing_store();

[Eric Holk, 2017/01/25 18:16:57]

It might not hurt to add `DCHECK(backing_store != nullptr)`, just in case that
was the actual issue we saw yesterday.

[gdeepti, 2017/01/25 18:59:19]

Done.

+    int64_t byte_length = NumberToSize(old_buffer->byte_length());
+    old_buffer->set_is_neuterable(true);
+    if (!has_guard_regions) {
+      old_buffer->set_is_external(true);
+      i_isolate->heap()->UnregisterArrayBuffer(*old_buffer);
+    }
+    old_buffer->Neuter();
+    if (!has_guard_regions) {
+      i_isolate->array_buffer_allocator()->Free(backing_store, byte_length);
+    } else {
+      base::OS::Free(backing_store, RoundUp(i::wasm::kWasmMaxHeapOffset,
+                                            base::OS::CommitPageSize()));
+      isolate->AdjustAmountOfExternalAllocatedMemory(-byte_length);
+    }
+  }
+
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
   return_value.Set(ret);
 }