src: explicitly allocate backing stores for v8 stat buffers

This fixes flaky tests that crashed because the allocations ended up at positions of previously allocated `ArrayBuffer`s that were still in the backing store table. In particular, there was a race condition window between destroying a Worker thread’s `Environment` and destroying its `Isolate` in which the underlying memory was already released but the `ArrayBuffer` was still existent, meaning that new memory could be allocated at the address of the previous `ArrayBuffer`. Refs: #30782 PR-URL: #30946 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
nodejs · Dec 14, 2019 · d06efaf · d06efaf
1 parent d502b83
commit d06efaf
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 62 deletions.
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -543,32 +543,35 @@ inline double Environment::get_default_trigger_async_id() {
 
 inline double* Environment::heap_statistics_buffer() const {
   CHECK_NOT_NULL(heap_statistics_buffer_);
-  return heap_statistics_buffer_;
+  return static_cast<double*>(heap_statistics_buffer_->Data());
 }
 
-inline void Environment::set_heap_statistics_buffer(double* pointer) {
-  CHECK_NULL(heap_statistics_buffer_);  // Should be set only once.
-  heap_statistics_buffer_ = pointer;
+inline void Environment::set_heap_statistics_buffer(
+    std::shared_ptr<v8::BackingStore> backing_store) {
+  CHECK(!heap_statistics_buffer_);  // Should be set only once.
+  heap_statistics_buffer_ = std::move(backing_store);
 }
 
 inline double* Environment::heap_space_statistics_buffer() const {
-  CHECK_NOT_NULL(heap_space_statistics_buffer_);
-  return heap_space_statistics_buffer_;
+  CHECK(heap_space_statistics_buffer_);
+  return static_cast<double*>(heap_space_statistics_buffer_->Data());
 }
 
-inline void Environment::set_heap_space_statistics_buffer(double* pointer) {
-  CHECK_NULL(heap_space_statistics_buffer_);  // Should be set only once.
-  heap_space_statistics_buffer_ = pointer;
+inline void Environment::set_heap_space_statistics_buffer(
+    std::shared_ptr<v8::BackingStore> backing_store) {
+  CHECK(!heap_space_statistics_buffer_);  // Should be set only once.
+  heap_space_statistics_buffer_ = std::move(backing_store);
 }
 
 inline double* Environment::heap_code_statistics_buffer() const {
-  CHECK_NOT_NULL(heap_code_statistics_buffer_);
-  return heap_code_statistics_buffer_;
+  CHECK(heap_code_statistics_buffer_);
+  return static_cast<double*>(heap_code_statistics_buffer_->Data());
 }
 
-inline void Environment::set_heap_code_statistics_buffer(double* pointer) {
-  CHECK_NULL(heap_code_statistics_buffer_);  // Should be set only once.
-  heap_code_statistics_buffer_ = pointer;
+inline void Environment::set_heap_code_statistics_buffer(
+    std::shared_ptr<v8::BackingStore> backing_store) {
+  CHECK(!heap_code_statistics_buffer_);  // Should be set only once.
+  heap_code_statistics_buffer_ = std::move(backing_store);
 }
 
 inline char* Environment::http_parser_buffer() const {

diff --git a/src/env.cc b/src/env.cc
@@ -413,10 +413,7 @@ Environment::~Environment() {
     tracing_controller->RemoveTraceStateObserver(trace_state_observer_.get());
   }
 
-  delete[] heap_statistics_buffer_;
-  delete[] heap_space_statistics_buffer_;
   delete[] http_parser_buffer_;
-  delete[] heap_code_statistics_buffer_;
 
   TRACE_EVENT_NESTABLE_ASYNC_END0(
     TRACING_CATEGORY_NODE1(environment), "Environment", this);

diff --git a/src/env.h b/src/env.h
@@ -1020,13 +1020,16 @@ class Environment : public MemoryRetainer {
       package_json_cache;
 
   inline double* heap_statistics_buffer() const;
-  inline void set_heap_statistics_buffer(double* pointer);
+  inline void set_heap_statistics_buffer(
+      std::shared_ptr<v8::BackingStore> backing_store);
 
   inline double* heap_space_statistics_buffer() const;
-  inline void set_heap_space_statistics_buffer(double* pointer);
+  inline void set_heap_space_statistics_buffer(
+      std::shared_ptr<v8::BackingStore> backing_store);
 
   inline double* heap_code_statistics_buffer() const;
-  inline void set_heap_code_statistics_buffer(double* pointer);
+  inline void set_heap_code_statistics_buffer(
+      std::shared_ptr<v8::BackingStore> backing_store);
 
   inline char* http_parser_buffer() const;
   inline void set_http_parser_buffer(char* buffer);
@@ -1365,9 +1368,9 @@ class Environment : public MemoryRetainer {
   int handle_cleanup_waiting_ = 0;
   int request_waiting_ = 0;
 
-  double* heap_statistics_buffer_ = nullptr;
-  double* heap_space_statistics_buffer_ = nullptr;
-  double* heap_code_statistics_buffer_ = nullptr;
+  std::shared_ptr<v8::BackingStore> heap_statistics_buffer_;
+  std::shared_ptr<v8::BackingStore> heap_space_statistics_buffer_;
+  std::shared_ptr<v8::BackingStore> heap_code_statistics_buffer_;
 
   char* http_parser_buffer_ = nullptr;
   bool http_parser_buffer_in_use_ = false;

diff --git a/src/node_v8.cc b/src/node_v8.cc
@@ -158,23 +158,12 @@ void Initialize(Local<Object> target,
                  "updateHeapStatisticsArrayBuffer",
                  UpdateHeapStatisticsArrayBuffer);
 
-  env->set_heap_statistics_buffer(new double[kHeapStatisticsPropertiesCount]);
-
   const size_t heap_statistics_buffer_byte_length =
       sizeof(*env->heap_statistics_buffer()) * kHeapStatisticsPropertiesCount;
 
-  std::unique_ptr<BackingStore> heap_statistics_backing =
-      ArrayBuffer::NewBackingStore(env->heap_statistics_buffer(),
-                                   heap_statistics_buffer_byte_length,
-                                   [](void*, size_t, void*){},
-                                   nullptr);
   Local<ArrayBuffer> heap_statistics_ab =
-      ArrayBuffer::New(env->isolate(),
-                       std::move(heap_statistics_backing));
-  // TODO(thangktran): drop this check when V8 is pumped to 8.0 .
-  if (!heap_statistics_ab->IsExternal())
-    heap_statistics_ab->Externalize(
-        heap_statistics_ab->GetBackingStore());
+      ArrayBuffer::New(env->isolate(), heap_statistics_buffer_byte_length);
+  env->set_heap_statistics_buffer(heap_statistics_ab->GetBackingStore());
   target->Set(env->context(),
               FIXED_ONE_BYTE_STRING(env->isolate(),
                                     "heapStatisticsArrayBuffer"),
@@ -193,25 +182,15 @@ void Initialize(Local<Object> target,
                  "updateHeapCodeStatisticsArrayBuffer",
                  UpdateHeapCodeStatisticsArrayBuffer);
 
-  env->set_heap_code_statistics_buffer(
-    new double[kHeapCodeStatisticsPropertiesCount]);
-
   const size_t heap_code_statistics_buffer_byte_length =
       sizeof(*env->heap_code_statistics_buffer())
       * kHeapCodeStatisticsPropertiesCount;
 
-  std::unique_ptr<BackingStore> heap_code_statistics_backing =
-      ArrayBuffer::NewBackingStore(env->heap_code_statistics_buffer(),
-                                   heap_code_statistics_buffer_byte_length,
-                                   [](void*, size_t, void*){},
-                                   nullptr);
   Local<ArrayBuffer> heap_code_statistics_ab =
       ArrayBuffer::New(env->isolate(),
-                       std::move(heap_code_statistics_backing));
-  // TODO(thangktran): drop this check when V8 is pumped to 8.0 .
-  if (!heap_code_statistics_ab->IsExternal())
-    heap_code_statistics_ab->Externalize(
-        heap_code_statistics_ab->GetBackingStore());
+                       heap_code_statistics_buffer_byte_length);
+  env->set_heap_code_statistics_buffer(
+      heap_code_statistics_ab->GetBackingStore());
   target->Set(env->context(),
               FIXED_ONE_BYTE_STRING(env->isolate(),
                                     "heapCodeStatisticsArrayBuffer"),
@@ -257,26 +236,16 @@ void Initialize(Local<Object> target,
                  "updateHeapSpaceStatisticsArrayBuffer",
                  UpdateHeapSpaceStatisticsBuffer);
 
-  env->set_heap_space_statistics_buffer(
-    new double[kHeapSpaceStatisticsPropertiesCount * number_of_heap_spaces]);
-
   const size_t heap_space_statistics_buffer_byte_length =
       sizeof(*env->heap_space_statistics_buffer()) *
       kHeapSpaceStatisticsPropertiesCount *
       number_of_heap_spaces;
 
-  std::unique_ptr<BackingStore> heap_space_statistics_backing =
-      ArrayBuffer::NewBackingStore(env->heap_space_statistics_buffer(),
-                                   heap_space_statistics_buffer_byte_length,
-                                   [](void*, size_t, void*){},
-                                   nullptr);
   Local<ArrayBuffer> heap_space_statistics_ab =
       ArrayBuffer::New(env->isolate(),
-                               std::move(heap_space_statistics_backing));
-  // TODO(thangktran): drop this check when V8 is pumped to 8.0 .
-  if (!heap_space_statistics_ab->IsExternal())
-    heap_space_statistics_ab->Externalize(
-        heap_space_statistics_ab->GetBackingStore());
+                       heap_space_statistics_buffer_byte_length);
+  env->set_heap_space_statistics_buffer(
+      heap_space_statistics_ab->GetBackingStore());
   target->Set(env->context(),
               FIXED_ONE_BYTE_STRING(env->isolate(),
                                     "heapSpaceStatisticsArrayBuffer"),