diff --git a/src/README.md b/src/README.md index 300b74c28d505c..5c21bcac22b31a 100644 --- a/src/README.md +++ b/src/README.md @@ -1041,6 +1041,17 @@ class MyWrap final : CPPGC_MIXIN(MyWrap) { } ``` +If the wrapper needs to perform cleanups when it's destroyed and that +cleanup relies on a living Node.js `Environment`, it should implement a +pattern like this: + +```c++ + ~MyWrap() { this->Clean(); } + void CleanEnvResource(Environment* env) override { + // Do cleanup that relies on a living Environemnt. + } +``` + `cppgc::GarbageCollected` types are expected to implement a `void Trace(cppgc::Visitor* visitor) const` method. When they are the final class in the hierarchy, this method must be marked `final`. For @@ -1195,6 +1206,8 @@ referrer->Set( ).ToLocalChecked(); ``` +#### Lifetime and cleanups of cppgc-managed objects + Typically, a newly created cppgc-managed wrapper object should be held alive by the JavaScript land (for example, by being returned by a method and staying alive in a closure). Long-lived cppgc objects can also @@ -1206,6 +1219,27 @@ it, this can happen at any time after the garbage collector notices that it's no longer reachable and before the V8 isolate is torn down. See the [Oilpan documentation in Chromium][] for more details. +If the cppgc-managed objects does not need to perform any special cleanup, +it's fine to use the default destructor. If it needs to perform only trivial +cleanup that only affects its own members without calling into JS, potentially +triggering garbage collection or relying on a living `Environment`, then it's +fine to just implement the trivial cleanup in the destructor directly. If it +needs to do any substantial cleanup that involves a living `Environment`, because +the destructor can be called at any time by the garbage collection, even after +the `Environment` is already gone, it must implement the cleanup with this pattern: + +```c++ + ~MyWrap() { this->Clean(); } + void CleanEnvResource(Environment* env) override { + // Do cleanup that relies on a living Environemnt. This would be + // called by CppgcMixin::Clean() first during Environment shutdown, + // while the Environment is still alive. If the destructor calls + // Clean() again later during garbage collection that happens after + // Environment shutdown, CleanEnvResource() would be skipped, preventing + // invalid access to the Environment. + } +``` + ### Callback scopes The public `CallbackScope` and the internally used `InternalCallbackScope` diff --git a/src/cppgc_helpers.h b/src/cppgc_helpers.h index 20b9004917cfbe..6e7385e84fef7d 100644 --- a/src/cppgc_helpers.h +++ b/src/cppgc_helpers.h @@ -25,20 +25,30 @@ namespace node { * with V8's GC scheduling. * * A cppgc-managed native wrapper should look something like this, note - * that per cppgc rules, CPPGC_MIXIN(Klass) must be at the left-most + * that per cppgc rules, CPPGC_MIXIN(MyWrap) must be at the left-most * position in the hierarchy (which ensures cppgc::GarbageCollected * is at the left-most position). * - * class Klass final : CPPGC_MIXIN(Klass) { + * class MyWrap final : CPPGC_MIXIN(MyWrap) { * public: - * SET_CPPGC_NAME(Klass) // Sets the heap snapshot name to "Node / Klass" + * SET_CPPGC_NAME(MyWrap) // Sets the heap snapshot name to "Node / MyWrap" * void Trace(cppgc::Visitor* visitor) const final { * CppgcMixin::Trace(visitor); * visitor->Trace(...); // Trace any additional owned traceable data * } * } + * + * If the wrapper needs to perform cleanups when it's destroyed and that + * cleanup relies on a living Node.js `Environment`, it should implement a + * pattern like this: + * + * ~MyWrap() { this->Clean(); } + * void CleanEnvResource(Environment* env) override { + * // Do cleanup that relies on a living Environemnt. + * } */ -class CppgcMixin : public cppgc::GarbageCollectedMixin { +class CppgcMixin : public cppgc::GarbageCollectedMixin, + public CppgcWrapperListNode { public: // To help various callbacks access wrapper objects with different memory // management, cppgc-managed objects share the same layout as BaseObjects. @@ -58,6 +68,7 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin { obj->SetAlignedPointerInInternalField( kEmbedderType, env->isolate_data()->embedder_id_for_cppgc()); obj->SetAlignedPointerInInternalField(kSlot, ptr); + env->cppgc_wrapper_list()->PushFront(ptr); } v8::Local object() const { @@ -88,8 +99,29 @@ class CppgcMixin : public cppgc::GarbageCollectedMixin { visitor->Trace(traced_reference_); } + // This implements CppgcWrapperListNode::Clean and is run for all the + // remaining Cppgc wrappers tracked in the Environment during Environment + // shutdown. The destruction of the wrappers would happen later, when the + // final garbage collection is triggered when CppHeap is torn down as part of + // the Isolate teardown. If subclasses of CppgcMixin wish to perform cleanups + // that depend on the Environment during destruction, they should implment it + // in a CleanEnvResource() override, and then call this->Clean() from their + // destructor. Outside of CleanEnvResource(), subclasses should avoid calling + // into JavaScript or perform any operation that can trigger garbage + // collection during the destruction. + void Clean() override { + if (env_ == nullptr) return; + this->CleanEnvResource(env_); + env_ = nullptr; + } + + // The default implementation of CleanEnvResource() is a no-op. Subclasses + // should override it to perform cleanup that require a living Environment, + // instead of doing these cleanups directly in the destructor. + virtual void CleanEnvResource(Environment* env) {} + private: - Environment* env_; + Environment* env_ = nullptr; v8::TracedReference traced_reference_; }; diff --git a/src/env.cc b/src/env.cc index f0f97244fdef63..f021fdad22f422 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1290,6 +1290,8 @@ void Environment::RunCleanup() { CleanupHandles(); } + for (CppgcWrapperListNode* handle : cppgc_wrapper_list_) handle->Clean(); + for (const int fd : unmanaged_fds_) { uv_fs_t close_req; uv_fs_close(nullptr, &close_req, fd, nullptr); diff --git a/src/env.h b/src/env.h index ec5b608cede6a1..4b385c1fa75d68 100644 --- a/src/env.h +++ b/src/env.h @@ -615,6 +615,12 @@ class Cleanable { friend class Environment; }; +class CppgcWrapperListNode { + public: + virtual void Clean() = 0; + ListNode wrapper_list_node; +}; + /** * Environment is a per-isolate data structure that represents an execution * environment. Each environment has a principal realm. An environment can @@ -703,6 +709,7 @@ class Environment final : public MemoryRetainer { Realm* realm, const ContextInfo& info); void UnassignFromContext(v8::Local context); + void UntrackContext(v8::Local context); void TrackShadowRealm(shadow_realm::ShadowRealm* realm); void UntrackShadowRealm(shadow_realm::ShadowRealm* realm); @@ -910,12 +917,18 @@ class Environment final : public MemoryRetainer { typedef ListHead HandleWrapQueue; typedef ListHead ReqWrapQueue; typedef ListHead CleanableQueue; + typedef ListHead + CppgcWrapperList; inline HandleWrapQueue* handle_wrap_queue() { return &handle_wrap_queue_; } inline CleanableQueue* cleanable_queue() { return &cleanable_queue_; } inline ReqWrapQueue* req_wrap_queue() { return &req_wrap_queue_; } + inline CppgcWrapperList* cppgc_wrapper_list() { + return &cppgc_wrapper_list_; + } // https://w3c.github.io/hr-time/#dfn-time-origin inline uint64_t time_origin() { @@ -1084,7 +1097,6 @@ class Environment final : public MemoryRetainer { v8::Local), const char* errmsg); void TrackContext(v8::Local context); - void UntrackContext(v8::Local context); std::list loaded_addons_; v8::Isolate* const isolate_; @@ -1203,6 +1215,8 @@ class Environment final : public MemoryRetainer { CleanableQueue cleanable_queue_; HandleWrapQueue handle_wrap_queue_; ReqWrapQueue req_wrap_queue_; + CppgcWrapperList cppgc_wrapper_list_; + int handle_cleanup_waiting_ = 0; int request_waiting_ = 0;