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

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 6eff8f2820848bcac4b38b3ad9727cdb29bde0a8..7446fe30ad3cf17b7053866c78d3b43c4d560046 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -61,12 +61,14 @@ static void MemoryFinalizer(const v8::WeakCallbackInfo<void>& data) {
   JSArrayBuffer** p = reinterpret_cast<JSArrayBuffer**>(data.GetParameter());
   JSArrayBuffer* buffer = *p;
 
-  void* memory = buffer->backing_store();
-  base::OS::Free(memory,
-                 RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize()));
+  if (!buffer->was_neutered()) {
+    void* memory = buffer->backing_store();

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

`DCHECK(memory != nullptr)` would probably be good here too (although hopefully
the was_neutered check covers that).

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

Done.

+    base::OS::Free(memory,
+                   RoundUp(kWasmMaxHeapOffset, base::OS::CommitPageSize()));
 
-  data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(
-      -buffer->byte_length()->Number());
+    data.GetIsolate()->AdjustAmountOfExternalAllocatedMemory(
+        -buffer->byte_length()->Number());
+  }
 
   GlobalHandles::Destroy(reinterpret_cast<Object**>(p));
 }
@@ -2349,25 +2351,14 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
     return Handle<JSArrayBuffer>::null();
   }
 
-  Handle<JSArrayBuffer> new_buffer;
-  if (!old_buffer.is_null() && old_buffer->has_guard_region()) {
-    // We don't move the backing store, we simply change the protection to make
-    // more of it accessible.
-    base::OS::Unprotect(old_buffer->backing_store(), new_size);
-    reinterpret_cast<v8::Isolate*>(isolate)
-        ->AdjustAmountOfExternalAllocatedMemory(pages * WasmModule::kPageSize);
-    Handle<Object> new_size_object =
-        isolate->factory()->NewNumberFromSize(new_size);
-    old_buffer->set_byte_length(*new_size_object);
-    new_buffer = old_buffer;
-  } else {
-    const bool enable_guard_regions = false;
-    new_buffer = NewArrayBuffer(isolate, new_size, enable_guard_regions);
-    if (new_buffer.is_null()) return new_buffer;
-    Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
-    if (old_size != 0) {
-      memcpy(new_mem_start, old_mem_start, old_size);
-    }
+  const bool enable_guard_regions =
+      (!old_buffer.is_null() && old_buffer->has_guard_region()) ? true : false;

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

You don't need to do the {x ? true : false}, you can just do {x}.

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

Done.

+  Handle<JSArrayBuffer> new_buffer =
+      NewArrayBuffer(isolate, new_size, enable_guard_regions);

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

I think should adjust the old logic here (just changing the protection) and not
just allocate a new buffer every time.

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

After offline discussions with Eric (summarized here -
https://bugs.chromium.org/p/v8/issues/detail?id=5886), we decided that this is
the safest/simplest way to fix it for now because additional book keeping is
needed in the Memory finalizer for this to be safe. Would this happen
automatically? If so, I can revert back to the previous logic. 

+  if (new_buffer.is_null()) return new_buffer;
+  Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
+  if (old_size != 0) {
+    memcpy(new_mem_start, old_mem_start, old_size);
   }
   return new_buffer;
 }