From b0074b26efba67b2f5e3af14ef700bc2c7531520 Mon Sep 17 00:00:00 2001 From: Benjamin Klum Date: Thu, 16 Nov 2023 17:34:24 +0100 Subject: [PATCH] #896 Fix hard crash when duplicating mappings that have Lua scripts This crash was introduced by v2.16.0-pre.2 commit 073b9b25, 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/lib/helgoboss-learn | 2 +- main/src/application/mode_model.rs | 5 ++- main/src/application/source_model.rs | 21 +++++----- main/src/base/clone_as_default.rs | 33 +++++++++++++++ main/src/base/eel.rs | 2 + main/src/base/mod.rs | 3 ++ main/src/domain/eel_midi_source_script.rs | 10 ++--- .../src/domain/flexible_midi_source_script.rs | 2 +- main/src/domain/lua_feedback_script.rs | 2 +- main/src/domain/lua_midi_source_script.rs | 2 +- main/src/domain/midi_source.rs | 40 ++++++++++++++++++- main/src/domain/mode.rs | 32 ++++++++++++++- main/src/domain/real_time_processor.rs | 9 +++-- .../src/infrastructure/ui/app/app_instance.rs | 1 - main/src/infrastructure/ui/app/app_library.rs | 4 +- .../ui/independent_panel_manager.rs | 4 +- 16 files changed, 138 insertions(+), 34 deletions(-) create mode 100644 main/src/base/clone_as_default.rs diff --git a/main/lib/helgoboss-learn b/main/lib/helgoboss-learn index f345ad5c4..1c50c253d 160000 --- a/main/lib/helgoboss-learn +++ b/main/lib/helgoboss-learn @@ -1 +1 @@ -Subproject commit f345ad5c4d654c910a673ac8a006b9185569254d +Subproject commit 1c50c253dd02ed4577aa7809fbf6a8b2928f1030 diff --git a/main/src/application/mode_model.rs b/main/src/application/mode_model.rs index c9e582519..63d86dc67 100644 --- a/main/src/application/mode_model.rs +++ b/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(), }, diff --git a/main/src/application/source_model.rs b/main/src/application/source_model.rs index fa57cd9ff..fbfcb3328 100644 --- a/main/src/application/source_model.rs +++ b/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 { diff --git a/main/src/base/clone_as_default.rs b/main/src/base/clone_as_default.rs new file mode 100644 index 000000000..bbb0f67d8 --- /dev/null +++ b/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); + +impl CloneAsDefault { + pub fn new(value: T) -> Self { + Self(value) + } + + pub fn get(&self) -> &T { + &self.0 + } +} + +impl Clone for CloneAsDefault { + fn clone(&self) -> Self { + Self(T::default()) + } +} diff --git a/main/src/base/eel.rs b/main/src/base/eel.rs index 978c9c3dc..2587af018 100644 --- a/main/src/base/eel.rs +++ b/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); } } diff --git a/main/src/base/mod.rs b/main/src/base/mod.rs index acb51c109..648fe7b87 100644 --- a/main/src/base/mod.rs +++ b/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::*; diff --git a/main/src/domain/eel_midi_source_script.rs b/main/src/domain/eel_midi_source_script.rs index 0656d86ed..cdb7dd403 100644 --- a/main/src/domain/eel_midi_source_script.rs +++ b/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, + 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 }) } } diff --git a/main/src/domain/flexible_midi_source_script.rs b/main/src/domain/flexible_midi_source_script.rs index edb93108e..6902749b8 100644 --- a/main/src/domain/flexible_midi_source_script.rs +++ b/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>), diff --git a/main/src/domain/lua_feedback_script.rs b/main/src/domain/lua_feedback_script.rs index 367da2152..3b8b3f184 100644 --- a/main/src/domain/lua_feedback_script.rs +++ b/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>, diff --git a/main/src/domain/lua_midi_source_script.rs b/main/src/domain/lua_midi_source_script.rs index 7a17b7aab..8e6b89025 100644 --- a/main/src/domain/lua_midi_source_script.rs +++ b/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>, diff --git a/main/src/domain/midi_source.rs b/main/src/domain/midi_source.rs index 2ce825a05..add8af23e 100644 --- a/main/src/domain/midi_source.rs +++ b/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>; +/// The helgoboss-learn MidiSource, integrated into ReaLearn. +/// +/// Now this needs some explanation: Why do we wrap the MIDI source script type with +/// `CloneAsDefault>`!? 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>>; + +pub type MidiSource = helgoboss_learn::MidiSource; + +impl MidiSourceScript for ScriptType { + fn execute( + &self, + input_value: FeedbackValue, + ) -> Result> { + let script = self + .get() + .as_ref() + .ok_or_else(|| Cow::Borrowed("script was removed on clone"))?; + script.execute(input_value) + } +} diff --git a/main/src/domain/mode.rs b/main/src/domain/mode.rs index d20bb589d..48a1c3431 100644 --- a/main/src/domain/mode.rs +++ b/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, ControlEventTimestamp>; +/// See [`crate::domain::MidiSource`] for an explanation of the feedback script wrapping. +type FeedbackScriptType = CloneAsDefault>>; + +pub type Mode = helgoboss_learn::Mode; + +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> { + self.get_script()?.feedback(input) + } + + fn used_props(&self) -> Result, Box> { + self.get_script()?.used_props() + } +} diff --git a/main/src/domain/real_time_processor.rs b/main/src/domain/real_time_processor.rs index e9910bacd..67cd5956d 100644 --- a/main/src/domain/real_time_processor.rs +++ b/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))); diff --git a/main/src/infrastructure/ui/app/app_instance.rs b/main/src/infrastructure/ui/app/app_instance.rs index 229533d56..cf1e1ff95 100644 --- a/main/src/infrastructure/ui/app/app_instance.rs +++ b/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; diff --git a/main/src/infrastructure/ui/app/app_library.rs b/main/src/infrastructure/ui/app/app_library.rs index 1b4b1c717..6e4527dda 100644 --- a/main/src/infrastructure/ui/app/app_library.rs +++ b/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)] diff --git a/main/src/infrastructure/ui/independent_panel_manager.rs b/main/src/infrastructure/ui/independent_panel_manager.rs index cd2bd9b46..7fe86971b 100644 --- a/main/src/infrastructure/ui/independent_panel_manager.rs +++ b/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;