Repository: denoland/deno_core
Pull request #1044 (2025-01-14 05:08:38)

Fix mutliple isolate borrows with #[stack_trace]

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/denoland/deno_core/pull/1044?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) Report
Attention: Patch coverage is `95.00000%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 81.68%. Comparing base [(`0c7f83e`)](https://app.codecov.io/gh/denoland/deno_core/commit/0c7f83ea8daabe8f541fc93ae97529dc446f5f24?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) to head [(`68101ba`)](https://app.codecov.io/gh/denoland/deno_core/commit/68101ba4cda3b817e6ac3837035d183f941a2652?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).
> Report is 232 commits behind head on main.

| [Files with missing lines](https://app.codecov.io/gh/denoland/deno_core/pull/1044?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) | Patch % | Lines |
|---|---|---|
| [testing/checkin/runner/ops.rs](https://app.codecov.io/gh/denoland/deno_core/pull/1044?src=pr&el=tree&filepath=testing%2Fcheckin%2Frunner%2Fops.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland#diff-dGVzdGluZy9jaGVja2luL3J1bm5lci9vcHMucnM=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/denoland/deno_core/pull/1044?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
+ Coverage   81.43%   81.68%   +0.24%     
==========================================
  Files          97      102       +5     
  Lines       23877    27909    +4032     
==========================================
+ Hits        19445    22797    +3352     
- Misses       4432     5112     +680     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/denoland/deno_core/pull/1044?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).


Patch:
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index 95d1da5..cb477f3 100644
--- a/ops/op2/dispatch_fast.rs
+++ b/ops/op2/dispatch_fast.rs
@@ -468,17 +468,6 @@ pub(crate) fn generate_dispatch_fast(
     quote!()
   };
 
-  let with_isolate = if generator_state.needs_fast_isolate
-    && !generator_state.needs_fast_scope
-  {
-    generator_state.needs_opctx = true;
-    gs_quote!(generator_state(opctx, scope) =>
-      (let mut #scope = unsafe { &mut *#opctx.isolate };)
-    )
-  } else {
-    quote!()
-  };
-
   let with_opctx = if generator_state.needs_opctx {
     generator_state.needs_fast_api_callback_options = true;
     gs_quote!(generator_state(opctx, fast_api_callback_options) => {
@@ -492,15 +481,13 @@ pub(crate) fn generate_dispatch_fast(
   };
 
   let with_self = if generator_state.needs_self {
-    generator_state.needs_fast_api_callback_options = true;
+    generator_state.needs_fast_isolate = true;
     let throw_exception = throw_type_error(
       generator_state,
       format!("expected {}", &generator_state.self_ty),
     );
-    gs_quote!(generator_state(self_ty, fast_api_callback_options) => {
-      // SAFETY: Isolate is valid if this function is being called.
-      let isolate = unsafe { &mut *#fast_api_callback_options.isolate };
-      let Some(self_) = deno_core::_ops::try_unwrap_cppgc_object::<#self_ty>(isolate, this.into()) else {
+    gs_quote!(generator_state(self_ty, scope) => {
+      let Some(self_) = deno_core::_ops::try_unwrap_cppgc_object::<#self_ty>(&mut #scope, this.into()) else {
         #throw_exception
       };
       let self_ = &*self_;
@@ -509,6 +496,17 @@ pub(crate) fn generate_dispatch_fast(
     quote!()
   };
 
+  let with_isolate = if generator_state.needs_fast_isolate
+    && !generator_state.needs_fast_scope
+    && !generator_state.needs_stack_trace
+  {
+    generator_state.needs_fast_api_callback_options = true;
+    gs_quote!(generator_state(scope, fast_api_callback_options) =>
+      (let mut #scope = unsafe { &mut *#fast_api_callback_options.isolate };)
+    )
+  } else {
+    quote!()
+  };
   let with_scope = if generator_state.needs_scope {
     let create_scope = create_scope(generator_state);
     gs_quote!(generator_state(scope) => {
@@ -591,8 +589,8 @@ pub(crate) fn generate_dispatch_fast(
       #with_opstate;
       #with_stack_trace
       #with_js_runtime_state
-      #with_self
       #with_isolate
+      #with_self
       let #result = {
         #(#call_args)*
         #call (#(#call_names),*)
@@ -632,6 +630,7 @@ fn map_v8_fastcall_arg_to_arg(
     needs_scope,
     needs_opctx,
     needs_fast_api_callback_options,
+    needs_fast_isolate,
     needs_js_runtime_state,
     ..
   } = generator_state;
@@ -799,13 +798,11 @@ fn map_v8_fastcall_arg_to_arg(
       let ty =
         syn::parse_str::<syn::Path>(ty).expect("Failed to reparse state type");
 
-      *needs_fast_api_callback_options = true;
+      *needs_fast_isolate = true;
       let throw_exception =
         throw_type_error(generator_state, format!("expected {ty:?}"));
-      gs_quote!(generator_state(fast_api_callback_options) => {
-        // SAFETY: Isolate is valid if this function is being called.
-        let isolate = unsafe { &mut *#fast_api_callback_options.isolate };
-        let Some(#arg_ident) = deno_core::_ops::try_unwrap_cppgc_object::<#ty>(isolate, #arg_ident) else {
+      gs_quote!(generator_state(scope) => {
+        let Some(#arg_ident) = deno_core::_ops::try_unwrap_cppgc_object::<#ty>(&mut #scope, #arg_ident) else {
           #throw_exception
         };
         let #arg_ident = &*#arg_ident;
@@ -815,15 +812,13 @@ fn map_v8_fastcall_arg_to_arg(
       let ty =
         syn::parse_str::<syn::Path>(ty).expect("Failed to reparse state type");
 
-      *needs_fast_api_callback_options = true;
+      *needs_fast_isolate = true;
       let throw_exception =
         throw_type_error(generator_state, format!("expected {ty:?}"));
-      gs_quote!(generator_state(fast_api_callback_options) => {
-        // SAFETY: Isolate is valid if this function is being called.
-        let isolate = unsafe { &mut *#fast_api_callback_options.isolate };
+      gs_quote!(generator_state(scope) => {
         let #arg_ident = if #arg_ident.is_null_or_undefined() {
           None
-        } else if let Some(#arg_ident) = deno_core::_ops::try_unwrap_cppgc_object::<#ty>(isolate, #arg_ident) {
+        } else if let Some(#arg_ident) = deno_core::_ops::try_unwrap_cppgc_object::<#ty>(&mut #scope, #arg_ident) {
           Some(#arg_ident)
         } else {
           #throw_exception


Test patch:
diff --git a/ops/op2/test_cases/sync/cppgc_resource.out b/ops/op2/test_cases/sync/cppgc_resource.out
index ac8eeaa..541cef8 100644
--- a/ops/op2/test_cases/sync/cppgc_resource.out
+++ b/ops/op2/test_cases/sync/cppgc_resource.out
@@ -100,11 +100,11 @@ const fn op_cppgc_object() -> ::deno_core::_ops::OpDecl {
             let fast_api_callback_options: &'s mut _ = unsafe {
                 &mut *fast_api_callback_options
             };
+            let mut scope = unsafe { &mut *fast_api_callback_options.isolate };
             let result = {
-                let isolate = unsafe { &mut *fast_api_callback_options.isolate };
                 let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Wrap,
-                >(isolate, arg0) else {
+                >(&mut scope, arg0) else {
                     {
                         let mut scope = unsafe {
                             deno_core::v8::CallbackScope::new(
@@ -293,13 +293,13 @@ const fn op_option_cppgc_object() -> ::deno_core::_ops::OpDecl {
             let fast_api_callback_options: &'s mut _ = unsafe {
                 &mut *fast_api_callback_options
             };
+            let mut scope = unsafe { &mut *fast_api_callback_options.isolate };
             let result = {
-                let isolate = unsafe { &mut *fast_api_callback_options.isolate };
                 let arg0 = if arg0.is_null_or_undefined() {
                     None
                 } else if let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Wrap,
-                >(isolate, arg0) {
+                >(&mut scope, arg0) {
                     Some(arg0)
                 } else {
                     {
@@ -588,11 +588,11 @@ const fn op_use_cppgc_object() -> ::deno_core::_ops::OpDecl {
             let fast_api_callback_options: &'s mut _ = unsafe {
                 &mut *fast_api_callback_options
             };
+            let mut scope = unsafe { &mut *fast_api_callback_options.isolate };
             let result = {
-                let isolate = unsafe { &mut *fast_api_callback_options.isolate };
                 let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Wrap,
-                >(isolate, arg0) else {
+                >(&mut scope, arg0) else {
                     {
                         let mut scope = unsafe {
                             deno_core::v8::CallbackScope::new(
@@ -878,11 +878,11 @@ const fn op_use_cppgc_object_option() -> ::deno_core::_ops::OpDecl {
             let fast_api_callback_options: &'s mut _ = unsafe {
                 &mut *fast_api_callback_options
             };
+            let mut scope = unsafe { &mut *fast_api_callback_options.isolate };
             let result = {
-                let isolate = unsafe { &mut *fast_api_callback_options.isolate };
                 let Some(arg0) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Wrap,
-                >(isolate, arg0) else {
+                >(&mut scope, arg0) else {
                     {
                         let mut scope = unsafe {
                             deno_core::v8::CallbackScope::new(
diff --git a/ops/op2/test_cases/sync/object_wrap.out b/ops/op2/test_cases/sync/object_wrap.out
index 1eaa1d7..94b8ace 100644
--- a/ops/op2/test_cases/sync/object_wrap.out
+++ b/ops/op2/test_cases/sync/object_wrap.out
@@ -565,10 +565,10 @@ impl Foo {
                 let fast_api_callback_options: &'s mut _ = unsafe {
                     &mut *fast_api_callback_options
                 };
-                let isolate = unsafe { &mut *fast_api_callback_options.isolate };
+                let mut scope = unsafe { &mut *fast_api_callback_options.isolate };
                 let Some(self_) = deno_core::_ops::try_unwrap_cppgc_object::<
                     Foo,
-                >(isolate, this.into()) else {
+                >(&mut scope, this.into()) else {
                     {
                         let mut scope = unsafe {
                             deno_core::v8::CallbackScope::new(
diff --git a/testing/checkin/runner/ops.rs b/testing/checkin/runner/ops.rs
index 528257d..83b7c5b 100644
--- a/testing/checkin/runner/ops.rs
+++ b/testing/checkin/runner/ops.rs
@@ -212,6 +212,10 @@ impl DOMPoint {
   #[fast]
   #[symbol("symbolMethod")]
   fn with_symbol(&self) {}
+
+  #[fast]
+  #[stack_trace]
+  fn with_stack_trace(&self) {}
 }
 
 #[repr(u8)]