#896 Fix hard crash when duplicating mappings that have Lua scripts

This crash was introduced by v2.16.0-pre.2 commit 073b9b2,
which removed manual code previously responsible for deferring
deallocation from real-time thread to main thread. However,
since Lua script engine isn't aware of our special auto-deferring
deallocator, deallocation was done in real-time thread. And since
it was in some way a shared copy of the compiled script, there
was a race condition and/or double free.

main/src/application/mode_model.rs

@@ -9,6 +9,7 @@ use helgoboss_learn::{
 };
 
 use crate::application::{Affected, Change, GetProcessingRelevance, ProcessingRelevance};
+use crate::base::CloneAsDefault;
 use realearn_api::persistence::FeedbackValueTable;
 use std::time::Duration;
 
@@ -644,7 +645,9 @@ impl ModeModel {
                 FeedbackType::Dynamic => {
                     let lua = unsafe { BackboneState::main_thread_lua() };
                     match LuaFeedbackScript::compile(lua, &self.textual_feedback_expression) {
-                        Ok(script) => FeedbackProcessor::Dynamic { script },
+                        Ok(script) => FeedbackProcessor::Dynamic {
+                            script: CloneAsDefault::new(Some(script)),
+                        },
                         Err(_) => FeedbackProcessor::Text {
                             expression: " ".to_string(),
                         },

main/src/application/source_model.rs

@@ -1,6 +1,7 @@
 use crate::application::{
     Affected, Change, GetProcessingRelevance, MappingProp, ProcessingRelevance,
 };
+use crate::base::CloneAsDefault;
 use crate::domain::{
     BackboneState, Compartment, CompartmentParamIndex, CompoundMappingSource, EelMidiSourceScript,
     ExtendedSourceCharacter, FlexibleMidiSourceScript, KeySource, Keystroke, LuaMidiSourceScript,
@@ -632,19 +633,19 @@ impl SourceModel {
                     },
                     Script => MidiSource::Script {
                         script: {
-                            match self.midi_script_kind {
-                                MidiScriptKind::Eel => {
-                                    EelMidiSourceScript::compile(&self.midi_script)
-                                        .ok()
-                                        .map(FlexibleMidiSourceScript::Eel)
-                                }
+                            let script = match self.midi_script_kind {
+                                MidiScriptKind::Eel => FlexibleMidiSourceScript::Eel(
+                                    EelMidiSourceScript::compile(&self.midi_script).ok()?,
+                                ),
                                 MidiScriptKind::Lua => {
                                     let lua = unsafe { BackboneState::main_thread_lua() };
-                                    LuaMidiSourceScript::compile(lua, &self.midi_script)
-                                        .ok()
-                                        .map(FlexibleMidiSourceScript::Lua)
+                                    FlexibleMidiSourceScript::Lua(
+                                        LuaMidiSourceScript::compile(lua, &self.midi_script)
+                                            .ok()?,
+                                    )
                                 }
-                            }
+                            };
+                            CloneAsDefault::new(Some(script))
                         },
                     },
                     Display => MidiSource::Display {

main/src/base/clone_as_default.rs

@@ -0,0 +1,33 @@
+/// A wrapper that implements Clone but not really by cloning the wrapped value but by
+/// creating the inner type's default value.
+///
+/// This is useful if you have a large nested value of which almost anything inside must be cloned
+/// but you also have a few values in there that don't suit themselves to be cloned (e.g. compiled
+/// scripts). This must be well documented though, it's a very surprising behavior.
+///
+/// Alternatives to be considered before reaching out to this:
+///
+/// - Making the whole graph not cloneable
+/// - Using `Rc` or `Arc` (is cloneable but means that the inner value is not "standalone" anymore,
+///   can be accessed from multiple places or in case of `Arc` even threads ... and it means that
+///   you have less control over when and in which thread deallocation happens).
+/// - Writing a dedicated method (not `clone`) which makes it clear that this is not a standard
+///   clone operation.
+#[derive(Debug)]
+pub struct CloneAsDefault<T>(T);
+
+impl<T> CloneAsDefault<T> {
+    pub fn new(value: T) -> Self {
+        Self(value)
+    }
+
+    pub fn get(&self) -> &T {
+        &self.0
+    }
+}
+
+impl<T: Default> Clone for CloneAsDefault<T> {
+    fn clone(&self) -> Self {
+        Self(T::default())
+    }
+}

main/src/base/eel.rs

@@ -132,6 +132,8 @@ impl Variable {
 impl Drop for Vm {
     fn drop(&mut self) {
         unsafe {
+            // TODO-high-ms2 Oh oh. Just realizing that this kind of deallocation is not covered
+            //  by our automatic deferred Rust deallocation. Look into it.
             root::NSEEL_VM_free(self.vm_ctx);
         }
     }

main/src/base/mod.rs

@@ -15,3 +15,6 @@ pub mod notification;
 pub mod eel;
 
 pub mod bindings;
+
+mod clone_as_default;
+pub use clone_as_default::*;

main/src/domain/eel_midi_source_script.rs

@@ -5,8 +5,6 @@ use helgoboss_learn::{
 };
 use std::borrow::Cow;
 
-use std::sync::Arc;
-
 #[derive(Debug)]
 struct EelUnit {
     // Declared above VM in order to be dropped before VM is dropped.
@@ -17,10 +15,10 @@ struct EelUnit {
     address: eel::Variable,
 }
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct EelMidiSourceScript {
     // Arc because EelUnit is not cloneable
-    eel_unit: Arc<EelUnit>,
+    eel_unit: EelUnit,
 }
 
 impl EelMidiSourceScript {
@@ -40,9 +38,7 @@ impl EelMidiSourceScript {
             msg_size,
             address,
         };
-        Ok(Self {
-            eel_unit: Arc::new(eel_unit),
-        })
+        Ok(Self { eel_unit })
     }
 }
 

main/src/domain/flexible_midi_source_script.rs

@@ -2,7 +2,7 @@ use crate::domain::{EelMidiSourceScript, LuaMidiSourceScript};
 use helgoboss_learn::{FeedbackValue, MidiSourceScript, MidiSourceScriptOutcome};
 use std::borrow::Cow;
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub enum FlexibleMidiSourceScript<'a> {
     Eel(EelMidiSourceScript),
     Lua(LuaMidiSourceScript<'a>),

main/src/domain/lua_feedback_script.rs

@@ -10,7 +10,7 @@ use std::cell::RefCell;
 use std::collections::HashSet;
 use std::error::Error;
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct LuaFeedbackScript<'lua> {
     lua: &'lua SafeLua,
     function: Function<'lua>,

main/src/domain/lua_midi_source_script.rs

@@ -7,7 +7,7 @@ use mlua::{Function, LuaSerdeExt, Table, ToLua, Value};
 use std::borrow::Cow;
 use std::error::Error;
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct LuaMidiSourceScript<'lua> {
     lua: &'lua SafeLua,
     function: Function<'lua>,

main/src/domain/midi_source.rs

@@ -1,3 +1,41 @@
+use crate::base::CloneAsDefault;
 use crate::domain::FlexibleMidiSourceScript;
+use helgoboss_learn::{FeedbackValue, MidiSourceScript, MidiSourceScriptOutcome};
+use std::borrow::Cow;
 
-pub type MidiSource = helgoboss_learn::MidiSource<FlexibleMidiSourceScript<'static>>;
+/// The helgoboss-learn MidiSource, integrated into ReaLearn.
+///
+/// Now this needs some explanation: Why do we wrap the MIDI source script type with
+/// `CloneAsDefault<Option<...>>`!? Because the script is compiled and therefore doesn't suit itself
+/// to being cloned. But we need the MidiSource to be cloneable because we clone it whenever we
+/// sync the mapping(s) from the main processor to the real-time processor. Fortunately, the
+/// real-time processor doesn't use the compiled scripts anyway because those scripts are
+/// responsible for feedback only.
+///
+/// Using `Arc` sounds like a good solution at first but it means that deallocation of the compiled
+/// script could be triggered *in the real-time thread*. Now, we have a custom global deallocator
+/// for automatically deferring deallocation if we are in a real-time thread. **But!** We use
+/// non-Rust script engines (EEL and Lua), so they are not aware of our global allocator ... and
+/// that means we would still get a real-time deallocation :/ Yes, we could handle this by manually
+/// sending the obsolete structs to a deallocation thread *before* the Rust wrappers around the
+/// script engines are even dropped (as we did before), but go there if the real-time processor
+/// doesn't even use the scripts.
+///
+/// Introducing a custom method (not `clone`) would be quite much effort because we can't
+/// derive its usage.
+type ScriptType = CloneAsDefault<Option<FlexibleMidiSourceScript<'static>>>;
+
+pub type MidiSource = helgoboss_learn::MidiSource<ScriptType>;
+
+impl MidiSourceScript for ScriptType {
+    fn execute(
+        &self,
+        input_value: FeedbackValue,
+    ) -> Result<MidiSourceScriptOutcome, Cow<'static, str>> {
+        let script = self
+            .get()
+            .as_ref()
+            .ok_or_else(|| Cow::Borrowed("script was removed on clone"))?;
+        script.execute(input_value)
+    }
+}

main/src/domain/mode.rs

@@ -1,4 +1,32 @@
+use crate::base::CloneAsDefault;
 use crate::domain::{ControlEventTimestamp, EelTransformation, LuaFeedbackScript};
+use helgoboss_learn::{FeedbackScript, FeedbackScriptInput, FeedbackScriptOutput};
+use std::borrow::Cow;
+use std::collections::HashSet;
+use std::error::Error;
 
-pub type Mode =
-    helgoboss_learn::Mode<EelTransformation, LuaFeedbackScript<'static>, ControlEventTimestamp>;
+/// See [`crate::domain::MidiSource`] for an explanation of the feedback script wrapping.
+type FeedbackScriptType = CloneAsDefault<Option<LuaFeedbackScript<'static>>>;
+
+pub type Mode = helgoboss_learn::Mode<EelTransformation, FeedbackScriptType, ControlEventTimestamp>;
+
+impl FeedbackScriptType {
+    fn get_script(&self) -> Result<&LuaFeedbackScript<'static>, Cow<'static, str>> {
+        self.get()
+            .as_ref()
+            .ok_or_else(|| Cow::Borrowed("script was removed on clone"))
+    }
+}
+
+impl FeedbackScript for FeedbackScriptType {
+    fn feedback(
+        &self,
+        input: FeedbackScriptInput,
+    ) -> Result<FeedbackScriptOutput, Cow<'static, str>> {
+        self.get_script()?.feedback(input)
+    }
+
+    fn used_props(&self) -> Result<HashSet<String>, Box<dyn Error>> {
+        self.get_script()?.used_props()
+    }
+}

main/src/domain/real_time_processor.rs

@@ -277,9 +277,12 @@ impl RealTimeProcessor {
                             LifecyclePhase::Deactivation,
                         );
                     }
-                    // Clear existing mappings (without deallocating)
-                    // TODO-high-ms2 If there's a mapping with a FlexibleMidiSourceScript and this
-                    //  contains Lua code, the Lua Drop handler allocates! (lua.rs line 1739)
+                    // Clear existing mappings
+                    // TODO-high-ms2 Look into eel.rs => The drop here causes EEL to free its memory
+                    //  and because it's not aware of our rt-friendly Rust allocator, it will
+                    //  deallocate in real-time! We don't have the same issue with the Lua scripts,
+                    //  simply because they don't even get synced over to the real-time processor
+                    //  (necessary for feedback only, which is handled in main processor).
                     self.mappings[compartment].clear();
                     // Set new mappings
                     self.mappings[compartment].extend(mappings.into_iter().map(|m| (m.id(), m)));

main/src/infrastructure/ui/app/app_instance.rs

@@ -8,7 +8,6 @@ use playtime_clip_engine::proto::{
     event_reply, reply, ClipEngineReceivers, Empty, EventReply, Reply,
 };
 use prost::Message;
-use reaper_low::raw;
 use std::cell::RefCell;
 use std::fmt::Debug;
 use std::rc::Rc;

main/src/infrastructure/ui/app/app_library.rs

@@ -1,6 +1,6 @@
 use crate::infrastructure::plugin::App;
 use crate::infrastructure::server::services::playtime_service::AppMatrixProvider;
-use crate::infrastructure::ui::{AppCallback, AppPanel, SharedAppInstance};
+use crate::infrastructure::ui::{AppCallback, SharedAppInstance};
 use anyhow::{anyhow, bail, Context, Result};
 use base::Global;
 use libloading::{Library, Symbol};
@@ -20,7 +20,7 @@ use std::ffi::{c_char, c_void, CString};
 use std::future::Future;
 use std::path::{Path, PathBuf};
 use std::ptr::{null_mut, NonNull};
-use swell_ui::{SharedView, Window};
+use swell_ui::Window;
 use tonic::Status;
 
 #[derive(Debug)]

main/src/infrastructure/ui/independent_panel_manager.rs

@@ -1,6 +1,4 @@
-use crate::infrastructure::ui::{
-    AppInstance, MainPanel, MappingPanel, SessionMessagePanel, SharedAppInstance,
-};
+use crate::infrastructure::ui::{MainPanel, MappingPanel, SessionMessagePanel, SharedAppInstance};
 use reaper_high::Reaper;
 use slog::debug;