From 6630d265939baab782de74beb27bb70066b96251 Mon Sep 17 00:00:00 2001 From: tkshsbcue Date: Sat, 14 Mar 2026 08:48:14 +0530 Subject: [PATCH 1/2] fix: replace panics with proper error handling in module env and dynamic import - Replace panic!() in load_dyn_import with JsNativeError::TypeError when a synthetic module is unexpectedly used as a referrer - Replace panic!() in ModuleEnvironment::set with JsNativeError::TypeError when attempting to modify an indirect binding reference - Propagate JsResult through the environment set() call chain: DeclarativeEnvironment, put_lexical_value, put_value_if_uninitialized - Update all callers to handle the new Result return type --- .../engine/src/builtins/function/arguments.rs | 4 +++- core/engine/src/builtins/function/mod.rs | 4 ++-- .../runtime/declarative/function.rs | 3 ++- .../runtime/declarative/global.rs | 5 +++-- .../runtime/declarative/lexical.rs | 5 +++-- .../environments/runtime/declarative/mod.rs | 14 ++++++++++--- .../runtime/declarative/module.rs | 17 ++++++++------- core/engine/src/environments/runtime/mod.rs | 21 +++++++++++++------ core/engine/src/module/source.rs | 6 +++--- core/engine/src/module/synthetic.rs | 5 +++-- core/engine/src/vm/opcode/call/mod.rs | 4 +++- core/engine/src/vm/opcode/define/mod.rs | 8 +++---- 12 files changed, 62 insertions(+), 34 deletions(-) diff --git a/core/engine/src/builtins/function/arguments.rs b/core/engine/src/builtins/function/arguments.rs index f912e2b2d8f..cee806b409e 100644 --- a/core/engine/src/builtins/function/arguments.rs +++ b/core/engine/src/builtins/function/arguments.rs @@ -137,7 +137,9 @@ impl MappedArguments { /// [spec]: https://tc39.es/ecma262/#sec-makeargsetter pub(crate) fn set(&self, index: u32, value: &JsValue) { if let Some(binding_index) = self.binding_indices.get(index as usize).copied().flatten() { - self.environment.set(binding_index, value.clone()); + // Mapped arguments always point to function-scope direct bindings, + // which cannot fail. + drop(self.environment.set(binding_index, value.clone())); } } } diff --git a/core/engine/src/builtins/function/mod.rs b/core/engine/src/builtins/function/mod.rs index b49bd55eea4..1280b28d536 100644 --- a/core/engine/src/builtins/function/mod.rs +++ b/core/engine/src/builtins/function/mod.rs @@ -1066,7 +1066,7 @@ pub(crate) fn function_call( BindingLocatorScope::Stack(index), 0, function_object.clone().into(), - ); + )?; last_env += 1; } @@ -1167,7 +1167,7 @@ fn function_construct( BindingLocatorScope::Stack(index), 0, this_function_object.clone().into(), - ); + )?; last_env += 1; } diff --git a/core/engine/src/environments/runtime/declarative/function.rs b/core/engine/src/environments/runtime/declarative/function.rs index 9e198882740..77bab55984f 100644 --- a/core/engine/src/environments/runtime/declarative/function.rs +++ b/core/engine/src/environments/runtime/declarative/function.rs @@ -49,8 +49,9 @@ impl FunctionEnvironment { /// /// Panics if the binding value is out of range. #[track_caller] - pub(crate) fn set(&self, index: u32, value: JsValue) { + pub(crate) fn set(&self, index: u32, value: JsValue) -> JsResult<()> { self.bindings.borrow_mut()[index as usize] = Some(value); + Ok(()) } /// Gets the bindings of this poisonable environment. diff --git a/core/engine/src/environments/runtime/declarative/global.rs b/core/engine/src/environments/runtime/declarative/global.rs index 9607a9cb052..20fefa0d8d1 100644 --- a/core/engine/src/environments/runtime/declarative/global.rs +++ b/core/engine/src/environments/runtime/declarative/global.rs @@ -1,4 +1,4 @@ -use crate::JsValue; +use crate::{JsResult, JsValue}; use boa_gc::{Finalize, GcRefCell, Trace}; #[derive(Debug, Trace, Finalize)] @@ -30,8 +30,9 @@ impl GlobalEnvironment { /// /// Panics if the binding value is out of range. #[track_caller] - pub(crate) fn set(&self, index: u32, value: JsValue) { + pub(crate) fn set(&self, index: u32, value: JsValue) -> JsResult<()> { self.bindings.borrow_mut()[index as usize] = Some(value); + Ok(()) } /// Gets the bindings of this poisonable environment. diff --git a/core/engine/src/environments/runtime/declarative/lexical.rs b/core/engine/src/environments/runtime/declarative/lexical.rs index 86592cf6746..6f9ca11a88b 100644 --- a/core/engine/src/environments/runtime/declarative/lexical.rs +++ b/core/engine/src/environments/runtime/declarative/lexical.rs @@ -1,6 +1,6 @@ use boa_gc::{Finalize, GcRefCell, Trace}; -use crate::JsValue; +use crate::{JsResult, JsValue}; #[derive(Debug, Trace, Finalize)] pub(crate) struct LexicalEnvironment { @@ -31,8 +31,9 @@ impl LexicalEnvironment { /// /// Panics if the binding value is out of range. #[track_caller] - pub(crate) fn set(&self, index: u32, value: JsValue) { + pub(crate) fn set(&self, index: u32, value: JsValue) -> JsResult<()> { self.bindings.borrow_mut()[index as usize] = Some(value); + Ok(()) } /// Gets the bindings of this poisonable environment. diff --git a/core/engine/src/environments/runtime/declarative/mod.rs b/core/engine/src/environments/runtime/declarative/mod.rs index fbb1df51c9d..56f786ece6c 100644 --- a/core/engine/src/environments/runtime/declarative/mod.rs +++ b/core/engine/src/environments/runtime/declarative/mod.rs @@ -82,12 +82,16 @@ impl DeclarativeEnvironment { /// Sets the binding value from the environment by index. /// + /// # Errors + /// + /// Returns an error if the binding is an indirect module reference. + /// /// # Panics /// /// Panics if the binding value is out of range. #[track_caller] - pub(crate) fn set(&self, index: u32, value: JsValue) { - self.kind.set(index, value); + pub(crate) fn set(&self, index: u32, value: JsValue) -> JsResult<()> { + self.kind.set(index, value) } /// `GetThisBinding` @@ -199,11 +203,15 @@ impl DeclarativeEnvironmentKind { /// Sets the binding value from the environment by index. /// + /// # Errors + /// + /// Returns an error if the binding is an indirect module reference. + /// /// # Panics /// /// Panics if the binding value is out of range. #[track_caller] - pub(crate) fn set(&self, index: u32, value: JsValue) { + pub(crate) fn set(&self, index: u32, value: JsValue) -> JsResult<()> { match self { Self::Lexical(inner) => inner.set(index, value), Self::Global(inner) => inner.set(index, value), diff --git a/core/engine/src/environments/runtime/declarative/module.rs b/core/engine/src/environments/runtime/declarative/module.rs index 989b71764d6..e36e77606c8 100644 --- a/core/engine/src/environments/runtime/declarative/module.rs +++ b/core/engine/src/environments/runtime/declarative/module.rs @@ -3,7 +3,7 @@ use std::cell::RefCell; use boa_ast::scope::Scope; use boa_gc::{Finalize, GcRefCell, Trace}; -use crate::{JsString, JsValue, module::Module}; +use crate::{JsNativeError, JsResult, JsString, JsValue, module::Module}; /// Type of accessor used to access an indirect binding. #[derive(Debug, Clone)] @@ -95,18 +95,21 @@ impl ModuleEnvironment { /// Sets the binding value from the environment by index. /// - /// # Panics + /// # Errors /// - /// Panics if the binding value is out of range. + /// Returns a `TypeError` if the binding is an indirect reference to another environment. #[track_caller] - pub(crate) fn set(&self, index: u32, value: JsValue) { + pub(crate) fn set(&self, index: u32, value: JsValue) -> JsResult<()> { let mut bindings = self.bindings.borrow_mut(); match &mut bindings[index as usize] { - BindingType::Direct(v) => *v = Some(value), - BindingType::Indirect(_) => { - panic!("cannot modify indirect references to other environments") + BindingType::Direct(v) => { + *v = Some(value); + Ok(()) } + BindingType::Indirect(_) => Err(JsNativeError::typ() + .with_message("cannot modify indirect references to other environments") + .into()), } } diff --git a/core/engine/src/environments/runtime/mod.rs b/core/engine/src/environments/runtime/mod.rs index 9af8cd35170..15508af7554 100644 --- a/core/engine/src/environments/runtime/mod.rs +++ b/core/engine/src/environments/runtime/mod.rs @@ -267,6 +267,10 @@ impl EnvironmentStack { /// Set the value of a lexical binding. /// + /// # Errors + /// + /// Returns an error if the binding is an indirect module reference. + /// /// # Panics /// /// Panics if the environment or binding index are out of range. @@ -276,7 +280,7 @@ impl EnvironmentStack { environment: BindingLocatorScope, binding_index: u32, value: JsValue, - ) { + ) -> JsResult<()> { let env = match environment { BindingLocatorScope::GlobalObject | BindingLocatorScope::GlobalDeclarative => { self.global() @@ -287,11 +291,15 @@ impl EnvironmentStack { .and_then(Environment::as_declarative) .expect("must be declarative environment"), }; - env.set(binding_index, value); + env.set(binding_index, value) } /// Set the value of a binding if it is uninitialized. /// + /// # Errors + /// + /// Returns an error if the binding is an indirect module reference. + /// /// # Panics /// /// Panics if the environment or binding index are out of range. @@ -301,7 +309,7 @@ impl EnvironmentStack { environment: BindingLocatorScope, binding_index: u32, value: JsValue, - ) { + ) -> JsResult<()> { let env = match environment { BindingLocatorScope::GlobalObject | BindingLocatorScope::GlobalDeclarative => { self.global() @@ -313,8 +321,9 @@ impl EnvironmentStack { .expect("must be declarative environment"), }; if env.get(binding_index).is_none() { - env.set(binding_index, value); + env.set(binding_index, value)?; } + Ok(()) } /// Push a private environment to the private environment stack. @@ -559,11 +568,11 @@ impl Context { } BindingLocatorScope::GlobalDeclarative => { let env = self.vm.frame().environments.global(); - env.set(locator.binding_index(), value); + env.set(locator.binding_index(), value)?; } BindingLocatorScope::Stack(index) => match self.environment_expect(index) { Environment::Declarative(decl) => { - decl.set(locator.binding_index(), value); + decl.set(locator.binding_index(), value)?; } Environment::Object(obj) => { let key = locator.name().clone(); diff --git a/core/engine/src/module/source.rs b/core/engine/src/module/source.rs index 8ec5ac26c83..b9e2ce0026b 100644 --- a/core/engine/src/module/source.rs +++ b/core/engine/src/module/source.rs @@ -1790,7 +1790,7 @@ impl SourceTextModule { locator.scope(), locator.binding_index(), namespace.into(), - ); + )?; } ImportBinding::Single { locator, @@ -1816,7 +1816,7 @@ impl SourceTextModule { locator.scope(), locator.binding_index(), namespace.into(), - ); + )?; } }, } @@ -1832,7 +1832,7 @@ impl SourceTextModule { locator.scope(), locator.binding_index(), function.into(), - ); + )?; } // 25. Remove moduleContext from the execution context stack. diff --git a/core/engine/src/module/synthetic.rs b/core/engine/src/module/synthetic.rs index 5cb7a3bbed4..4144cd3a059 100644 --- a/core/engine/src/module/synthetic.rs +++ b/core/engine/src/module/synthetic.rs @@ -217,7 +217,7 @@ impl SyntheticModule { export_name.to_std_string_escaped() )) })?; - env.set(locator.binding_index(), export_value); + env.set(locator.binding_index(), export_value)?; Ok(()) } @@ -346,7 +346,8 @@ impl SyntheticModule { locator.scope(), locator.binding_index(), JsValue::undefined(), - ); + ) + .expect("synthetic module bindings are always direct"); } let env = envs diff --git a/core/engine/src/vm/opcode/call/mod.rs b/core/engine/src/vm/opcode/call/mod.rs index 7d9d12b40ea..e0891728695 100644 --- a/core/engine/src/vm/opcode/call/mod.rs +++ b/core/engine/src/vm/opcode/call/mod.rs @@ -378,7 +378,9 @@ async fn load_dyn_import( match referrer { Referrer::Module(mod_ref) => { let ModuleKind::SourceText(src) = mod_ref.kind() else { - panic!("referrer cannot be a synthetic module"); + return Err(JsNativeError::typ() + .with_message("referrer cannot be a synthetic module") + .into()); }; let mut loaded_modules = src.loaded_modules().borrow_mut(); diff --git a/core/engine/src/vm/opcode/define/mod.rs b/core/engine/src/vm/opcode/define/mod.rs index e79283fb20c..b5bfc032d6d 100644 --- a/core/engine/src/vm/opcode/define/mod.rs +++ b/core/engine/src/vm/opcode/define/mod.rs @@ -16,7 +16,7 @@ pub(crate) struct DefVar; impl DefVar { #[inline(always)] - pub(super) fn operation(index: IndexOperand, context: &mut Context) { + pub(super) fn operation(index: IndexOperand, context: &mut Context) -> JsResult<()> { // TODO: spec specifies to return `empty` on empty vars, but we're trying to initialize. let binding_locator = context.vm.frame().code_block.bindings[usize::from(index)].clone(); @@ -28,7 +28,7 @@ impl DefVar { binding_locator.scope(), binding_locator.binding_index(), JsValue::undefined(), - ); + ) } } @@ -80,14 +80,14 @@ impl PutLexicalValue { pub(super) fn operation( (value, index): (RegisterOperand, IndexOperand), context: &mut Context, - ) { + ) -> JsResult<()> { let value = context.vm.get_register(value.into()).clone(); let binding_locator = context.vm.frame().code_block.bindings[usize::from(index)].clone(); context.vm.frame_mut().environments.put_lexical_value( binding_locator.scope(), binding_locator.binding_index(), value, - ); + ) } } From 3bf6c1a051d9000924c1bfec8272a88940a5c1a7 Mon Sep 17 00:00:00 2001 From: tkshsbcue Date: Sat, 14 Mar 2026 19:06:38 +0530 Subject: [PATCH 2/2] fix: use PanicError instead of TypeError for indirect binding set ModuleEnvironment::set on an indirect binding indicates an internal bug in the bytecompiler or VM binding resolution, not a user-facing error. Use PanicError to signal this correctly. --- .../src/environments/runtime/declarative/module.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/core/engine/src/environments/runtime/declarative/module.rs b/core/engine/src/environments/runtime/declarative/module.rs index e36e77606c8..214a0e161f8 100644 --- a/core/engine/src/environments/runtime/declarative/module.rs +++ b/core/engine/src/environments/runtime/declarative/module.rs @@ -3,7 +3,7 @@ use std::cell::RefCell; use boa_ast::scope::Scope; use boa_gc::{Finalize, GcRefCell, Trace}; -use crate::{JsNativeError, JsResult, JsString, JsValue, module::Module}; +use crate::{JsResult, JsString, JsValue, error::PanicError, module::Module}; /// Type of accessor used to access an indirect binding. #[derive(Debug, Clone)] @@ -107,9 +107,10 @@ impl ModuleEnvironment { *v = Some(value); Ok(()) } - BindingType::Indirect(_) => Err(JsNativeError::typ() - .with_message("cannot modify indirect references to other environments") - .into()), + BindingType::Indirect(_) => Err(PanicError::new( + "cannot modify indirect references to other environments", + ) + .into()), } }