Skip to content
Permalink
Browse files

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>
  • Loading branch information
addaleax authored and gabrielschulhof committed Dec 13, 2019
1 parent d502b83 commit d06efafe6b5885e14441409a21eab810cdae754b
Showing with 34 additions and 62 deletions.
  1. +17 −14 src/env-inl.h
  2. +0 −3 src/env.cc
  3. +9 −6 src/env.h
  4. +8 −39 src/node_v8.cc
@@ -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 {
@@ -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);
@@ -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;
@@ -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"),

0 comments on commit d06efaf

Please sign in to comment.
You can’t perform that action at this time.