fixup! lib: allow CJS source map cache to be reclaimed

Author: legendecas

lib/internal/source_map/source_map_cache_map.js

@@ -5,94 +5,99 @@ const {
   ObjectFreeze,
   SafeFinalizationRegistry,
   SafeMap,
-  SafeWeakMap,
   SafeWeakRef,
   SymbolIterator,
 } = primordials;
+const {
+  privateSymbols: {
+    source_map_data_private_symbol,
+  },
+} = internalBinding('util');
 
 /**
- * Specialized WeakMap that caches source map entries by `filename` and `sourceURL`.
- * Cached entries can be iterated with `for..of`.
+ * Specialized map of WeakRefs to module instances that caches source map
+ * entries by `filename` and `sourceURL`. Cached entries can be iterated with
+ * `for..of` syntax.
  *
  * The cache map maintains the cache entries by:
- * - `weakTargetMap`(Map): a strong sourceURL -> WeakRef(Module),
- * - `weakMap`(WeakMap): a Module instance object -> source map data.
+ * - `weakModuleMap`(Map): a strong sourceURL -> WeakRef(Module),
+ * - WeakRef(Module[source_map_data_private_symbol]): source map data.
  *
- * Obsolete `weakTargetMap` entries are removed by the `finalizationRegistry` callback.
- * This pattern decouples the strong url reference to the source map data and allow the
- * cache to be reclaimed eagerly, without depending on a undeterministic callback of a
- * finalization registry.
+ * Obsolete `weakModuleMap` entries are removed by the `finalizationRegistry`
+ * callback. This pattern decouples the strong url reference to the source map
+ * data and allow the cache to be reclaimed eagerly, without depending on an
+ * undeterministic callback of a finalization registry.
  */
 class SourceMapCacheMap {
   /**
    * @type {Map<string, WeakRef<*>>}
    * The cached module instance can be removed from the global module registry
    * with approaches like mutating `require.cache`.
-   * The `weakTargetMap` exposes entries by `filename` and `sourceURL`.
+   * The `weakModuleMap` exposes entries by `filename` and `sourceURL`.
    * In the case of mutated module registry, obsolete entries are removed from
    * the cache by the `finalizationRegistry`.
    */
-  #weakTargetMap = new SafeMap();
-  #weakMap = new SafeWeakMap();
+  #weakModuleMap = new SafeMap();
 
   #cleanup = ({ keys }) => {
     // Delete the entry if the weak target has been reclaimed.
     // If the weak target is not reclaimed, the entry was overridden by a new
     // weak target.
     ArrayPrototypeForEach(keys, (key) => {
-      const ref = this.#weakTargetMap.get(key);
+      const ref = this.#weakModuleMap.get(key);
       if (ref && ref.deref() === undefined) {
-        this.#weakTargetMap.delete(key);
+        this.#weakModuleMap.delete(key);
       }
     });
   };
   #finalizationRegistry = new SafeFinalizationRegistry(this.#cleanup);
 
   /**
-   * Sets the value for the given key, associated with the given weakTarget.
+   * Sets the value for the given key, associated with the given module
+   * instance.
    * @param {string[]} keys array of urls to index the value entry.
-   * @param {*} value the value entry.
-   * @param {object} weakTarget an object that can be weakly referenced and
-   * invalidate the [key, value] entry once this object is reclaimed.
+   * @param {*} sourceMapData the value entry.
+   * @param {object} moduleInstance an object that can be weakly referenced and
+   * invalidate the [key, value] entry after this object is reclaimed.
    */
-  set(keys, value, weakTarget) {
-    const weakRef = new SafeWeakRef(weakTarget);
-    ArrayPrototypeForEach(keys, (key) => this.#weakTargetMap.set(key, weakRef));
-    this.#weakMap.set(weakTarget, value);
-    this.#finalizationRegistry.register(weakTarget, { keys });
+  set(keys, sourceMapData, moduleInstance) {
+    const weakRef = new SafeWeakRef(moduleInstance);
+    ArrayPrototypeForEach(keys, (key) => this.#weakModuleMap.set(key, weakRef));
+    moduleInstance[source_map_data_private_symbol] = sourceMapData;
+    this.#finalizationRegistry.register(moduleInstance, { keys });
   }
 
   /**
    * Get an entry by the given key.
    * @param {string} key a file url or source url
    */
   get(key) {
-    const weakRef = this.#weakTargetMap.get(key);
-    const target = weakRef?.deref();
-    if (target === undefined) {
+    const weakRef = this.#weakModuleMap.get(key);
+    const moduleInstance = weakRef?.deref();
+    if (moduleInstance === undefined) {
       return;
     }
-    return this.#weakMap.get(target);
+    return moduleInstance[source_map_data_private_symbol];
   }
 
   /**
    * Estimate the size of the cache. The actual size may be smaller because
    * some entries may be reclaimed with the module instance.
    */
   get size() {
-    return this.#weakTargetMap.size;
+    return this.#weakModuleMap.size;
   }
 
   [SymbolIterator]() {
-    const iterator = this.#weakTargetMap.entries();
+    const iterator = this.#weakModuleMap.entries();
 
     const next = () => {
       const result = iterator.next();
       if (result.done) return result;
       const { 0: key, 1: weakRef } = result.value;
-      const target = weakRef.deref();
-      if (target == null) return next();
-      const value = this.#weakMap.get(target);
+      const moduleInstance = weakRef.deref();
+      if (moduleInstance == null) return next();
+      const value = moduleInstance[source_map_data_private_symbol];
       return { done: false, value: [key, value] };
     };
 

src/env_properties.h

@@ -29,7 +29,8 @@
   V(untransferable_object_private_symbol, "node:untransferableObject")         \
   V(exit_info_private_symbol, "node:exit_info_private_symbol")                 \
   V(promise_trace_id, "node:promise_trace_id")                                 \
-  V(require_private_symbol, "node:require_private_symbol")
+  V(require_private_symbol, "node:require_private_symbol")                     \
+  V(source_map_data_private_symbol, "node:source_map_data_private_symbol")
 
 // Symbols are per-isolate primitives but Environment proxies them
 // for the sake of convenience.

src/module_wrap.cc

@@ -248,6 +248,14 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
+  // Initialize an empty slot for source map cache before the object is frozen.
+  if (that->SetPrivate(context,
+                       realm->isolate_data()->source_map_data_private_symbol(),
+                       Undefined(isolate))
+          .IsNothing()) {
+    return;
+  }
+
   // Use the extras object as an object whose GetCreationContext() will be the
   // original `context`, since the `Context` itself strictly speaking cannot
   // be stored in an internal field.