src: detach cppgc wrappers from their Realm before it is freed · nodejs/node@c909c63 · GitHub
Skip to content

Commit c909c63

Browse files
codebyterecodebytere
committed
src: detach cppgc wrappers from their Realm before it is freed
`Realm::RunCleanup()` finalizes the cppgc-managed wrappers it tracks so that none of them touches the Realm once it is gone, but it reaches them through weak persistents, and the GC clears those as soon as it finds a wrapper dead. With lazy and concurrent sweeping the destructor can run much later, so a wrapper collected shortly before `FreeEnvironment()` and swept after it was skipped by the cleanup and kept its `realm_`: `~CppgcMixin()` then wrote `should_purge_empty_cppgc_wrappers_` into the freed Realm, and a subclass destructor calling `Finalize()` as documented would have called `Clean()` with a dangling Realm. A Worker that compiles a few `vm.Script`s, gets a full GC from external memory pressure and calls `process.exit()` is enough to hit the first case. Move the Realm pointer into the list node, which the wrapper now owns and deletes in its destructor. `CppgcWrapperList::Cleanup()` unlinks every node, finalizing the wrappers that are still alive and clearing the Realm pointer for the collected ones, which only their own destructor may still touch. `Realm::PendingCleanup()` accounts for the list so it is always drained. The purge flag, its GC epilogue callback and `PurgeEmpty()` are no longer needed, and removing them also stops the list nodes of wrappers that are alive at `FreeEnvironment()` from leaking. Refs: #56534 Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com> PR-URL: #65778 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 7709fca commit c909c63

7 files changed

Lines changed: 156 additions & 84 deletions

File tree

‎src/cppgc_helpers-inl.h‎

Lines changed: 15 additions & 7 deletions

‎src/cppgc_helpers.cc‎

Lines changed: 7 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
1-
#include "cppgc_helpers.h"
2-
#include "env-inl.h"
1+
#include "cppgc_helpers.h" // NOLINT(build/include_inline)
2+
#include "cppgc_helpers-inl.h"
33

44
namespace node {
55

66
void CppgcWrapperList::Cleanup() {
7-
for (auto node : *this) {
8-
CppgcMixin* ptr = node->persistent.Get();
9-
if (ptr != nullptr) {
10-
ptr->Finalize();
11-
}
7+
while (!IsEmpty()) {
8+
CppgcWrapperListNode* node = PopFront();
9+
CppgcMixin* wrapper = node->persistent.Get();
10+
if (wrapper != nullptr) wrapper->Finalize();
11+
node->realm = nullptr;
1212
}
1313
}
1414

@@ -23,18 +23,4 @@ void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const {
2323
}
2424
}
2525
}
26-
27-
void CppgcWrapperList::PurgeEmpty() {
28-
for (auto weak_it = begin(); weak_it != end();) {
29-
CppgcWrapperListNode* node = *weak_it;
30-
auto next_it = ++weak_it;
31-
// The underlying cppgc wrapper has already been garbage collected.
32-
// Remove it from the list.
33-
if (!node->persistent) {
34-
node->persistent.Clear();
35-
delete node;
36-
}
37-
weak_it = next_it;
38-
}
39-
}
4026
} // namespace node

‎src/cppgc_helpers.h‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,9 @@ class CppgcWrapperListNode;
4747
* cleanup relies on a living Node.js `Realm`, it should implement a
4848
* pattern like this:
4949
*
50-
* ~MyWrap() { this->Destroy(); }
50+
* ~MyWrap() { this->Finalize(); }
5151
* void Clean(Realm* env) override {
52-
* // Do cleanup that relies on a living Environemnt.
52+
* // Do cleanup that relies on a living Realm.
5353
* }
5454
*/
5555
class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
@@ -68,7 +68,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
6868

6969
inline v8::Local<v8::Object> object() const;
7070
inline Environment* env() const;
71-
inline Realm* realm() const { return realm_; }
71+
inline Realm* realm() const;
7272
inline v8::Local<v8::Object> object(v8::Isolate* isolate) const {
7373
return traced_reference_.Get(isolate);
7474
}
@@ -95,11 +95,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
9595
// destructor. Outside of Finalize(), subclasses should avoid calling
9696
// into JavaScript or perform any operation that can trigger garbage
9797
// collection during the destruction.
98-
void Finalize() {
99-
if (realm_ == nullptr) return;
100-
this->Clean(realm_);
101-
realm_ = nullptr;
102-
}
98+
inline void Finalize();
10399

104100
// The default implementation of Clean() is a no-op. If subclasses wish
105101
// to perform cleanup that require a living Realm, they should
@@ -110,10 +106,8 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
110106

111107
inline ~CppgcMixin();
112108

113-
friend class CppgcWrapperListNode;
114-
115109
private:
116-
Realm* realm_ = nullptr;
110+
CppgcWrapperListNode* list_node_ = nullptr;
117111
v8::TracedReference<v8::Object> traced_reference_;
118112
};
119113

‎src/node_realm-inl.h‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,11 +133,13 @@ void Realm::TrackBaseObject(BaseObject* bo) {
133133
++base_object_count_;
134134
}
135135

136-
CppgcWrapperListNode::CppgcWrapperListNode(CppgcMixin* ptr) : persistent(ptr) {}
136+
CppgcWrapperListNode::CppgcWrapperListNode(Realm* realm, CppgcMixin* wrapper)
137+
: realm(realm), persistent(wrapper) {}
137138

138-
void Realm::TrackCppgcWrapper(CppgcMixin* handle) {
139-
DCHECK_EQ(handle->realm(), this);
140-
cppgc_wrapper_list_.PushFront(new CppgcWrapperListNode(handle));
139+
CppgcWrapperListNode* Realm::TrackCppgcWrapper(CppgcMixin* handle) {
140+
CppgcWrapperListNode* node = new CppgcWrapperListNode(this, handle);
141+
cppgc_wrapper_list_.PushFront(node);
142+
return node;
141143
}
142144

143145
void Realm::UntrackBaseObject(BaseObject* bo) {
@@ -146,7 +148,7 @@ void Realm::UntrackBaseObject(BaseObject* bo) {
146148
}
147149

148150
bool Realm::PendingCleanup() const {
149-
return !base_object_list_.IsEmpty();
151+
return !base_object_list_.IsEmpty() || !cppgc_wrapper_list_.IsEmpty();
150152
}
151153

152154
} // namespace node

‎src/node_realm.cc‎

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ namespace node {
1010

1111
using v8::Context;
1212
using v8::EscapableHandleScope;
13-
using v8::GCCallbackFlags;
14-
using v8::GCType;
1513
using v8::HandleScope;
1614
using v8::Isolate;
1715
using v8::Local;
@@ -25,26 +23,11 @@ Realm::Realm(Environment* env, v8::Local<v8::Context> context, Kind kind)
2523
: env_(env), isolate_(Isolate::GetCurrent()), kind_(kind) {
2624
context_.Reset(isolate_, context);
2725
env->AssignToContext(context, this, ContextInfo(""));
28-
// The environment can also purge empty wrappers in the check callback,
29-
// though that may be a bit excessive depending on usage patterns.
30-
// For now using the GC epilogue is adequate.
31-
isolate_->AddGCEpilogueCallback(PurgeEmptyCppgcWrappers, this);
3226
}
3327

3428
Realm::~Realm() {
35-
isolate_->RemoveGCEpilogueCallback(PurgeEmptyCppgcWrappers, this);
3629
CHECK_EQ(base_object_count_, 0);
37-
}
38-
39-
void Realm::PurgeEmptyCppgcWrappers(Isolate* isolate,
40-
GCType type,
41-
GCCallbackFlags flags,
42-
void* data) {
43-
Realm* realm = static_cast<Realm*>(data);
44-
if (realm->should_purge_empty_cppgc_wrappers_) {
45-
realm->cppgc_wrapper_list_.PurgeEmpty();
46-
realm->should_purge_empty_cppgc_wrappers_ = false;
47-
}
30+
CHECK(cppgc_wrapper_list_.IsEmpty());
4831
}
4932

5033
void Realm::MemoryInfo(MemoryTracker* tracker) const {

‎src/node_realm.h‎

Lines changed: 7 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,16 @@ using BindingDataStore =
2727
static_cast<size_t>(BindingDataType::kBindingDataTypeCount)>;
2828

2929
/**
30-
* This is a wrapper around a weak persistent of CppgcMixin, used in the
31-
* CppgcWrapperList to avoid accessing already garbage collected CppgcMixins.
30+
* Owned by a CppgcMixin and linked into its Realm's list until the Realm
31+
* cleans up and clears `realm`. The Realm only calls into wrappers the GC
32+
* still considers alive (the weak persistent); a collected wrapper whose
33+
* destructor runs later sees `realm == nullptr` instead of a freed Realm.
3234
*/
3335
class CppgcWrapperListNode {
3436
public:
35-
explicit inline CppgcWrapperListNode(CppgcMixin* ptr);
36-
inline explicit operator bool() const { return !persistent; }
37-
inline CppgcMixin* operator->() const { return persistent.Get(); }
38-
inline CppgcMixin* operator*() const { return persistent.Get(); }
37+
inline CppgcWrapperListNode(Realm* realm, CppgcMixin* wrapper);
3938

39+
Realm* realm;
4040
cppgc::WeakPersistent<CppgcMixin> persistent;
4141
// Used by ContainerOf in the ListNode implementation for fast manipulation of
4242
// CppgcWrapperList.
@@ -53,7 +53,6 @@ class CppgcWrapperList
5353
public MemoryRetainer {
5454
public:
5555
void Cleanup();
56-
void PurgeEmpty();
5756

5857
SET_MEMORY_INFO_NAME(CppgcWrapperList)
5958
SET_SELF_SIZE(CppgcWrapperList)
@@ -148,7 +147,7 @@ class Realm : public MemoryRetainer {
148147
// Base object count created after the bootstrap of the realm.
149148
inline int64_t base_object_created_after_bootstrap() const;
150149

151-
inline void TrackCppgcWrapper(CppgcMixin* handle);
150+
inline CppgcWrapperListNode* TrackCppgcWrapper(CppgcMixin* handle);
152151
inline CppgcWrapperList* cppgc_wrapper_list() { return &cppgc_wrapper_list_; }
153152

154153
#define V(PropertyName, TypeName) \
@@ -164,14 +163,6 @@ class Realm : public MemoryRetainer {
164163
// it's only used for tests.
165164
std::vector<std::string> builtins_in_snapshot;
166165

167-
// This used during the destruction of cppgc wrappers to inform a GC epilogue
168-
// callback to clean up the weak persistents used to track cppgc wrappers if
169-
// the wrappers are already garbage collected to prevent holding on to
170-
// excessive useless persistents.
171-
inline void set_should_purge_empty_cppgc_wrappers(bool value) {
172-
should_purge_empty_cppgc_wrappers_ = value;
173-
}
174-
175166
protected:
176167
~Realm();
177168

@@ -181,17 +172,11 @@ class Realm : public MemoryRetainer {
181172
// Shorthand for isolate pointer.
182173
v8::Isolate* isolate_;
183174
v8::Global<v8::Context> context_;
184-
bool should_purge_empty_cppgc_wrappers_ = false;
185175

186176
#define V(PropertyName, TypeName) v8::Global<TypeName> PropertyName##_;
187177
PER_REALM_STRONG_PERSISTENT_VALUES(V)
188178
#undef V
189179

190-
static void PurgeEmptyCppgcWrappers(v8::Isolate* isolate,
191-
v8::GCType type,
192-
v8::GCCallbackFlags flags,
193-
void* data);
194-
195180
private:
196181
void InitializeContext(v8::Local<v8::Context> context,
197182
const RealmSerializeInfo* realm_info);

‎test/cctest/test_cppgc.cc‎

Lines changed: 114 additions & 0 deletions

0 commit comments

Comments
 (0)