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

src/wasm/wasm-js.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 "src/api-natives.h"
 #include "src/api.h"
 #include "src/asmjs/asm-js.h"
 #include "src/asmjs/asm-typer.h"
 #include "src/asmjs/asm-wasm-builder.h"
 #include "src/assert-scope.h"
@@ skipping 714 matching lines @@
   }
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   int32_t ret = i::wasm::GrowWebAssemblyMemory(
       i_isolate, receiver, static_cast<uint32_t>(delta_size));
   if (ret == -1) {
     v8::Local<v8::Value> e = v8::Exception::RangeError(
         v8_str(isolate, "Unable to grow instance memory."));
     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);
 }
 
 void WebAssemblyMemoryGetBuffer(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::Isolate* isolate = args.GetIsolate();
   Local<Context> context = isolate->GetCurrentContext();
   i::Handle<i::Context> i_context = Utils::OpenHandle(*context);
   if (!BrandCheck(isolate, Utils::OpenHandle(*args.This()),
@@ skipping 220 matching lines @@
   i::Handle<i::Symbol> symbol(isolate->context()->wasm_memory_sym(), isolate);
   return HasBrand(value, symbol);
 }
 
 bool WasmJs::IsWasmTableObject(Isolate* isolate, Handle<Object> value) {
   i::Handle<i::Symbol> symbol(isolate->context()->wasm_table_sym(), isolate);
   return HasBrand(value, symbol);
 }
 }  // namespace internal
 }  // namespace v8