From 40987178c4f9baf54599b502f943be76f42d6f85 Mon Sep 17 00:00:00 2001
From: ud2 <sjx233@qq.com>
Date: Mon, 8 May 2023 06:27:59 +0800
Subject: fix(core): always report the first error on unhandled rejection
 (#18992)

The root cause of denoland/deno_std#3320, I believe, is that
`pending_promise_rejections` is a `HashMap` whose entries are in
arbitrary order, and as a result either of the two errors (`AddrInUse`
and `TypeError`) may be selected when determining which one to report. I
changed the field to a `VecDeque` so that the first error (`AddrInUse`
in this case) is always selected.
---
 core/bindings.rs       |  4 ++--
 core/ops_builtin_v8.rs |  7 ++++---
 core/realm.rs          | 19 +++----------------
 core/runtime.rs        | 19 ++++++++++++++++++-
 4 files changed, 27 insertions(+), 22 deletions(-)

(limited to 'core')

diff --git a/core/bindings.rs b/core/bindings.rs
index 1437bc657..8ad3948a5 100644
--- a/core/bindings.rs
+++ b/core/bindings.rs
@@ -499,12 +499,12 @@ pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) {
         let error_global = v8::Global::new(scope, error);
         context_state
           .pending_promise_rejections
-          .insert(promise_global, error_global);
+          .push_back((promise_global, error_global));
       }
       PromiseHandlerAddedAfterReject => {
         context_state
           .pending_promise_rejections
-          .remove(&promise_global);
+          .retain(|(key, _)| key != &promise_global);
       }
       PromiseRejectAfterResolved => {}
       PromiseResolveAfterResolved => {
diff --git a/core/ops_builtin_v8.rs b/core/ops_builtin_v8.rs
index a77e7a7e6..8da484225 100644
--- a/core/ops_builtin_v8.rs
+++ b/core/ops_builtin_v8.rs
@@ -894,7 +894,7 @@ fn op_store_pending_promise_rejection<'a>(
   let error_global = v8::Global::new(scope, reason.v8_value);
   context_state
     .pending_promise_rejections
-    .insert(promise_global, error_global);
+    .push_back((promise_global, error_global));
 }
 
 #[op(v8)]
@@ -909,7 +909,7 @@ fn op_remove_pending_promise_rejection<'a>(
   let promise_global = v8::Global::new(scope, promise_value);
   context_state
     .pending_promise_rejections
-    .remove(&promise_global);
+    .retain(|(key, _)| key != &promise_global);
 }
 
 #[op(v8)]
@@ -924,7 +924,8 @@ fn op_has_pending_promise_rejection<'a>(
   let promise_global = v8::Global::new(scope, promise_value);
   context_state
     .pending_promise_rejections
-    .contains_key(&promise_global)
+    .iter()
+    .any(|(key, _)| key == &promise_global)
 }
 
 #[op(v8)]
diff --git a/core/realm.rs b/core/realm.rs
index f907553f0..375f74088 100644
--- a/core/realm.rs
+++ b/core/realm.rs
@@ -7,8 +7,8 @@ use crate::runtime::exception_to_err_result;
 use crate::JsRuntime;
 use anyhow::Error;
 use std::cell::RefCell;
-use std::collections::HashMap;
 use std::collections::HashSet;
+use std::collections::VecDeque;
 use std::hash::BuildHasherDefault;
 use std::hash::Hasher;
 use std::option::Option;
@@ -43,7 +43,7 @@ pub(crate) struct ContextState {
   pub(crate) js_format_exception_cb: Option<Rc<v8::Global<v8::Function>>>,
   pub(crate) js_wasm_streaming_cb: Option<Rc<v8::Global<v8::Function>>>,
   pub(crate) pending_promise_rejections:
-    HashMap<v8::Global<v8::Promise>, v8::Global<v8::Value>>,
+    VecDeque<(v8::Global<v8::Promise>, v8::Global<v8::Value>)>,
   pub(crate) unrefed_ops: HashSet<i32, BuildHasherDefault<IdentityHasher>>,
   // We don't explicitly re-read this prop but need the slice to live alongside
   // the context
@@ -270,22 +270,9 @@ impl<'s> JsRealmLocal<'s> {
     let context_state_rc = self.state(scope);
     let mut context_state = context_state_rc.borrow_mut();
 
-    if context_state.pending_promise_rejections.is_empty() {
+    let Some((_, handle)) = context_state.pending_promise_rejections.pop_front() else {
       return Ok(());
-    }
-
-    let key = {
-      context_state
-        .pending_promise_rejections
-        .keys()
-        .next()
-        .unwrap()
-        .clone()
     };
-    let handle = context_state
-      .pending_promise_rejections
-      .remove(&key)
-      .unwrap();
     drop(context_state);
 
     let exception = v8::Local::new(scope, handle);
diff --git a/core/runtime.rs b/core/runtime.rs
index 8c78be55b..bb77bb25a 100644
--- a/core/runtime.rs
+++ b/core/runtime.rs
@@ -1831,7 +1831,7 @@ impl JsRuntime {
             .state(tc_scope)
             .borrow_mut()
             .pending_promise_rejections
-            .remove(&promise_global);
+            .retain(|(key, _)| key != &promise_global);
         }
       }
       let promise_global = v8::Global::new(tc_scope, promise);
@@ -4138,6 +4138,23 @@ Deno.core.opAsync("op_async_serialize_object_with_numbers_as_keys", {
       .contains("JavaScript execution has been terminated"));
   }
 
+  #[tokio::test]
+  async fn test_unhandled_rejection_order() {
+    let mut runtime = JsRuntime::new(Default::default());
+    runtime
+      .execute_script_static(
+        "",
+        r#"
+        for (let i = 0; i < 100; i++) {
+          Promise.reject(i);
+        }
+        "#,
+      )
+      .unwrap();
+    let err = runtime.run_event_loop(false).await.unwrap_err();
+    assert_eq!(err.to_string(), "Uncaught (in promise) 0");
+  }
+
   #[tokio::test]
   async fn test_set_promise_reject_callback() {
     static PROMISE_REJECT: AtomicUsize = AtomicUsize::new(0);
-- 
cgit v1.2.3