Skip to content

Commit e73a286

Browse files
committed
Fix closure captured envs and preserve globals for async callbacks
- Start capture from immediate environment and collect necessary captured envs - Fallback to topmost/global env for unresolved free names (fixes missing builtins like Error) - Keep hidden strong reference "__captured_env_wrapper" on activations to prevent premature drop - Add lightweight wrapper env to retain captured envs and link prototype to first captured env - Add tests/timeout_closure.rs regression test
1 parent 04f90da commit e73a286

File tree

2 files changed

+116
-7
lines changed

2 files changed

+116
-7
lines changed

src/core/value.rs

Lines changed: 91 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1048,7 +1048,9 @@ impl ClosureData {
10481048
let mut free_names: HashSet<String> = referenced.difference(&declared).cloned().collect();
10491049

10501050
let mut captured_envs: Vec<JSObjectDataPtr> = Vec::new();
1051-
let mut cur_opt = env.borrow().prototype.clone().and_then(|w| w.upgrade());
1051+
// Start checking from the immediate environment (to capture locals
1052+
// like `resolve`/`reject`), then walk up the prototype chain.
1053+
let mut cur_opt = Some(env.clone());
10521054
while let Some(cur_rc) = cur_opt {
10531055
if free_names.is_empty() {
10541056
break;
@@ -1069,6 +1071,36 @@ impl ClosureData {
10691071
cur_opt = cur_rc.borrow().prototype.clone().and_then(|w| w.upgrade());
10701072
}
10711073

1074+
// If there are still unresolved free names, try a precise fallback:
1075+
// check only the topmost (global) environment and capture it only
1076+
// if it owns any of the remaining free names. This avoids retaining
1077+
// the entire chain while still fixing references to global builtins
1078+
// (e.g. `Error`) that may live at the top.
1079+
if !free_names.is_empty() {
1080+
// Find the topmost env
1081+
let mut top_opt = Some(env.clone());
1082+
let mut last = env.clone();
1083+
while let Some(t) = top_opt {
1084+
last = t.clone();
1085+
top_opt = t.borrow().prototype.clone().and_then(|w| w.upgrade());
1086+
}
1087+
// See if the topmost env defines any remaining free names
1088+
let mut matched_top: Vec<String> = Vec::new();
1089+
for name in free_names.iter() {
1090+
if get_own_property(&last, &name.clone().into()).is_some() {
1091+
matched_top.push(name.clone());
1092+
}
1093+
}
1094+
if !matched_top.is_empty() {
1095+
if !captured_envs.iter().any(|c| Rc::ptr_eq(c, &last)) {
1096+
captured_envs.push(last.clone());
1097+
}
1098+
for m in matched_top {
1099+
free_names.remove(&m);
1100+
}
1101+
}
1102+
}
1103+
10721104
ClosureData {
10731105
params: params.to_vec(),
10741106
body: body.to_vec(),
@@ -1304,6 +1336,14 @@ pub fn prepare_function_call_env(
13041336
log::trace!("prepare_function_call_env - created func_env_ptr={:p}", Rc::as_ptr(&func_env));
13051337
if let Some(captured_env) = captured_env_opt {
13061338
func_env.borrow_mut().prototype = Some(Rc::downgrade(captured_env));
1339+
// Keep a hidden strong reference to the captured env/wrapper so
1340+
// it is not dropped while this activation is alive. This ensures
1341+
// the prototype chain used for variable lookup remains valid
1342+
// for asynchronous callbacks.
1343+
func_env.borrow_mut().insert(
1344+
PropertyKey::String("__captured_env_wrapper".to_string()),
1345+
Rc::new(RefCell::new(Value::Object(captured_env.clone()))),
1346+
);
13071347
// If the captured environment contains iterator sentinel keys
13081348
// like `__array` or `__str`, keep a hidden strong reference on
13091349
// the activation to ensure the sentinel object remains alive
@@ -1417,16 +1457,60 @@ pub fn prepare_closure_call_env(
14171457
// executable closure under the internal `"__closure__"` property.
14181458
#[allow(clippy::type_complexity)]
14191459
pub fn extract_closure_from_value(val: &Value) -> Option<(Vec<DestructuringElement>, Vec<Statement>, JSObjectDataPtr)> {
1460+
// Helper: when a closure has multiple `captured_envs`, create a
1461+
// lightweight wrapper environment that holds strong references to
1462+
// those captured envs and whose prototype points to the first
1463+
// captured env. This wrapper can be used as the `captured_env` in
1464+
// existing call sites without changing their signatures.
1465+
fn make_wrapper_for_captured_envs(captured_envs: &[JSObjectDataPtr], fallback: &JSObjectDataPtr) -> JSObjectDataPtr {
1466+
if captured_envs.is_empty() {
1467+
return fallback.clone();
1468+
}
1469+
let wrapper = new_js_object_data();
1470+
// Link wrapper prototype to the first captured env so lookups work
1471+
wrapper.borrow_mut().prototype = Some(Rc::downgrade(&captured_envs[0]));
1472+
// Retain strong references on the wrapper under hidden keys
1473+
for (i, env) in captured_envs.iter().enumerate() {
1474+
let key = PropertyKey::String(format!("__captured_env_{}", i));
1475+
wrapper.borrow_mut().insert(key, Rc::new(RefCell::new(Value::Object(env.clone()))));
1476+
}
1477+
wrapper
1478+
}
1479+
14201480
match val {
1421-
Value::Closure(data) => Some((data.params.clone(), data.body.clone(), data.env.clone())),
1422-
Value::AsyncClosure(data) => Some((data.params.clone(), data.body.clone(), data.env.clone())),
1423-
Value::GeneratorFunction(_, data) => Some((data.params.clone(), data.body.clone(), data.env.clone())),
1481+
Value::Closure(data) => Some((
1482+
data.params.clone(),
1483+
data.body.clone(),
1484+
make_wrapper_for_captured_envs(&data.captured_envs, &data.env),
1485+
)),
1486+
Value::AsyncClosure(data) => Some((
1487+
data.params.clone(),
1488+
data.body.clone(),
1489+
make_wrapper_for_captured_envs(&data.captured_envs, &data.env),
1490+
)),
1491+
Value::GeneratorFunction(_, data) => Some((
1492+
data.params.clone(),
1493+
data.body.clone(),
1494+
make_wrapper_for_captured_envs(&data.captured_envs, &data.env),
1495+
)),
14241496
Value::Object(object) => {
14251497
if let Ok(Some(cl_rc)) = obj_get_key_value(object, &"__closure__".into()) {
14261498
match &*cl_rc.borrow() {
1427-
Value::Closure(data) => Some((data.params.clone(), data.body.clone(), data.env.clone())),
1428-
Value::AsyncClosure(data) => Some((data.params.clone(), data.body.clone(), data.env.clone())),
1429-
Value::GeneratorFunction(_, data) => Some((data.params.clone(), data.body.clone(), data.env.clone())),
1499+
Value::Closure(data) => Some((
1500+
data.params.clone(),
1501+
data.body.clone(),
1502+
make_wrapper_for_captured_envs(&data.captured_envs, &data.env),
1503+
)),
1504+
Value::AsyncClosure(data) => Some((
1505+
data.params.clone(),
1506+
data.body.clone(),
1507+
make_wrapper_for_captured_envs(&data.captured_envs, &data.env),
1508+
)),
1509+
Value::GeneratorFunction(_, data) => Some((
1510+
data.params.clone(),
1511+
data.body.clone(),
1512+
make_wrapper_for_captured_envs(&data.captured_envs, &data.env),
1513+
)),
14301514
_ => None,
14311515
}
14321516
} else {

tests/timeout_closure.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
use javascript::*;
2+
3+
#[test]
4+
fn timeout_closure_handles_error_constructor() {
5+
let script = r#"
6+
// Ensure an async callback that rejects with `new Error(...)` can resolve Error
7+
function doSomethingCritical(){ return new Promise((r)=>{ setTimeout(()=>r('crit'), 5); }); }
8+
function doSomethingOptional(){ return new Promise((resolve,reject)=>{ setTimeout(()=>reject(new Error('optional failed')), 5); }); }
9+
function doSomethingExtraNice(x){ return new Promise((r)=>{ setTimeout(()=>r('extra-'+x), 5); }); }
10+
function moreCriticalStuff(){ return new Promise((r)=>{ setTimeout(()=>r('done'), 5); }); }
11+
12+
// Chain where optional rejects but is caught; ensure no ReferenceError occurs
13+
doSomethingCritical()
14+
.then((result) =>
15+
doSomethingOptional()
16+
.then((optionalResult) => doSomethingExtraNice(optionalResult))
17+
.catch((e) => { /* swallow optional error */ }),
18+
)
19+
.then(() => moreCriticalStuff())
20+
.catch((e) => { throw new Error('serious error: ' + e.message); });
21+
"#;
22+
23+
let res = evaluate_script(script, None::<&std::path::Path>);
24+
assert!(res.is_ok(), "evaluate_script failed: {:?}", res.err());
25+
}

0 commit comments

Comments
 (0)