worker: keep allocators for transferred SAB instances alive longer

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: nodejs/node-v8#115
PR-URL: #29637
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Author: addaleax

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;

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);
 

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()

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=(

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');