[wasm] Do not free externalized buffers when detaching

Once a buffer has been externalized, V8 is no longer responsible for managing
the memory. The fact that V8 was freeing was leading to double free errors once
Blink's GC got around to freeing the buffer too.

Bug: chromium:730171, chromium:731046
Change-Id: Ib18a7e37cafd51bce0c5a983d5cf8f3e64eb2c13
Reviewed-on: https://chromium-review.googlesource.com/530132
Commit-Queue: Brad Nelson <bradnelson@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45882}

src/wasm/wasm-module.cc

@@ -479,14 +479,14 @@ void wasm::DetachWebAssemblyMemoryBuffer(Isolate* isolate,
   if (!is_external) {
     buffer->set_is_external(true);
     isolate->heap()->UnregisterArrayBuffer(*buffer);
-  }
-  if (free_memory) {
-    // We need to free the memory before neutering the buffer because
-    // FreeBackingStore reads buffer->allocation_base(), which is nulled out by
-    // Neuter. This means there is a dangling pointer until we neuter the
-    // buffer. Since there is no way for the user to directly call
-    // FreeBackingStore, we can ensure this is safe.
-    buffer->FreeBackingStore();
+    if (free_memory) {
+      // We need to free the memory before neutering the buffer because
+      // FreeBackingStore reads buffer->allocation_base(), which is nulled out
+      // by Neuter. This means there is a dangling pointer until we neuter the
+      // buffer. Since there is no way for the user to directly call
+      // FreeBackingStore, we can ensure this is safe.
+      buffer->FreeBackingStore();
+    }
   }
   buffer->set_is_neuterable(true);
   buffer->Neuter();

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

@@ -1100,6 +1100,8 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
                                   {}, {})
             .ToHandleChecked();
     Handle<JSArrayBuffer> memory(instance->memory_buffer(), isolate);
+    void* const old_allocation_base = memory->allocation_base();
+    size_t const old_allocation_length = memory->allocation_length();
 
     // Fake the Embedder flow by creating a memory object, externalize and grow.
     Handle<WasmMemoryObject> mem_obj =
@@ -1122,9 +1124,12 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMem) {
     // 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);
+    isolate->array_buffer_allocator()->Free(
+        old_allocation_base, old_allocation_length, allocation_mode);
     memory->set_allocation_base(nullptr);
     memory->set_allocation_length(0);
   }
@@ -1152,3 +1157,22 @@ TEST(Run_WasmModule_Buffer_Externalized_GrowMemMemSize) {
   }
   Cleanup();
 }
+
+TEST(Run_WasmModule_Buffer_Externalized_Detach) {
+  {
+    // Regression test for
+    // https://bugs.chromium.org/p/chromium/issues/detail?id=731046
+    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, backing_store,
+        16 * WasmModule::kPageSize, false, false);
+    v8::Utils::ToLocal(buffer)->Externalize();
+    wasm::DetachWebAssemblyMemoryBuffer(isolate, buffer, true);
+    isolate->array_buffer_allocator()->Free(backing_store,
+                                            16 * WasmModule::kPageSize);
+  }
+  Cleanup();
+}