[wasm] GrowMemory should use array_buffer_allocator instead of realloc.

src/runtime/runtime-wasm.cc

Index: src/runtime/runtime-wasm.cc
diff --git a/src/runtime/runtime-wasm.cc b/src/runtime/runtime-wasm.cc
index 6fc72767ea57bba5cbb4fe9c9be65433ebb4bca0..6410646850ab122a971a0764e5f703cbe30944eb 100644
--- a/src/runtime/runtime-wasm.cc
+++ b/src/runtime/runtime-wasm.cc
@@ -86,14 +86,23 @@ RUNTIME_FUNCTION(Runtime_WasmGrowMemory) {
         wasm::WasmModule::kMaxMemPages * wasm::WasmModule::kPageSize) {
       return *isolate->factory()->NewNumberFromInt(-1);
     }
-    new_mem_start = static_cast<Address>(realloc(old_mem_start, new_size));
+    new_mem_start =
+        static_cast<Address>(isolate->array_buffer_allocator()->Allocate(
+            static_cast<uint32_t>(new_size)));
     if (new_mem_start == NULL) {
       return *isolate->factory()->NewNumberFromInt(-1);
     }
+#if DEBUG
+    // Double check the API allocator actually zero-initialized the memory.
+    for (size_t i = old_size; i < new_size; i++) {
+      DCHECK_EQ(0, new_mem_start[i]);
+    }
+#endif
+    // Copy contents of the old buffer to the new buffer before detaching old
+    // buffer
+    memcpy(new_mem_start, old_mem_start, old_size);
     old_buffer->set_is_external(true);
     isolate->heap()->UnregisterArrayBuffer(*old_buffer);

[gdeepti, 2016/09/08 06:04:47]

Is this the right way to deal with the old buffer? My understanding is
unregistering the old_buffer will get the garbage collector to free the memory. 

[Michael Lippautz, 2016/09/08 09:43:31]

UnregisterArrayBuffer will make the GC stop tracking the backing store of this
JSArrayBuffer. So it's not that the buffer is freed but rather ownership is
transferred from the GC to the embedder.

The prime example is graphics code that works with JSArrayBuffer until a certain
point where they want to send it to the GPU without copy, so they unregister the
buffer, effectively taking over ownership (from the GC).

For wasm you have 2 choices:
- Don't externalize and UnregisterArrayBuffer (get rid of line 104 and 105). The
GC will collect the backing store for you in this case because you didn't
specify it as external on line 109.
- Add a array_buffer_allocator()->Free() after UnregisterArrayBuffer because
wasm is now the owner of the backing store.

[ahaas, 2016/09/08 12:23:33]

Hi Deepti, I talked with Ben about it, and we think that it would be best to go
for option 1, "don't externalize and UnregisterArrayBuffer".

[gdeepti, 2016/09/08 22:44:50]

Thanks for explaining that, I've removed the code that externalizes and uses
UnregisterArrayBuffer. 

-    // Zero initializing uninitialized memory from realloc
-    memset(new_mem_start + old_size, 0, new_size - old_size);
   }
 
   Handle<JSArrayBuffer> buffer = isolate->factory()->NewJSArrayBuffer();
@@ -102,7 +111,6 @@ RUNTIME_FUNCTION(Runtime_WasmGrowMemory) {
 
   // Set new buffer to be wasm memory
   module_object->SetInternalField(kWasmMemArrayBuffer, *buffer);
-
   CHECK(wasm::UpdateWasmModuleMemory(module_object, old_mem_start,
                                      new_mem_start, old_size, new_size));