src: annotate BaseObjects in the heap snapshots correctly

[joyeecheung]

src: annotate BaseObjects in the heap snapshots correctly

This fixes two issues in the BaseObject views in the heap snapshots:

1. BaseObjects are not conceptually roots when the environment and
   the realms are also showing up in the heap snapshot. Rather, they
   should be considered being held alive by the BaseObjectList in
   the realms, which are in turn held alive by Environment. The
   actual root from the containment view should be the Environment
   instead.
2. The concept of DOM detaching does not really apply to Node.js
   wrappers, and it's confusing to connect that with the weakness
   or detachment (native weakness) of BaseObjects, especially if we
   consider the Environment to be the root instead. To avoid the
   confusion, just restore to the default detachedness for them.

test: use findByRetainingPath in heapdump tests

This makes sure that the tests are run on actual heap snapshots
and prints out missing paths when it cannot be found, which
makes failures easier to debug, and removes the unnecessary
requirement for BaseObjects to be root - which would make
the heap snapshot containment view rather noisy and is not
conceptually correct, since they are actually held by the
BaseObjectList held by the realms.

Previously with this snippet

const v8 = require('v8');
const { Worker } = require('worker_threads');
const worker = new Worker('setInterval(() => {}, 100);', { eval: true });
console.log(v8.writeHeapSnapshot());
worker.terminate();

The containment view looked like this:

the base objects are again listed in Environment -> principal_realm -> base_object_list, but with some of other objects (mostly BindingData objects) shown as detached, it's a bit confusing here because the "detached from DOM" concept does not really apply to Node.js. Node.js does not have a DOM tree, and the nodes here would be shown as detached even when they are not reachable by user JS.

After this patch, the containment view looks like this

The base objects are usually referenced Environment -> principal_realm -> base_object_list which is more conceptually correct, and we no longer show the confusing detached state:

[nodejs-github-bot]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (8e4e4df) to head (11115da).
Report is 178 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57417      +/-   ##
==========================================
- Coverage   90.24%   90.23%   -0.02%     
==========================================
  Files         630      630              
  Lines      185705   185701       -4     
  Branches    36407    36405       -2     
==========================================
- Hits       167591   167559      -32     
- Misses      11002    11014      +12     
- Partials     7112     7128      +16     

Files with missing lines	Coverage Δ
src/base_object-inl.h	84.76% <ø> (-4.78%)	⬇️
src/base_object.cc	84.31% <ø> (-0.31%)	⬇️
src/base_object.h	100.00% <ø> (ø)

... and 26 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.

[addaleax]

1. BaseObjects are not conceptually roots when the environment and
   the realms are also showing up in the heap snapshot. Rather, they
   should be considered being held alive by the BaseObjectList in
   the realms, which are in turn held alive by Environment. The
   actual root from the containment view should be the Environment
   instead.

I would disagree here, tbh? GC roots are objects that are keeping other objects alive but which aren't inherently kept alive by other objects, and that's what non-weak BaseObjects are. If anything, it's the other way around – an Environment, Realm, etc. stays alive because there are non-weak BaseObjects, not the other way around.

[joyeecheung]

If anything, it's the other way around – an Environment, Realm, etc. stays alive because there are non-weak BaseObjects,

We actively clean up BaseObjects when a Realm is shutting down.

node/src/node_realm.cc

Line 218 in 8496670

base_object_list_.Cleanup();

On the other hand, I don't think we do anything to Environment or Realm when BaseObjects are going away other than removing them from the list tracked by the Realm (and the list is only needed for iterating over all the BaseObject from a living Realm, and for doing debugging checks).

https://github.com/nodejs/node/blob/711f449c51297057df9b263d5133581410e1e9bc/src/base_object.cc#L31-L50

Technically, we can create a Realm without any BaseObjects (e.g. for vm contexts, though we aren't currently doing it), just that it would be very limited because BaseObjects are how we expose functionality to JS land. On the other hand, it makes no sense to create a BaseObject without a realm. So I think it's fair to say that BaseObjects are held alive by a realm because it cannot exist on its own, while a Realm can and a Realm can go away when there are still strong BaseObjects in it (specifically all binding data are alive when a Realm goes a way and they are actively purged by the Realm destructor).

I think what you were having in mind is more that a BaseObject cannot go away before its Realm goes away, since a BaseObject cannot live on its own. But that doesn't mean the BaseObject is holding the Realm alive, just means the Realm must clean this BaseObject up before it shuts down itself, which is what happens most of the time, that maps to the concept that a holder should destruct its members before it destructs itself in C++.

[addaleax]

We actively clean up BaseObjects when a Realm is shutting down.

Yeah, but that's a forced clean up, because, as you say, the BaseObject is not allowed to outlive the Realm. That's different from the Realm keeping it alive though.

If you follow that chain of thought further, you could also say that the Realms are each alive because the runtime instance keeps it alive, and at the end you'd say that all objects are kept alive by the process, which is ... technically true in some form, but not really useful information, right?

I guess ultimately there's some lack of clarity for me around the expected semantics of IsRootNode(). I had assumed that it would stand for 'is a GC root' aka 'keeps other objects alive on its own', which non-weak BaseObjects clearly are and do.

[joyeecheung]

I guess ultimately there's some lack of clarity for me around the expected semantics of IsRootNode(). I had assumed that it would stand for 'is a GC root' aka 'keeps other objects alive on its own', which non-weak BaseObjects clearly are and do.

From what I can tell IsRootNode() is more about "whether it shows up as a root in the heap snapshot". Whether they are actual GC roots are independent of that decision, IsRootNode() is mostly about ergonomics in how the heap snapshot is presented. For example if we take a screenshot in Chrome and show it in containment view

We can see that Chrome selectively mark different categories of GC roots as individual roots, or fold them inside other conceptual roots, and leave the rest in a (GC roots) list. While there might be many DOM objects that keeps e.g. event listeners alive, they are not individually marked as GC roots but instead folded under separate Windows.

If you follow that chain of thought further, you could also say that the Realms are each alive because the runtime instance keeps it alive, and at the end you'd say that all objects are kept alive by the process, which is ... technically true in some form, but not really useful information, right?

I think that comes down to a line where the concept of containment stops being helpful. For me having Environment or Realm as the root look more pleasant, but individual BaseObject sitting on the C++/JS boundaries are too low-level and muddles the containment view. Folding them under different Environment/Realm also helps you build a mental picture about which Environment/Realm is holding what. Similarly when I look at a snapshot from the browser, I'd prefer not to see individual DOM objects sitting on the C++/JS boundaries muddling the containment view, and would prefer that they are folded under the Window that they belong to.

[nodejs-github-bot]

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

[joyeecheung]

CI is happy. @legendecas @jasnell can you take a look again? @addaleax are your comments blocking?

[jasnell]

Yeah, still looks good to me but I do think @addaleax raises some good points. I'd like to get a bit more clarification from them.

[joyeecheung]

I am planning to land this if there are no more blocking concerns by the end of week.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in c1b15a4...6e60ab7

[aduh95]

Cherry-picking this on v22.x-staging makes GHA fail: https://github.com/nodejs/node/actions/runs/14868900356/job/41752483961