From 0301ce9f55d84c9a828b9e8571ab8ca0bd85b51f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 14:29:22 +0200 Subject: [PATCH 1/4] src: move IsolateData out of Environment A follow-up commit is going to make IsolateData creation explicit. In order for that to work, it needs to move out of Environment. PR-URL: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- src/env-inl.h | 20 ++++++------ src/env.h | 85 +++++++++++++++++++++++++-------------------------- 2 files changed, 50 insertions(+), 55 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 97e1ba8f764ac8..a8a7d0255211b0 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -15,13 +15,12 @@ namespace node { -inline Environment::IsolateData* Environment::IsolateData::Get( - v8::Isolate* isolate) { +inline IsolateData* IsolateData::Get(v8::Isolate* isolate) { return static_cast(isolate->GetData(kIsolateSlot)); } -inline Environment::IsolateData* Environment::IsolateData::GetOrCreate( - v8::Isolate* isolate, uv_loop_t* loop) { +inline IsolateData* IsolateData::GetOrCreate(v8::Isolate* isolate, + uv_loop_t* loop) { IsolateData* isolate_data = Get(isolate); if (isolate_data == nullptr) { isolate_data = new IsolateData(isolate, loop); @@ -31,7 +30,7 @@ inline Environment::IsolateData* Environment::IsolateData::GetOrCreate( return isolate_data; } -inline void Environment::IsolateData::Put() { +inline void IsolateData::Put() { if (--ref_count_ == 0) { isolate()->SetData(kIsolateSlot, nullptr); delete this; @@ -47,8 +46,7 @@ inline void Environment::IsolateData::Put() { // // One byte because our strings are ASCII and we can safely skip V8's UTF-8 // decoding step. It's a one-time cost, but why pay it when you don't have to? -inline Environment::IsolateData::IsolateData(v8::Isolate* isolate, - uv_loop_t* loop) +inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop) : event_loop_(loop), isolate_(isolate), #define V(PropertyName, StringValue) \ @@ -75,11 +73,11 @@ inline Environment::IsolateData::IsolateData(v8::Isolate* isolate, #undef V ref_count_(0) {} -inline uv_loop_t* Environment::IsolateData::event_loop() const { +inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } -inline v8::Isolate* Environment::IsolateData::isolate() const { +inline v8::Isolate* IsolateData::isolate() const { return isolate_; } @@ -432,7 +430,7 @@ inline ares_task_list* Environment::cares_task_list() { return &cares_task_list_; } -inline Environment::IsolateData* Environment::isolate_data() const { +inline IsolateData* Environment::isolate_data() const { return isolate_data_; } @@ -543,7 +541,7 @@ inline v8::Local Environment::NewInternalFieldObject() { #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ inline \ - v8::Local Environment::IsolateData::PropertyName() const { \ + v8::Local IsolateData::PropertyName() const { \ /* Strings are immutable so casting away const-ness here is okay. */ \ return const_cast(this)->PropertyName ## _.Get(isolate()); \ } diff --git a/src/env.h b/src/env.h index 4c310c8831fcf8..bbbd3f4b36e68d 100644 --- a/src/env.h +++ b/src/env.h @@ -302,6 +302,47 @@ struct ares_task_t { RB_HEAD(ares_task_list, ares_task_t); +class IsolateData { + public: + static inline IsolateData* GetOrCreate(v8::Isolate* isolate, uv_loop_t* loop); + inline void Put(); + inline uv_loop_t* event_loop() const; + +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ + inline v8::Local PropertyName() const; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) +#undef V +#undef VS +#undef VP + + private: + static const int kIsolateSlot = NODE_ISOLATE_SLOT; + + inline static IsolateData* Get(v8::Isolate* isolate); + inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop); + inline v8::Isolate* isolate() const; + + uv_loop_t* const event_loop_; + v8::Isolate* const isolate_; + +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ + v8::Eternal PropertyName ## _; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) +#undef V +#undef VS +#undef VP + + unsigned int ref_count_; + + DISALLOW_COPY_AND_ASSIGN(IsolateData); +}; + class Environment { public: class AsyncHooks { @@ -568,9 +609,6 @@ class Environment { static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX; private: - static const int kIsolateSlot = NODE_ISOLATE_SLOT; - - class IsolateData; inline Environment(v8::Local context, uv_loop_t* loop); inline ~Environment(); inline IsolateData* isolate_data() const; @@ -615,47 +653,6 @@ class Environment { ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V - // Per-thread, reference-counted singleton. - class IsolateData { - public: - static inline IsolateData* GetOrCreate(v8::Isolate* isolate, - uv_loop_t* loop); - inline void Put(); - inline uv_loop_t* event_loop() const; - -#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) -#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) -#define V(TypeName, PropertyName, StringValue) \ - inline v8::Local PropertyName() const; - PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) - PER_ISOLATE_STRING_PROPERTIES(VS) -#undef V -#undef VS -#undef VP - - private: - inline static IsolateData* Get(v8::Isolate* isolate); - inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop); - inline v8::Isolate* isolate() const; - - uv_loop_t* const event_loop_; - v8::Isolate* const isolate_; - -#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) -#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) -#define V(TypeName, PropertyName, StringValue) \ - v8::Eternal PropertyName ## _; - PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) - PER_ISOLATE_STRING_PROPERTIES(VS) -#undef V -#undef VS -#undef VP - - unsigned int ref_count_; - - DISALLOW_COPY_AND_ASSIGN(IsolateData); - }; - DISALLOW_COPY_AND_ASSIGN(Environment); }; From c3cd453cbae84ff6d639a2b8d7776a15b3e3431b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 16:42:52 +0200 Subject: [PATCH 2/4] src: make IsolateData creation explicit Make it easier to reason about the lifetime and the ownership of the IsolateData instance by making its creation explicit and by removing reference counting logic. The creator of the Environment is now responsible for passing in the IsolateData instance and for keeping it alive as long as the Environment is alive. PR-URL: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- src/debug-agent.cc | 10 +++++-- src/env-inl.h | 54 +++++++++--------------------------- src/env.h | 23 +++++----------- src/node.cc | 69 +++++++++++++++++++--------------------------- src/node.h | 20 +++++--------- 5 files changed, 63 insertions(+), 113 deletions(-) diff --git a/src/debug-agent.cc b/src/debug-agent.cc index e4e0ea061bd30f..78eacf49ab2fc8 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -172,9 +172,15 @@ void Agent::WorkerRun() { Local context = Context::New(isolate); Context::Scope context_scope(context); + + // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences + // a nullptr when a context hasn't been entered first. The symbol registry + // is a lazily created JS object but object instantiation does not work + // without a context. + IsolateData isolate_data(isolate, &child_loop_); + Environment* env = CreateEnvironment( - isolate, - &child_loop_, + &isolate_data, context, arraysize(argv), argv, diff --git a/src/env-inl.h b/src/env-inl.h index a8a7d0255211b0..cf62ca7eb481db 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -15,28 +15,6 @@ namespace node { -inline IsolateData* IsolateData::Get(v8::Isolate* isolate) { - return static_cast(isolate->GetData(kIsolateSlot)); -} - -inline IsolateData* IsolateData::GetOrCreate(v8::Isolate* isolate, - uv_loop_t* loop) { - IsolateData* isolate_data = Get(isolate); - if (isolate_data == nullptr) { - isolate_data = new IsolateData(isolate, loop); - isolate->SetData(kIsolateSlot, isolate_data); - } - isolate_data->ref_count_ += 1; - return isolate_data; -} - -inline void IsolateData::Put() { - if (--ref_count_ == 0) { - isolate()->SetData(kIsolateSlot, nullptr); - delete this; - } -} - // Create string properties as internalized one byte strings. // // Internalized because it makes property lookups a little faster and because @@ -46,9 +24,8 @@ inline void IsolateData::Put() { // // One byte because our strings are ASCII and we can safely skip V8's UTF-8 // decoding step. It's a one-time cost, but why pay it when you don't have to? -inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop) - : event_loop_(loop), - isolate_(isolate), +inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop) + : #define V(PropertyName, StringValue) \ PropertyName ## _( \ isolate, \ @@ -71,16 +48,12 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* loop) sizeof(StringValue) - 1).ToLocalChecked()), PER_ISOLATE_STRING_PROPERTIES(V) #undef V - ref_count_(0) {} + isolate_(isolate), event_loop_(event_loop) {} inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } -inline v8::Isolate* IsolateData::isolate() const { - return isolate_; -} - inline Environment::AsyncHooks::AsyncHooks() { for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0; } @@ -176,9 +149,9 @@ inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() { fields_[kNoZeroFill] = 0; } -inline Environment* Environment::New(v8::Local context, - uv_loop_t* loop) { - Environment* env = new Environment(context, loop); +inline Environment* Environment::New(IsolateData* isolate_data, + v8::Local context) { + Environment* env = new Environment(isolate_data, context); env->AssignToContext(context); return env; } @@ -212,11 +185,11 @@ inline Environment* Environment::GetCurrent( return static_cast(data.As()->Value()); } -inline Environment::Environment(v8::Local context, - uv_loop_t* loop) +inline Environment::Environment(IsolateData* isolate_data, + v8::Local context) : isolate_(context->GetIsolate()), - isolate_data_(IsolateData::GetOrCreate(context->GetIsolate(), loop)), - timer_base_(uv_now(loop)), + isolate_data_(isolate_data), + timer_base_(uv_now(isolate_data->event_loop())), using_domains_(false), printed_error_(false), trace_sync_io_(false), @@ -253,7 +226,6 @@ inline Environment::~Environment() { #define V(PropertyName, TypeName) PropertyName ## _.Reset(); ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V - isolate_data()->Put(); delete[] heap_statistics_buffer_; delete[] heap_space_statistics_buffer_; @@ -541,9 +513,9 @@ inline v8::Local Environment::NewInternalFieldObject() { #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ inline \ - v8::Local IsolateData::PropertyName() const { \ + v8::Local IsolateData::PropertyName(v8::Isolate* isolate) const { \ /* Strings are immutable so casting away const-ness here is okay. */ \ - return const_cast(this)->PropertyName ## _.Get(isolate()); \ + return const_cast(this)->PropertyName ## _.Get(isolate); \ } PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) PER_ISOLATE_STRING_PROPERTIES(VS) @@ -555,7 +527,7 @@ inline v8::Local Environment::NewInternalFieldObject() { #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ inline v8::Local Environment::PropertyName() const { \ - return isolate_data()->PropertyName(); \ + return isolate_data()->PropertyName(isolate()); \ } PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) PER_ISOLATE_STRING_PROPERTIES(VS) diff --git a/src/env.h b/src/env.h index bbbd3f4b36e68d..a6aea815fcd75a 100644 --- a/src/env.h +++ b/src/env.h @@ -304,14 +304,13 @@ RB_HEAD(ares_task_list, ares_task_t); class IsolateData { public: - static inline IsolateData* GetOrCreate(v8::Isolate* isolate, uv_loop_t* loop); - inline void Put(); + inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop); inline uv_loop_t* event_loop() const; #define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ - inline v8::Local PropertyName() const; + inline v8::Local PropertyName(v8::Isolate* isolate) const; PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) PER_ISOLATE_STRING_PROPERTIES(VS) #undef V @@ -319,15 +318,6 @@ class IsolateData { #undef VP private: - static const int kIsolateSlot = NODE_ISOLATE_SLOT; - - inline static IsolateData* Get(v8::Isolate* isolate); - inline explicit IsolateData(v8::Isolate* isolate, uv_loop_t* loop); - inline v8::Isolate* isolate() const; - - uv_loop_t* const event_loop_; - v8::Isolate* const isolate_; - #define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) #define V(TypeName, PropertyName, StringValue) \ @@ -338,7 +328,8 @@ class IsolateData { #undef VS #undef VP - unsigned int ref_count_; + v8::Isolate* const isolate_; + uv_loop_t* const event_loop_; DISALLOW_COPY_AND_ASSIGN(IsolateData); }; @@ -474,8 +465,8 @@ class Environment { const v8::PropertyCallbackInfo& info); // See CreateEnvironment() in src/node.cc. - static inline Environment* New(v8::Local context, - uv_loop_t* loop); + static inline Environment* New(IsolateData* isolate_data, + v8::Local context); inline void CleanupHandles(); inline void Dispose(); @@ -609,7 +600,7 @@ class Environment { static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX; private: - inline Environment(v8::Local context, uv_loop_t* loop); + inline Environment(IsolateData* isolate_data, v8::Local context); inline ~Environment(); inline IsolateData* isolate_data() const; diff --git a/src/node.cc b/src/node.cc index 258ebb596a08ca..025c1a921a884b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4264,42 +4264,6 @@ int EmitExit(Environment* env) { } -// Just a convenience method -Environment* CreateEnvironment(Isolate* isolate, - Local context, - int argc, - const char* const* argv, - int exec_argc, - const char* const* exec_argv) { - Environment* env; - Context::Scope context_scope(context); - - env = CreateEnvironment(isolate, - uv_default_loop(), - context, - argc, - argv, - exec_argc, - exec_argv); - - LoadEnvironment(env); - - return env; -} - -static Environment* CreateEnvironment(Isolate* isolate, - Local context, - NodeInstanceData* instance_data) { - return CreateEnvironment(isolate, - instance_data->event_loop(), - context, - instance_data->argc(), - instance_data->argv(), - instance_data->exec_argc(), - instance_data->exec_argv()); -} - - static void HandleCloseCb(uv_handle_t* handle) { Environment* env = reinterpret_cast(handle->data); env->FinishHandleCleanup(handle); @@ -4314,17 +4278,27 @@ static void HandleCleanup(Environment* env, } -Environment* CreateEnvironment(Isolate* isolate, - uv_loop_t* loop, +IsolateData* CreateIsolateData(Isolate* isolate, uv_loop_t* loop) { + return new IsolateData(isolate, loop); +} + + +void FreeIsolateData(IsolateData* isolate_data) { + delete isolate_data; +} + + +Environment* CreateEnvironment(IsolateData* isolate_data, Local context, int argc, const char* const* argv, int exec_argc, const char* const* exec_argv) { + Isolate* isolate = context->GetIsolate(); HandleScope handle_scope(isolate); Context::Scope context_scope(context); - Environment* env = Environment::New(context, loop); + Environment* env = Environment::New(isolate_data, context); isolate->SetAutorunMicrotasks(false); @@ -4412,10 +4386,23 @@ static void StartNodeInstance(void* arg) { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); Local context = Context::New(isolate); - Environment* env = CreateEnvironment(isolate, context, instance_data); - array_buffer_allocator->set_env(env); Context::Scope context_scope(context); + // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences + // a nullptr when a context hasn't been entered first. The symbol registry + // is a lazily created JS object but object instantiation does not work + // without a context. + IsolateData isolate_data(isolate, instance_data->event_loop()); + + Environment* env = CreateEnvironment(&isolate_data, + context, + instance_data->argc(), + instance_data->argv(), + instance_data->exec_argc(), + instance_data->exec_argv()); + + array_buffer_allocator->set_env(env); + isolate->SetAbortOnUncaughtExceptionCallback( ShouldAbortOnUncaughtException); diff --git a/src/node.h b/src/node.h index 42c5ac59d7ecf2..c1c149cdc30eb9 100644 --- a/src/node.h +++ b/src/node.h @@ -190,28 +190,22 @@ NODE_EXTERN void Init(int* argc, int* exec_argc, const char*** exec_argv); +class IsolateData; class Environment; -NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate, - struct uv_loop_s* loop, - v8::Local context, - int argc, - const char* const* argv, - int exec_argc, - const char* const* exec_argv); -NODE_EXTERN void LoadEnvironment(Environment* env); -NODE_EXTERN void FreeEnvironment(Environment* env); +NODE_EXTERN IsolateData* CreateIsolateData(v8::Isolate* isolate, + struct uv_loop_s* loop); +NODE_EXTERN void FreeIsolateData(IsolateData* isolate_data); -// NOTE: Calling this is the same as calling -// CreateEnvironment() + LoadEnvironment() from above. -// `uv_default_loop()` will be passed as `loop`. -NODE_EXTERN Environment* CreateEnvironment(v8::Isolate* isolate, +NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data, v8::Local context, int argc, const char* const* argv, int exec_argc, const char* const* exec_argv); +NODE_EXTERN void LoadEnvironment(Environment* env); +NODE_EXTERN void FreeEnvironment(Environment* env); NODE_EXTERN void EmitBeforeExit(Environment* env); NODE_EXTERN int EmitExit(Environment* env); From 334ef4f19dc0bbdf9a9fde9f4a6e7042a6c6e2a3 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 19:58:31 +0200 Subject: [PATCH 3/4] lib,src: drop dependency on v8::Private::ForApi() Said function requires that a v8::Context has been entered first, introducing a chicken-and-egg problem when creating the first context. PR-URL: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- lib/internal/util.js | 9 ++++--- src/debug-agent.cc | 8 +----- src/env-inl.h | 6 +---- src/node.cc | 8 +----- src/node_util.cc | 38 ++++++++++++++++++++++------- test/parallel/test-util-internal.js | 38 ++++++++++++++++++----------- 6 files changed, 62 insertions(+), 45 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 9ecdf17ecda571..a0c2412dce3bdd 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -3,6 +3,9 @@ const binding = process.binding('util'); const prefix = `(${process.release.name}:${process.pid}) `; +const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol']; +const kDecoratedPrivateSymbolIndex = binding['decorated_private_symbol']; + exports.getHiddenValue = binding.getHiddenValue; exports.setHiddenValue = binding.setHiddenValue; @@ -65,14 +68,14 @@ exports._deprecate = function(fn, msg) { exports.decorateErrorStack = function decorateErrorStack(err) { if (!(exports.isError(err) && err.stack) || - exports.getHiddenValue(err, 'node:decorated') === true) + exports.getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true) return; - const arrow = exports.getHiddenValue(err, 'node:arrowMessage'); + const arrow = exports.getHiddenValue(err, kArrowMessagePrivateSymbolIndex); if (arrow) { err.stack = arrow + err.stack; - exports.setHiddenValue(err, 'node:decorated', true); + exports.setHiddenValue(err, kDecoratedPrivateSymbolIndex, true); } }; diff --git a/src/debug-agent.cc b/src/debug-agent.cc index 78eacf49ab2fc8..a8797a5bcf4887 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -169,16 +169,10 @@ void Agent::WorkerRun() { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); + IsolateData isolate_data(isolate, &child_loop_); Local context = Context::New(isolate); Context::Scope context_scope(context); - - // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences - // a nullptr when a context hasn't been entered first. The symbol registry - // is a lazily created JS object but object instantiation does not work - // without a context. - IsolateData isolate_data(isolate, &child_loop_); - Environment* env = CreateEnvironment( &isolate_data, context, diff --git a/src/env-inl.h b/src/env-inl.h index cf62ca7eb481db..9836f14da42fbc 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -29,7 +29,7 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop) #define V(PropertyName, StringValue) \ PropertyName ## _( \ isolate, \ - v8::Private::ForApi( \ + v8::Private::New( \ isolate, \ v8::String::NewFromOneByte( \ isolate, \ @@ -545,10 +545,6 @@ inline v8::Local Environment::NewInternalFieldObject() { ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V -#undef ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES -#undef PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES -#undef PER_ISOLATE_STRING_PROPERTIES - } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node.cc b/src/node.cc index 025c1a921a884b..2f64e36e5935e6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4385,15 +4385,9 @@ static void StartNodeInstance(void* arg) { Locker locker(isolate); Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); + IsolateData isolate_data(isolate, instance_data->event_loop()); Local context = Context::New(isolate); Context::Scope context_scope(context); - - // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences - // a nullptr when a context hasn't been entered first. The symbol registry - // is a lazily created JS object but object instantiation does not work - // without a context. - IsolateData isolate_data(isolate, instance_data->event_loop()); - Environment* env = CreateEnvironment(&isolate_data, context, instance_data->argc(), diff --git a/src/node_util.cc b/src/node_util.cc index 9fc94acf094426..5089188d502bf2 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -9,11 +9,11 @@ namespace util { using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; +using v8::Integer; using v8::Local; using v8::Object; using v8::Private; using v8::Proxy; -using v8::String; using v8::Value; @@ -53,18 +53,28 @@ static void GetProxyDetails(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret); } +inline Local IndexToPrivateSymbol(Environment* env, uint32_t index) { +#define V(name, _) &Environment::name, + static Local (Environment::*const methods[])() const = { + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) + }; +#undef V + CHECK_LT(index, arraysize(methods)); + return (env->*methods[index])(); +} + static void GetHiddenValue(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args[0]->IsObject()) return env->ThrowTypeError("obj must be an object"); - if (!args[1]->IsString()) - return env->ThrowTypeError("name must be a string"); + if (!args[1]->IsUint32()) + return env->ThrowTypeError("index must be an uint32"); Local obj = args[0].As(); - Local name = args[1].As(); - auto private_symbol = Private::ForApi(env->isolate(), name); + auto index = args[1]->Uint32Value(env->context()).FromJust(); + auto private_symbol = IndexToPrivateSymbol(env, index); auto maybe_value = obj->GetPrivate(env->context(), private_symbol); args.GetReturnValue().Set(maybe_value.ToLocalChecked()); @@ -76,12 +86,12 @@ static void SetHiddenValue(const FunctionCallbackInfo& args) { if (!args[0]->IsObject()) return env->ThrowTypeError("obj must be an object"); - if (!args[1]->IsString()) - return env->ThrowTypeError("name must be a string"); + if (!args[1]->IsUint32()) + return env->ThrowTypeError("index must be an uint32"); Local obj = args[0].As(); - Local name = args[1].As(); - auto private_symbol = Private::ForApi(env->isolate(), name); + auto index = args[1]->Uint32Value(env->context()).FromJust(); + auto private_symbol = IndexToPrivateSymbol(env, index); auto maybe_value = obj->SetPrivate(env->context(), private_symbol, args[2]); args.GetReturnValue().Set(maybe_value.FromJust()); @@ -97,6 +107,16 @@ void Initialize(Local target, VALUE_METHOD_MAP(V) #undef V +#define V(name, _) \ + target->Set(context, \ + FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ + Integer::NewFromUnsigned(env->isolate(), index++)); + { + uint32_t index = 0; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) + } +#undef V + env->SetMethod(target, "getHiddenValue", GetHiddenValue); env->SetMethod(target, "setHiddenValue", SetHiddenValue); env->SetMethod(target, "getProxyDetails", GetProxyDetails); diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js index 2110b1785131e9..0a6621e3ec0c81 100644 --- a/test/parallel/test-util-internal.js +++ b/test/parallel/test-util-internal.js @@ -6,15 +6,18 @@ const assert = require('assert'); const internalUtil = require('internal/util'); const spawnSync = require('child_process').spawnSync; -function getHiddenValue(obj, name) { +const binding = process.binding('util'); +const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol']; + +function getHiddenValue(obj, index) { return function() { - internalUtil.getHiddenValue(obj, name); + internalUtil.getHiddenValue(obj, index); }; } -function setHiddenValue(obj, name, val) { +function setHiddenValue(obj, index, val) { return function() { - internalUtil.setHiddenValue(obj, name, val); + internalUtil.setHiddenValue(obj, index, val); }; } @@ -23,29 +26,36 @@ assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/); assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/); assert.throws(getHiddenValue('bar', 'foo'), /obj must be an object/); assert.throws(getHiddenValue(85, 'foo'), /obj must be an object/); -assert.throws(getHiddenValue({}), /name must be a string/); -assert.throws(getHiddenValue({}, null), /name must be a string/); -assert.throws(getHiddenValue({}, []), /name must be a string/); -assert.deepStrictEqual(internalUtil.getHiddenValue({}, 'foo'), undefined); +assert.throws(getHiddenValue({}), /index must be an uint32/); +assert.throws(getHiddenValue({}, null), /index must be an uint32/); +assert.throws(getHiddenValue({}, []), /index must be an uint32/); +assert.deepStrictEqual( + internalUtil.getHiddenValue({}, kArrowMessagePrivateSymbolIndex), + undefined); assert.throws(setHiddenValue(), /obj must be an object/); assert.throws(setHiddenValue(null, 'foo'), /obj must be an object/); assert.throws(setHiddenValue(undefined, 'foo'), /obj must be an object/); assert.throws(setHiddenValue('bar', 'foo'), /obj must be an object/); assert.throws(setHiddenValue(85, 'foo'), /obj must be an object/); -assert.throws(setHiddenValue({}), /name must be a string/); -assert.throws(setHiddenValue({}, null), /name must be a string/); -assert.throws(setHiddenValue({}, []), /name must be a string/); +assert.throws(setHiddenValue({}), /index must be an uint32/); +assert.throws(setHiddenValue({}, null), /index must be an uint32/); +assert.throws(setHiddenValue({}, []), /index must be an uint32/); const obj = {}; -assert.strictEqual(internalUtil.setHiddenValue(obj, 'foo', 'bar'), true); -assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar'); +assert.strictEqual( + internalUtil.setHiddenValue(obj, kArrowMessagePrivateSymbolIndex, 'bar'), + true); +assert.strictEqual( + internalUtil.getHiddenValue(obj, kArrowMessagePrivateSymbolIndex), + 'bar'); let arrowMessage; try { require('../fixtures/syntax/bad_syntax'); } catch (err) { - arrowMessage = internalUtil.getHiddenValue(err, 'node:arrowMessage'); + arrowMessage = + internalUtil.getHiddenValue(err, kArrowMessagePrivateSymbolIndex); } assert(/bad_syntax\.js:1/.test(arrowMessage)); From 27e84ddd4e10cdb40d6e89d872277abe16830eb0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 20:58:31 +0200 Subject: [PATCH 4/4] lib,src: clean up ArrayBufferAllocator Remove the direct dependency on node::Environment (which is per-context) from node::ArrayBufferAllocator (which is per-isolate.) Contexts that want to toggle the zero fill flag, now do so through a field that is owned by ArrayBufferAllocator. Better, still not great. PR-URL: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- lib/buffer.js | 18 ++++++++---------- src/debug-agent.cc | 3 ++- src/env-inl.h | 36 ++++++++---------------------------- src/env.h | 30 +++++------------------------- src/node.cc | 17 ++++++----------- src/node_buffer.cc | 20 ++++++++------------ src/node_internals.h | 6 ++---- 7 files changed, 39 insertions(+), 91 deletions(-) diff --git a/lib/buffer.js b/lib/buffer.js index 2472390086057c..54096aa99410c1 100644 --- a/lib/buffer.js +++ b/lib/buffer.js @@ -59,18 +59,16 @@ Buffer.prototype.swap32 = function swap32() { return swap32n.apply(this); }; -const flags = bindingObj.flags; -const kNoZeroFill = 0; +// |binding.zeroFill| can be undefined when running inside an isolate where we +// do not own the ArrayBuffer allocator. Zero fill is always on in that case. +const zeroFill = bindingObj.zeroFill || [0]; function createBuffer(size, noZeroFill) { - flags[kNoZeroFill] = noZeroFill ? 1 : 0; - try { - const ui8 = new Uint8Array(size); - Object.setPrototypeOf(ui8, Buffer.prototype); - return ui8; - } finally { - flags[kNoZeroFill] = 0; - } + if (noZeroFill) + zeroFill[0] = 0; // Reset by the runtime. + const ui8 = new Uint8Array(size); + Object.setPrototypeOf(ui8, Buffer.prototype); + return ui8; } function createPool() { diff --git a/src/debug-agent.cc b/src/debug-agent.cc index a8797a5bcf4887..e5dc346dc6a354 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -169,7 +169,8 @@ void Agent::WorkerRun() { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); - IsolateData isolate_data(isolate, &child_loop_); + IsolateData isolate_data(isolate, &child_loop_, + array_buffer_allocator.zero_fill_field()); Local context = Context::New(isolate); Context::Scope context_scope(context); diff --git a/src/env-inl.h b/src/env-inl.h index 9836f14da42fbc..008e5103b16c6e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -24,7 +24,8 @@ namespace node { // // One byte because our strings are ASCII and we can safely skip V8's UTF-8 // decoding step. It's a one-time cost, but why pay it when you don't have to? -inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop) +inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop, + uint32_t* zero_fill_field) : #define V(PropertyName, StringValue) \ PropertyName ## _( \ @@ -48,12 +49,17 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop) sizeof(StringValue) - 1).ToLocalChecked()), PER_ISOLATE_STRING_PROPERTIES(V) #undef V - isolate_(isolate), event_loop_(event_loop) {} + isolate_(isolate), event_loop_(event_loop), + zero_fill_field_(zero_fill_field) {} inline uv_loop_t* IsolateData::event_loop() const { return event_loop_; } +inline uint32_t* IsolateData::zero_fill_field() const { + return zero_fill_field_; +} + inline Environment::AsyncHooks::AsyncHooks() { for (int i = 0; i < kFieldsCount; i++) fields_[i] = 0; } @@ -128,27 +134,6 @@ inline void Environment::TickInfo::set_index(uint32_t value) { fields_[kIndex] = value; } -inline Environment::ArrayBufferAllocatorInfo::ArrayBufferAllocatorInfo() { - for (int i = 0; i < kFieldsCount; ++i) - fields_[i] = 0; -} - -inline uint32_t* Environment::ArrayBufferAllocatorInfo::fields() { - return fields_; -} - -inline int Environment::ArrayBufferAllocatorInfo::fields_count() const { - return kFieldsCount; -} - -inline bool Environment::ArrayBufferAllocatorInfo::no_zero_fill() const { - return fields_[kNoZeroFill] != 0; -} - -inline void Environment::ArrayBufferAllocatorInfo::reset_fill_flag() { - fields_[kNoZeroFill] = 0; -} - inline Environment* Environment::New(IsolateData* isolate_data, v8::Local context) { Environment* env = new Environment(isolate_data, context); @@ -318,11 +303,6 @@ inline Environment::TickInfo* Environment::tick_info() { return &tick_info_; } -inline Environment::ArrayBufferAllocatorInfo* - Environment::array_buffer_allocator_info() { - return &array_buffer_allocator_info_; -} - inline uint64_t Environment::timer_base() const { return timer_base_; } diff --git a/src/env.h b/src/env.h index a6aea815fcd75a..d252e5db6ce264 100644 --- a/src/env.h +++ b/src/env.h @@ -304,8 +304,10 @@ RB_HEAD(ares_task_list, ares_task_t); class IsolateData { public: - inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop); + inline IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop, + uint32_t* zero_fill_field = nullptr); inline uv_loop_t* event_loop() const; + inline uint32_t* zero_fill_field() const; #define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) #define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) @@ -330,6 +332,7 @@ class IsolateData { v8::Isolate* const isolate_; uv_loop_t* const event_loop_; + uint32_t* const zero_fill_field_; DISALLOW_COPY_AND_ASSIGN(IsolateData); }; @@ -414,27 +417,6 @@ class Environment { DISALLOW_COPY_AND_ASSIGN(TickInfo); }; - class ArrayBufferAllocatorInfo { - public: - inline uint32_t* fields(); - inline int fields_count() const; - inline bool no_zero_fill() const; - inline void reset_fill_flag(); - - private: - friend class Environment; // So we can call the constructor. - inline ArrayBufferAllocatorInfo(); - - enum Fields { - kNoZeroFill, - kFieldsCount - }; - - uint32_t fields_[kFieldsCount]; - - DISALLOW_COPY_AND_ASSIGN(ArrayBufferAllocatorInfo); - }; - typedef void (*HandleCleanupCb)(Environment* env, uv_handle_t* handle, void* arg); @@ -497,7 +479,6 @@ class Environment { inline AsyncHooks* async_hooks(); inline DomainFlag* domain_flag(); inline TickInfo* tick_info(); - inline ArrayBufferAllocatorInfo* array_buffer_allocator_info(); inline uint64_t timer_base() const; static inline Environment* from_cares_timer_handle(uv_timer_t* handle); @@ -505,6 +486,7 @@ class Environment { inline ares_channel cares_channel(); inline ares_channel* cares_channel_ptr(); inline ares_task_list* cares_task_list(); + inline IsolateData* isolate_data() const; inline bool using_domains() const; inline void set_using_domains(bool value); @@ -602,7 +584,6 @@ class Environment { private: inline Environment(IsolateData* isolate_data, v8::Local context); inline ~Environment(); - inline IsolateData* isolate_data() const; v8::Isolate* const isolate_; IsolateData* const isolate_data_; @@ -613,7 +594,6 @@ class Environment { AsyncHooks async_hooks_; DomainFlag domain_flag_; TickInfo tick_info_; - ArrayBufferAllocatorInfo array_buffer_allocator_info_; const uint64_t timer_base_; uv_timer_t cares_timer_handle_; ares_channel cares_channel_; diff --git a/src/node.cc b/src/node.cc index 2f64e36e5935e6..3c508183c922d8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -971,11 +971,9 @@ Local WinapiErrnoException(Isolate* isolate, void* ArrayBufferAllocator::Allocate(size_t size) { - if (env_ == nullptr || - !env_->array_buffer_allocator_info()->no_zero_fill() || - zero_fill_all_buffers) + if (zero_fill_field_ || zero_fill_all_buffers) return calloc(size, 1); - env_->array_buffer_allocator_info()->reset_fill_flag(); + zero_fill_field_ = 1; return malloc(size); } @@ -4363,8 +4361,8 @@ Environment* CreateEnvironment(IsolateData* isolate_data, static void StartNodeInstance(void* arg) { NodeInstanceData* instance_data = static_cast(arg); Isolate::CreateParams params; - ArrayBufferAllocator* array_buffer_allocator = new ArrayBufferAllocator(); - params.array_buffer_allocator = array_buffer_allocator; + ArrayBufferAllocator array_buffer_allocator; + params.array_buffer_allocator = &array_buffer_allocator; #ifdef NODE_ENABLE_VTUNE_PROFILING params.code_event_handler = vTune::GetVtuneCodeEventHandler(); #endif @@ -4385,7 +4383,8 @@ static void StartNodeInstance(void* arg) { Locker locker(isolate); Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); - IsolateData isolate_data(isolate, instance_data->event_loop()); + IsolateData isolate_data(isolate, instance_data->event_loop(), + array_buffer_allocator.zero_fill_field()); Local context = Context::New(isolate); Context::Scope context_scope(context); Environment* env = CreateEnvironment(&isolate_data, @@ -4395,8 +4394,6 @@ static void StartNodeInstance(void* arg) { instance_data->exec_argc(), instance_data->exec_argv()); - array_buffer_allocator->set_env(env); - isolate->SetAbortOnUncaughtExceptionCallback( ShouldAbortOnUncaughtException); @@ -4464,7 +4461,6 @@ static void StartNodeInstance(void* arg) { __lsan_do_leak_check(); #endif - array_buffer_allocator->set_env(nullptr); env->Dispose(); env = nullptr; } @@ -4477,7 +4473,6 @@ static void StartNodeInstance(void* arg) { CHECK_NE(isolate, nullptr); isolate->Dispose(); isolate = nullptr; - delete array_buffer_allocator; } int Start(int argc, char** argv) { diff --git a/src/node_buffer.cc b/src/node_buffer.cc index bb77387e8f87b7..bb94263ccd5b64 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -1224,18 +1224,14 @@ void SetupBufferJS(const FunctionCallbackInfo& args) { env->SetMethod(proto, "copy", Copy); - CHECK(args[1]->IsObject()); - Local bObj = args[1].As(); - - uint32_t* const fields = env->array_buffer_allocator_info()->fields(); - uint32_t const fields_count = - env->array_buffer_allocator_info()->fields_count(); - - Local array_buffer = - ArrayBuffer::New(env->isolate(), fields, sizeof(*fields) * fields_count); - - bObj->Set(String::NewFromUtf8(env->isolate(), "flags"), - Uint32Array::New(array_buffer, 0, fields_count)); + if (auto zero_fill_field = env->isolate_data()->zero_fill_field()) { + CHECK(args[1]->IsObject()); + auto binding_object = args[1].As(); + auto array_buffer = ArrayBuffer::New(env->isolate(), zero_fill_field, 1); + auto name = FIXED_ONE_BYTE_STRING(env->isolate(), "zeroFill"); + auto value = Uint32Array::New(array_buffer, 0, 1); + CHECK(binding_object->Set(env->context(), name, value).FromJust()); + } } diff --git a/src/node_internals.h b/src/node_internals.h index 64134d9ab8d9b7..56606613ddfeb9 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -204,16 +204,14 @@ void ThrowUVException(v8::Isolate* isolate, class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator { public: - ArrayBufferAllocator() : env_(nullptr) { } - - inline void set_env(Environment* env) { env_ = env; } + inline uint32_t* zero_fill_field() { return &zero_fill_field_; } virtual void* Allocate(size_t size); // Defined in src/node.cc virtual void* AllocateUninitialized(size_t size) { return malloc(size); } virtual void Free(void* data, size_t) { free(data); } private: - Environment* env_; + uint32_t zero_fill_field_ = 1; // Boolean but exposed as uint32 to JS land. }; // Clear any domain and/or uncaughtException handlers to force the error's