From 4013cf16b5ae899b6cbb074b50797f33e6b64bb0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 7 Jan 2020 14:18:56 +0100 Subject: [PATCH 1/2] src: remove uses of node::InitializeV8Platform() This requires minor changes to src/env.cc to deal with `node::tracing::AgentWriterHandle::GetTracingController()` now possibly returning a nullptr, because the cctest doesn't set one. It seems plausible to me that embedders won't set one either so that seems like an okay change to make. It avoids embedders having to track down nullptr segfaults. --- src/env.cc | 8 ++++---- src/node.cc | 3 ++- test/cctest/node_test_fixture.h | 10 +++++++--- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/env.cc b/src/env.cc index 482d2191fa2105..51c5cc7c497f23 100644 --- a/src/env.cc +++ b/src/env.cc @@ -332,8 +332,8 @@ Environment::Environment(IsolateData* isolate_data, if (tracing::AgentWriterHandle* writer = GetTracingAgentWriter()) { trace_state_observer_ = std::make_unique(this); - TracingController* tracing_controller = writer->GetTracingController(); - tracing_controller->AddTraceStateObserver(trace_state_observer_.get()); + if (TracingController* tracing_controller = writer->GetTracingController()) + tracing_controller->AddTraceStateObserver(trace_state_observer_.get()); } destroy_async_id_list_.reserve(512); @@ -409,8 +409,8 @@ Environment::~Environment() { if (trace_state_observer_) { tracing::AgentWriterHandle* writer = GetTracingAgentWriter(); CHECK_NOT_NULL(writer); - TracingController* tracing_controller = writer->GetTracingController(); - tracing_controller->RemoveTraceStateObserver(trace_state_observer_.get()); + if (TracingController* tracing_controller = writer->GetTracingController()) + tracing_controller->RemoveTraceStateObserver(trace_state_observer_.get()); } delete[] http_parser_buffer_; diff --git a/src/node.cc b/src/node.cc index e48827d0cb236a..461bdc7f6661ae 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1077,7 +1077,8 @@ InitializationResult InitializeOncePerProcess(int argc, char** argv) { V8::SetEntropySource(crypto::EntropySource); #endif // HAVE_OPENSSL - InitializeV8Platform(per_process::cli_options->v8_thread_pool_size); + per_process::v8_platform.Initialize( + per_process::cli_options->v8_thread_pool_size); V8::Initialize(); performance::performance_v8_start = PERFORMANCE_NOW(); per_process::v8_initialized = true; diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h index 44f4466356e1b7..7002f9856d833b 100644 --- a/test/cctest/node_test_fixture.h +++ b/test/cctest/node_test_fixture.h @@ -80,9 +80,13 @@ class NodeZeroIsolateTestFixture : public ::testing::Test { tracing_agent = std::make_unique(); node::tracing::TraceEventHelper::SetAgent(tracing_agent.get()); + node::tracing::TracingController* tracing_controller = + tracing_agent->GetTracingController(); CHECK_EQ(0, uv_loop_init(¤t_loop)); - platform.reset(static_cast( - node::InitializeV8Platform(4))); + static constexpr int kV8ThreadPoolSize = 4; + platform.reset( + new node::NodePlatform(kV8ThreadPoolSize, tracing_controller)); + v8::V8::InitializePlatform(platform.get()); v8::V8::Initialize(); } @@ -108,7 +112,7 @@ class NodeTestFixture : public NodeZeroIsolateTestFixture { void SetUp() override { NodeZeroIsolateTestFixture::SetUp(); - isolate_ = NewIsolate(allocator.get(), ¤t_loop); + isolate_ = NewIsolate(allocator.get(), ¤t_loop, platform.get()); CHECK_NOT_NULL(isolate_); isolate_->Enter(); } From 3e04100be4730bea94d6c56693be597d3b555f06 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 7 Jan 2020 14:18:56 +0100 Subject: [PATCH 2/2] src: remove node::InitializeV8Platform() This API method was introduced in commit 90ae4bd0c9 ("src: add InitializeV8Platform function") from July 2018 but wasn't properly exported and therefore not usable on Windows or with shared library builds. The motivation from the commit log is mainly about making it easier to wire up the cctests and there are better ways to do that. Refs: https://github.com/nodejs/node/pull/31217 --- src/api/environment.cc | 5 ----- src/node.h | 1 - 2 files changed, 6 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 3a7e06d8d0e9bc..4999349d282de1 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -360,11 +360,6 @@ MultiIsolatePlatform* CreatePlatform( return new NodePlatform(thread_pool_size, tracing_controller); } -MultiIsolatePlatform* InitializeV8Platform(int thread_pool_size) { - per_process::v8_platform.Initialize(thread_pool_size); - return per_process::v8_platform.Platform(); -} - void FreePlatform(MultiIsolatePlatform* platform) { delete platform; } diff --git a/src/node.h b/src/node.h index d43693c03389ef..00b0c0ea7e4541 100644 --- a/src/node.h +++ b/src/node.h @@ -401,7 +401,6 @@ NODE_EXTERN MultiIsolatePlatform* GetMainThreadMultiIsolatePlatform(); NODE_EXTERN MultiIsolatePlatform* CreatePlatform( int thread_pool_size, node::tracing::TracingController* tracing_controller); -MultiIsolatePlatform* InitializeV8Platform(int thread_pool_size); NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform); NODE_EXTERN void EmitBeforeExit(Environment* env);