Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v18.x] src: per-realm binding data #47174

Closed
wants to merge 1 commit into from
+205 −197
Closed

[v18.x] src: per-realm binding data #47174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ Which explains that the unregistered external reference is

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::AddBindingData`, which gives
that state is through the use of `Realm::AddBindingData`, which gives
binding functions access to an object for storing such state.
That object is always a [`BaseObject`][].

Expand All @@ -507,7 +507,7 @@ class BindingData : public BaseObject {

// Available for binding functions, e.g. the HTTP Parser constructor:
static void New(const FunctionCallbackInfo<Value>& args) {
BindingData* binding_data = Environment::GetBindingData<BindingData>(args);
BindingData* binding_data = Realm::GetBindingData<BindingData>(args);
new Parser(binding_data, args.This());
}

Expand All @@ -517,12 +517,12 @@ void InitializeHttpParser(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
Environment* env = Environment::GetCurrent(context);
Realm* realm = Realm::GetCurrent(context);
BindingData* const binding_data =
env->AddBindingData<BindingData>(context, target);
realm->AddBindingData<BindingData>(context, target);
if (binding_data == nullptr) return;

Local<FunctionTemplate> t = env->NewFunctionTemplate(Parser::New);
Local<FunctionTemplate> t = NewFunctionTemplate(realm->isolate(), Parser::New);
...
}
```
Expand Down
5 changes: 4 additions & 1 deletion src/base_object-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@
namespace node {

BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
: BaseObject(env->principal_realm(), object) {}
: BaseObject(env->principal_realm(), object) {
// TODO(legendecas): Check the shorthand is only used in the principal realm
// while allowing to create a BaseObject in a vm context.
}

// static
v8::Local<v8::FunctionTemplate> BaseObject::GetConstructorTemplate(
Expand Down
42 changes: 0 additions & 42 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,48 +197,6 @@ inline Environment* Environment::GetCurrent(
return GetCurrent(info.GetIsolate()->GetCurrentContext());
}

template <typename T, typename U>
inline T* Environment::GetBindingData(const v8::PropertyCallbackInfo<U>& info) {
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());
}

template <typename T>
inline T* Environment::GetBindingData(v8::Local<v8::Context> context) {
BindingDataStore* map = static_cast<BindingDataStore*>(
context->GetAlignedPointerFromEmbedderData(
ContextEmbedderIndex::kBindingListIndex));
DCHECK_NOT_NULL(map);
auto it = map->find(T::type_name);
if (UNLIKELY(it == map->end())) return nullptr;
T* result = static_cast<T*>(it->second.get());
DCHECK_NOT_NULL(result);
DCHECK_EQ(result->env(), GetCurrent(context));
return result;
}

template <typename T>
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);
BindingDataStore* map = static_cast<BindingDataStore*>(
context->GetAlignedPointerFromEmbedderData(
ContextEmbedderIndex::kBindingListIndex));
DCHECK_NOT_NULL(map);
auto result = map->emplace(T::type_name, item);
CHECK(result.second);
DCHECK_EQ(GetBindingData<T>(context), item.get());
return item.get();
}

inline v8::Isolate* Environment::isolate() const {
return isolate_;
}
Expand Down
4 changes: 2 additions & 2 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,8 @@ void Environment::AssignToContext(Local<v8::Context> context,
context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kRealm, realm);
// Used to retrieve bindings
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kBindingListIndex, &(this->bindings_));
ContextEmbedderIndex::kBindingDataStoreIndex,
realm->binding_data_store());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is called from node_contextify.cc like this:

env->AssignToContext(v8_context, nullptr, info);

This appears to get an invalid value in that flow. What's the expected behavior? Should the embedder data be nullptr when realm is nullptr?

Copy link
Contributor

@jkrems jkrems Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully looked into how this slot is used but I suspect the fix is:

  // Used to retrieve bindings
  context->SetAlignedPointerInEmbedderData(
      ContextEmbedderIndex::kBindingDataStoreIndex,
      realm != nullptr ? realm->binding_data_store() : nullptr);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending #48802 in case that is the fix.


// ContextifyContexts will update this to a pointer to the native object.
context->SetAlignedPointerInEmbedderData(
Expand Down Expand Up @@ -1010,7 +1011,6 @@ MaybeLocal<Value> Environment::RunSnapshotDeserializeMain() const {
void Environment::RunCleanup() {
started_cleanup_ = true;
TRACE_EVENT0(TRACING_CATEGORY_NODE1(environment), "RunCleanup");
bindings_.clear();
// Only BaseObject's cleanups are registered as per-realm cleanup hooks now.
// Defer the BaseObject cleanup after handles are cleaned up.
CleanupHandles();
Expand Down
21 changes: 0 additions & 21 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,25 +571,6 @@ class Environment : public MemoryRetainer {
static inline Environment* GetCurrent(
const v8::PropertyCallbackInfo<T>& info);

// Methods created using SetMethod(), SetPrototypeMethod(), etc. inside
// this scope can access the created T* object using
// GetBindingData<T>(args) later.
template <typename T>
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);
template <typename T>
static inline T* GetBindingData(v8::Local<v8::Context> context);

typedef std::unordered_map<
FastStringKey,
BaseObjectPtr<BaseObject>,
FastStringKey::Hash> BindingDataStore;

// Create an Environment without initializing a main Context. Use
// InitializeMainContext() to initialize a main context for it.
Environment(IsolateData* isolate_data,
Expand Down Expand Up @@ -1108,8 +1089,6 @@ class Environment : public MemoryRetainer {
void RequestInterruptFromV8();
static void CheckImmediate(uv_check_t* handle);

BindingDataStore bindings_;

CleanupQueue cleanup_queue_;
bool started_cleanup_ = false;

Expand Down
23 changes: 10 additions & 13 deletions src/node_blob.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,17 @@ void Blob::Initialize(
Local<Value> unused,
Local<Context> context,
void* priv) {
Environment* env = Environment::GetCurrent(context);
Realm* realm = Realm::GetCurrent(context);

BlobBindingData* const binding_data =
env->AddBindingData<BlobBindingData>(context, target);
realm->AddBindingData<BlobBindingData>(context, target);
if (binding_data == nullptr) return;

SetMethod(context, target, "createBlob", New);
SetMethod(context, target, "storeDataObject", StoreDataObject);
SetMethod(context, target, "getDataObject", GetDataObject);
SetMethod(context, target, "revokeDataObject", RevokeDataObject);
FixedSizeBlobCopyJob::Initialize(env, target);
FixedSizeBlobCopyJob::Initialize(realm->env(), target);
}

Local<FunctionTemplate> Blob::GetConstructorTemplate(Environment* env) {
Expand Down Expand Up @@ -236,8 +236,7 @@ std::unique_ptr<worker::TransferData> Blob::CloneForMessaging() const {

void Blob::StoreDataObject(const v8::FunctionCallbackInfo<v8::Value>& args) {
Environment* env = Environment::GetCurrent(args);
BlobBindingData* binding_data =
Environment::GetBindingData<BlobBindingData>(args);
BlobBindingData* binding_data = Realm::GetBindingData<BlobBindingData>(args);

CHECK(args[0]->IsString()); // ID key
CHECK(Blob::HasInstance(env, args[1])); // Blob
Expand All @@ -260,8 +259,7 @@ void Blob::StoreDataObject(const v8::FunctionCallbackInfo<v8::Value>& args) {
}

void Blob::RevokeDataObject(const v8::FunctionCallbackInfo<v8::Value>& args) {
BlobBindingData* binding_data =
Environment::GetBindingData<BlobBindingData>(args);
BlobBindingData* binding_data = Realm::GetBindingData<BlobBindingData>(args);

Environment* env = Environment::GetCurrent(args);
CHECK(args[0]->IsString()); // ID key
Expand All @@ -272,8 +270,7 @@ void Blob::RevokeDataObject(const v8::FunctionCallbackInfo<v8::Value>& args) {
}

void Blob::GetDataObject(const v8::FunctionCallbackInfo<v8::Value>& args) {
BlobBindingData* binding_data =
Environment::GetBindingData<BlobBindingData>(args);
BlobBindingData* binding_data = Realm::GetBindingData<BlobBindingData>(args);

Environment* env = Environment::GetCurrent(args);
CHECK(args[0]->IsString());
Expand Down Expand Up @@ -428,8 +425,8 @@ BlobBindingData::StoredDataObject::StoredDataObject(
length(length_),
type(type_) {}

BlobBindingData::BlobBindingData(Environment* env, Local<Object> wrap)
: SnapshotableObject(env, wrap, type_int) {
BlobBindingData::BlobBindingData(Realm* realm, Local<Object> wrap)
: SnapshotableObject(realm, wrap, type_int) {
MakeWeak();
}

Expand Down Expand Up @@ -465,9 +462,9 @@ void BlobBindingData::Deserialize(Local<Context> context,
InternalFieldInfoBase* info) {
DCHECK_EQ(index, BaseObject::kEmbedderType);
HandleScope scope(context->GetIsolate());
Environment* env = Environment::GetCurrent(context);
Realm* realm = Realm::GetCurrent(context);
BlobBindingData* binding =
env->AddBindingData<BlobBindingData>(context, holder);
realm->AddBindingData<BlobBindingData>(context, holder);
CHECK_NOT_NULL(binding);
}

Expand Down
2 changes: 1 addition & 1 deletion src/node_blob.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ class FixedSizeBlobCopyJob : public AsyncWrap, public ThreadPoolWork {

class BlobBindingData : public SnapshotableObject {
public:
explicit BlobBindingData(Environment* env, v8::Local<v8::Object> wrap);
explicit BlobBindingData(Realm* realm, v8::Local<v8::Object> wrap);

using InternalFieldInfo = InternalFieldInfoBase;

Expand Down
6 changes: 3 additions & 3 deletions src/node_context_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ namespace node {
#define NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX 34
#endif

#ifndef NODE_BINDING_LIST
#define NODE_BINDING_LIST_INDEX 35
#ifndef NODE_BINDING_DATA_STORE_INDEX
#define NODE_BINDING_DATA_STORE_INDEX 35
#endif

#ifndef NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX
Expand All @@ -51,7 +51,7 @@ enum ContextEmbedderIndex {
kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX,
kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX,
kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX,
kBindingListIndex = NODE_BINDING_LIST_INDEX,
kBindingDataStoreIndex = NODE_BINDING_DATA_STORE_INDEX,
kAllowCodeGenerationFromStrings =
NODE_CONTEXT_ALLOW_CODE_GENERATION_FROM_STRINGS_INDEX,
kContextifyContext = NODE_CONTEXT_CONTEXTIFY_CONTEXT_INDEX,
Expand Down
2 changes: 1 addition & 1 deletion src/node_file-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ FSReqBase* GetReqWrap(const v8::FunctionCallbackInfo<v8::Value>& args,
return Unwrap<FSReqBase>(value.As<v8::Object>());
}

BindingData* binding_data = Environment::GetBindingData<BindingData>(args);
BindingData* binding_data = Realm::GetBindingData<BindingData>(args);
Environment* env = binding_data->env();
if (value->StrictEquals(env->fs_use_promises_symbol())) {
if (use_bigint) {
Expand Down
Loading