Fix panic when trying to access layout cache of destroyed items

This can be reproduced by deleting the last item of the printer queue in the printer demo. It is a regression showing up because we now emit the MouseExit event after the mouse grab as released. The problem is that we upgrade the weak item, and call geometry() on it. Calling geometry will re-evaluate the layout cache which will re-evaluate the model which will result in the component being removed and the cache entry having less item than expected. It is ok to simply return 0. for these layout location since the item will disapear anyway.
task-jp · Sep 8, 2021 · fafcbfd · fafcbfd
1 parent e8c1fcc
commit fafcbfd
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 4 deletions.
diff --git a/api/sixtyfps-cpp/include/sixtyfps.h b/api/sixtyfps-cpp/include/sixtyfps.h
@@ -403,6 +403,12 @@ inline bool operator!=(const LayoutInfo &a, const LayoutInfo &b)
 }
 
 namespace private_api {
+/// Access the layout cache of an item within a repeater
+inline float layout_cache_access(const SharedVector<float> &cache, int offset, int repeater_index) {
+    size_t idx = size_t(cache[offset]) + repeater_index * 2;
+    return idx < cache.size() ? cache[idx] : 0;
+}
+
 // models
 struct AbstractRepeaterView
 {

diff --git a/sixtyfps_compiler/generator/cpp.rs b/sixtyfps_compiler/generator/cpp.rs
@@ -1854,7 +1854,7 @@ fn compile_expression(
         Expression::LayoutCacheAccess { layout_cache_prop, index, repeater_index } => {
             let cache = access_named_reference(layout_cache_prop, component, "self");
             if let Some(ri) = repeater_index {
-                format!("[&](auto cache) {{ return cache[int(cache[{}]) + {} * 2]; }} ({}.get())", index, compile_expression(ri, component), cache)
+                format!("sixtyfps::private_api::layout_cache_access({}.get(), {}, {})", cache, index, compile_expression(ri, component))
             } else {
                 format!("{}.get()[{}]", cache, index)
             }

diff --git a/sixtyfps_compiler/generator/rust.rs b/sixtyfps_compiler/generator/rust.rs
@@ -1573,7 +1573,7 @@ fn compile_expression(expr: &Expression, component: &Rc<Component>) -> TokenStre
                 let offset = compile_expression(ri, component);
                 quote!({
                     let cache = #cache.get();
-                    cache[(cache[#index] as usize) + #offset as usize * 2]
+                    *cache.get((cache[#index] as usize) + #offset as usize * 2).unwrap_or(&0.)
                 })
             } else {
                 quote!(#cache.get()[#index])

diff --git a/sixtyfps_runtime/corelib/layout.rs b/sixtyfps_runtime/corelib/layout.rs
@@ -450,6 +450,13 @@ pub struct BoxLayoutCellData {
 
 /// Solve a BoxLayout
 pub fn solve_box_layout(data: &BoxLayoutData, repeater_indexes: Slice<u32>) -> SharedVector<Coord> {
+    let mut result = SharedVector::<f32>::default();
+    result.resize(data.cells.len() * 2 + repeater_indexes.len(), 0.);
+
+    if data.cells.is_empty() {
+        return result;
+    }
+
     let mut layout_data: Vec<_> = data
         .cells
         .iter()
@@ -514,8 +521,6 @@ pub fn solve_box_layout(data: &BoxLayoutData, repeater_indexes: Slice<u32>) -> S
         }
     }
 
-    let mut result = SharedVector::<f32>::default();
-    result.resize(data.cells.len() * 2 + repeater_indexes.len(), 0.);
     let res = result.make_mut_slice();
 
     // The index/2 in result in which we should add the next repeated item

diff --git a/tests/cases/crashes/layout_deleted_item.60 b/tests/cases/crashes/layout_deleted_item.60
@@ -0,0 +1,84 @@
+/* LICENSE BEGIN
+    This file is part of the SixtyFPS Project -- https://sixtyfps.io
+    Copyright (c) 2021 Olivier Goffart <olivier.goffart@sixtyfps.io>
+    Copyright (c) 2021 Simon Hausmann <simon.hausmann@sixtyfps.io>
+
+    SPDX-License-Identifier: GPL-3.0-only
+    This file is also available under commercial licensing terms.
+    Please contact info@sixtyfps.io for more information.
+LICENSE END */
+
+// issue #177
+
+export TestCase := Window {
+    width: 100px;
+    height: 100px;
+
+    callback clicked;
+    clicked => { debug("Hello"); model= []; }
+    property <bool> hover <=> under.has-hover;
+    property<[int]> model: [1];
+    VerticalLayout {
+        under := TouchArea {
+            HorizontalLayout {
+                for value in model: TouchArea {
+                    horizontal-stretch: 5;
+                    vertical-stretch: 5;
+                    clicked => { root.clicked(); }
+                    Rectangle { background: blue; }
+                }
+            }
+        }
+        Rectangle {
+            horizontal-stretch: 0;
+            vertical-stretch: 0;
+            background: yellow;
+        }
+    }
+
+
+}
+
+/*
+
+```cpp
+auto handle = TestCase::create();
+const TestCase &instance = *handle;
+auto vec_model = std::make_shared<sixtyfps::VectorModel<int>>(std::vector<int>{1, 2});
+instance.set_model(vec_model);
+instance.on_clicked([vec_model] { vec_model->erase(vec_model->row_count()-1); });
+sixtyfps::testing::send_mouse_click(&instance, 95., 5.);
+assert_eq(instance.get_model()->row_count(), 1);
+assert(instance.get_hover());
+sixtyfps::testing::send_mouse_click(&instance, 95., 5.);
+assert_eq(instance.get_model()->row_count(), 0);
+assert(!instance.get_hover());
+```
+
+```rust
+use sixtyfps::Model;
+let instance = TestCase::new();
+let vec_model = std::rc::Rc::new(sixtyfps::VecModel::from(vec![1i32, 2i32]));
+instance.set_model(sixtyfps::ModelHandle::from(vec_model.clone() as std::rc::Rc<dyn Model<Data = i32>>));
+instance.on_clicked(move || dbg!(vec_model.remove(vec_model.row_count() - 1)));
+sixtyfps::testing::send_mouse_click(&instance, 95., 5.);
+assert_eq!(instance.get_model().row_count(), 1);
+assert!(instance.get_hover());
+sixtyfps::testing::send_mouse_click(&instance, 95., 5.);
+assert_eq!(instance.get_model().row_count(), 0);
+assert!(!instance.get_hover());
+```
+
+```js
+var instance = new sixtyfps.TestCase({
+    clicked: function() { var x = instance.model; x.pop(); instance.model = x; }
+});
+instance.model = [1, 2];
+instance.send_mouse_click(5., 5.);
+assert.equal(instance.model.length, 1);
+assert(instance.hover);
+instance.send_mouse_click(5., 5.);
+assert.equal(instance.model.length, 0);
+assert(!instance.hover);
+```
+*/