From 04df7dbadbf6cc2c45dd8e9e9fb467a34f3cc612 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Fri, 20 Sep 2019 23:42:31 +0200
Subject: [PATCH] worker: keep allocators for transferred SAB instances alive
 longer
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Keep the `ArrayBuffer::Allocator` behind a `SharedArrayBuffer` instance
alive for at least as long as the receiving Isolate lives, if the
`SharedArrayBuffer` instance isn’t already destroyed through GC.

This is to work around the fact that V8 7.9 started refactoring
how backing stores for `SharedArrayBuffer` instances work, changing
the timing of the call that releases the backing store to be
during Isolate disposal.

The flag added to the test is optional but helps verify that the
backing store is actually free’d at the end of the test and does not
leak memory.

Fixes: https://github.com/nodejs/node-v8/issues/115
PR-URL: https://github.com/nodejs/node/pull/29637
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 src/env.cc                                    | 15 ++++++++++++
 src/env.h                                     |  8 +++++++
 src/sharedarraybuffer_metadata.cc             | 24 +++++++++++++++++++
 src/sharedarraybuffer_metadata.h              |  3 +++
 ...er-sharedarraybuffer-from-worker-thread.js |  1 +
 5 files changed, 51 insertions(+)

diff --git a/src/env.cc b/src/env.cc
index 257bf78519f32d..2400785ea82fe4 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -1036,6 +1036,21 @@ char* Environment::Reallocate(char* data, size_t old_size, size_t size) {
   return new_data;
 }
 
+void Environment::AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
+    std::shared_ptr<v8::ArrayBuffer::Allocator> allocator) {
+  if (keep_alive_allocators_ == nullptr) {
+    MultiIsolatePlatform* platform = isolate_data()->platform();
+    CHECK_NOT_NULL(platform);
+
+    keep_alive_allocators_ = new ArrayBufferAllocatorList();
+    platform->AddIsolateFinishedCallback(isolate(), [](void* data) {
+      delete static_cast<ArrayBufferAllocatorList*>(data);
+    }, static_cast<void*>(keep_alive_allocators_));
+  }
+
+  keep_alive_allocators_->insert(allocator);
+}
+
 void AsyncRequest::Install(Environment* env, void* data, uv_async_cb target) {
   CHECK_NULL(async_);
   env_ = env;
diff --git a/src/env.h b/src/env.h
index 9b93b065c71cf9..cafe35022926ce 100644
--- a/src/env.h
+++ b/src/env.h
@@ -1252,6 +1252,10 @@ class Environment : public MemoryRetainer {
 
 #endif  // HAVE_INSPECTOR
 
+  // Only available if a MultiIsolatePlatform is in use.
+  void AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
+      std::shared_ptr<v8::ArrayBuffer::Allocator>);
+
  private:
   template <typename Fn>
   inline void CreateImmediate(Fn&& cb,
@@ -1426,6 +1430,10 @@ class Environment : public MemoryRetainer {
   // Used by embedders to shutdown running Node instance.
   AsyncRequest thread_stopper_;
 
+  typedef std::unordered_set<std::shared_ptr<v8::ArrayBuffer::Allocator>>
+      ArrayBufferAllocatorList;
+  ArrayBufferAllocatorList* keep_alive_allocators_ = nullptr;
+
   template <typename T>
   void ForEachBaseObject(T&& iterator);
 
diff --git a/src/sharedarraybuffer_metadata.cc b/src/sharedarraybuffer_metadata.cc
index b9d86d05ff20f3..fc3bcdf3d3b6b7 100644
--- a/src/sharedarraybuffer_metadata.cc
+++ b/src/sharedarraybuffer_metadata.cc
@@ -50,6 +50,30 @@ class SABLifetimePartner : public BaseObject {
     : BaseObject(env, obj),
       reference(std::move(r)) {
     MakeWeak();
+    env->AddCleanupHook(CleanupHook, static_cast<void*>(this));
+  }
+
+  ~SABLifetimePartner() {
+    env()->RemoveCleanupHook(CleanupHook, static_cast<void*>(this));
+  }
+
+  static void CleanupHook(void* data) {
+    // There is another cleanup hook attached to this object because it is a
+    // BaseObject. Cleanup hooks are triggered in reverse order of addition,
+    // so if this object is destroyed through GC, the destructor removes all
+    // hooks associated with this object, meaning that this cleanup hook
+    // only runs at the end of the Environment’s lifetime.
+    // In that case, V8 still knows about the SharedArrayBuffer and tries to
+    // free it when the last Isolate with access to it is disposed; for that,
+    // the ArrayBuffer::Allocator needs to be kept alive longer than this
+    // object and longer than the Environment instance.
+    //
+    // This is a workaround for https://github.com/nodejs/node-v8/issues/115
+    // (introduced in V8 7.9) and we should be able to remove it once V8
+    // ArrayBuffer::Allocator refactoring/removal is complete.
+    SABLifetimePartner* self = static_cast<SABLifetimePartner*>(data);
+    self->env()->AddArrayBufferAllocatorToKeepAliveUntilIsolateDispose(
+        self->reference->allocator());
   }
 
   SET_NO_MEMORY_INFO()
diff --git a/src/sharedarraybuffer_metadata.h b/src/sharedarraybuffer_metadata.h
index 8da603978a990b..4d89f08ee10daf 100644
--- a/src/sharedarraybuffer_metadata.h
+++ b/src/sharedarraybuffer_metadata.h
@@ -37,6 +37,9 @@ class SharedArrayBufferMetadata
   // count is increased by 1.
   v8::MaybeLocal<v8::SharedArrayBuffer> GetSharedArrayBuffer(
       Environment* env, v8::Local<v8::Context> context);
+  std::shared_ptr<v8::ArrayBuffer::Allocator> allocator() const {
+    return allocator_;
+  }
 
   SharedArrayBufferMetadata(SharedArrayBufferMetadata&& other) = delete;
   SharedArrayBufferMetadata& operator=(
diff --git a/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js b/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js
index 60e8a5d52ab5bf..56dfe2ec41ab3a 100644
--- a/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js
+++ b/test/parallel/test-worker-sharedarraybuffer-from-worker-thread.js
@@ -1,3 +1,4 @@
+// Flags: --debug-arraybuffer-allocations
 'use strict';
 const common = require('../common');
 const assert = require('assert');