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

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/assembler-inl.h"
 #include "src/base/adapters.h"
 #include "src/base/atomic-utils.h"
 #include "src/code-stubs.h"
@@ skipping 43 matching lines @@
       it.rinfo()->set_target_object(*new_ref);
     }
   }
 }
 
 static void MemoryFinalizer(const v8::WeakCallbackInfo<void>& data) {
   DisallowHeapAllocation no_gc;
   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));
 }
 
 #if V8_TARGET_ARCH_64_BIT
 const bool kGuardRegionsSupported = true;
 #else
 const bool kGuardRegionsSupported = false;
 #endif
 
@@ skipping 2262 matching lines @@
     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 ||
       FLAG_wasm_max_mem_pages * WasmModule::kPageSize < new_size) {
     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;
 }
 
 void UncheckedUpdateInstanceMemory(Isolate* isolate,
                                    Handle<WasmInstanceObject> instance,
                                    Address old_mem_start, uint32_t old_size) {
   DCHECK(instance->has_memory_buffer());
   Handle<JSArrayBuffer> new_buffer(instance->memory_buffer());
   uint32_t new_size = new_buffer->byte_length()->Number();
@@ skipping 373 matching lines @@
   Handle<FixedArray> storage = factory->NewFixedArray(num_custom_sections);
   JSArray::SetContent(array_object, storage);
   array_object->set_length(Smi::FromInt(num_custom_sections));
 
   for (int i = 0; i < num_custom_sections; i++) {
     storage->set(i, *matching_sections[i]);
   }
 
   return array_object;
 }