Skip to content

Commit 3cec5e6

Browse files
tkshsbcueRahul-2k4
andauthored
fix(vm): reset implicit return value when pushing new call frames (#4… (#5313)
# PR: fix(vm): reset implicit return value when pushing new call frames Fixes #4485. `Vm::return_value` is shared across all call frames but never gets reset in `push_frame()`. This means functions without an explicit `return` leak the caller's last evaluated expression as their return value: ```js function f() {} 5; f(); // returns 5 instead of undefined ``` What happens is `SetAccumulator` writes `5` into `vm.return_value` for the caller, then `f()` pushes a new frame without clearing it. When `f` returns, `handle_return()` picks up the stale `5`. The fix resets `self.return_value = JsValue::undefined()` at the top of `push_frame()`. ### Generators This was the main concern from the PR #4713 review — whether resetting in `push_frame()` would break generator resumption since `GeneratorContext::resume()` also calls `push_frame()` with `REGISTERS_ALREADY_PUSHED`. Traced through the code and it's fine: `GeneratorYield` sets `return_value` explicitly before calling `handle_yield()`, and `handle_yield()` consumes it via `take_return_value()`. On resume, the generator runs more opcodes and will set a fresh `return_value` before the next yield/return. The reset only affects the initial state going into the frame, which is what we want. Added generator-specific regression tests as @jedel1043 requested in #4713. ### Tests - Plain functions leaking caller values - Constructor implicit returns - Cross-eval isolation - Generator yield + return correctness - Generator implicit return producing `undefined` - Nested call chain independence --------- Co-authored-by: Rahul Tripathi <rahultripathi7009@gmail.com>
1 parent df1f0ff commit 3cec5e6

2 files changed

Lines changed: 129 additions & 0 deletions

File tree

core/engine/src/tests/function.rs

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,132 @@ fn empty_function_returns_undefined() {
1818
)]);
1919
}
2020

21+
#[test]
22+
fn implicit_return_does_not_leak_last_evaluated_expression() {
23+
run_test_actions([TestAction::assert_eq(
24+
indoc! {r#"
25+
let seen;
26+
27+
function g(inner) {
28+
seen = inner;
29+
}
30+
31+
function f() {}
32+
33+
5;
34+
let outer = g(f());
35+
seen;
36+
"#},
37+
JsValue::undefined(),
38+
)]);
39+
}
40+
41+
#[test]
42+
fn implicit_constructor_return_uses_constructed_object() {
43+
run_test_actions([TestAction::assert(indoc! {r#"
44+
let seen;
45+
46+
function g(inner) {
47+
seen = inner;
48+
}
49+
50+
function f() {}
51+
52+
({});
53+
let outer = g(new f());
54+
Object.getPrototypeOf(seen) === f.prototype;
55+
"#})]);
56+
}
57+
58+
#[test]
59+
fn implicit_return_does_not_leak_from_previous_eval() {
60+
run_test_actions([
61+
TestAction::run("function f() {}"),
62+
TestAction::run("let seen; function g(inner) { seen = inner; }"),
63+
TestAction::run("5;"),
64+
TestAction::run("let outer = g(f());"),
65+
TestAction::assert_eq("seen", JsValue::undefined()),
66+
]);
67+
}
68+
69+
#[test]
70+
fn implicit_return_does_not_leak_completion_value_from_same_eval() {
71+
run_test_actions([
72+
TestAction::run(indoc! {r#"
73+
eval(`
74+
function f() {}
75+
globalThis.seen = "initial";
76+
function g(inner) { globalThis.seen = inner; }
77+
5;
78+
let outer = g(f());
79+
`);
80+
"#}),
81+
TestAction::assert_eq("globalThis.seen", JsValue::undefined()),
82+
]);
83+
}
84+
85+
#[test]
86+
fn implicit_constructor_return_does_not_leak_completion_value_from_same_eval() {
87+
run_test_actions([
88+
TestAction::run(indoc! {r#"
89+
eval(`
90+
function f() {}
91+
globalThis.prototype_matches = false;
92+
function g(inner) {
93+
globalThis.prototype_matches = Object.getPrototypeOf(inner) === f.prototype;
94+
}
95+
({});
96+
let outer = g(new f());
97+
`);
98+
"#}),
99+
TestAction::assert("globalThis.prototype_matches"),
100+
]);
101+
}
102+
103+
/// Regression test for issue #4485.
104+
/// Checks that generator resumption via `g.next(f())` is not affected
105+
/// by resetting the accumulator in `push_frame`.
106+
#[test]
107+
fn generator_resumption_not_affected_by_return_value_reset() {
108+
run_test_actions([TestAction::assert(indoc! {r#"
109+
let seen;
110+
111+
function* gen() {
112+
seen = yield 1;
113+
}
114+
115+
function f() {}
116+
117+
5;
118+
var g = gen();
119+
g.next();
120+
g.next(f());
121+
seen === undefined;
122+
"#})]);
123+
}
124+
125+
/// Regression test for issue #4485.
126+
/// Checks that generator resumption via `g.next(new f())` receives
127+
/// the constructed object, not a stale caller expression.
128+
#[test]
129+
fn generator_resumption_with_constructor_not_affected_by_return_value_reset() {
130+
run_test_actions([TestAction::assert(indoc! {r#"
131+
let seen;
132+
133+
function* gen() {
134+
seen = yield 1;
135+
}
136+
137+
function f() {}
138+
139+
({});
140+
var g = gen();
141+
g.next();
142+
g.next(new f());
143+
Object.getPrototypeOf(seen) === f.prototype;
144+
"#})]);
145+
}
146+
21147
#[test]
22148
fn property_accessor_member_expression_dot_notation_on_function() {
23149
run_test_actions([TestAction::assert_eq(

core/engine/src/vm/mod.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,9 @@ impl Vm {
586586
}
587587

588588
pub(crate) fn push_frame(&mut self, mut frame: CallFrame) {
589+
// Each function call starts with an implicit `undefined` return value.
590+
self.return_value = JsValue::undefined();
591+
589592
// NOTE: We need to check if we already pushed the registers,
590593
// since generator-like functions push the same call
591594
// frame with pre-built stack and registers (fp and rp already set).

0 commit comments

Comments
 (0)