Skip to content

src: make realm binding data store weak#47688

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

Closed
legendecas wants to merge 2 commits into from
Closed

src: make realm binding data store weak #47688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5aa1ab3
src: remove aliased buffer weak callback
legendecas May 8, 2023
952cabc
src: make realm binding data store weak
legendecas Apr 2, 2023
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
15 changes: 3 additions & 12 deletions src/aliased_buffer-inl.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -70,8 +70,8 @@ AliasedBufferBase<NativeT, V8T>::AliasedBufferBase(
count_(that.count_),
byte_offset_(that.byte_offset_),
buffer_(that.buffer_){
DCHECK(is_valid());
js_array_ = v8::Global<V8T>(that.isolate_, that.GetJSArray());
DCHECK(is_valid());
}

template <typename NativeT, typename V8T>
Expand DownExpand Up@@ -126,19 +126,10 @@ void AliasedBufferBase<NativeT, V8T>::Release(){
js_array_.Reset();
}

template <typename NativeT, typename V8T>
inlinevoid AliasedBufferBase<NativeT, V8T>::WeakCallback(
const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data){
AliasedBufferBase<NativeT, V8T>* buffer = data.GetParameter();
DCHECK(buffer->is_valid());
buffer->cleared_ = true;
buffer->js_array_.Reset();
}

template <typename NativeT, typename V8T>
inlinevoid AliasedBufferBase<NativeT, V8T>::MakeWeak(){
DCHECK(is_valid());
js_array_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
js_array_.SetWeak();
}

template <typename NativeT, typename V8T>
Expand DownExpand Up@@ -223,7 +214,7 @@ void AliasedBufferBase<NativeT, V8T>::reserve(size_t new_capacity){

template <typename NativeT, typename V8T>
inlinebool AliasedBufferBase<NativeT, V8T>::is_valid() const{
return index_ == nullptr && !cleared_;
return index_ == nullptr && !js_array_.IsEmpty();
}

template <typename NativeT, typename V8T>
Expand Down
3 changes: 0 additions & 3 deletions src/aliased_buffer.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -173,14 +173,11 @@ class AliasedBufferBase : public MemoryRetainer{

private:
inlineboolis_valid() const;
staticinlinevoidWeakCallback(
const v8::WeakCallbackInfo<AliasedBufferBase<NativeT, V8T>>& data);
v8::Isolate* isolate_ = nullptr;
size_t count_ = 0;
size_t byte_offset_ = 0;
NativeT* buffer_ = nullptr;
v8::Global<V8T> js_array_;
bool cleared_ = false;

// Deserialize data
const AliasedBufferIndex* index_ = nullptr;
Expand Down
6 changes: 6 additions & 0 deletions src/base_object-inl.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -289,6 +289,12 @@ template <typename T, typename... Args>
BaseObjectPtr<T> MakeBaseObject(Args&&... args){
return BaseObjectPtr<T>(new T(std::forward<Args>(args)...));
}
template <typename T, typename... Args>
BaseObjectWeakPtr<T> MakeWeakBaseObject(Args&&... args){
T* target = new T(std::forward<Args>(args)...);
target->MakeWeak();
return BaseObjectWeakPtr<T>(target);
}

template <typename T, typename... Args>
BaseObjectPtr<T> MakeDetachedBaseObject(Args&&... args){
Expand Down
4 changes: 4 additions & 0 deletions src/base_object.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -302,6 +302,10 @@ using BaseObjectWeakPtr = BaseObjectPtrImpl<T, true>
template <typename T, typename... Args>
inline BaseObjectPtr<T> MakeBaseObject(Args&&... args);
// Create a BaseObject instance and return a pointer to it.
// This variant makes the object a weak GC root by default.
template <typename T, typename... Args>
inline BaseObjectWeakPtr<T> MakeWeakBaseObject(Args&&... args);
// Create a BaseObject instance and return a pointer to it.
// This variant detaches the object by default, meaning that the caller fully
// owns it, and once the last BaseObjectPtr to it is destroyed, the object
// itself is also destroyed.
Expand Down
30 changes: 20 additions & 10 deletions src/env.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -594,6 +594,20 @@ void Environment::AssignToContext(Local<v8::Context> context,
TrackContext(context);
}

void Environment::UnassignFromContext(Local<v8::Context> context){
if (!context.IsEmpty()){
context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kEnvironment,
nullptr);
context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kRealm,
nullptr);
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kBindingDataStoreIndex, nullptr);
context->SetAlignedPointerInEmbedderData(
ContextEmbedderIndex::kContextifyContext, nullptr);
}
UntrackContext(context);
}

void Environment::TryLoadAddon(
const char* filename,
int flags,
Expand DownExpand Up@@ -819,7 +833,6 @@ void Environment::InitializeMainContext(Local<Context> context,
const EnvSerializeInfo* env_info){
principal_realm_ = std::make_unique<PrincipalRealm>(
this, context, MAYBE_FIELD_PTR(env_info, principal_realm));
AssignToContext(context, principal_realm_.get(), ContextInfo(""));
if (env_info != nullptr){
DeserializeProperties(env_info);
}
Expand DownExpand Up@@ -889,9 +902,9 @@ Environment::~Environment(){
inspector_agent_.reset();
#endif

ctx->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kEnvironment,
nullptr);
ctx->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kRealm, nullptr);
// Sub-realms should have been cleared with Environment's cleanup.
DCHECK_EQ(shadow_realms_.size(), 0);
principal_realm_.reset();

if (trace_state_observer_){
tracing::AgentWriterHandle* writer = GetTracingAgentWriter();
Expand All@@ -914,10 +927,6 @@ Environment::~Environment(){
addon.Close();
}
}

for (auto realm : shadow_realms_){
realm->OnEnvironmentDestruct();
Copy link
Member

@joyeecheungjoyeecheungJun 6, 2023

Choose a reason for hiding this comment

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

Can you add a test that creates a bunch of shadow realms and then generates a heap snapshot? I think that's how I ran into the shadow realms outliving shadow realms issue and is why this is necessary.

Copy link
MemberAuthor

@legendecaslegendecasJun 8, 2023

Choose a reason for hiding this comment

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

Added a new test test/pummel/test-heapdump-shadow-realm.js.

}
}

void Environment::InitializeLibuv(){
Expand DownExpand Up@@ -1713,6 +1722,9 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info){
std::cerr << *info << "\n"
}

// Deserialize the realm's properties before running the deserialize
// requests as the requests may need to access the realm's properties.
principal_realm_->DeserializeProperties(&info->principal_realm);
RunDeserializeRequests();

async_hooks_.Deserialize(ctx);
Expand All@@ -1723,8 +1735,6 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info){
exit_info_.Deserialize(ctx);
stream_base_state_.Deserialize(ctx);
should_abort_on_uncaught_toggle_.Deserialize(ctx);

principal_realm_->DeserializeProperties(&info->principal_realm);
}

uint64_t GuessMemoryAvailableToTheProcess(){
Expand Down
5 changes: 3 additions & 2 deletions src/env.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -649,8 +649,7 @@ class Environment : public MemoryRetainer{
void AssignToContext(v8::Local<v8::Context> context,
Realm* realm,
const ContextInfo& info);
void TrackContext(v8::Local<v8::Context> context);
void UntrackContext(v8::Local<v8::Context> context);
void UnassignFromContext(v8::Local<v8::Context> context);
void TrackShadowRealm(shadow_realm::ShadowRealm* realm);
void UntrackShadowRealm(shadow_realm::ShadowRealm* realm);

Expand DownExpand Up@@ -1002,6 +1001,8 @@ class Environment : public MemoryRetainer{
private:
inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
const char* errmsg);
void TrackContext(v8::Local<v8::Context> context);
void UntrackContext(v8::Local<v8::Context> context);

std::list<binding::DLib> loaded_addons_;
v8::Isolate* const isolate_;
Expand Down
4 changes: 2 additions & 2 deletions src/inspector_js_api.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -172,12 +172,12 @@ static bool InspectorEnabled(Environment* env){
}

void SetConsoleExtensionInstaller(const FunctionCallbackInfo<Value>& info){
auto env = Environment::GetCurrent(info);
Realm* realm = Realm::GetCurrent(info);

CHECK_EQ(info.Length(), 1);
CHECK(info[0]->IsFunction());

env->set_inspector_console_extension_installer(info[0].As<Function>());
realm->set_inspector_console_extension_installer(info[0].As<Function>());
}

void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args){
Expand Down
2 changes: 1 addition & 1 deletion src/node_contextify.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -163,7 +163,7 @@ ContextifyContext::~ContextifyContext(){
Isolate* isolate = env()->isolate();
HandleScope scope(isolate);

env()->UntrackContext(PersistentToLocal::Weak(isolate, context_));
env()->UnassignFromContext(PersistentToLocal::Weak(isolate, context_));
context_.Reset();
}

Expand Down
9 changes: 7 additions & 2 deletions src/node_realm-inl.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -86,8 +86,13 @@ inline T* Realm::AddBindingData(v8::Local<v8::Context> context,
Args&&... args){
DCHECK_EQ(GetCurrent(context), this);
// This won't compile if T is not a BaseObject subclass.
BaseObjectPtr<T> item =
MakeDetachedBaseObject<T>(this, target, std::forward<Args>(args)...);
static_assert(std::is_base_of_v<BaseObject, T>);
// The binding data must be weak so that it won't keep the realm reachable
// from strong GC roots indefinitely. The wrapper object of binding data
// should be referenced from JavaScript, thus the binding data should be
// reachable throughout the lifetime of the realm.
BaseObjectWeakPtr<T> item =
MakeWeakBaseObject<T>(this, target, std::forward<Args>(args)...);
DCHECK_EQ(context->GetAlignedPointerFromEmbedderData(
ContextEmbedderIndex::kBindingDataStoreIndex),
&binding_data_store_);
Expand Down
12 changes: 12 additions & 0 deletions src/node_realm.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -21,6 +21,7 @@ using v8::Value;
Realm::Realm(Environment* env, v8::Local<v8::Context> context, Kind kind)
: env_(env), isolate_(context->GetIsolate()), kind_(kind){
context_.Reset(isolate_, context);
env->AssignToContext(context, this, ContextInfo(""));
}

Realm::~Realm(){
Expand DownExpand Up@@ -278,11 +279,15 @@ v8::Local<v8::Context> Realm::context() const{
return PersistentToLocal::Strong(context_);
}

// Per-realm strong value accessors. The per-realm values should avoid being
// accessed across realms.
#define V(PropertyName, TypeName) \
v8::Local<TypeName> PrincipalRealm::PropertyName() const{\
return PersistentToLocal::Strong(PropertyName##_); \
} \
void PrincipalRealm::set_##PropertyName(v8::Local<TypeName> value){\
DCHECK_IMPLIES(!value.IsEmpty(), \
isolate()->GetCurrentContext() == context()); \
PropertyName##_.Reset(isolate(), value); \
}
PER_REALM_STRONG_PERSISTENT_VALUES(V)
Expand All@@ -300,6 +305,13 @@ PrincipalRealm::PrincipalRealm(Environment* env,
}
}

PrincipalRealm::~PrincipalRealm(){
DCHECK(!context_.IsEmpty());

HandleScope handle_scope(isolate());
env_->UnassignFromContext(context());
}

MaybeLocal<Value> PrincipalRealm::BootstrapRealm(){
HandleScope scope(isolate_);

Expand Down
8 changes: 4 additions & 4 deletions src/node_realm.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -21,9 +21,9 @@ struct RealmSerializeInfo{
friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i);
};

using BindingDataStore = std::array<BaseObjectPtr<BaseObject>,
static_cast<size_t>(
BindingDataType::kBindingDataTypeCount)>
using BindingDataStore =
std::array<BaseObjectWeakPtr<BaseObject>,
static_cast<size_t>(BindingDataType::kBindingDataTypeCount)>

/**
* node::Realm is a container for a set of JavaScript objects and functions
Expand DownExpand Up@@ -162,7 +162,7 @@ class PrincipalRealm : public Realm{
PrincipalRealm(Environment* env,
v8::Local<v8::Context> context,
const RealmSerializeInfo* realm_info);
~PrincipalRealm() = default;
~PrincipalRealm();

SET_MEMORY_INFO_NAME(PrincipalRealm)
SET_SELF_SIZE(PrincipalRealm)
Expand Down
43 changes: 33 additions & 10 deletions src/node_shadow_realm.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5,6 +5,7 @@
namespace node{
namespace shadow_realm{
using v8::Context;
using v8::EscapableHandleScope;
using v8::HandleScope;
using v8::Local;
using v8::MaybeLocal;
Expand All@@ -15,7 +16,6 @@ using TryCatchScope = node::errors::TryCatchScope;
// static
ShadowRealm* ShadowRealm::New(Environment* env){
ShadowRealm* realm = new ShadowRealm(env);
env->AssignToContext(realm->context(), realm, ContextInfo(""));

// We do not expect the realm bootstrapping to throw any
// exceptions. If it does, exit the current Node.js instance.
Expand All@@ -31,39 +31,62 @@ ShadowRealm* ShadowRealm::New(Environment* env){
MaybeLocal<Context> HostCreateShadowRealmContextCallback(
Local<Context> initiator_context){
Environment* env = Environment::GetCurrent(initiator_context);
EscapableHandleScope scope(env->isolate());
ShadowRealm* realm = ShadowRealm::New(env);
if (realm != nullptr){
return realm->context();
return scope.Escape(realm->context());
}
return MaybeLocal<Context>();
}

// static
void ShadowRealm::WeakCallback(const v8::WeakCallbackInfo<ShadowRealm>& data){
ShadowRealm* realm = data.GetParameter();
realm->context_.Reset();

// Yield to pending weak callbacks before deleting the realm.
// This is necessary to avoid cleaning up base objects before their scheduled
// weak callbacks are invoked, which can lead to accessing to v8 apis during
// the first pass of the weak callback.
realm->env()->SetImmediate([realm](Environment* env){delete realm});
Copy link
Member

@joyeecheungjoyeecheungApr 24, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I just remembered that technically shadow realms can outlive the environment...(which is why I added the if (env_ != nullptr) branch in the destructor of shadow realms, I think I ran into that before)

Copy link
Member

@mcollinamcollinaApr 24, 2023

Choose a reason for hiding this comment

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

This is going to lead to crashes if the ream has access to any libuv handles within it. The problem is that any handles could emit data and ultimately call into JS.

IMHO the mechanism needs to be more sophisticated and we should track any native resource that the realm can spawn and let the ream be collected only after they have all been disposed.

Copy link
MemberAuthor

@legendecaslegendecasApr 25, 2023

Choose a reason for hiding this comment

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

Thanks for the ideas. Also, I noticed that the ASan build is failing for the URL binding data. I'm going to see how to address the failure too.

Copy link
MemberAuthor

@legendecaslegendecasMay 8, 2023

Choose a reason for hiding this comment

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

I've added a cleanup for ShadowRealms when the environment is being freed. This ensures that no shadow realms can outlive the lifetime of the environment. Similar to BaseObjects, no BaseObjects can outlive their creation realm (https://github.com/nodejs/node/blob/main/src/node_realm.cc#L27).

As for the ASan failure of the URL binding data, I've removed the aliased buffer's weak callback as the validness of the aliased buffer can be checked with the emptiness of the persistent handle.

Would you mind taking a look again? Thank you!

// Remove the cleanup hook to avoid deleting the realm again.
realm->env()->RemoveCleanupHook(DeleteMe, realm);
}

// static
void ShadowRealm::DeleteMe(void* data){
ShadowRealm* realm = static_cast<ShadowRealm*>(data);
// Clear the context handle to avoid invoking the weak callback again.
// Also, the context internal slots are cleared and the context is no longer
// reference to the realm.
delete realm;
}

ShadowRealm::ShadowRealm(Environment* env)
: Realm(env, NewContext(env->isolate()), kShadowRealm){
env->TrackShadowRealm(this);
context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
CreateProperties();

env->TrackShadowRealm(this);
env->AddCleanupHook(DeleteMe, this);
}

ShadowRealm::~ShadowRealm(){
while (HasCleanupHooks()){
RunCleanup();
}
if (env_ != nullptr){
env_->UntrackShadowRealm(this);

env_->UntrackShadowRealm(this);

if (context_.IsEmpty()){
// This most likely happened because the weak callback cleared it.
return;
}
}

void ShadowRealm::OnEnvironmentDestruct(){
CHECK_NOT_NULL(env_);
env_ = nullptr; // This means that the shadow realm has out-lived the
// environment.
{
HandleScope handle_scope(isolate());
env_->UnassignFromContext(context());
}
}

v8::Local<v8::Context> ShadowRealm::context() const{
Expand Down
3 changes: 1 addition & 2 deletions src/node_shadow_realm.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -24,13 +24,12 @@ class ShadowRealm : public Realm{
PER_REALM_STRONG_PERSISTENT_VALUES(V)
#undef V

void OnEnvironmentDestruct();

protected:
v8::MaybeLocal<v8::Value> BootstrapRealm() override;

private:
static void WeakCallback(const v8::WeakCallbackInfo<ShadowRealm>& data);
static void DeleteMe(void* data);

explicit ShadowRealm(Environment* env);
~ShadowRealm();
Expand Down
1 change: 1 addition & 0 deletions src/node_url.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -40,6 +40,7 @@ BindingData::BindingData(Realm* realm, v8::Local<v8::Object> object)
FIXED_ONE_BYTE_STRING(realm->isolate(), "urlComponents"),
url_components_buffer_.GetJSArray())
.Check();
url_components_buffer_.MakeWeak();
}

bool BindingData::PrepareForSerialization(v8::Local<v8::Context> context,
Expand Down
3 changes: 0 additions & 3 deletions test/known_issues/known_issues.status
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,9 +11,6 @@ prefix known_issues
# foreseeable future. The test itself is flaky and skipped. It
# serves as a demonstration of the issue only.
test-vm-timeout-escape-queuemicrotask: SKIP
# Skipping it because it crashes out of OOM instead of exiting.
# https://github.com/nodejs/node/issues/47353
test-shadow-realm-gc: SKIP

[$system==win32]

Expand Down
Loading