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

Commit adbbfa8

Browse files
codebytereinoway46
codebytere
authored and
inoway46
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 4396c9b commit adbbfa8

7 files changed

Lines changed: 156 additions & 85 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
@@ -135,11 +135,13 @@ void Realm::TrackBaseObject(BaseObject* bo) {
135135
++base_object_count_;
136136
}
137137

138-
CppgcWrapperListNode::CppgcWrapperListNode(CppgcMixin* ptr) : persistent(ptr) {}
138+
CppgcWrapperListNode::CppgcWrapperListNode(Realm* realm, CppgcMixin* wrapper)
139+
: realm(realm), persistent(wrapper) {}
139140

140-
void Realm::TrackCppgcWrapper(CppgcMixin* handle) {
141-
DCHECK_EQ(handle->realm(), this);
142-
cppgc_wrapper_list_.PushFront(new CppgcWrapperListNode(handle));
141+
CppgcWrapperListNode* Realm::TrackCppgcWrapper(CppgcMixin* handle) {
142+
CppgcWrapperListNode* node = new CppgcWrapperListNode(this, handle);
143+
cppgc_wrapper_list_.PushFront(node);
144+
return node;
143145
}
144146

145147
void Realm::UntrackBaseObject(BaseObject* bo) {
@@ -148,7 +150,7 @@ void Realm::UntrackBaseObject(BaseObject* bo) {
148150
}
149151

150152
bool Realm::PendingCleanup() const {
151-
return !base_object_list_.IsEmpty();
153+
return !base_object_list_.IsEmpty() || !cppgc_wrapper_list_.IsEmpty();
152154
}
153155

154156
} // namespace node

‎src/node_realm.cc‎

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

1111
using v8::Context;
1212
using v8::EscapableHandleScope;
13-
using v8::GCCallbackFlags;
14-
using v8::GCType;
1513
using v8::HandleScope;
16-
using v8::Isolate;
1714
using v8::Local;
1815
using v8::MaybeLocal;
1916
using v8::Object;
@@ -25,26 +22,11 @@ Realm::Realm(Environment* env, v8::Local<v8::Context> context, Kind kind)
2522
: env_(env), isolate_(context->GetIsolate()), kind_(kind) {
2623
context_.Reset(isolate_, context);
2724
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);
3225
}
3326

3427
Realm::~Realm() {
35-
isolate_->RemoveGCEpilogueCallback(PurgeEmptyCppgcWrappers, this);
3628
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-
}
29+
CHECK(cppgc_wrapper_list_.IsEmpty());
4830
}
4931

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

‎src/node_realm.h‎

Lines changed: 7 additions & 22 deletions

0 commit comments

Comments
 (0)