[wasm] Do not unregister an ArrayBuffer if it is already external

 - Currently if GrowMemory is called with pages = 0, an attempt is made to
   unregister the ArrayBuffer even if it is external. Cleanup so all Detaching
   of ArrayBuffer is centralized to one method, and can only be called fromJS.
 - Gate creating WeakHandles to the memory on the buffer having guard pages
   enabled. Currently creating a WeakHandle is gated only on if the buffer
   is_external true. If a buffer is marked is_external = true to begin with,
   the WeakHandle is created and the Finalizer is run causing the program to
   crash.

BUG=chromium:717647

Review-Url: https://codereview.chromium.org/2867233002
Cr-Commit-Position: refs/heads/master@{#45238}

src/wasm/wasm-js.cc

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

src/wasm/wasm-module.cc

@@ -855,7 +855,7 @@ Handle<JSArrayBuffer> wasm::SetupArrayBuffer(Isolate* isolate,
   buffer->set_is_wasm_buffer(true);
   buffer->set_has_guard_region(enable_guard_regions);
 
-  if (is_external) {
+  if (enable_guard_regions) {
     // We mark the buffer as external if we allocated it here with guard
     // pages. That means we need to arrange for it to be freed.
 
@@ -2231,7 +2231,8 @@ bool wasm::IsWasmCodegenAllowed(Isolate* isolate, Handle<Context> context) {
 }
 
 void wasm::DetachWebAssemblyMemoryBuffer(Isolate* isolate,
-                                         Handle<JSArrayBuffer> buffer) {
+                                         Handle<JSArrayBuffer> buffer,
+                                         bool free_memory) {
   int64_t byte_length =
       buffer->byte_length()->IsNumber()
           ? static_cast<uint32_t>(buffer->byte_length()->Number())
@@ -2247,6 +2248,9 @@ void wasm::DetachWebAssemblyMemoryBuffer(Isolate* isolate,
   }
   buffer->set_is_neuterable(true);
   buffer->Neuter();
+  // Neuter but do not free, as when pages == 0, the backing store is being used
+  // by the new buffer.
+  if (!free_memory) return;
   if (has_guard_regions) {
     base::OS::Free(backing_store, RoundUp(i::wasm::kWasmMaxHeapOffset,
                                           base::OS::CommitPageSize()));

src/wasm/wasm-module.h

@@ -438,7 +438,8 @@ Handle<JSArrayBuffer> SetupArrayBuffer(Isolate*, void* backing_store,
                                        bool enable_guard_regions);
 
 void DetachWebAssemblyMemoryBuffer(Isolate* isolate,
-                                   Handle<JSArrayBuffer> buffer);
+                                   Handle<JSArrayBuffer> buffer,
+                                   bool free_memory);
 
 void UpdateDispatchTables(Isolate* isolate, Handle<FixedArray> dispatch_tables,
                           int index, Handle<JSFunction> js_function);

src/wasm/wasm-objects.cc

@@ -476,21 +476,13 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
   Handle<JSArrayBuffer> new_buffer;
   // Return current size if grow by 0.
   if (pages == 0) {
-    // Even for pages == 0, we need to attach a new JSArrayBuffer and neuter the
-    // old one to be spec compliant.
-    if (!old_buffer.is_null() && old_buffer->backing_store() != nullptr) {
+    // Even for pages == 0, we need to attach a new JSArrayBuffer with the same
+    // backing store and neuter the old one to be spec compliant.
+    if (!old_buffer.is_null() && old_size != 0) {
       new_buffer = SetupArrayBuffer(isolate, old_buffer->backing_store(),
                                     old_size, old_buffer->is_external(),
                                     old_buffer->has_guard_region());
       memory_object->set_buffer(*new_buffer);
-      old_buffer->set_is_neuterable(true);
-      if (!old_buffer->has_guard_region()) {
-        old_buffer->set_is_external(true);
-        isolate->heap()->UnregisterArrayBuffer(*old_buffer);
-      }
-      // Neuter but don't free the memory because it is now being used by
-      // new_buffer.
-      old_buffer->Neuter();
     }
     DCHECK_EQ(0, old_size % WasmModule::kPageSize);
     return old_size / WasmModule::kPageSize;

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

@@ -1112,6 +1112,7 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
     void* backing_store = memory->backing_store();
     uint64_t byte_length = NumberToSize(memory->byte_length());
     uint32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 4);
+    wasm::DetachWebAssemblyMemoryBuffer(isolate, memory, true);
     CHECK_EQ(16, result);
     if (!memory->has_guard_region()) {
       isolate->array_buffer_allocator()->Free(backing_store, byte_length);
@@ -1133,3 +1134,24 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
   }
   Cleanup();
 }
+
+TEST(Run_WasmModule_Buffer_Externalized_GrowMemMemSize) {
+  {
+    Isolate* isolate = CcTest::InitIsolateOnce();
+    HandleScope scope(isolate);
+    void* backing_store =
+        isolate->array_buffer_allocator()->Allocate(16 * WasmModule::kPageSize);
+    Handle<JSArrayBuffer> buffer = wasm::SetupArrayBuffer(
+        isolate, backing_store, 16 * WasmModule::kPageSize, false, false);
+    Handle<WasmMemoryObject> mem_obj =
+        WasmMemoryObject::New(isolate, buffer, 100);
+    v8::Utils::ToLocal(buffer)->Externalize();
+    int32_t result = WasmMemoryObject::Grow(isolate, mem_obj, 0);
+    wasm::DetachWebAssemblyMemoryBuffer(isolate, buffer, false);
+    CHECK_EQ(16, result);
+
+    isolate->array_buffer_allocator()->Free(backing_store,
+                                            16 * WasmModule::kPageSize);
+  }
+  Cleanup();
+}