[joyeecheung]

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.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64424/

[codecov]

Codecov Report

Attention: Patch coverage is 58.21918% with 61 lines in your changes missing coverage. Please review.

Project coverage is 90.12%. Comparing base (4bfcad1) to head (bdd34b1).
Report is 286 commits behind head on main.

Files with missing lines	Patch %	Lines
src/memory_tracker-inl.h	22.41%	42 Missing and 3 partials ⚠️
src/cppgc_helpers.cc	75.00%	4 Missing and 2 partials ⚠️
src/cppgc_helpers-inl.h	86.20%	1 Missing and 3 partials ⚠️
src/cppgc_helpers.h	66.66%	4 Missing ⚠️
src/node_contextify.h	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56534      +/-   ##
==========================================
- Coverage   90.14%   90.12%   -0.03%     
==========================================
  Files         630      632       +2     
  Lines      186784   186905     +121     
  Branches    36653    36682      +29     
==========================================
+ Hits       168377   168447      +70     
- Misses      11205    11247      +42     
- Partials     7202     7211       +9     

Files with missing lines	Coverage Δ
src/memory_tracker.h	100.00% <100.00%> (ø)
src/node_realm-inl.h	89.83% <100.00%> (+0.73%)	⬆️
src/node_realm.cc	75.14% <100.00%> (+1.73%)	⬆️
src/node_realm.h	100.00% <100.00%> (ø)
src/node_contextify.h	70.58% <0.00%> (-9.42%)	⬇️
src/cppgc_helpers-inl.h	86.20% <86.20%> (ø)
src/cppgc_helpers.h	73.33% <66.66%> (-13.04%)	⬇️
src/cppgc_helpers.cc	75.00% <75.00%> (ø)
src/memory_tracker-inl.h	57.43% <22.41%> (-11.51%)	⬇️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64638/

[joyeecheung]

Trying to rebase #51017 on top of it to verify the "use list for heap snapshot integration" idea. It seems we'll need to update the MemoryTracker code to stop creating duplicate nodes for cppgc wrappers (either that, or use something different for the cppgc wrapper list). Still looking into it..

[jasnell]

Some of these rules may be non-obvious to many developers. For example, let's say someone has a wrapped object that is holding a BaseObjectPtr<Foo> as a member... Just based on reading this documentation, as a newcomer I would not know which of these conditions may apply. Do I need to clear the BaseObjectPtr in the CleanEnvResource callback? What about if it has a v8::Global<...> member field? Is it ok to just let that be cleared by the default destructor? etc. I think calling out a few more detailed examples (and not partial examples like you have currently) of each case would be most helpful.

[joyeecheung]

let's say someone has a wrapped object that is holding a BaseObjectPtr<Foo> as a member

I don't think this would be supported in the nearest future, or will ever be supported - a cppgc-object should not hold on to a BaseObjectPtr<Foo> without converting that BaseObjectPtr<Foo> to cppgc-managed first - and then in that case they should use cppgc::Member<Foo> to hold on to that instead. The migration needs to be done bottom-to-top. In the case of using cppgc::Member<Foo>, nothing special needs to be done in the destructor, V8 will handle that properly.

What about if it has a v8::Global<...> member field?

I think in most cases, this should be v8::TracedReference instead. In that case nothing special needs to be done in the destructor. This has been documented in the Creating C++ to JavaScript references in cppgc-managed objects section.

[joyeecheung]

Added a note about not supporting cross BaseObject and cppgc-managed wrapeprs. I don't think it's impossible but someone needs to implement some helpers for that first (I am not 100% sure what needs to be done, but using cppgc::Persistent would be a start).

[jasnell]

I wonder if we can somehow have c++ linting detect it for us if someone includes a BaseObjectPtr member in one of these? .... in any case, this all sounds fine, I'd just like to make sure this gets included somehow in the documentation.

[joyeecheung]

If it comes up in the future that some people attempt to use BaseObjectPtr in a cppgc wrapper, we can consider adding some linting rules. For now it seems a bit too premature. Since we are still in an early phase of cppgc migration, I think the rule of thumb is probably:

• If you are not looking into also updating the cppgc helpers, only use cppgc on classes that aren't more than what existing cppgc wrappers are doing (at this point in time, v8 handles and MemoryRetainer over other plain types are supported, cppgc members are supposed to be supported by we haven't tried it internally yet)
• If you want to tread on untrodden paths and manage more complex memory structures, like a cppgc wrapper referencing BaseObject, expect to figure out how to make it work and update the cppgc helpers yourself.

[joyeecheung]

I did a few updates to the design:

• The cleanup pattern is now Finalize() calling Clean() which takes a Realm. I think Finalize() sounds better and is less confusing.
• The wrapper list is now per-Realm instead of per-Environment, similar to how BaseObjects are managed.
• This makes the CppgcMixin a MemoryRetainer and includes de-duplication code of the MemoryRetainerNode, so that subclasses can implement MemoryInfo to track additional data just like before. For an example of what it looks like, see wip: crypto: use cppgc to manage Hash #51017 (comment)