Skip to content

Commit a35cc21

Browse files
joyeecheungtargos
joyeecheung
authored and
targos
committed
src: track cppgc wrappers with a list in Realm
This allows us to perform cleanups of cppgc wrappers that rely on a living Realm during Realm shutdown. Otherwise the cleanup may happen during object destruction, which can be triggered by GC after Realm shutdown, leading to invalid access to Realm. The general pattern for this type of non-trivial destruction is designed to be: ``` class MyWrap final : CPPGC_MIXIN(MyWrap) { public: ~MyWrap() { this->Finalize(); } void Clean(Realm* realm) override { // Do cleanup that relies on a living Realm. This would be // called by CppgcMixin::Finalize() first during Realm // shutdown, while the Realm is still alive. If the destructor // calls Finalize() again later during garbage collection that // happens after Realm shutdown, Clean() would be skipped, // preventing invalid access to the Realm. } } ``` In addition, this allows us to trace external memory held by the wrappers in the heap snapshots if we add synthethic edges between the wrappers and other nodes in the embdder graph callback, or to perform snapshot serialization for them. PR-URL: #56534 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 402ac8b commit a35cc21

11 files changed

Lines changed: 417 additions & 49 deletions

‎node.gyp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@
205205
'src/connect_wrap.h',
206206
'src/connection_wrap.h',
207207
'src/cppgc_helpers.h',
208+
'src/cppgc_helpers.cc',
208209
'src/dataqueue/queue.h',
209210
'src/debug_utils.h',
210211
'src/debug_utils-inl.h',

‎src/README.md‎

Lines changed: 77 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,6 +1131,17 @@ class MyWrap final : CPPGC_MIXIN(MyWrap) {
11311131
}
11321132
```
11331133

1134+
If the wrapper needs to perform cleanups when it's destroyed and that
1135+
cleanup relies on a living Node.js `Realm`, it should implement a
1136+
pattern like this:
1137+
1138+
```cpp
1139+
~MyWrap() { this->Finalize(); }
1140+
void Clean(Realm* env) override {
1141+
// Do cleanup that relies on a living Realm.
1142+
}
1143+
```
1144+
11341145
`cppgc::GarbageCollected` types are expected to implement a
11351146
`void Trace(cppgc::Visitor* visitor) const` method. When they are the
11361147
final class in the hierarchy, this method must be marked `final`. For
@@ -1285,16 +1296,76 @@ referrer->Set(
12851296
).ToLocalChecked();
12861297
```
12871298

1299+
#### Creating references between cppgc-managed objects and `BaseObject`s
1300+
1301+
This is currently unsupported with the existing helpers. If this has
1302+
to be done, new helpers must be implemented first. Consult the cppgc
1303+
headers when trying to implement it.
1304+
1305+
Another way to work around it is to always do the migration bottom-to-top.
1306+
If a cppgc-managed object needs to reference a `BaseObject`, convert
1307+
that `BaseObject` to be cppgc-managed first, and then use `cppgc::Member`
1308+
to create the references.
1309+
1310+
#### Lifetime and cleanups of cppgc-managed objects
1311+
12881312
Typically, a newly created cppgc-managed wrapper object should be held alive
12891313
by the JavaScript land (for example, by being returned by a method and
12901314
staying alive in a closure). Long-lived cppgc objects can also
12911315
be held alive from C++ using persistent handles (see
12921316
`deps/v8/include/cppgc/persistent.h`) or as members of other living
12931317
cppgc-managed objects (see `deps/v8/include/cppgc/member.h`) if necessary.
1294-
Its destructor will be called when no other objects from the V8 heap reference
1295-
it, this can happen at any time after the garbage collector notices that
1296-
it's no longer reachable and before the V8 isolate is torn down.
1297-
See the [Oilpan documentation in Chromium][] for more details.
1318+
1319+
When a cppgc-managed object is no longer reachable in the heap, its destructor
1320+
will be invoked by the garbage collection, which can happen after the `Realm`
1321+
is already gone, or after any object it references is gone. It is therefore
1322+
unsafe to invoke V8 APIs directly in the destructors. To ensure safety,
1323+
the cleanups of a cppgc-managed object should adhere to different patterns,
1324+
depending on what it needs to do:
1325+
1326+
1. If it does not need to do any non-trivial cleanup, nor does its members, just use
1327+
the default destructor. Cleanup of `v8::TracedReference` and
1328+
`cppgc::Member` are already handled automatically by V8 so if they are all the
1329+
non-trivial members the class has, this case applies.
1330+
2. If the cleanup relies on a living `Realm`, but does not need to access V8
1331+
APIs, the class should use this pattern in its class body:
1332+
1333+
```cpp
1334+
~MyWrap() { this->Finalize(); }
1335+
void Clean(Realm* env) override {
1336+
// Do cleanup that relies on a living Realm. This would be
1337+
// called by CppgcMixin::Finalize() first during Realm shutdown,
1338+
// while the Realm is still alive. If the destructor calls
1339+
// Finalize() again later during garbage collection that happens after
1340+
// Realm shutdown, Clean() would be skipped, preventing
1341+
// invalid access to the Realm.
1342+
}
1343+
```
1344+
1345+
If implementers want to call `Finalize()` from `Clean()` again, they
1346+
need to make sure that calling `Clean()` recursively is safe.
1347+
3. If the cleanup relies on access to the V8 heap, including using any V8
1348+
handles, in addition to 2, it should use the `CPPGC_USING_PRE_FINALIZER`
1349+
macro (from the [`cppgc/prefinalizer.h` header][]) in the private
1350+
section of its class body:
1351+
1352+
```cpp
1353+
private:
1354+
CPPGC_USING_PRE_FINALIZER(MyWrap, Finalize);
1355+
```
1356+
1357+
Both the destructor and the pre-finalizer are always called on the thread
1358+
in which the object is created.
1359+
1360+
It's worth noting that the use of pre-finalizers would have a negative impact
1361+
on the garbage collection performance as V8 needs to scan all of them during
1362+
each sweeping. If the object is expected to be created frequently in large
1363+
amounts in the application, it's better to avoid access to the V8 heap in its
1364+
cleanup to avoid having to use a pre-finalizer.
1365+
1366+
For more information about the cleanup of cppgc-managed objects and
1367+
what can be done in a pre-finalizer, see the [cppgc documentation][] and
1368+
the [`cppgc/prefinalizer.h` header][].
12981369

12991370
### Callback scopes
13001371

@@ -1421,6 +1492,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
14211492
[`async_hooks` module]: https://nodejs.org/api/async_hooks.html
14221493
[`async_wrap.h`]: async_wrap.h
14231494
[`base_object.h`]: base_object.h
1495+
[`cppgc/prefinalizer.h` header]: ../deps/v8/include/cppgc/prefinalizer.h
14241496
[`handle_wrap.h`]: handle_wrap.h
14251497
[`memory_tracker.h`]: memory_tracker.h
14261498
[`req_wrap.h`]: req_wrap.h
@@ -1432,6 +1504,7 @@ static void GetUserInfo(const FunctionCallbackInfo<Value>& args) {
14321504
[`vm` module]: https://nodejs.org/api/vm.html
14331505
[binding function]: #binding-functions
14341506
[cleanup hooks]: #cleanup-hooks
1507+
[cppgc documentation]: ../deps/v8/include/cppgc/README.md
14351508
[event loop]: #event-loop
14361509
[exception handling]: #exception-handling
14371510
[fast API calls]: ../doc/contributing/adding-v8-fast-api.md

‎src/cppgc_helpers-inl.h‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
#ifndef SRC_CPPGC_HELPERS_INL_H_
2+
#define SRC_CPPGC_HELPERS_INL_H_
3+
4+
#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
5+
6+
#include "cppgc_helpers.h"
7+
#include "env-inl.h"
8+
9+
namespace node {
10+
11+
template <typename T>
12+
void CppgcMixin::Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj) {
13+
CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount);
14+
ptr->realm_ = realm;
15+
v8::Isolate* isolate = realm->isolate();
16+
ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
17+
// Note that ptr must be of concrete type T in Wrap.
18+
v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, ptr);
19+
// Keep the layout consistent with BaseObjects.
20+
obj->SetAlignedPointerInInternalField(
21+
kEmbedderType, realm->isolate_data()->embedder_id_for_cppgc());
22+
obj->SetAlignedPointerInInternalField(kSlot, ptr);
23+
realm->TrackCppgcWrapper(ptr);
24+
}
25+
26+
template <typename T>
27+
void CppgcMixin::Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
28+
Wrap(ptr, env->principal_realm(), obj);
29+
}
30+
31+
template <typename T>
32+
T* CppgcMixin::Unwrap(v8::Local<v8::Object> obj) {
33+
// We are not using v8::Object::Unwrap currently because that requires
34+
// access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
35+
// with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
36+
// signature consistent with BaseObject::Unwrap() to avoid churn. Since
37+
// cppgc-managed objects share the same layout as BaseObjects, just unwrap
38+
// from the pointer in the internal field, which should be valid as long as
39+
// the object is still alive.
40+
if (obj->InternalFieldCount() != T::kInternalFieldCount) {
41+
return nullptr;
42+
}
43+
T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(T::kSlot));
44+
return ptr;
45+
}
46+
47+
v8::Local<v8::Object> CppgcMixin::object() const {
48+
return traced_reference_.Get(realm_->isolate());
49+
}
50+
51+
Environment* CppgcMixin::env() const {
52+
return realm_->env();
53+
}
54+
55+
CppgcMixin::~CppgcMixin() {
56+
if (realm_ != nullptr) {
57+
realm_->set_should_purge_empty_cppgc_wrappers(true);
58+
}
59+
}
60+
61+
} // namespace node
62+
63+
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
64+
65+
#endif // SRC_CPPGC_HELPERS_INL_H_

‎src/cppgc_helpers.cc‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
#include "cppgc_helpers.h"
2+
#include "env-inl.h"
3+
4+
namespace node {
5+
6+
void CppgcWrapperList::Cleanup() {
7+
for (auto node : *this) {
8+
CppgcMixin* ptr = node->persistent.Get();
9+
if (ptr != nullptr) {
10+
ptr->Finalize();
11+
}
12+
}
13+
}
14+
15+
void CppgcWrapperList::MemoryInfo(MemoryTracker* tracker) const {
16+
for (auto node : *this) {
17+
CppgcMixin* ptr = node->persistent.Get();
18+
if (ptr != nullptr) {
19+
tracker->Track(ptr);
20+
}
21+
}
22+
}
23+
24+
void CppgcWrapperList::PurgeEmpty() {
25+
for (auto weak_it = begin(); weak_it != end();) {
26+
CppgcWrapperListNode* node = *weak_it;
27+
auto next_it = ++weak_it;
28+
// The underlying cppgc wrapper has already been garbage collected.
29+
// Remove it from the list.
30+
if (!node->persistent) {
31+
node->persistent.Clear();
32+
delete node;
33+
}
34+
weak_it = next_it;
35+
}
36+
}
37+
} // namespace node

‎src/cppgc_helpers.h‎

Lines changed: 61 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,19 @@
66
#include <type_traits> // std::remove_reference
77
#include "cppgc/garbage-collected.h"
88
#include "cppgc/name-provider.h"
9-
#include "env.h"
9+
#include "cppgc/persistent.h"
1010
#include "memory_tracker.h"
11+
#include "util.h"
1112
#include "v8-cppgc.h"
1213
#include "v8-sandbox.h"
1314
#include "v8.h"
1415

1516
namespace node {
1617

18+
class Environment;
19+
class Realm;
20+
class CppgcWrapperListNode;
21+
1722
/**
1823
* This is a helper mixin with a BaseObject-like interface to help
1924
* implementing wrapper objects managed by V8's cppgc (Oilpan) library.
@@ -25,20 +30,29 @@ namespace node {
2530
* with V8's GC scheduling.
2631
*
2732
* A cppgc-managed native wrapper should look something like this, note
28-
* that per cppgc rules, CPPGC_MIXIN(Klass) must be at the left-most
33+
* that per cppgc rules, CPPGC_MIXIN(MyWrap) must be at the left-most
2934
* position in the hierarchy (which ensures cppgc::GarbageCollected
3035
* is at the left-most position).
3136
*
32-
* class Klass final : CPPGC_MIXIN(Klass) {
37+
* class MyWrap final : CPPGC_MIXIN(MyWrap) {
3338
* public:
34-
* SET_CPPGC_NAME(Klass) // Sets the heap snapshot name to "Node / Klass"
39+
* SET_CPPGC_NAME(MyWrap) // Sets the heap snapshot name to "Node / MyWrap"
3540
* void Trace(cppgc::Visitor* visitor) const final {
3641
* CppgcMixin::Trace(visitor);
3742
* visitor->Trace(...); // Trace any additional owned traceable data
3843
* }
3944
* }
45+
*
46+
* If the wrapper needs to perform cleanups when it's destroyed and that
47+
* cleanup relies on a living Node.js `Realm`, it should implement a
48+
* pattern like this:
49+
*
50+
* ~MyWrap() { this->Destroy(); }
51+
* void Clean(Realm* env) override {
52+
* // Do cleanup that relies on a living Environemnt.
53+
* }
4054
*/
41-
class CppgcMixin : public cppgc::GarbageCollectedMixin {
55+
class CppgcMixin : public cppgc::GarbageCollectedMixin, public MemoryRetainer {
4256
public:
4357
// To help various callbacks access wrapper objects with different memory
4458
// management, cppgc-managed objects share the same layout as BaseObjects.
@@ -48,48 +62,58 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin {
4862
// invoked from the child class constructor, per cppgc::GarbageCollectedMixin
4963
// rules.
5064
template <typename T>
51-
static void Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj) {
52-
CHECK_GE(obj->InternalFieldCount(), T::kInternalFieldCount);
53-
ptr->env_ = env;
54-
v8::Isolate* isolate = env->isolate();
55-
ptr->traced_reference_ = v8::TracedReference<v8::Object>(isolate, obj);
56-
v8::Object::Wrap<v8::CppHeapPointerTag::kDefaultTag>(isolate, obj, ptr);
57-
// Keep the layout consistent with BaseObjects.
58-
obj->SetAlignedPointerInInternalField(
59-
kEmbedderType, env->isolate_data()->embedder_id_for_cppgc());
60-
obj->SetAlignedPointerInInternalField(kSlot, ptr);
61-
}
65+
static inline void Wrap(T* ptr, Realm* realm, v8::Local<v8::Object> obj);
66+
template <typename T>
67+
static inline void Wrap(T* ptr, Environment* env, v8::Local<v8::Object> obj);
6268

63-
v8::Local<v8::Object> object() const {
64-
return traced_reference_.Get(env_->isolate());
69+
inline v8::Local<v8::Object> object() const;
70+
inline Environment* env() const;
71+
inline Realm* realm() const { return realm_; }
72+
inline v8::Local<v8::Object> object(v8::Isolate* isolate) const {
73+
return traced_reference_.Get(isolate);
6574
}
6675

67-
Environment* env() const { return env_; }
68-
6976
template <typename T>
70-
static T* Unwrap(v8::Local<v8::Object> obj) {
71-
// We are not using v8::Object::Unwrap currently because that requires
72-
// access to isolate which the ASSIGN_OR_RETURN_UNWRAP macro that we'll shim
73-
// with ASSIGN_OR_RETURN_UNWRAP_GC doesn't take, and we also want a
74-
// signature consistent with BaseObject::Unwrap() to avoid churn. Since
75-
// cppgc-managed objects share the same layout as BaseObjects, just unwrap
76-
// from the pointer in the internal field, which should be valid as long as
77-
// the object is still alive.
78-
if (obj->InternalFieldCount() != T::kInternalFieldCount) {
79-
return nullptr;
80-
}
81-
T* ptr = static_cast<T*>(obj->GetAlignedPointerFromInternalField(T::kSlot));
82-
return ptr;
83-
}
77+
static inline T* Unwrap(v8::Local<v8::Object> obj);
8478

8579
// Subclasses are expected to invoke CppgcMixin::Trace() in their own Trace()
8680
// methods.
8781
void Trace(cppgc::Visitor* visitor) const override {
8882
visitor->Trace(traced_reference_);
8983
}
9084

85+
// TODO(joyeecheung): use ObjectSizeTrait;
86+
inline size_t SelfSize() const override { return sizeof(*this); }
87+
inline bool IsCppgcWrapper() const override { return true; }
88+
89+
// This is run for all the remaining Cppgc wrappers tracked in the Realm
90+
// during Realm shutdown. The destruction of the wrappers would happen later,
91+
// when the final garbage collection is triggered when CppHeap is torn down as
92+
// part of the Isolate teardown. If subclasses of CppgcMixin wish to perform
93+
// cleanups that depend on the Realm during destruction, they should implment
94+
// it in a Clean() override, and then call this->Finalize() from their
95+
// destructor. Outside of Finalize(), subclasses should avoid calling
96+
// into JavaScript or perform any operation that can trigger garbage
97+
// collection during the destruction.
98+
void Finalize() {
99+
if (realm_ == nullptr) return;
100+
this->Clean(realm_);
101+
realm_ = nullptr;
102+
}
103+
104+
// The default implementation of Clean() is a no-op. If subclasses wish
105+
// to perform cleanup that require a living Realm, they should
106+
// should put the cleanups in a Clean() override, and call this->Finalize()
107+
// in the destructor, instead of doing those cleanups directly in the
108+
// destructor.
109+
virtual void Clean(Realm* realm) {}
110+
111+
inline ~CppgcMixin();
112+
113+
friend class CppgcWrapperListNode;
114+
91115
private:
92-
Environment* env_;
116+
Realm* realm_ = nullptr;
93117
v8::TracedReference<v8::Object> traced_reference_;
94118
};
95119

@@ -105,7 +129,8 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin {
105129
#define SET_CPPGC_NAME(Klass) \
106130
inline const char* GetHumanReadableName() const final { \
107131
return "Node / " #Klass; \
108-
}
132+
} \
133+
inline const char* MemoryInfoName() const override { return #Klass; }
109134

110135
/**
111136
* Similar to ASSIGN_OR_RETURN_UNWRAP() but works on cppgc-managed types

0 commit comments

Comments
 (0)