Fix const detection with two ways binding

The const detection for two way binding was not detecting change if one of the property was set to a const value in a component using it. This would cause the compiler to generate call set_content on one of the property in a two way bindings, and later, the "const sentinel" be present in the dependency list, causing crash. To avoid segfault for similar bug in the future, added added an assert! in the property system to detect that. Fixes slint-ui#2185
task-jp · Feb 7, 2023 · dddfcf5 · dddfcf5
1 parent 7990bfe
commit dddfcf5
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 5 deletions.
diff --git a/internal/compiler/passes/binding_analysis.rs b/internal/compiler/passes/binding_analysis.rs
@@ -524,8 +524,8 @@ fn propagate_is_set_on_aliases(component: &Rc<Component>, reverse_aliases: &mut
     }
 
     fn mark_alias(alias: &NamedReference) {
+        alias.mark_as_set();
         if !alias.is_externally_modified() {
-            alias.mark_as_set();
             if let Some(bind) = alias.element().borrow().bindings.get(alias.name()) {
                 propagate_alias(&bind.borrow())
             }

diff --git a/internal/core/properties.rs b/internal/core/properties.rs
@@ -493,8 +493,8 @@ impl PropertyHandle {
                     (*binding).dependencies.set(0);
                 } else {
                     DependencyListHead::mem_move(
-                        (&mut (*binding).dependencies) as *mut _ as *mut _,
-                        self.handle.as_ptr() as *mut _,
+                        (*binding).dependencies.as_ptr() as *mut DependencyListHead,
+                        self.handle.as_ptr() as *mut DependencyListHead,
                     );
                 }
                 ((*binding).vtable.drop)(binding);
@@ -540,8 +540,8 @@ impl PropertyHandle {
                 (*binding).dependencies.set(const_sentinel);
             } else {
                 DependencyListHead::mem_move(
-                    self.handle.as_ptr() as *mut _,
-                    (&mut (*binding).dependencies) as *mut _ as *mut _,
+                    self.handle.as_ptr() as *mut DependencyListHead,
+                    (*binding).dependencies.as_ptr() as *mut DependencyListHead,
                 );
             }
         }
@@ -657,10 +657,23 @@ impl Drop for PropertyHandle {
 
 /// Safety: the dependency list must be valid and consistent
 unsafe fn mark_dependencies_dirty(dependencies: *mut DependencyListHead) {
+    debug_assert!(!core::ptr::eq(
+        *(dependencies as *mut *const u32),
+        (&CONSTANT_PROPERTY_SENTINEL) as *const u32,
+    ));
     DependencyListHead::for_each(&*dependencies, |binding| {
         let binding: &BindingHolder = &**binding;
         let was_dirty = binding.dirty.replace(true);
         (binding.vtable.mark_dirty)(binding as *const BindingHolder, was_dirty);
+
+        assert!(
+            !core::ptr::eq(
+                binding.dependencies.as_ptr() as *const u32,
+                (&CONSTANT_PROPERTY_SENTINEL) as *const u32,
+            ),
+            "Const property marked as dirty"
+        );
+
         mark_dependencies_dirty(binding.dependencies.as_ptr() as *mut DependencyListHead)
     });
 }
@@ -1247,6 +1260,10 @@ impl<DirtyHandler: PropertyDirtyHandler> PropertyTracker<DirtyHandler> {
         if CURRENT_BINDING.is_set() {
             CURRENT_BINDING.with(|cur_binding| {
                 if let Some(cur_binding) = cur_binding {
+                    debug_assert!(!core::ptr::eq(
+                        self.holder.dependencies.get() as *const u32,
+                        (&CONSTANT_PROPERTY_SENTINEL) as *const u32,
+                    ));
                     cur_binding.register_self_as_dependency(
                         self.holder.dependencies.as_ptr() as *mut DependencyListHead,
                         #[cfg(slint_debug_property)]

diff --git a/tests/cases/bindings/change_sub_property_indirection_issue2185.slint b/tests/cases/bindings/change_sub_property_indirection_issue2185.slint
@@ -0,0 +1,59 @@
+// Copyright © SixtyFPS GmbH <info@slint-ui.com>
+// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-commercial
+
+component Buggy {
+    in-out property<string> magic: "Nope";
+    property<bool> condition: false;
+
+    public function change_condition() {
+        condition = !condition;
+    }
+
+    HorizontalLayout {
+        if root.condition: TouchArea {
+            property <string> innermagic <=> root.magic;
+            clicked => { self.innermagic = "Hello"; }
+            horizontal-stretch: 1;
+        }
+
+        Rectangle {
+            background: red;
+            TouchArea {
+                clicked => { change-condition() }
+            }
+        }
+    }
+}
+
+
+component TestCase {
+    width: 300px;
+    height: 100px;
+
+    public function change_condition() {
+        b.change-condition();
+    }
+
+    b := Buggy {
+        magic: "Hi";
+    }
+
+    out property <string> magic: b.magic;
+}
+
+
+/*
+
+```rust
+let instance = TestCase::new();
+assert_eq!(instance.get_magic(), slint::SharedString::from("Hi"));
+instance.invoke_change_condition();
+instance.invoke_change_condition();
+instance.invoke_change_condition();
+assert_eq!(instance.get_magic(), slint::SharedString::from("Hi"));
+slint_testing::send_mouse_click(&instance, 10., 10.);
+assert_eq!(instance.get_magic(), slint::SharedString::from("Hello"));
+```
+
+
+*/