fix: Remove unsafety by moving Executable to a heap vector

Author: aapoalas

nova_vm/src/ecmascript/builtins/builtin_constructor.rs

@@ -26,7 +26,7 @@ use crate::{
             BUILTIN_STRING_MEMORY,
         },
     },
-    engine::{Executable, HeapAllocatedBytecode},
+    engine::Executable,
     heap::{
         indexes::BuiltinConstructorIndex, CompactionLists, CreateHeapData, Heap, HeapMarkAndSweep,
         ObjectEntry, ObjectEntryPropertyDescriptor, WorkQueues,
@@ -411,17 +411,13 @@ pub(crate) fn create_builtin_constructor(
         agent.heap.create_null_object(&entries)
     };
 
-    let compiled_initializer_bytecode = args
-        .compiled_initializer_bytecode
-        .map(HeapAllocatedBytecode::new);
-
     // 13. Return func.
     agent.heap.create(BuiltinConstructorHeapData {
         // 10. Perform SetFunctionLength(func, length).
         // Skipped as length of builtin constructors is always 0.
         // 8. Set func.[[Realm]] to realm.
         realm,
-        compiled_initializer_bytecode,
+        compiled_initializer_bytecode: args.compiled_initializer_bytecode,
         is_derived: args.is_derived,
         object_index: Some(backing_object),
         environment: args.env,

nova_vm/src/ecmascript/builtins/control_abstraction_objects/generator_objects.rs

@@ -15,9 +15,7 @@ use crate::{
             InternalMethods, InternalSlots, IntoObject, IntoValue, Object, OrdinaryObject, Value,
         },
     },
-    engine::{
-        local_value::ObjectScopeRoot, ExecutionResult, HeapAllocatedBytecode, SuspendedVm, Vm,
-    },
+    engine::{local_value::ObjectScopeRoot, Executable, ExecutionResult, SuspendedVm, Vm},
     heap::{
         indexes::{BaseIndex, GeneratorIndex},
         CompactionLists, CreateHeapData, Heap, HeapMarkAndSweep, WorkQueues,
@@ -365,7 +363,7 @@ pub(crate) enum GeneratorState {
     // SUSPENDED-START has `vm_or_args` set to Arguments, SUSPENDED-YIELD has it set to Vm.
     Suspended {
         vm_or_args: VmOrArguments,
-        executable: HeapAllocatedBytecode,
+        executable: Executable,
         execution_context: ExecutionContext,
     },
     Executing,

nova_vm/src/ecmascript/builtins/global_object.rs

@@ -28,7 +28,7 @@ use crate::{
         },
         types::{Function, IntoValue, String, Value, BUILTIN_STRING_MEMORY},
     },
-    engine::{Executable, HeapAllocatedBytecode, Vm},
+    engine::{Executable, Vm},
     heap::IntrinsicFunctionIndexes,
 };
 
@@ -335,13 +335,13 @@ pub fn perform_eval(
         // allocation for no good reason.
         // Second, this executable is not accessible by the heap and will thus
         // break if garbage collection triggers within the eval.
-        let exe = HeapAllocatedBytecode::new(Executable::compile_eval_body(agent, &script.body));
+        let exe = Executable::compile_eval_body(agent, &script.body);
         // a. Set result to Completion(Evaluation of body).
         // 30. If result is a normal completion and result.[[Value]] is empty, then
         // a. Set result to NormalCompletion(undefined).
         let result = Vm::execute(agent, exe, None).into_js_result();
         // SAFETY: No one can access the bytecode anymore.
-        unsafe { exe.drop() };
+        unsafe { exe.try_drop(agent) };
         result
     } else {
         Err(result.err().unwrap())

nova_vm/src/ecmascript/scripts_and_modules/script.rs

@@ -19,7 +19,7 @@ use crate::{
         },
         types::{IntoValue, String, Value, BUILTIN_STRING_MEMORY},
     },
-    engine::{Executable, HeapAllocatedBytecode, Vm},
+    engine::{Executable, Vm},
     heap::{CompactionLists, HeapMarkAndSweep, WorkQueues},
 };
 use ahash::AHashSet;
@@ -141,7 +141,7 @@ pub struct Script {
     pub(crate) ecmascript_code: ManuallyDrop<Program<'static>>,
 
     /// Stores the compiled bytecode of a Script.
-    pub(crate) compiled_bytecode: Option<HeapAllocatedBytecode>,
+    pub(crate) compiled_bytecode: Option<Executable>,
 
     /// ### \[\[LoadedModules]]
     ///
@@ -306,7 +306,7 @@ pub fn script_evaluation(agent: &mut Agent, script: Script) -> JsResult<Value> {
 
     // 13. If result.[[Type]] is normal, then
     let result: JsResult<Value> = if result.is_ok() {
-        let bytecode = HeapAllocatedBytecode::new(Executable::compile_script(agent, script));
+        let bytecode = Executable::compile_script(agent, script);
         agent[script].compiled_bytecode = Some(bytecode);
         // a. Set result to Completion(Evaluation of script).
         // b. If result.[[Type]] is normal and result.[[Value]] is empty, then

nova_vm/src/ecmascript/syntax_directed_operations/function_definitions.rs

@@ -34,7 +34,7 @@ use crate::{
             Value, BUILTIN_STRING_MEMORY,
         },
     },
-    engine::{Executable, ExecutionResult, FunctionExpression, HeapAllocatedBytecode, Vm},
+    engine::{Executable, ExecutionResult, FunctionExpression, Vm},
     heap::CreateHeapData,
 };
 use oxc_ast::ast::{self};
@@ -271,7 +271,7 @@ pub(crate) fn evaluate_function_body(
         exe
     } else {
         let data = CompileFunctionBodyData::new(agent, function_object);
-        let exe = HeapAllocatedBytecode::new(Executable::compile_function_body(agent, data));
+        let exe = Executable::compile_function_body(agent, data);
         agent[function_object].compiled_bytecode = Some(exe);
         exe
     };
@@ -298,7 +298,7 @@ pub(crate) fn evaluate_async_function_body(
         exe
     } else {
         let data = CompileFunctionBodyData::new(agent, function_object);
-        let exe = HeapAllocatedBytecode::new(Executable::compile_function_body(agent, data));
+        let exe = Executable::compile_function_body(agent, data);
         agent[function_object].compiled_bytecode = Some(exe);
         exe
     };
@@ -375,7 +375,7 @@ pub(crate) fn evaluate_generator_body(
     // 4. Perform GeneratorStart(G, FunctionBody).
     // SAFETY: We're alive so SourceCode must be too.
     let data = CompileFunctionBodyData::new(agent, function_object);
-    let executable = HeapAllocatedBytecode::new(Executable::compile_function_body(agent, data));
+    let executable = Executable::compile_function_body(agent, data);
     agent[generator].generator_state = Some(GeneratorState::Suspended {
         vm_or_args: VmOrArguments::Arguments(arguments_list.0.into()),
         executable,

nova_vm/src/ecmascript/types/language/function/data.rs

@@ -11,7 +11,7 @@ use crate::{
         scripts_and_modules::source_code::SourceCode,
         types::{OrdinaryObject, String, Value},
     },
-    engine::HeapAllocatedBytecode,
+    engine::Executable,
     heap::element_array::ElementsVector,
 };
 
@@ -66,7 +66,7 @@ pub struct BuiltinConstructorHeapData {
     /// Base.
     pub(crate) is_derived: bool,
     /// Stores the compiled bytecode of class field initializers.
-    pub(crate) compiled_initializer_bytecode: Option<HeapAllocatedBytecode>,
+    pub(crate) compiled_initializer_bytecode: Option<Executable>,
     /// ### \[\[Environment]]
     ///
     /// This is required for class field initializers.
@@ -85,38 +85,14 @@ pub struct BuiltinConstructorHeapData {
     pub(crate) source_code: SourceCode,
 }
 
-// SAFETY: We promise not to ever mutate the Executable, especially not from
-// foreign threads.
-unsafe impl Send for BuiltinConstructorHeapData {}
-
-impl Drop for BuiltinConstructorHeapData {
-    fn drop(&mut self) {
-        if let Some(exe) = self.compiled_initializer_bytecode.take() {
-            // SAFETY: No references to this compiled bytecode should exist as
-            // otherwise we should not have been garbage collected.
-            unsafe { exe.drop() };
-        }
-    }
-}
-
 #[derive(Debug)]
 pub struct ECMAScriptFunctionHeapData {
     pub(crate) object_index: Option<OrdinaryObject>,
     pub(crate) length: u8,
     pub(crate) ecmascript_function: ECMAScriptFunctionObjectHeapData,
     /// Stores the compiled bytecode of an ECMAScript function.
-    pub(crate) compiled_bytecode: Option<HeapAllocatedBytecode>,
+    pub(crate) compiled_bytecode: Option<Executable>,
     pub(crate) name: Option<String>,
 }
 
 unsafe impl Send for ECMAScriptFunctionHeapData {}
-
-impl Drop for ECMAScriptFunctionHeapData {
-    fn drop(&mut self) {
-        if let Some(exe) = self.compiled_bytecode.take() {
-            // SAFETY: No references to this compiled bytecode should exist as
-            // otherwise we should not have been garbage collected.
-            unsafe { exe.drop() };
-        }
-    }
-}

nova_vm/src/engine/bytecode.rs

@@ -2,16 +2,17 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
+mod bytecode_compiler;
 mod executable;
-mod heap_allocated_bytecode;
 mod instructions;
 pub(super) mod iterator;
 mod vm;
 
+pub(crate) use bytecode_compiler::{
+    is_reference, CompileContext, CompileEvaluation, NamedEvaluationParameter,
+};
 pub(crate) use executable::{
-    is_reference, CompileContext, CompileEvaluation, Executable, FunctionExpression, IndexType,
-    NamedEvaluationParameter, SendableRef,
+    Executable, ExecutableHeapData, FunctionExpression, IndexType, SendableRef,
 };
-pub(crate) use heap_allocated_bytecode::HeapAllocatedBytecode;
 pub(crate) use instructions::{Instruction, InstructionIter};
 pub(crate) use vm::{instanceof_operator, ExecutionResult, SuspendedVm, Vm};