Fix panic when calling function that mutates itself (#667)

boa/src/builtins/function/mod.rs

@@ -20,13 +20,16 @@ use crate::{
     },
     environment::lexical_environment::Environment,
     exec::Interpreter,
-    syntax::ast::node::{FormalParameter, StatementList},
+    syntax::ast::node::{FormalParameter, RcStatementList},
     BoaProfiler, Result,
 };
 use bitflags::bitflags;
 use gc::{unsafe_empty_trace, Finalize, Trace};
 use std::fmt::{self, Debug};
 
+#[cfg(test)]
+mod tests;
+
 /// _fn(this, arguments, ctx) -> Result<Value>_ - The signature of a built-in function
 pub type NativeFunction = fn(&Value, &[Value], &mut Interpreter) -> Result<Value>;
 
@@ -102,7 +105,7 @@ pub enum Function {
     BuiltIn(BuiltInFunction, FunctionFlags),
     Ordinary {
         flags: FunctionFlags,
-        body: StatementList,
+        body: RcStatementList,
         params: Box<[FormalParameter]>,
         environment: Environment,
     },

boa/src/builtins/function/tests.rs

@@ -1,12 +1,10 @@
-use crate::exec::Executor;
-use crate::realm::Realm;
-use crate::{builtins::value::from_value, forward, forward_val};
+use crate::{exec::Interpreter, forward, forward_val, realm::Realm};
 
 #[allow(clippy::float_cmp)]
 #[test]
 fn check_arguments_object() {
     let realm = Realm::create();
-    let mut engine = Executor::new(realm);
+    let mut engine = Interpreter::new(realm);
     let init = r#"
         function jason(a, b) {
             return arguments[0];
@@ -15,11 +13,33 @@ fn check_arguments_object() {
         "#;
 
     eprintln!("{}", forward(&mut engine, init));
-    let expected_return_val = 100;
+
     let return_val = forward_val(&mut engine, "val").expect("value expected");
     assert_eq!(return_val.is_integer(), true);
     assert_eq!(
-        from_value::<i32>(return_val).expect("Could not convert value to i32"),
-        expected_return_val
+        return_val
+            .to_i32(&mut engine)
+            .expect("Could not convert value to i32"),
+        100
+    );
+}
+
+#[test]
+fn check_self_mutating_func() {
+    let realm = Realm::create();
+    let mut engine = Interpreter::new(realm);
+    let func = r#"
+        function x() {
+	        x.y = 3;
+        }
+        x();
+        "#;
+    eprintln!("{}", forward(&mut engine, func));
+    let y = forward_val(&mut engine, "x.y").expect("value expected");
+    assert_eq!(y.is_integer(), true);
+    assert_eq!(
+        y.to_i32(&mut engine)
+            .expect("Could not convert value to i32"),
+        3
     );
 }

boa/src/builtins/object/gcobject.rs

@@ -5,12 +5,13 @@
 use super::{Object, PROTOTYPE};
 use crate::{
     builtins::{
-        function::{create_unmapped_arguments_object, BuiltInFunction, Function},
+        function::{create_unmapped_arguments_object, BuiltInFunction, Function, NativeFunction},
         Value,
     },
     environment::{
         function_environment_record::BindingStatus, lexical_environment::new_function_environment,
     },
+    syntax::ast::node::statement_list::RcStatementList,
     Executable, Interpreter, Result,
 };
 use gc::{Finalize, Gc, GcCell, GcCellRef, GcCellRefMut, Trace};
@@ -32,6 +33,13 @@ pub type RefMut<'object> = GcCellRefMut<'object, Object>;
 #[derive(Trace, Finalize, Clone)]
 pub struct GcObject(Gc<GcCell<Object>>);
 
+// This is needed for the call method since we cannot mutate the function itself since we
+// already borrow it so we get the function body clone it then drop the borrow and run the body
+enum FunctionBody {
+    BuiltIn(NativeFunction),
+    Ordinary(RcStatementList),
+}
+
 impl GcObject {
     /// Create a new `GcObject` from a `Object`.
     #[inline]
@@ -99,11 +107,12 @@ impl GcObject {
     // <https://tc39.es/ecma262/#sec-ecmascript-function-objects-call-thisargument-argumentslist>
     pub fn call(&self, this: &Value, args: &[Value], ctx: &mut Interpreter) -> Result<Value> {
         let this_function_object = self.clone();
-        let object = self.borrow();
-        if let Some(function) = object.as_function() {
+        let f_body = if let Some(function) = self.borrow().as_function() {
             if function.is_callable() {
                 match function {
-                    Function::BuiltIn(BuiltInFunction(function), _) => function(this, args, ctx),
+                    Function::BuiltIn(BuiltInFunction(function), _) => {
+                        FunctionBody::BuiltIn(*function)
+                    }
                     Function::Ordinary {
                         body,
                         params,
@@ -151,19 +160,24 @@ impl GcObject {
 
                         ctx.realm.environment.push(local_env);
 
-                        // Call body should be set before reaching here
-                        let result = body.run(ctx);
-
-                        // local_env gets dropped here, its no longer needed
-                        ctx.realm.environment.pop();
-                        result
+                        FunctionBody::Ordinary(body.clone())
                     }
                 }
             } else {
-                ctx.throw_type_error("function object is not callable")
+                return ctx.throw_type_error("function object is not callable");
             }
         } else {
-            ctx.throw_type_error("not a function")
+            return ctx.throw_type_error("not a function");
+        };
+
+        match f_body {
+            FunctionBody::BuiltIn(func) => func(this, args, ctx),
+            FunctionBody::Ordinary(body) => {
+                let result = body.run(ctx);
+                ctx.realm.environment.pop();
+
+                result
+            }
         }
     }
 

boa/src/exec/mod.rs

@@ -34,7 +34,7 @@ use crate::{
     realm::Realm,
     syntax::ast::{
         constant::Const,
-        node::{FormalParameter, Node, StatementList},
+        node::{FormalParameter, Node, RcStatementList, StatementList},
     },
     BoaProfiler, Result,
 };
@@ -141,7 +141,7 @@ impl Interpreter {
         let params_len = params.len();
         let func = Function::Ordinary {
             flags,
-            body: body.into(),
+            body: RcStatementList::from(body.into()),
             params,
             environment: self.realm.environment.get_current_environment().clone(),
         };

boa/src/syntax/ast/node/mod.rs

@@ -35,7 +35,7 @@ pub use self::{
     operator::{Assign, BinOp, UnaryOp},
     return_smt::Return,
     spread::Spread,
-    statement_list::StatementList,
+    statement_list::{RcStatementList, StatementList},
     switch::{Case, Switch},
     throw::Throw,
     try_node::{Catch, Finally, Try},

boa/src/syntax/ast/node/statement_list.rs

@@ -1,8 +1,10 @@
 //! Statement list node.
 
 use super::Node;
-use gc::{Finalize, Trace};
+use gc::{unsafe_empty_trace, Finalize, Trace};
 use std::fmt;
+use std::ops::Deref;
+use std::rc::Rc;
 
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
@@ -62,3 +64,28 @@ impl fmt::Display for StatementList {
         self.display(f, 0)
     }
 }
+
+// List of statements wrapped with Rc. We need this for self mutating functions.
+// Since we need to cheaply clone the function body and drop the borrow of the function object to
+// mutably borrow the function object and call this cloned function body
+#[derive(Clone, Debug, Finalize, PartialEq)]
+pub struct RcStatementList(Rc<StatementList>);
+
+impl Deref for RcStatementList {
+    type Target = StatementList;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl From<StatementList> for RcStatementList {
+    #[inline]
+    fn from(statementlist: StatementList) -> Self {
+        Self(Rc::from(statementlist))
+    }
+}
+
+// SAFETY: This is safe for types not containing any `Trace` types.
+unsafe impl Trace for RcStatementList {
+    unsafe_empty_trace!();
+}