[wasm] grow_memory(0) should detach the underlying ArrayBuffer

The WebAssembly JS API specification [1] covers the JS-visible side-effects
of executing a grow_memory operation and states that a successful
grow operation should always detach any prior array buffer.

[1] https://github.com/WebAssembly/spec/blob/master/document/js-api/index.bs

R=mstarzinger@chromium.org,gdeepti@chromium.org

Bug: 
Change-Id: Ib9232e01209ba546c0bba1c9408c92da60ff6d92
Reviewed-on: https://chromium-review.googlesource.com/860011Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50627}

src/wasm/wasm-js.cc

@@ -841,10 +841,6 @@ void WebAssemblyMemoryGrow(const v8::FunctionCallbackInfo<v8::Value>& args) {
     thrower.RangeError("Unable to grow instance memory.");
     return;
   }
-  if (!old_buffer->is_shared()) {
-    bool free_memory = delta_size != 0 && !old_buffer->has_guard_region();
-    i::wasm::DetachMemoryBuffer(i_isolate, old_buffer, free_memory);
-  }
   v8::ReturnValue<v8::Value> return_value = args.GetReturnValue();
   return_value.Set(ret);
 }

src/wasm/wasm-memory.cc

@@ -154,8 +154,11 @@ Handle<JSArrayBuffer> NewArrayBuffer(Isolate* isolate, size_t size,
                           size, is_external, require_guard_regions, shared);
 }
 
-void ExternalizeMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
-                             bool free_memory) {
+void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
+                        bool free_memory) {
+  if (buffer->is_shared()) return;  // Detaching shared buffers is impossible.
+  DCHECK(!buffer->is_neuterable());
+
   const bool is_external = buffer->is_external();
   DCHECK(!buffer->is_neuterable());
   if (!is_external) {
@@ -170,12 +173,7 @@ void ExternalizeMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
       buffer->FreeBackingStore();
     }
   }
-}
 
-void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
-                        bool free_memory) {
-  DCHECK(!buffer->is_neuterable());
-  ExternalizeMemoryBuffer(isolate, buffer, free_memory);
   DCHECK(buffer->is_external());
   buffer->set_is_neuterable(true);
   buffer->Neuter();

src/wasm/wasm-memory.h

@@ -42,8 +42,6 @@ Handle<JSArrayBuffer> SetupArrayBuffer(
     void* backing_store, size_t size, bool is_external,
     bool enable_guard_regions, SharedFlag shared = SharedFlag::kNotShared);
 
-void ExternalizeMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
-                             bool free_memory);
 void DetachMemoryBuffer(Isolate* isolate, Handle<JSArrayBuffer> buffer,
                         bool free_memory);
 

src/wasm/wasm-objects.cc

@@ -418,21 +418,32 @@ Handle<JSArrayBuffer> GrowMemoryBuffer(Isolate* isolate,
     bool has_guard_region = old_buffer->has_guard_region();
     bool is_external = old_buffer->is_external();
     // Disconnect buffer early so GC won't free it.
-    i::wasm::ExternalizeMemoryBuffer(isolate, old_buffer, false);
+    i::wasm::DetachMemoryBuffer(isolate, old_buffer, false);
     Handle<JSArrayBuffer> new_buffer = wasm::SetupArrayBuffer(
         isolate, allocation_base, allocation_length, backing_store, new_size,
         is_external, has_guard_region);
     return new_buffer;
   } else {
+    bool free_memory = false;
     Handle<JSArrayBuffer> new_buffer;
-    new_buffer = wasm::NewArrayBuffer(isolate, new_size, enable_guard_regions);
-    if (new_buffer.is_null() || old_size == 0) return new_buffer;
-    Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
-    memcpy(new_mem_start, old_mem_start, old_size);
-    DCHECK(old_buffer.is_null() || !old_buffer->is_shared());
-    DCHECK(old_buffer.is_null() || !old_buffer->has_guard_region());
-    bool free_memory = pages != 0;
-    i::wasm::ExternalizeMemoryBuffer(isolate, old_buffer, free_memory);
+    if (pages != 0) {
+      // Allocate a new buffer and memcpy the old contents.
+      free_memory = true;
+      new_buffer =
+          wasm::NewArrayBuffer(isolate, new_size, enable_guard_regions);
+      if (new_buffer.is_null() || old_size == 0) return new_buffer;
+      Address new_mem_start = static_cast<Address>(new_buffer->backing_store());
+      memcpy(new_mem_start, old_mem_start, old_size);
+      DCHECK(old_buffer.is_null() || !old_buffer->is_shared());
+      DCHECK(old_buffer.is_null() || !old_buffer->has_guard_region());
+    } else {
+      // Reuse the prior backing store, but allocate a new array buffer.
+      new_buffer = wasm::SetupArrayBuffer(
+          isolate, old_buffer->allocation_base(),
+          old_buffer->allocation_length(), old_buffer->backing_store(),
+          new_size, old_buffer->is_external(), old_buffer->has_guard_region());
+    }
+    i::wasm::DetachMemoryBuffer(isolate, old_buffer, free_memory);
     return new_buffer;
   }
 }
@@ -587,40 +598,14 @@ Handle<WasmInstanceObject> WasmInstanceObject::New(
   return instance;
 }
 
-int32_t WasmInstanceObject::GetMemorySize() {
-  if (!has_memory_object()) return 0;
-  uint32_t bytes = memory_object()->array_buffer()->byte_length()->Number();
-  DCHECK_EQ(0, bytes % wasm::kWasmPageSize);
-  return bytes / wasm::kWasmPageSize;
-}
-
 int32_t WasmInstanceObject::GrowMemory(Isolate* isolate,
                                        Handle<WasmInstanceObject> instance,
                                        uint32_t pages) {
-  if (pages == 0) return instance->GetMemorySize();
   DCHECK(instance->has_memory_object());
   return WasmMemoryObject::Grow(
       isolate, handle(instance->memory_object(), isolate), pages);
 }
 
-uint32_t WasmInstanceObject::GetMaxMemoryPages() {
-  if (has_memory_object()) {
-    if (memory_object()->has_maximum_pages()) {
-      uint32_t maximum =
-          static_cast<uint32_t>(memory_object()->maximum_pages());
-      if (maximum < FLAG_wasm_max_mem_pages) return maximum;
-    }
-  }
-  uint32_t module_maximum_pages =
-      compiled_module()->shared()->module()->maximum_pages;
-  Isolate* isolate = GetIsolate();
-  DCHECK(compiled_module()->shared()->module()->is_wasm());
-  isolate->counters()->wasm_wasm_max_mem_pages_count()->AddSample(
-      module_maximum_pages);
-  if (module_maximum_pages != 0) return module_maximum_pages;
-  return FLAG_wasm_max_mem_pages;
-}
-
 WasmInstanceObject* WasmInstanceObject::GetOwningInstance(
     const wasm::WasmCode* code) {
   DisallowHeapAllocation no_gc;

src/wasm/wasm-objects.h

@@ -234,13 +234,9 @@ class WasmInstanceObject : public JSObject {
 
   static Handle<WasmInstanceObject> New(Isolate*, Handle<WasmCompiledModule>);
 
-  int32_t GetMemorySize();
-
   static int32_t GrowMemory(Isolate*, Handle<WasmInstanceObject>,
                             uint32_t pages);
 
-  uint32_t GetMaxMemoryPages();
-
   // Assumed to be called with a code object associated to a wasm module
   // instance. Intended to be called from runtime functions. Returns nullptr on
   // failing to get owning instance.

test/cctest/wasm/test-run-wasm-module.cc

@@ -1059,12 +1059,35 @@ TEST(MemoryWithOOBEmptyDataSegment) {
   Cleanup();
 }
 
+// Utility to free the allocated memory for a buffer that is manually
+// externalized in a test.
+struct ManuallyExternalizedBuffer {
+  Isolate* isolate_;
+  Handle<JSArrayBuffer> buffer_;
+  void* allocation_base_;
+  size_t allocation_length_;
+
+  ManuallyExternalizedBuffer(JSArrayBuffer* buffer, Isolate* isolate)
+      : isolate_(isolate),
+        buffer_(buffer, isolate),
+        allocation_base_(buffer->allocation_base()),
+        allocation_length_(buffer->allocation_length()) {
+    if (!buffer->has_guard_region()) {
+      v8::Utils::ToLocal(buffer_)->Externalize();
+    }
+  }
+  ~ManuallyExternalizedBuffer() {
+    if (!buffer_->has_guard_region()) {
+      isolate_->array_buffer_allocator()->Free(
+          allocation_base_, allocation_length_, buffer_->allocation_mode());
+    }
+  }
+};
+
 TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
   {
     Isolate* isolate = CcTest::InitIsolateOnce();
     HandleScope scope(isolate);
-    // Initial memory size = 16 + GrowWebAssemblyMemory(4) + GrowMemory(6)
-    static const int kExpectedValue = 26;
     TestSignatures sigs;
     v8::internal::AccountingAllocator allocator;
     Zone zone(&allocator, ZONE_NAME);
@@ -1085,43 +1108,28 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
                                   ModuleWireBytes(buffer.begin(), buffer.end()),
                                   {}, {})
             .ToHandleChecked();
-    Handle<JSArrayBuffer> memory(instance->memory_object()->array_buffer(),
-                                 isolate);
-    Handle<WasmMemoryObject> mem_obj(instance->memory_object(), isolate);
-    void* const old_allocation_base = memory->allocation_base();
-    size_t const old_allocation_length = memory->allocation_length();
+    Handle<WasmMemoryObject> memory_object(instance->memory_object(), isolate);
 
-    // Fake the Embedder flow by externalizing the memory object, and grow.
-    v8::Utils::ToLocal(memory)->Externalize();
+    // Fake the Embedder flow by externalizing the array buffer.
+    ManuallyExternalizedBuffer buffer1(memory_object->array_buffer(), isolate);
 
-    uint32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 4);
-    bool free_memory = !memory->has_guard_region();
-    wasm::DetachMemoryBuffer(isolate, memory, free_memory);
+    // Grow using the API.
+    uint32_t result = WasmMemoryObject::Grow(isolate, memory_object, 4);
     CHECK_EQ(16, result);
-    memory = handle(mem_obj->array_buffer());
-    instance->memory_object()->set_array_buffer(*memory);
-    // Externalize should make no difference without the JS API as in this case
-    // the buffer is not detached.
-    if (!memory->has_guard_region()) {
-      v8::Utils::ToLocal(memory)->Externalize();
-    }
+    CHECK(buffer1.buffer_->was_neutered());  // growing always neuters
+    CHECK_EQ(0, buffer1.buffer_->byte_length()->Number());
+
+    CHECK_NE(*buffer1.buffer_, memory_object->array_buffer());
+
+    // Fake the Embedder flow by externalizing the array buffer.
+    ManuallyExternalizedBuffer buffer2(memory_object->array_buffer(), isolate);
+
+    // Grow using an internal WASM bytecode.
     result = testing::RunWasmModuleForTesting(isolate, instance, 0, nullptr);
-    CHECK_EQ(kExpectedValue, result);
-    // Free the buffer as the tracker does not know about it.
-    const v8::ArrayBuffer::Allocator::AllocationMode allocation_mode =
-        memory->allocation_mode();
-    CHECK_NOT_NULL(memory->allocation_base());
-    isolate->array_buffer_allocator()->Free(memory->allocation_base(),
-                                            memory->allocation_length(),
-                                            allocation_mode);
-    if (free_memory) {
-      // GrowMemory without guard pages enabled allocates an extra buffer,
-      // that needs to be freed as well
-      isolate->array_buffer_allocator()->Free(
-          old_allocation_base, old_allocation_length, allocation_mode);
-    }
-    memory->set_allocation_base(nullptr);
-    memory->set_allocation_length(0);
+    CHECK_EQ(26, result);
+    CHECK(buffer2.buffer_->was_neutered());  // growing always neuters
+    CHECK_EQ(0, buffer2.buffer_->byte_length()->Number());
+    CHECK_NE(*buffer2.buffer_, memory_object->array_buffer());
   }
   Cleanup();
 }
@@ -1139,7 +1147,6 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMemMemSize) {
         WasmMemoryObject::New(isolate, buffer, 100);
     v8::Utils::ToLocal(buffer)->Externalize();
     int32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 0);
-    wasm::DetachMemoryBuffer(isolate, buffer, false);
     CHECK_EQ(16, result);
 
     isolate->array_buffer_allocator()->Free(backing_store, 16 * kWasmPageSize);

test/mjsunit/wasm/grow-memory-detaching.js

+// Copyright 2016 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.
+
+// Flags: --expose-wasm
+
+load("test/mjsunit/wasm/wasm-constants.js");
+load("test/mjsunit/wasm/wasm-module-builder.js");
+
+let module = (() => {
+  let builder = new WasmModuleBuilder();
+  builder.addMemory(1, kV8MaxPages, false);
+  builder.addFunction("grow_memory", kSig_i_i)
+              .addBody([kExprGetLocal, 0, kExprGrowMemory, kMemoryZero])
+    .exportFunc();
+  builder.exportMemoryAs("memory");
+  return builder.toModule();
+})();
+
+(function TestDetachingViaAPI() {
+  print("TestDetachingViaAPI...");
+  let memory = new WebAssembly.Memory({initial: 1, maximum: 100});
+  let growMem = (pages) => memory.grow(pages);
+
+  let b1 = memory.buffer;
+  assertEquals(kPageSize, b1.byteLength);
+
+  growMem(0);
+  let b2 = memory.buffer;
+  assertFalse(b1 === b2);
+  assertEquals(0, b1.byteLength);
+  assertEquals(kPageSize, b2.byteLength);
+
+  growMem(1);
+  let b3 = memory.buffer;
+  assertFalse(b1 === b3);
+  assertFalse(b2 === b3);
+  assertEquals(0, b1.byteLength);
+  assertEquals(0, b2.byteLength);
+  assertEquals(2 * kPageSize, b3.byteLength);
+})();
+
+(function TestDetachingViaBytecode() {
+  print("TestDetachingViaBytecode...");
+  let instance = new WebAssembly.Instance(module);
+  let growMem = instance.exports.grow_memory;
+  let memory = instance.exports.memory;
+
+  let b1 = memory.buffer;
+  assertEquals(kPageSize, b1.byteLength);
+
+  growMem(0);
+  let b2 = memory.buffer;
+  assertFalse(b1 === b2);
+  assertEquals(0, b1.byteLength);
+  assertEquals(kPageSize, b2.byteLength);
+
+  growMem(1);
+  let b3 = memory.buffer;
+  assertFalse(b1 === b3);
+  assertFalse(b2 === b3);
+  assertEquals(0, b1.byteLength);
+  assertEquals(0, b2.byteLength);
+  assertEquals(2 * kPageSize, b3.byteLength);
+})();