fixup! squash! src: retrieve binding data from the context

Author: addaleax

src/README.md

@@ -400,16 +400,22 @@ NODE_MODULE_CONTEXT_AWARE_INTERNAL(cares_wrap, Initialize)
 
 Some internal bindings, such as the HTTP parser, maintain internal state that
 only affects that particular binding. In that case, one common way to store
-that state is through the use of `Environment::BindingScope`, which gives all
-new functions created within it access to an object for storing such state.
+that state is through the use of `Environment::AddBindingData`, which gives
+binding functions access to an object for storing such state.
 That object is always a [`BaseObject`][].
 
+Its class needs to have a static `binding_data_name` field that based on a
+constant string, in order to disambiguate it from other classes of this type,
+and which could e.g. match the binding’s name.
+
 ```c++
 // In the HTTP parser source code file:
 class BindingData : public BaseObject {
  public:
   BindingData(Environment* env, Local<Object> obj) : BaseObject(env, obj) {}
 
+  static constexpr FastStringKey binding_data_name { "http_parser" };
+
   std::vector<char> parser_buffer;
   bool parser_buffer_in_use = false;
 
@@ -422,18 +428,17 @@ static void New(const FunctionCallbackInfo<Value>& args) {
   new Parser(binding_data, args.This());
 }
 
-// ... because the initialization function told the Environment to use this
-// BindingData class for all functions created by it:
+// ... because the initialization function told the Environment to store the
+// BindingData object:
 void InitializeHttpParser(Local<Object> target,
                           Local<Value> unused,
                           Local<Context> context,
                           void* priv) {
   Environment* env = Environment::GetCurrent(context);
-  Environment::BindingScope<BindingData> binding_scope(context, target);
-  if (!binding_scope) return;
-  BindingData* binding_data = binding_scope.data;
+  BindingData* const binding_data =
+      env->AddBindingData<BindingData>(context, target);
+  if (binding_data == nullptr) return;
 
-  // Created within the Environment::BindingScope
   Local<FunctionTemplate> t = env->NewFunctionTemplate(Parser::New);
   ...
 }

src/env-inl.h

@@ -333,58 +333,43 @@ inline Environment* Environment::GetCurrent(
 
 template <typename T, typename U>
 inline T* Environment::GetBindingData(const v8::PropertyCallbackInfo<U>& info) {
-  return GetBindingData<T>(info.GetIsolate()->GetCurrentContext(), info.Data());
+  return GetBindingData<T>(info.GetIsolate()->GetCurrentContext());
 }
 
 template <typename T>
 inline T* Environment::GetBindingData(
     const v8::FunctionCallbackInfo<v8::Value>& info) {
-  return GetBindingData<T>(info.GetIsolate()->GetCurrentContext(), info.Data());
+  return GetBindingData<T>(info.GetIsolate()->GetCurrentContext());
 }
 
 template <typename T>
-inline T* Environment::GetBindingData(v8::Local<v8::Context> context,
-                                      v8::Local<v8::Value> val) {
-  DCHECK(val->IsUint32());
-  uint32_t index = val.As<v8::Uint32>()->Value();
-  BindingDataList* list = static_cast<BindingDataList*>(
+inline T* Environment::GetBindingData(v8::Local<v8::Context> context) {
+  BindingDataStore* list = static_cast<BindingDataStore*>(
       context->GetAlignedPointerFromEmbedderData(
           ContextEmbedderIndex::kBindingListIndex));
   DCHECK_NOT_NULL(list);
-  DCHECK_GT(list->size(), index);
-  T* result = static_cast<T*>(list->at(index).get());
+  auto it = list->find(T::binding_data_name);
+  DCHECK_NE(it, list->end());
+  T* result = static_cast<T*>(it->second.get());
   DCHECK_NOT_NULL(result);
   DCHECK_EQ(result->env(), GetCurrent(context));
   return result;
 }
 
 template <typename T>
-inline std::pair<T*, uint32_t> Environment::NewBindingData(
+inline T* Environment::AddBindingData(
     v8::Local<v8::Context> context,
     v8::Local<v8::Object> target) {
   DCHECK_EQ(GetCurrent(context), this);
   // This won't compile if T is not a BaseObject subclass.
   BaseObjectPtr<T> item = MakeDetachedBaseObject<T>(this, target);
-  BindingDataList* list = static_cast<BindingDataList*>(
+  BindingDataStore* list = static_cast<BindingDataStore*>(
       context->GetAlignedPointerFromEmbedderData(
           ContextEmbedderIndex::kBindingListIndex));
   DCHECK_NOT_NULL(list);
-  size_t index = list->size();
-  list->emplace_back(item);
-  return std::make_pair(item.get(), index);
-}
-
-template <typename T>
-Environment::BindingScope<T>::BindingScope(v8::Local<v8::Context> context,
-                                           v8::Local<v8::Object> target)
-    : env(Environment::GetCurrent(context)) {
-  std::tie(data, env->current_callback_data_) =
-      env->NewBindingData<T>(context, target);
-}
-
-template <typename T>
-Environment::BindingScope<T>::~BindingScope() {
-  env->current_callback_data_ = env->default_callback_data_;
+  auto result = list->emplace(T::binding_data_name, item);
+  CHECK(result.second);
+  return item.get();
 }
 
 inline Environment* Environment::GetThreadLocalEnv() {
@@ -1103,8 +1088,7 @@ inline v8::Local<v8::FunctionTemplate>
                                      v8::Local<v8::Signature> signature,
                                      v8::ConstructorBehavior behavior,
                                      v8::SideEffectType side_effect_type) {
-  v8::Local<v8::Value> external = current_callback_data();
-  return v8::FunctionTemplate::New(isolate(), callback, external,
+  return v8::FunctionTemplate::New(isolate(), callback, v8::Local<v8::Value>(),
                                    signature, 0, behavior, side_effect_type);
 }
 
@@ -1300,14 +1284,6 @@ v8::Local<v8::Context> Environment::context() const {
   return PersistentToLocal::Strong(context_);
 }
 
-v8::Local<v8::Value> Environment::as_callback_data() const {
-  return v8::Integer::NewFromUnsigned(isolate(), default_callback_data_);
-}
-
-v8::Local<v8::Value> Environment::current_callback_data() const {
-  return v8::Integer::NewFromUnsigned(isolate(), current_callback_data_);
-}
-
 }  // namespace node
 
 // These two files depend on each other. Including base_object-inl.h after this

src/env.cc

@@ -261,16 +261,6 @@ void TrackingTraceStateObserver::UpdateTraceCategoryState() {
   USE(cb->Call(env_->context(), Undefined(isolate), arraysize(args), args));
 }
 
-class NoBindingData : public BaseObject {
- public:
-  NoBindingData(Environment* env, Local<Object> obj)
-      : BaseObject(env, obj) {}
-
-  SET_NO_MEMORY_INFO()
-  SET_MEMORY_INFO_NAME(NoBindingData)
-  SET_SELF_SIZE(NoBindingData)
-};
-
 void Environment::CreateProperties() {
   HandleScope handle_scope(isolate_);
   Local<Context> ctx = context();
@@ -282,11 +272,6 @@ void Environment::CreateProperties() {
         BaseObject::kInternalFieldCount);
 
     set_binding_data_ctor_template(templ);
-    Local<Function> ctor = templ->GetFunction(ctx).ToLocalChecked();
-    Local<Object> obj = ctor->NewInstance(ctx).ToLocalChecked();
-    uint32_t index = NewBindingData<NoBindingData>(ctx, obj).second;
-    default_callback_data_ = index;
-    current_callback_data_ = index;
   }
 
   // Store primordials setup by the per-context script in the environment.

src/env.h

@@ -864,38 +864,22 @@ class Environment : public MemoryRetainer {
 
   // Methods created using SetMethod(), SetPrototypeMethod(), etc. inside
   // this scope can access the created T* object using
-  // GetBindingData<T>(args.Data()) later.
+  // GetBindingData<T>(args) later.
   template <typename T>
-  struct BindingScope {
-    explicit inline BindingScope(v8::Local<v8::Context> context,
-                                 v8::Local<v8::Object> target);
-    inline ~BindingScope();
-
-    T* data = nullptr;
-    Environment* env;
-
-    inline operator bool() const { return data != nullptr; }
-    inline bool operator !() const { return data == nullptr; }
-  };
-
+  T* AddBindingData(v8::Local<v8::Context> context,
+                    v8::Local<v8::Object> target);
   template <typename T, typename U>
   static inline T* GetBindingData(const v8::PropertyCallbackInfo<U>& info);
   template <typename T>
   static inline T* GetBindingData(
       const v8::FunctionCallbackInfo<v8::Value>& info);
-
-  // Unwrap a subclass T object from a v8::Value which needs to be an
-  // v8::Uint32
-  template <typename T>
-  static inline T* GetBindingData(v8::Local<v8::Context> context,
-                                  v8::Local<v8::Value> val);
-  // Create a BindingData of subclass T, put it into the context binding list,
-  // return the index as an integer
   template <typename T>
-  inline std::pair<T*, uint32_t> NewBindingData(v8::Local<v8::Context> context,
-                                                v8::Local<v8::Object> target);
+  static inline T* GetBindingData(v8::Local<v8::Context> context);
 
-  typedef std::vector<BaseObjectPtr<BaseObject>> BindingDataList;
+  typedef std::unordered_map<
+      FastStringKey,
+      BaseObjectPtr<BaseObject>,
+      FastStringKey::Hash> BindingDataStore;
 
   static uv_key_t thread_local_env;
   static inline Environment* GetThreadLocalEnv();
@@ -1146,8 +1130,6 @@ class Environment : public MemoryRetainer {
 #undef V
 
   inline v8::Local<v8::Context> context() const;
-  inline v8::Local<v8::Value> as_callback_data() const;
-  inline v8::Local<v8::Value> current_callback_data() const;
 
 #if HAVE_INSPECTOR
   inline inspector::Agent* inspector_agent() const {
@@ -1443,7 +1425,7 @@ class Environment : public MemoryRetainer {
   void RequestInterruptFromV8();
   static void CheckImmediate(uv_check_t* handle);
 
-  BindingDataList bindings_;
+  BindingDataStore bindings_;
 
   // Use an unordered_set, so that we have efficient insertion and removal.
   std::unordered_set<CleanupHookCallback,
@@ -1469,9 +1451,6 @@ class Environment : public MemoryRetainer {
 
   v8::Global<v8::Context> context_;
 
-  uint32_t default_callback_data_;
-  uint32_t current_callback_data_;
-
   // Keeps the main script source alive is one was passed to LoadEnvironment().
   // We should probably find a way to just use plain `v8::String`s created from
   // the source passed to LoadEnvironment() directly instead.

src/fs_event_wrap.cc

@@ -108,7 +108,7 @@ void FSEventWrap::Initialize(Local<Object> target,
   Local<FunctionTemplate> get_initialized_templ =
       FunctionTemplate::New(env->isolate(),
                             GetInitialized,
-                            env->current_callback_data(),
+                            Local<Value>(),
                             Signature::New(env->isolate(), t));
 
   t->PrototypeTemplate()->SetAccessorProperty(

src/node.cc

@@ -331,7 +331,7 @@ MaybeLocal<Value> Environment::BootstrapNode() {
 
   Local<String> env_string = FIXED_ONE_BYTE_STRING(isolate_, "env");
   Local<Object> env_var_proxy;
-  if (!CreateEnvVarProxy(context(), isolate_, current_callback_data())
+  if (!CreateEnvVarProxy(context(), isolate_, Local<Value>())
            .ToLocal(&env_var_proxy) ||
       process_object()->Set(context(), env_string, env_var_proxy).IsNothing()) {
     return MaybeLocal<Value>();

src/node_crypto.cc

@@ -505,7 +505,7 @@ void SecureContext::Initialize(Environment* env, Local<Object> target) {
   Local<FunctionTemplate> ctx_getter_templ =
       FunctionTemplate::New(env->isolate(),
                             CtxGetter,
-                            env->current_callback_data(),
+                            Local<Value>(),
                             Signature::New(env->isolate(), t));
 
 
@@ -5103,7 +5103,7 @@ void DiffieHellman::Initialize(Environment* env, Local<Object> target) {
     Local<FunctionTemplate> verify_error_getter_templ =
         FunctionTemplate::New(env->isolate(),
                               DiffieHellman::VerifyErrorGetter,
-                              env->current_callback_data(),
+                              Local<Value>(),
                               Signature::New(env->isolate(), t),
                               /* length */ 0,
                               ConstructorBehavior::kThrow,

src/node_file.cc

@@ -2303,15 +2303,18 @@ void BindingData::MemoryInfo(MemoryTracker* tracker) const {
                       file_handle_read_wrap_freelist);
 }
 
+// TODO(addaleax): Remove once we're on C++17.
+constexpr FastStringKey BindingData::binding_data_name;
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
                 void* priv) {
   Environment* env = Environment::GetCurrent(context);
   Isolate* isolate = env->isolate();
-  Environment::BindingScope<BindingData> binding_scope(context, target);
-  if (!binding_scope) return;
-  BindingData* binding_data = binding_scope.data;
+  BindingData* const binding_data =
+      env->AddBindingData<BindingData>(context, target);
+  if (binding_data == nullptr) return;
 
   env->SetMethod(target, "access", Access);
   env->SetMethod(target, "close", Close);

src/node_file.h

@@ -26,6 +26,8 @@ class BindingData : public BaseObject {
   std::vector<BaseObjectPtr<FileHandleReadWrap>>
       file_handle_read_wrap_freelist;
 
+  static constexpr FastStringKey binding_data_name { "fs" };
+
   void MemoryInfo(MemoryTracker* tracker) const override;
   SET_SELF_SIZE(BindingData)
   SET_MEMORY_INFO_NAME(BindingData)

src/node_http2.cc

@@ -2941,6 +2941,9 @@ void Http2State::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackField("root_buffer", root_buffer);
 }
 
+// TODO(addaleax): Remove once we're on C++17.
+constexpr FastStringKey Http2State::binding_data_name;
+
 // Set up the process.binding('http2') binding.
 void Initialize(Local<Object> target,
                 Local<Value> unused,
@@ -2950,9 +2953,8 @@ void Initialize(Local<Object> target,
   Isolate* isolate = env->isolate();
   HandleScope handle_scope(isolate);
 
-  Environment::BindingScope<Http2State> binding_scope(context, target);
-  if (!binding_scope) return;
-  Http2State* state = binding_scope.data;
+  Http2State* const state = env->AddBindingData<Http2State>(context, target);
+  if (state == nullptr) return;
 
 #define SET_STATE_TYPEDARRAY(name, field)             \
   target->Set(context,                                \