buffer: release buffers with free callbacks on env exit

Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. PR-URL: nodejs#30551 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
addaleax · Jan 14, 2020 · 16996ec · 16996ec
1 parent 288ae47
commit 16996ec
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 10 deletions.
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -53,6 +53,7 @@ using v8::Context;
 using v8::EscapableHandleScope;
 using v8::FunctionCallbackInfo;
 using v8::Global;
+using v8::HandleScope;
 using v8::Int32;
 using v8::Integer;
 using v8::Isolate;
@@ -73,8 +74,10 @@ namespace {
 
 class CallbackInfo {
  public:
+  ~CallbackInfo();
+
   static inline void Free(char* data, void* hint);
-  static inline CallbackInfo* New(Isolate* isolate,
+  static inline CallbackInfo* New(Environment* env,
                                   Local<ArrayBuffer> object,
                                   FreeCallback callback,
                                   char* data,
@@ -84,9 +87,10 @@ class CallbackInfo {
   CallbackInfo& operator=(const CallbackInfo&) = delete;
 
  private:
+  static void CleanupHook(void* data);
   static void WeakCallback(const WeakCallbackInfo<CallbackInfo>&);
   inline void WeakCallback(Isolate* isolate);
-  inline CallbackInfo(Isolate* isolate,
+  inline CallbackInfo(Environment* env,
                       Local<ArrayBuffer> object,
                       FreeCallback callback,
                       char* data,
@@ -95,6 +99,7 @@ class CallbackInfo {
   FreeCallback const callback_;
   char* const data_;
   void* const hint_;
+  Environment* const env_;
 };
 
 
@@ -103,31 +108,53 @@ void CallbackInfo::Free(char* data, void*) {
 }
 
 
-CallbackInfo* CallbackInfo::New(Isolate* isolate,
+CallbackInfo* CallbackInfo::New(Environment* env,
                                 Local<ArrayBuffer> object,
                                 FreeCallback callback,
                                 char* data,
                                 void* hint) {
-  return new CallbackInfo(isolate, object, callback, data, hint);
+  return new CallbackInfo(env, object, callback, data, hint);
 }
 
 
-CallbackInfo::CallbackInfo(Isolate* isolate,
+CallbackInfo::CallbackInfo(Environment* env,
                            Local<ArrayBuffer> object,
                            FreeCallback callback,
                            char* data,
                            void* hint)
-    : persistent_(isolate, object),
+    : persistent_(env->isolate(), object),
       callback_(callback),
       data_(data),
-      hint_(hint) {
+      hint_(hint),
+      env_(env) {
   ArrayBuffer::Contents obj_c = object->GetContents();
   CHECK_EQ(data_, static_cast<char*>(obj_c.Data()));
   if (object->ByteLength() != 0)
     CHECK_NOT_NULL(data_);
 
   persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
-  isolate->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
+  env->AddCleanupHook(CleanupHook, this);
+  env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
+}
+
+
+CallbackInfo::~CallbackInfo() {
+  persistent_.Reset();
+  env_->RemoveCleanupHook(CleanupHook, this);
+}
+
+
+void CallbackInfo::CleanupHook(void* data) {
+  CallbackInfo* self = static_cast<CallbackInfo*>(data);
+
+  {
+    HandleScope handle_scope(self->env_->isolate());
+    Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
+    CHECK(!ab.IsEmpty());
+    ab->Detach();
+  }
+
+  self->WeakCallback(self->env_->isolate());
 }
 
 
@@ -391,7 +418,7 @@ MaybeLocal<Object> New(Environment* env,
   }
   MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
 
-  CallbackInfo::New(env->isolate(), ab, callback, data, hint);
+  CallbackInfo::New(env, ab, callback, data, hint);
 
   if (ui.IsEmpty())
     return MaybeLocal<Object>();

diff --git a/test/addons/worker-buffer-callback/binding.cc b/test/addons/worker-buffer-callback/binding.cc
@@ -3,17 +3,24 @@
 #include <v8.h>
 
 using v8::Context;
+using v8::FunctionCallbackInfo;
 using v8::Isolate;
 using v8::Local;
 using v8::Object;
 using v8::Value;
 
+uint32_t free_call_count = 0;
 char data[] = "hello";
 
+void GetFreeCallCount(const FunctionCallbackInfo<Value>& args) {
+  args.GetReturnValue().Set(free_call_count);
+}
+
 void Initialize(Local<Object> exports,
                 Local<Value> module,
                 Local<Context> context) {
   Isolate* isolate = context->GetIsolate();
+  NODE_SET_METHOD(exports, "getFreeCallCount", GetFreeCallCount);
   exports->Set(context,
                v8::String::NewFromUtf8(
                    isolate, "buffer", v8::NewStringType::kNormal)
@@ -22,7 +29,9 @@ void Initialize(Local<Object> exports,
                    isolate,
                    data,
                    sizeof(data),
-                   [](char* data, void* hint) {},
+                   [](char* data, void* hint) {
+                     free_call_count++;
+                   },
                    nullptr).ToLocalChecked()).Check();
 }
 

diff --git a/test/addons/worker-buffer-callback/test-free-called.js b/test/addons/worker-buffer-callback/test-free-called.js
@@ -0,0 +1,17 @@
+'use strict';
+const common = require('../../common');
+const path = require('path');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
+const { getFreeCallCount } = require(binding);
+
+// Test that buffers allocated with a free callback through our APIs are
+// released when a Worker owning it exits.
+
+const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true });
+
+assert.strictEqual(getFreeCallCount(), 0);
+w.on('exit', common.mustCall(() => {
+  assert.strictEqual(getFreeCallCount(), 1);
+}));
diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
@@ -1,3 +1,4 @@
+#include "node_buffer.h"
 #include "node_internals.h"
 #include "libplatform/libplatform.h"
 
@@ -185,3 +186,34 @@ static void at_exit_js(void* arg) {
   assert(obj->IsObject());
   called_at_exit_js = true;
 }
+
+static char hello[] = "hello";
+
+TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) {
+  // Test that a Buffer allocated with a free callback is detached after
+  // its callback has been called.
+  const v8::HandleScope handle_scope(isolate_);
+  const Argv argv;
+
+  int callback_calls = 0;
+
+  v8::Local<v8::ArrayBuffer> ab;
+  {
+    Env env {handle_scope, argv};
+    v8::Local<v8::Object> buf_obj = node::Buffer::New(
+        isolate_,
+        hello,
+        sizeof(hello),
+        [](char* data, void* hint) {
+          CHECK_EQ(data, hello);
+          ++*static_cast<int*>(hint);
+        },
+        &callback_calls).ToLocalChecked();
+    CHECK(buf_obj->IsUint8Array());
+    ab = buf_obj.As<v8::Uint8Array>()->Buffer();
+    CHECK_EQ(ab->ByteLength(), sizeof(hello));
+  }
+
+  CHECK_EQ(callback_calls, 1);
+  CHECK_EQ(ab->ByteLength(), 0);
+}