Merge pull request #6662 from dotCooCoo/fix/script-finally-return-drops-throw-site

Scripts: a return from a finally block drops the discarded error's throw site

crates/js/src/interp.rs

@@ -300,6 +300,12 @@ pub(crate) const MAX_APPLY_ARGS: usize = 1 << 16;
 /// The estimated bytes of a name's entries in the list and the set `for-in` builds.
 const KEY_ENTRY_BYTES: usize = 32;
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: the [`Interp::finally_sites`] entries that leaving a block or a frame has looked at.
+    pub(crate) static SITE_CHECKS: std::cell::Cell<u64> = const { std::cell::Cell::new(0) };
+}
+
 /// The estimated bytes of the index names `0` to `len - 1` while `for-in` lists them.
 fn index_keys_bytes(len: usize) -> usize {
     let mut total = 0usize;
@@ -619,9 +625,16 @@ impl Interp {
     }
 
     /// Removes the [`Self::finally_sites`] entries whose stack slot is at or above `len`: those of
-    /// `finally` blocks that were left.
+    /// `finally` blocks that were left. The slots rise along the list, so only its end is looked at.
     fn drop_left_finally_sites(&mut self, len: usize) {
-        self.finally_sites.retain(|&(_, slot)| slot < len);
+        while let Some(&(_, slot)) = self.finally_sites.last() {
+            #[cfg(test)]
+            SITE_CHECKS.with(|c| c.set(c.get() + 1));
+            if slot < len {
+                break;
+            }
+            self.finally_sites.pop();
+        }
     }
 
     pub(crate) fn collect_garbage(&mut self) {
@@ -784,6 +797,8 @@ impl Interp {
     fn return_value(&mut self, v: Value) -> JsResult<Option<Value>> {
         let Some(frame) = self.frames.pop() else { return internal("return without a frame") };
         self.stack.truncate(frame.base);
+        // A `return` in a finally block leaves the block, and the value takes its exception's slot.
+        self.drop_left_finally_sites(frame.base);
         let v = match frame.construct {
             Some(o) if !matches!(v, Value::Object(_)) => Value::Object(o),
             _ => v,

crates/js/src/interp_tests.rs

@@ -3,7 +3,7 @@ use std::rc::Rc;
 use proptest::prelude::*;
 
 use crate::bytecode::{Code, Handler, Op, Source, verify};
-use crate::{JsValue, LimitKind, Limits, Options, Realm, RunError, RunErrorKind};
+use crate::{JsValue, LimitKind, Limits, Options, Progress, Realm, RunError, RunErrorKind};
 
 fn run(src: &str) -> JsValue {
     match Realm::new().run(src) {
@@ -357,6 +357,58 @@ fn function_source_text_counts_toward_the_heap() {
     assert_eq!(realm.run("(function f(a) { return a; }).toString()"), Ok(JsValue::String("function f(a) { return a; }".into())));
 }
 
+#[test]
+fn a_function_that_returns_from_a_finally_block_does_not_keep_its_code() {
+    // The function's source holds a 256 Ki-unit comment. Its result stays on the stack while the
+    // second call runs, after the function was dropped. A collection then leaves its code out of
+    // the live total, as it does for a function whose finally block ran without an exception.
+    let live = |body: &str| {
+        let mut r = small(Limits::default());
+        let src = format!(
+            "var s = 'x'; for (var i = 0; i < 18; i++) s += s;\n\
+             var f = Function('/*' + s + '*/ try {{ {body} }} finally {{ return 0; }}'); s = null;\n\
+             f() + (f = null, (function () {{ var a = 0; for (var i = 0; i < 50; i++) a += i; return a; }})())"
+        );
+        if let Err(e) = r.start(&src) {
+            panic!("{e:?}");
+        }
+        let mut bytes = None;
+        loop {
+            match r.advance(1) {
+                Ok(Progress::Paused) => {
+                    let i = r.interp();
+                    if i.frames.len() == 2 && i.stack.len() > 1 {
+                        i.collect_garbage();
+                        bytes = Some(i.heap.bytes);
+                    }
+                }
+                Ok(Progress::Done(v)) => {
+                    assert_eq!(v, JsValue::Number(1225.0));
+                    break;
+                }
+                Err(e) => panic!("{e:?}"),
+            }
+        }
+        let Some(bytes) = bytes else { panic!("the second call never paused") };
+        bytes
+    };
+    let (plain, thrown) = (live(""), live("throw 1;"));
+    assert!(thrown < plain + 4096, "{thrown} > {plain}");
+}
+
+#[test]
+fn a_return_looks_only_at_the_finally_sites_of_its_own_frame() {
+    // 200 finally blocks run at once, each for its own exception, and the innermost calls a
+    // function 1,000 times. Each return looks at the last saved site only, not at all 200.
+    let src = "function g() { return 1; }\n\
+               function f(n) { try { throw n; } finally { if (n) { f(n - 1); } else { for (var i = 0; i < 1000; i++) g(); } } }\n\
+               try { f(199); } catch (e) { e; }";
+    crate::interp::SITE_CHECKS.with(|c| c.set(0));
+    assert_eq!(small(Limits::default()).run(src), Ok(JsValue::Number(0.0)));
+    let checks = crate::interp::SITE_CHECKS.with(std::cell::Cell::get);
+    assert!(checks < 10_000, "{checks} saved sites looked at");
+}
+
 #[test]
 fn for_in_names_take_fuel_and_heap_under_later_limits() {
     // The first run, under the default limits, builds an object with many properties. Later runs