Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0500967

Browse filesBrowse files
legendecasRafaelGSS
legendecas
authored and
RafaelGSS
committed
src: make realm binding data store weak
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. PR-URL: #47688 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent 120ac74 commit 0500967
Copy full SHA for 0500967
Expand file treeCollapse file tree

16 files changed

+148
-49
lines changed
Open diff view settings
Collapse file

‎src/base_object-inl.h‎

Copy file name to clipboardExpand all lines: src/base_object-inl.h
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,12 @@ template <typename T, typename... Args>
289289
BaseObjectPtr<T> MakeBaseObject(Args&&... args) {
290290
return BaseObjectPtr<T>(new T(std::forward<Args>(args)...));
291291
}
292+
template <typename T, typename... Args>
293+
BaseObjectWeakPtr<T> MakeWeakBaseObject(Args&&... args) {
294+
T* target = new T(std::forward<Args>(args)...);
295+
target->MakeWeak();
296+
return BaseObjectWeakPtr<T>(target);
297+
}
292298

293299
template <typename T, typename... Args>
294300
BaseObjectPtr<T> MakeDetachedBaseObject(Args&&... args) {
Collapse file

‎src/base_object.h‎

Copy file name to clipboardExpand all lines: src/base_object.h
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,10 @@ using BaseObjectWeakPtr = BaseObjectPtrImpl<T, true>;
302302
template <typename T, typename... Args>
303303
inline BaseObjectPtr<T> MakeBaseObject(Args&&... args);
304304
// Create a BaseObject instance and return a pointer to it.
305+
// This variant makes the object a weak GC root by default.
306+
template <typename T, typename... Args>
307+
inline BaseObjectWeakPtr<T> MakeWeakBaseObject(Args&&... args);
308+
// Create a BaseObject instance and return a pointer to it.
305309
// This variant detaches the object by default, meaning that the caller fully
306310
// owns it, and once the last BaseObjectPtr to it is destroyed, the object
307311
// itself is also destroyed.
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+20-10Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,20 @@ void Environment::AssignToContext(Local<v8::Context> context,
594594
TrackContext(context);
595595
}
596596

597+
void Environment::UnassignFromContext(Local<v8::Context> context) {
598+
if (!context.IsEmpty()) {
599+
context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kEnvironment,
600+
nullptr);
601+
context->SetAlignedPointerInEmbedderData(ContextEmbedderIndex::kRealm,
602+
nullptr);
603+
context->SetAlignedPointerInEmbedderData(
604+
ContextEmbedderIndex::kBindingDataStoreIndex, nullptr);
605+
context->SetAlignedPointerInEmbedderData(
606+
ContextEmbedderIndex::kContextifyContext, nullptr);
607+
}
608+
UntrackContext(context);
609+
}
610+
597611
void Environment::TryLoadAddon(
598612
const char* filename,
599613
int flags,
@@ -822,7 +836,6 @@ void Environment::InitializeMainContext(Local<Context> context,
822836
const EnvSerializeInfo* env_info) {
823837
principal_realm_ = std::make_unique<PrincipalRealm>(
824838
this, context, MAYBE_FIELD_PTR(env_info, principal_realm));
825-
AssignToContext(context, principal_realm_.get(), ContextInfo(""));
826839
if (env_info != nullptr) {
827840
DeserializeProperties(env_info);
828841
}
@@ -892,9 +905,9 @@ Environment::~Environment() {
892905
inspector_agent_.reset();
893906
#endif
894907

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

899912
if (trace_state_observer_) {
900913
tracing::AgentWriterHandle* writer = GetTracingAgentWriter();
@@ -917,10 +930,6 @@ Environment::~Environment() {
917930
addon.Close();
918931
}
919932
}
920-
921-
for (auto realm : shadow_realms_) {
922-
realm->OnEnvironmentDestruct();
923-
}
924933
}
925934

926935
void Environment::InitializeLibuv() {
@@ -1716,6 +1725,9 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) {
17161725
std::cerr << *info << "\n";
17171726
}
17181727

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

17211733
async_hooks_.Deserialize(ctx);
@@ -1726,8 +1738,6 @@ void Environment::DeserializeProperties(const EnvSerializeInfo* info) {
17261738
exit_info_.Deserialize(ctx);
17271739
stream_base_state_.Deserialize(ctx);
17281740
should_abort_on_uncaught_toggle_.Deserialize(ctx);
1729-
1730-
principal_realm_->DeserializeProperties(&info->principal_realm);
17311741
}
17321742

17331743
uint64_t GuessMemoryAvailableToTheProcess() {
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+3-2Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -649,8 +649,7 @@ class Environment : public MemoryRetainer {
649649
void AssignToContext(v8::Local<v8::Context> context,
650650
Realm* realm,
651651
const ContextInfo& info);
652-
void TrackContext(v8::Local<v8::Context> context);
653-
void UntrackContext(v8::Local<v8::Context> context);
652+
void UnassignFromContext(v8::Local<v8::Context> context);
654653
void TrackShadowRealm(shadow_realm::ShadowRealm* realm);
655654
void UntrackShadowRealm(shadow_realm::ShadowRealm* realm);
656655

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

10061007
std::list<binding::DLib> loaded_addons_;
10071008
v8::Isolate* const isolate_;
Collapse file

‎src/inspector_js_api.cc‎

Copy file name to clipboardExpand all lines: src/inspector_js_api.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -172,12 +172,12 @@ static bool InspectorEnabled(Environment* env) {
172172
}
173173

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

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

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

183183
void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ ContextifyContext::~ContextifyContext() {
163163
Isolate* isolate = env()->isolate();
164164
HandleScope scope(isolate);
165165

166-
env()->UntrackContext(PersistentToLocal::Weak(isolate, context_));
166+
env()->UnassignFromContext(PersistentToLocal::Weak(isolate, context_));
167167
context_.Reset();
168168
}
169169

Collapse file

‎src/node_realm-inl.h‎

Copy file name to clipboardExpand all lines: src/node_realm-inl.h
+7-2Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,13 @@ inline T* Realm::AddBindingData(v8::Local<v8::Context> context,
8686
Args&&... args) {
8787
DCHECK_EQ(GetCurrent(context), this);
8888
// This won't compile if T is not a BaseObject subclass.
89-
BaseObjectPtr<T> item =
90-
MakeDetachedBaseObject<T>(this, target, std::forward<Args>(args)...);
89+
static_assert(std::is_base_of_v<BaseObject, T>);
90+
// The binding data must be weak so that it won't keep the realm reachable
91+
// from strong GC roots indefinitely. The wrapper object of binding data
92+
// should be referenced from JavaScript, thus the binding data should be
93+
// reachable throughout the lifetime of the realm.
94+
BaseObjectWeakPtr<T> item =
95+
MakeWeakBaseObject<T>(this, target, std::forward<Args>(args)...);
9196
DCHECK_EQ(context->GetAlignedPointerFromEmbedderData(
9297
ContextEmbedderIndex::kBindingDataStoreIndex),
9398
&binding_data_store_);
Collapse file

‎src/node_realm.cc‎

Copy file name to clipboardExpand all lines: src/node_realm.cc
+12Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ using v8::Value;
2121
Realm::Realm(Environment* env, v8::Local<v8::Context> context, Kind kind)
2222
: env_(env), isolate_(context->GetIsolate()), kind_(kind) {
2323
context_.Reset(isolate_, context);
24+
env->AssignToContext(context, this, ContextInfo(""));
2425
}
2526

2627
Realm::~Realm() {
@@ -278,11 +279,15 @@ v8::Local<v8::Context> Realm::context() const {
278279
return PersistentToLocal::Strong(context_);
279280
}
280281

282+
// Per-realm strong value accessors. The per-realm values should avoid being
283+
// accessed across realms.
281284
#define V(PropertyName, TypeName) \
282285
v8::Local<TypeName> PrincipalRealm::PropertyName() const { \
283286
return PersistentToLocal::Strong(PropertyName##_); \
284287
} \
285288
void PrincipalRealm::set_##PropertyName(v8::Local<TypeName> value) { \
289+
DCHECK_IMPLIES(!value.IsEmpty(), \
290+
isolate()->GetCurrentContext() == context()); \
286291
PropertyName##_.Reset(isolate(), value); \
287292
}
288293
PER_REALM_STRONG_PERSISTENT_VALUES(V)
@@ -300,6 +305,13 @@ PrincipalRealm::PrincipalRealm(Environment* env,
300305
}
301306
}
302307

308+
PrincipalRealm::~PrincipalRealm() {
309+
DCHECK(!context_.IsEmpty());
310+
311+
HandleScope handle_scope(isolate());
312+
env_->UnassignFromContext(context());
313+
}
314+
303315
MaybeLocal<Value> PrincipalRealm::BootstrapRealm() {
304316
HandleScope scope(isolate_);
305317

Collapse file

‎src/node_realm.h‎

Copy file name to clipboardExpand all lines: src/node_realm.h
+4-4Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ struct RealmSerializeInfo {
2121
friend std::ostream& operator<<(std::ostream& o, const RealmSerializeInfo& i);
2222
};
2323

24-
using BindingDataStore = std::array<BaseObjectPtr<BaseObject>,
25-
static_cast<size_t>(
26-
BindingDataType::kBindingDataTypeCount)>;
24+
using BindingDataStore =
25+
std::array<BaseObjectWeakPtr<BaseObject>,
26+
static_cast<size_t>(BindingDataType::kBindingDataTypeCount)>;
2727

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

167167
SET_MEMORY_INFO_NAME(PrincipalRealm)
168168
SET_SELF_SIZE(PrincipalRealm)
Collapse file

‎src/node_shadow_realm.cc‎

Copy file name to clipboardExpand all lines: src/node_shadow_realm.cc
+33-10Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
namespace node {
66
namespace shadow_realm {
77
using v8::Context;
8+
using v8::EscapableHandleScope;
89
using v8::HandleScope;
910
using v8::Local;
1011
using v8::MaybeLocal;
@@ -15,7 +16,6 @@ using TryCatchScope = node::errors::TryCatchScope;
1516
// static
1617
ShadowRealm* ShadowRealm::New(Environment* env) {
1718
ShadowRealm* realm = new ShadowRealm(env);
18-
env->AssignToContext(realm->context(), realm, ContextInfo(""));
1919

2020
// We do not expect the realm bootstrapping to throw any
2121
// exceptions. If it does, exit the current Node.js instance.
@@ -31,39 +31,62 @@ ShadowRealm* ShadowRealm::New(Environment* env) {
3131
MaybeLocal<Context> HostCreateShadowRealmContextCallback(
3232
Local<Context> initiator_context) {
3333
Environment* env = Environment::GetCurrent(initiator_context);
34+
EscapableHandleScope scope(env->isolate());
3435
ShadowRealm* realm = ShadowRealm::New(env);
3536
if (realm != nullptr) {
36-
return realm->context();
37+
return scope.Escape(realm->context());
3738
}
3839
return MaybeLocal<Context>();
3940
}
4041

4142
// static
4243
void ShadowRealm::WeakCallback(const v8::WeakCallbackInfo<ShadowRealm>& data) {
4344
ShadowRealm* realm = data.GetParameter();
45+
realm->context_.Reset();
46+
47+
// Yield to pending weak callbacks before deleting the realm.
48+
// This is necessary to avoid cleaning up base objects before their scheduled
49+
// weak callbacks are invoked, which can lead to accessing to v8 apis during
50+
// the first pass of the weak callback.
51+
realm->env()->SetImmediate([realm](Environment* env) { delete realm; });
52+
// Remove the cleanup hook to avoid deleting the realm again.
53+
realm->env()->RemoveCleanupHook(DeleteMe, realm);
54+
}
55+
56+
// static
57+
void ShadowRealm::DeleteMe(void* data) {
58+
ShadowRealm* realm = static_cast<ShadowRealm*>(data);
59+
// Clear the context handle to avoid invoking the weak callback again.
60+
// Also, the context internal slots are cleared and the context is no longer
61+
// reference to the realm.
4462
delete realm;
4563
}
4664

4765
ShadowRealm::ShadowRealm(Environment* env)
4866
: Realm(env, NewContext(env->isolate()), kShadowRealm) {
49-
env->TrackShadowRealm(this);
5067
context_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
5168
CreateProperties();
69+
70+
env->TrackShadowRealm(this);
71+
env->AddCleanupHook(DeleteMe, this);
5272
}
5373

5474
ShadowRealm::~ShadowRealm() {
5575
while (HasCleanupHooks()) {
5676
RunCleanup();
5777
}
58-
if (env_ != nullptr) {
59-
env_->UntrackShadowRealm(this);
78+
79+
env_->UntrackShadowRealm(this);
80+
81+
if (context_.IsEmpty()) {
82+
// This most likely happened because the weak callback cleared it.
83+
return;
6084
}
61-
}
6285

63-
void ShadowRealm::OnEnvironmentDestruct() {
64-
CHECK_NOT_NULL(env_);
65-
env_ = nullptr; // This means that the shadow realm has out-lived the
66-
// environment.
86+
{
87+
HandleScope handle_scope(isolate());
88+
env_->UnassignFromContext(context());
89+
}
6790
}
6891

6992
v8::Local<v8::Context> ShadowRealm::context() const {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.