From ea9c3ffaa240fc968832871000832eda1b92934a Mon Sep 17 00:00:00 2001
From: snek <snek@deno.com>
Date: Wed, 16 Oct 2024 12:30:19 +0200
Subject: [PATCH] fix: node-api function call should use preamble (#26297)

`napi_call_function` should use our equiv of NAPI_PREAMBLE (`&mut Env`
instead of `*mut Env`) since it needs to set error codes based on
whether the body of the function raised a JS exception.

Fixes: https://github.com/denoland/deno/issues/26282
---
 cli/napi/js_native_api.rs   | 19 +++++++++----------
 tests/napi/callback_test.js | 11 ++++++++++-
 tests/napi/src/callback.rs  | 30 ++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs
index e922d8c3f2..1d2c99c2c0 100644
--- a/cli/napi/js_native_api.rs
+++ b/cli/napi/js_native_api.rs
@@ -1694,15 +1694,14 @@ fn napi_get_new_target(
 }
 
 #[napi_sym]
-fn napi_call_function(
-  env_ptr: *mut Env,
-  recv: napi_value,
-  func: napi_value,
+fn napi_call_function<'s>(
+  env: &'s mut Env,
+  recv: napi_value<'s>,
+  func: napi_value<'s>,
   argc: usize,
-  argv: *const napi_value,
-  result: *mut napi_value,
+  argv: *const napi_value<'s>,
+  result: *mut napi_value<'s>,
 ) -> napi_status {
-  let env = check_env!(env_ptr);
   check_arg!(env, recv);
   let args = if argc > 0 {
     check_arg!(env, argv);
@@ -1716,11 +1715,11 @@ fn napi_call_function(
   let Some(func) =
     func.and_then(|f| v8::Local::<v8::Function>::try_from(f).ok())
   else {
-    return napi_set_last_error(env, napi_function_expected);
+    return napi_function_expected;
   };
 
   let Some(v) = func.call(&mut env.scope(), recv.unwrap(), args) else {
-    return napi_set_last_error(env_ptr, napi_generic_failure);
+    return napi_generic_failure;
   };
 
   if !result.is_null() {
@@ -1729,7 +1728,7 @@ fn napi_call_function(
     }
   }
 
-  return napi_clear_last_error(env_ptr);
+  napi_ok
 }
 
 #[napi_sym]
diff --git a/tests/napi/callback_test.js b/tests/napi/callback_test.js
index 98622d48d1..c132fefa18 100644
--- a/tests/napi/callback_test.js
+++ b/tests/napi/callback_test.js
@@ -1,6 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
-import { assertEquals, loadTestLibrary } from "./common.js";
+import { assertEquals, assertThrows, loadTestLibrary } from "./common.js";
 
 const callback = loadTestLibrary();
 
@@ -36,3 +36,12 @@ Deno.test("napi callback run with args & recv", function () {
   );
   assertEquals(result, 69);
 });
+
+Deno.test("napi callback handles errors correctly", function () {
+  const e = new Error("hi!");
+  assertThrows(() => {
+    callback.test_callback_throws(() => {
+      throw e;
+    });
+  }, e);
+});
diff --git a/tests/napi/src/callback.rs b/tests/napi/src/callback.rs
index 8909f51768..2512f6a38f 100644
--- a/tests/napi/src/callback.rs
+++ b/tests/napi/src/callback.rs
@@ -8,6 +8,7 @@ use napi_sys::ValueType::napi_object;
 use napi_sys::ValueType::napi_undefined;
 use napi_sys::*;
 use std::ptr;
+use Status::napi_pending_exception;
 
 /// `test_callback_run((a, b) => a + b, [1, 2])` => 3
 extern "C" fn test_callback_run(
@@ -98,6 +99,34 @@ extern "C" fn test_callback_run_with_recv(
   result
 }
 
+extern "C" fn test_callback_throws(
+  env: napi_env,
+  info: napi_callback_info,
+) -> napi_value {
+  let (args, ..) = napi_get_callback_info!(env, info, 1);
+
+  let mut global: napi_value = ptr::null_mut();
+  assert_napi_ok!(napi_get_global(env, &mut global));
+
+  let mut argv = vec![];
+  let mut result: napi_value = ptr::null_mut();
+  assert_eq!(
+    unsafe {
+      napi_call_function(
+        env,
+        global,  // recv
+        args[0], // cb
+        argv.len(),
+        argv.as_mut_ptr(),
+        &mut result,
+      )
+    },
+    napi_pending_exception
+  );
+
+  result
+}
+
 pub fn init(env: napi_env, exports: napi_value) {
   let properties = &[
     napi_new_property!(env, "test_callback_run", test_callback_run),
@@ -106,6 +135,7 @@ pub fn init(env: napi_env, exports: napi_value) {
       "test_callback_run_with_recv",
       test_callback_run_with_recv
     ),
+    napi_new_property!(env, "test_callback_throws", test_callback_throws),
   ];
 
   assert_napi_ok!(napi_define_properties(