From 2d2eb70b51f5dadbc0523bba733d317e4b47e6c2 Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Thu, 9 Jun 2022 17:55:37 +0200 Subject: [PATCH] Fix panic about multiple mutable borrows with the software renderer When using repeaters - like in the slide puzzle - and during renderer a component gets deleted, we call free_graphics_resources and try to free the dirty rectangle list in the partial renderer cache. At that point the cache is already mutably borrowed, which causes a panic. As remedy, apply the mutable borrow more fine grained and not right when calling render(). --- internal/backends/mcu/lib.rs | 4 ++-- internal/backends/mcu/simulator.rs | 4 ++-- internal/core/item_rendering.rs | 22 +++++++++++++--------- internal/core/swrenderer.rs | 16 +++++++++------- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/internal/backends/mcu/lib.rs b/internal/backends/mcu/lib.rs index 007f97a58..9c19c0fec 100644 --- a/internal/backends/mcu/lib.rs +++ b/internal/backends/mcu/lib.rs @@ -131,7 +131,7 @@ mod the_backend { items: &mut dyn Iterator>>, ) { super::LINE_RENDERER.with(|renderer| { - renderer.borrow_mut().free_graphics_resources(items); + renderer.borrow().free_graphics_resources(items); }); } @@ -346,7 +346,7 @@ mod the_backend { }; LINE_RENDERER.with(|renderer| { - renderer.borrow_mut().render( + renderer.borrow().render( runtime_window, window.initial_dirty_region_for_next_frame.take(), buffer_provider, diff --git a/internal/backends/mcu/simulator.rs b/internal/backends/mcu/simulator.rs index 864616fea..783748470 100644 --- a/internal/backends/mcu/simulator.rs +++ b/internal/backends/mcu/simulator.rs @@ -143,7 +143,7 @@ impl PlatformWindow for SimulatorWindow { items: &mut dyn Iterator>>, ) { super::LINE_RENDERER.with(|cache| { - cache.borrow_mut().free_graphics_resources(items); + cache.borrow().free_graphics_resources(items); }); } @@ -325,7 +325,7 @@ impl WinitWindow for SimulatorWindow { } } super::LINE_RENDERER.with(|renderer| { - renderer.borrow_mut().render( + renderer.borrow().render( runtime_window, self.initial_dirty_region_for_next_frame.take(), BufferProvider { diff --git a/internal/core/item_rendering.rs b/internal/core/item_rendering.rs index 5afc59651..41801e5a4 100644 --- a/internal/core/item_rendering.rs +++ b/internal/core/item_rendering.rs @@ -382,7 +382,7 @@ pub type DirtyRegion = euclid::default::Box2D; /// Put this structure in the renderer to help with partial rendering pub struct PartialRenderer<'a, T> { - cache: &'a mut PartialRenderingCache, + cache: &'a RefCell, /// The region of the screen which is considered dirty and that should be repainted pub dirty_region: DirtyRegion, actual_renderer: T, @@ -391,7 +391,7 @@ pub struct PartialRenderer<'a, T> { impl<'a, T> PartialRenderer<'a, T> { /// Create a new PartialRenderer pub fn new( - cache: &'a mut PartialRenderingCache, + cache: &'a RefCell, initial_dirty_region: DirtyRegion, actual_renderer: T, ) -> Self { @@ -404,7 +404,9 @@ impl<'a, T> PartialRenderer<'a, T> { crate::item_tree::visit_items( component, crate::item_tree::TraversalOrder::BackToFront, - |_, item, _, offset| match item.cached_rendering_data_offset().get_entry(self.cache) + |_, item, _, offset| match item + .cached_rendering_data_offset() + .get_entry(&mut self.cache.borrow_mut()) { Some(CachedGraphicsData { data, dependency_tracker: Some(tr) }) => { if tr.is_dirty() { @@ -435,11 +437,11 @@ impl<'a, T> PartialRenderer<'a, T> { } fn do_rendering( - cache: &mut PartialRenderingCache, + cache: &RefCell, rendering_data: &CachedRenderingData, render_fn: impl FnOnce() -> Rect, ) { - if let Some(entry) = rendering_data.get_entry(cache) { + if let Some(entry) = rendering_data.get_entry(&mut cache.borrow_mut()) { entry .dependency_tracker .get_or_insert_with(|| Box::pin(crate::properties::PropertyTracker::default())) @@ -447,6 +449,7 @@ impl<'a, T> PartialRenderer<'a, T> { .evaluate(render_fn); } else { let cache_entry = crate::graphics::CachedGraphicsData::new(render_fn); + let mut cache = cache.borrow_mut(); rendering_data.cache_index.set(cache.insert(cache_entry)); rendering_data.cache_generation.set(cache.generation()); } @@ -461,7 +464,7 @@ impl<'a, T> PartialRenderer<'a, T> { macro_rules! forward_rendering_call { (fn $fn:ident($Ty:ty)) => { fn $fn(&mut self, obj: Pin<&$Ty>, item_rc: &ItemRc) { - Self::do_rendering(&mut self.cache, &obj.cached_rendering_data, || { + Self::do_rendering(&self.cache, &obj.cached_rendering_data, || { self.actual_renderer.$fn(obj, item_rc); type Ty = $Ty; let width = Ty::FIELD_OFFSETS.width.apply_pin(obj).get_untracked(); @@ -477,7 +480,8 @@ macro_rules! forward_rendering_call { impl<'a, T: ItemRenderer> ItemRenderer for PartialRenderer<'a, T> { fn filter_item(&mut self, item: Pin) -> (bool, Rect) { let rendering_data = item.cached_rendering_data_offset(); - let item_geometry = match rendering_data.get_entry(self.cache) { + let mut cache = self.cache.borrow_mut(); + let item_geometry = match rendering_data.get_entry(&mut cache) { Some(CachedGraphicsData { data, dependency_tracker }) => { dependency_tracker .get_or_insert_with(|| Box::pin(crate::properties::PropertyTracker::default())) @@ -489,8 +493,8 @@ impl<'a, T: ItemRenderer> ItemRenderer for PartialRenderer<'a, T> { let cache_entry = crate::graphics::CachedGraphicsData::new(|| item.as_ref().geometry()); let geom = cache_entry.data; - rendering_data.cache_index.set(self.cache.insert(cache_entry)); - rendering_data.cache_generation.set(self.cache.generation()); + rendering_data.cache_index.set(cache.insert(cache_entry)); + rendering_data.cache_generation.set(cache.generation()); geom } }; diff --git a/internal/core/swrenderer.rs b/internal/core/swrenderer.rs index 3ca9721c4..223c5a465 100644 --- a/internal/core/swrenderer.rs +++ b/internal/core/swrenderer.rs @@ -15,6 +15,7 @@ use crate::textlayout::{FontMetrics as _, TextParagraphLayout}; use crate::{Color, Coord, ImageInner, StaticTextures}; use alloc::rc::Rc; use alloc::{vec, vec::Vec}; +use core::cell::RefCell; use core::pin::Pin; pub use draw_functions::TargetPixel; @@ -41,7 +42,7 @@ pub trait LineBufferProvider { #[derive(Default)] pub struct LineRenderer { - partial_cache: crate::item_rendering::PartialRenderingCache, + partial_cache: RefCell, } impl LineRenderer { @@ -56,7 +57,7 @@ impl LineRenderer { /// (can we call the line_buffer function from different thread?) /// TODO: should `initial_dirty_region` be set from a different call? pub fn render( - &mut self, + &self, window: Rc, initial_dirty_region: crate::item_rendering::DirtyRegion, line_buffer: impl LineBufferProvider, @@ -73,18 +74,19 @@ impl LineRenderer { window_item.background(), size.cast(), initial_dirty_region, - &mut self.partial_cache, + &self.partial_cache, line_buffer, ); } } pub fn free_graphics_resources( - &mut self, + &self, items: &mut dyn Iterator>>, ) { for item in items { - let cache_entry = item.cached_rendering_data_offset().release(&mut self.partial_cache); + let cache_entry = + item.cached_rendering_data_offset().release(&mut self.partial_cache.borrow_mut()); drop(cache_entry); } } @@ -95,7 +97,7 @@ fn render_window_frame( background: Color, size: PhysicalSize, initial_dirty_region: crate::item_rendering::DirtyRegion, - cache: &mut PartialRenderingCache, + cache: &RefCell, mut line_buffer: impl LineBufferProvider, ) { let mut scene = @@ -367,7 +369,7 @@ fn prepare_scene( size: PhysicalSize, initial_dirty_region: crate::item_rendering::DirtyRegion, line_buffer: &mut impl LineBufferProvider, - cache: &mut PartialRenderingCache, + cache: &RefCell, ) -> Scene { let factor = ScaleFactor::new(runtime_window.scale_factor()); let prepare_scene = PrepareScene::new(size, factor, runtime_window.default_font_properties());