fix(napi): clear currently registering module slot (#19249)

This commit fixes problem with loading N-API modules that use 
the "old" way of registration (using "napi_module_register" API).
The slot was not cleared after loading modules, causing subsequent
calls that use the new way of registration (using 
"napi_register_module_v1" API) to try and load the previous module.

Ref #16460

---------

Co-authored-by: Divy Srivastava <[email protected]>

.github/workflows/ci.generate.ts

@@ -17,7 +17,7 @@ const Runners = (() => {
 })();
 // bump the number at the start when you want to purge the cache
 const prCacheKeyPrefix =
-  "29-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-";
+  "31-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-";
 
 const installPkgsCommand =
   "sudo apt-get install --no-install-recommends debootstrap clang-15 lld-15";
@@ -480,7 +480,7 @@ const ci = {
               "~/.cargo/git/db",
             ].join("\n"),
             key:
-              "29-cargo-home-${{ matrix.os }}-${{ hashFiles('Cargo.lock') }}",
+              "31-cargo-home-${{ matrix.os }}-${{ hashFiles('Cargo.lock') }}",
           },
         },
         {

.github/workflows/ci.yml

@@ -293,7 +293,7 @@ jobs:
             ~/.cargo/registry/index
             ~/.cargo/registry/cache
             ~/.cargo/git/db
-          key: '29-cargo-home-${{ matrix.os }}-${{ hashFiles(''Cargo.lock'') }}'
+          key: '31-cargo-home-${{ matrix.os }}-${{ hashFiles(''Cargo.lock'') }}'
         if: '!(github.event_name == ''pull_request'' && matrix.skip_pr)'
       - name: Restore cache build output (PR)
         uses: actions/cache/restore@v3
@@ -305,7 +305,7 @@ jobs:
             !./target/*/*.zip
             !./target/*/*.tar.gz
           key: never_saved
-          restore-keys: '29-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-'
+          restore-keys: '31-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-'
       - name: Apply and update mtime cache
         if: '!(github.event_name == ''pull_request'' && matrix.skip_pr) && (!startsWith(github.ref, ''refs/tags/''))'
         uses: ./.github/mtime_cache
@@ -589,7 +589,7 @@ jobs:
             !./target/*/gn_out
             !./target/*/*.zip
             !./target/*/*.tar.gz
-          key: '29-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
+          key: '31-cargo-target-${{ matrix.os }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
   publish-canary:
     name: publish canary
     runs-on: ubuntu-22.04

cli/napi/env.rs

@@ -136,9 +136,10 @@ fn node_api_get_module_file_name(
 
 #[napi_sym::napi_sym]
 fn napi_module_register(module: *const NapiModule) -> napi_status {
-  MODULE.with(|cell| {
+  MODULE_TO_REGISTER.with(|cell| {
     let mut slot = cell.borrow_mut();
-    slot.replace(module);
+    let prev = slot.replace(module);
+    assert!(prev.is_none());
   });
   napi_ok
 }

ext/napi/lib.rs

@@ -81,9 +81,7 @@ pub const napi_would_deadlock: napi_status = 21;
 pub const NAPI_AUTO_LENGTH: usize = usize::MAX;
 
 thread_local! {
-  pub static MODULE: RefCell<Option<*const NapiModule>> = RefCell::new(None);
-  pub static ASYNC_WORK_SENDER: RefCell<Option<mpsc::UnboundedSender<PendingNapiAsyncWork>>> = RefCell::new(None);
-  pub static THREAD_SAFE_FN_SENDER: RefCell<Option<mpsc::UnboundedSender<ThreadSafeFunctionStatus>>> = RefCell::new(None);
+  pub static MODULE_TO_REGISTER: RefCell<Option<*const NapiModule>> = RefCell::new(None);
 }
 
 type napi_addon_register_func =
@@ -346,15 +344,6 @@ impl Env {
     >,
     tsfn_ref_counters: Arc<Mutex<ThreadsafeFunctionRefCounters>>,
   ) -> Self {
-    let sc = sender.clone();
-    ASYNC_WORK_SENDER.with(|s| {
-      s.replace(Some(sc));
-    });
-    let ts = threadsafe_function_sender.clone();
-    THREAD_SAFE_FN_SENDER.with(|s| {
-      s.replace(Some(ts));
-    });
-
     Self {
       isolate_ptr,
       context: context.into_raw(),
@@ -559,7 +548,6 @@ where
 {
   let permissions = op_state.borrow_mut::<NP>();
   permissions.check(Some(&PathBuf::from(&path)))?;
-
   let (
     async_work_sender,
     tsfn_sender,
@@ -622,77 +610,67 @@ where
     Err(e) => return Err(type_error(e.to_string())),
   };
 
-  MODULE.with(|cell| {
-    let slot = *cell.borrow();
-    let obj = match slot {
-      Some(nm) => {
-        // SAFETY: napi_register_module guarantees that `nm` is valid.
-        let nm = unsafe { &*nm };
-        assert_eq!(nm.nm_version, 1);
-        // SAFETY: we are going blind, calling the register function on the other side.
-        let maybe_exports = unsafe {
-          (nm.nm_register_func)(
-            env_ptr,
-            std::mem::transmute::<v8::Local<v8::Value>, napi_value>(
-              exports.into(),
-            ),
-          )
-        };
-
-        let exports = maybe_exports
-          .as_ref()
-          .map(|_| unsafe {
-            // SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
-            // to a value, they have the same layout
-            std::mem::transmute::<napi_value, v8::Local<v8::Value>>(
-              maybe_exports,
-            )
-          })
-          .unwrap_or_else(|| {
-            // If the module didn't return anything, we use the exports object.
-            exports.into()
-          });
-
-        Ok(serde_v8::Value { v8_value: exports })
-      }
-      None => {
-        // Initializer callback.
-        // SAFETY: we are going blind, calling the register function on the other side.
-        unsafe {
-          let init = library
-            .get::<unsafe extern "C" fn(
-              env: napi_env,
-              exports: napi_value,
-            ) -> napi_value>(b"napi_register_module_v1")
-            .expect("napi_register_module_v1 not found");
-          let maybe_exports = init(
-            env_ptr,
-            std::mem::transmute::<v8::Local<v8::Value>, napi_value>(
-              exports.into(),
-            ),
-          );
-
-          let exports = maybe_exports
-            .as_ref()
-            .map(|_| {
-              // SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
-              // to a value, they have the same layout
-              std::mem::transmute::<napi_value, v8::Local<v8::Value>>(
-                maybe_exports,
-              )
-            })
-            .unwrap_or_else(|| {
-              // If the module didn't return anything, we use the exports object.
-              exports.into()
-            });
-
-          Ok(serde_v8::Value { v8_value: exports })
-        }
+  let maybe_module = MODULE_TO_REGISTER.with(|cell| {
+    let mut slot = cell.borrow_mut();
+    slot.take()
+  });
+
+  if let Some(module_to_register) = maybe_module {
+    // SAFETY: napi_register_module guarantees that `module_to_register` is valid.
+    let nm = unsafe { &*module_to_register };
+    assert_eq!(nm.nm_version, 1);
+    // SAFETY: we are going blind, calling the register function on the other side.
+    let maybe_exports = unsafe {
+      (nm.nm_register_func)(
+        env_ptr,
+        std::mem::transmute::<v8::Local<v8::Value>, napi_value>(exports.into()),
+      )
+    };
+
+    let exports = if maybe_exports.is_some() {
+      // SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
+      // to a value, they have the same layout
+      unsafe {
+        std::mem::transmute::<napi_value, v8::Local<v8::Value>>(maybe_exports)
       }
+    } else {
+      exports.into()
     };
+
     // NAPI addons can't be unloaded, so we're going to "forget" the library
     // object so it lives till the program exit.
     std::mem::forget(library);
-    obj
-  })
+    return Ok(serde_v8::Value { v8_value: exports });
+  }
+
+  // Initializer callback.
+  // SAFETY: we are going blind, calling the register function on the other side.
+
+  let maybe_exports = unsafe {
+    let init = library
+      .get::<unsafe extern "C" fn(
+        env: napi_env,
+        exports: napi_value,
+      ) -> napi_value>(b"napi_register_module_v1")
+      .expect("napi_register_module_v1 not found");
+    init(
+      env_ptr,
+      std::mem::transmute::<v8::Local<v8::Value>, napi_value>(exports.into()),
+    )
+  };
+
+  let exports = if maybe_exports.is_some() {
+    // SAFETY: v8::Local is a pointer to a value and napi_value is also a pointer
+    // to a value, they have the same layout
+    unsafe {
+      std::mem::transmute::<napi_value, v8::Local<v8::Value>>(maybe_exports)
+    }
+  } else {
+    exports.into()
+  };
+
+  // NAPI addons can't be unloaded, so we're going to "forget" the library
+  // object so it lives till the program exit.
+  std::mem::forget(library);
+  Ok(serde_v8::Value { v8_value: exports })
 }

test_napi/.gitignore

@@ -1,4 +1,7 @@
 package-lock.json
 
 # Test generated artifacts
-.swc
+.swc
+*.dylib
+*.so
+*.dll

test_napi/common.js

@@ -9,17 +9,19 @@ export {
 export { fromFileUrl } from "../test_util/std/path/mod.ts";
 
 const targetDir = Deno.execPath().replace(/[^\/\\]+$/, "");
-const [libPrefix, libSuffix] = {
+export const [libPrefix, libSuffix] = {
   darwin: ["lib", "dylib"],
   linux: ["lib", "so"],
   windows: ["", "dll"],
 }[Deno.build.os];
 
+const ops = Deno[Deno.internal].core.ops;
+
 export function loadTestLibrary() {
   const specifier = `${targetDir}/${libPrefix}test_napi.${libSuffix}`;
 
   // Internal, used in ext/node
-  return Deno[Deno.internal].core.ops.op_napi_open(specifier, {
+  return ops.op_napi_open(specifier, {
     Buffer: {},
   });
 }

test_napi/init_test.js

@@ -0,0 +1,14 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+import { assert, libSuffix } from "./common.js";
+
+const ops = Deno[Deno.internal].core.ops;
+
+Deno.test("ctr initialization (napi_module_register)", {
+  ignore: Deno.build.os == "windows",
+}, function () {
+  const path = new URL(`./module.${libSuffix}`, import.meta.url).pathname;
+  const obj = ops.op_napi_open(path, {});
+  assert(obj != null);
+  assert(typeof obj === "object");
+});

test_napi/module.c

@@ -0,0 +1,68 @@
+typedef struct napi_module {
+  int nm_version;
+  unsigned int nm_flags;
+  const char* nm_filename;
+  void* nm_register_func;
+  const char* nm_modname;
+  void* nm_priv;
+  void* reserved[4];
+} napi_module;
+
+#ifdef _WIN32
+#define NAPI_EXTERN __declspec(dllexport)
+#define NAPI_CDECL __cdecl
+#else
+#define NAPI_EXTERN __attribute__((visibility("default")))
+#define NAPI_CDECL
+#endif
+
+NAPI_EXTERN void NAPI_CDECL
+napi_module_register(napi_module* mod);
+
+#if defined(_MSC_VER)
+#if defined(__cplusplus)
+#define NAPI_C_CTOR(fn)                                                        \
+  static void NAPI_CDECL fn(void);                                             \
+  namespace {                                                                  \
+  struct fn##_ {                                                               \
+    fn##_() { fn(); }                                                          \
+  } fn##_v_;                                                                   \
+  }                                                                            \
+  static void NAPI_CDECL fn(void)
+#else  // !defined(__cplusplus)
+#pragma section(".CRT$XCU", read)
+// The NAPI_C_CTOR macro defines a function fn that is called during CRT
+// initialization.
+// C does not support dynamic initialization of static variables and this code
+// simulates C++ behavior. Exporting the function pointer prevents it from being
+// optimized. See for details:
+// https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-initialization?view=msvc-170
+#define NAPI_C_CTOR(fn)                                                        \
+  static void NAPI_CDECL fn(void);                                             \
+  __declspec(dllexport, allocate(".CRT$XCU")) void(NAPI_CDECL * fn##_)(void) = \
+      fn;                                                                      \
+  static void NAPI_CDECL fn(void)
+#endif  // defined(__cplusplus)
+#else
+#define NAPI_C_CTOR(fn)                                                        \
+  static void fn(void) __attribute__((constructor));                           \
+  static void fn(void)
+#endif
+
+#define NAPI_MODULE_TEST(modname, regfunc)                                     \
+  static napi_module _module = {                                               \
+      1,                                                                       \
+      0,                                                                       \
+      __FILE__,                                                                \
+      regfunc,                                                                 \
+      #modname,                                                                \
+      0,                                                                       \
+      {0},                                                                     \
+  };                                                                           \
+  NAPI_C_CTOR(_register_##modname) { napi_module_register(&_module); }         \
+
+void* init(void* env __attribute__((unused)), void* exports) {
+  return exports;
+}
+
+NAPI_MODULE_TEST(TEST_NAPI_MODULE_NAME, init)

test_napi/tests/napi_tests.rs

@@ -18,6 +18,35 @@ fn build() {
   }
   let build_plugin_output = build_plugin.output().unwrap();
   assert!(build_plugin_output.status.success());
+
+  // cc module.c -undefined dynamic_lookup -shared -Wl,-no_fixup_chains -dynamic -o module.dylib
+  #[cfg(not(target_os = "windows"))]
+  {
+    let out = if cfg!(target_os = "macos") {
+      "module.dylib"
+    } else {
+      "module.so"
+    };
+
+    let mut cc = Command::new("cc");
+
+    #[cfg(not(target_os = "macos"))]
+    let c_module = cc.arg("module.c").arg("-shared").arg("-o").arg(out);
+
+    #[cfg(target_os = "macos")]
+    let c_module = {
+      cc.arg("module.c")
+        .arg("-undefined")
+        .arg("dynamic_lookup")
+        .arg("-shared")
+        .arg("-Wl,-no_fixup_chains")
+        .arg("-dynamic")
+        .arg("-o")
+        .arg(out)
+    };
+    let c_module_output = c_module.output().unwrap();
+    assert!(c_module_output.status.success());
+  }
 }
 
 #[test]