Selaa lähdekoodia

Fix leak in render macro

Jonathan Kelley 1 vuosi sitten
vanhempi
commit
44a27bf8a3

+ 2 - 1
packages/core/src/arena.rs

@@ -1,4 +1,5 @@
-use crate::ScopeState;use std::ptr::NonNull;
+use crate::ScopeState;
+use std::ptr::NonNull;
 
 use crate::{
     innerlude::DirtyScope, nodes::RenderReturn, nodes::VNode, virtual_dom::VirtualDom,

+ 8 - 9
packages/core/src/fragment.rs

@@ -28,15 +28,14 @@ use crate::innerlude::*;
 #[allow(non_upper_case_globals, non_snake_case)]
 pub fn Fragment<'a>(cx: Scope<'a, FragmentProps<'a>>) -> Element {
     let children = cx.props.0.as_ref()?;
-    Some(VNode {
-        key: children.key,
-        parent: children.parent.clone(),
-        stable_id: children.stable_id.clone(),
-        template: children.template.clone(),
-        root_ids: children.root_ids.clone(),
-        dynamic_nodes: children.dynamic_nodes,
-        dynamic_attrs: children.dynamic_attrs,
-    })
+    Some(cx.vnode(
+        children.parent.clone(),
+        children.key,
+        children.template.clone(),
+        children.root_ids.clone(),
+        children.dynamic_nodes,
+        children.dynamic_attrs,
+    ))
 }
 
 pub struct FragmentProps<'a>(Element<'a>);

+ 10 - 10
packages/core/src/lazynodes.rs

@@ -24,7 +24,7 @@ use crate::{innerlude::VNode, ScopeState};
 ///
 /// ```rust, ignore
 /// LazyNodes::new(|f| {
-///        static TEMPLATE: dioxus::core::Template = dioxus::core::Template {
+///     static TEMPLATE: dioxus::core::Template = dioxus::core::Template {
 ///         name: "main.rs:5:5:20", // Source location of the template for hot reloading
 ///         roots: &[
 ///             dioxus::core::TemplateNode::Element {
@@ -37,19 +37,19 @@ use crate::{innerlude::VNode, ScopeState};
 ///         node_paths: &[],
 ///         attr_paths: &[],
 ///     };
-///     dioxus::core::VNode {
-///         parent: None,
-///         key: None,
-///         template: std::cell::Cell::new(TEMPLATE),
-///         root_ids: dioxus::core::exports::bumpalo::collections::Vec::with_capacity_in(
+///     f.vnode(
+///         None,
+///         None,
+///         std::cell::Cell::new(TEMPLATE),
+///         dioxus::core::exports::bumpalo::collections::Vec::with_capacity_in(
 ///                 1usize,
 ///                 f.bump(),
 ///             )
 ///             .into(),
-///         dynamic_nodes: f.bump().alloc([]),
-///         dynamic_attrs: f.bump().alloc([]),
-///     })
-/// }
+///         f.bump().alloc([]),
+///         f.bump().alloc([]),
+///     )
+/// })
 /// ```
 ///
 /// Find more information about how to construct [`VNode`] at <https://dioxuslabs.com/learn/0.4/contributing/walkthrough_readme#the-rsx-macro>

+ 19 - 19
packages/core/src/nodes.rs

@@ -40,6 +40,7 @@ impl<'a> Default for RenderReturn<'a> {
 ///
 /// The dynamic parts of the template are stored separately from the static parts. This allows faster diffing by skipping
 /// static parts of the template.
+#[non_exhaustive]
 #[derive(Debug, Clone)]
 pub struct VNode<'a> {
     /// The key given to the root of this template.
@@ -70,20 +71,19 @@ pub struct VNode<'a> {
 impl<'a> VNode<'a> {
     /// Create a template with no nodes that will be skipped over during diffing
     pub fn empty(cx: &'a ScopeState) -> Element<'a> {
-        Some(VNode {
-            key: None,
-            parent: Default::default(),
-            stable_id: Default::default(),
-            root_ids: RefCell::new(bumpalo::collections::Vec::new_in(cx.bump())),
-            dynamic_nodes: &[],
-            dynamic_attrs: &[],
-            template: Cell::new(Template {
+        Some(cx.vnode(
+            Cell::new(None),
+            None,
+            Cell::new(Template {
                 name: "dioxus-empty",
                 roots: &[],
                 node_paths: &[],
                 attr_paths: &[],
             }),
-        })
+            RefCell::new(bumpalo::collections::Vec::new_in(cx.bump())),
+            &[],
+            &[],
+        ))
     }
 
     /// Create a new VNode
@@ -820,16 +820,16 @@ impl<'b> IntoDynNode<'b> for Arguments<'_> {
 }
 
 impl<'a> IntoDynNode<'a> for &'a VNode<'a> {
-    fn into_dyn_node(self, _cx: &'a ScopeState) -> DynamicNode<'a> {
-        DynamicNode::Fragment(_cx.bump().alloc([VNode {
-            parent: self.parent.clone(),
-            stable_id: self.stable_id.clone(),
-            template: self.template.clone(),
-            root_ids: self.root_ids.clone(),
-            key: self.key,
-            dynamic_nodes: self.dynamic_nodes,
-            dynamic_attrs: self.dynamic_attrs,
-        }]))
+    fn into_dyn_node(self, cx: &'a ScopeState) -> DynamicNode<'a> {
+        let vnode = cx.vnode(
+            self.parent.clone(),
+            self.key,
+            self.template.clone(),
+            self.root_ids.clone(),
+            self.dynamic_nodes,
+            self.dynamic_attrs,
+        );
+        DynamicNode::Fragment(cx.bump().alloc([vnode]))
     }
 }
 

+ 76 - 48
packages/core/src/scopes.rs

@@ -2,13 +2,13 @@ use crate::{
     any_props::AnyProps,
     any_props::VProps,
     bump_frame::BumpFrame,
-    innerlude::{DynamicNode, EventHandler, VComponent, VNodeId, VText},
+    innerlude::{DynamicNode, ElementRef, EventHandler, VComponent, VNodeId, VText},
     lazynodes::LazyNodes,
     nodes::{IntoAttributeValue, IntoDynNode, RenderReturn},
     runtime::Runtime,
     scope_context::ScopeContext,
-    AnyValue, Attribute, AttributeType, AttributeValue, Element, Event, MountedAttribute,
-    Properties, TaskId,
+    AnyValue, Attribute, AttributeType, AttributeValue, Element, ElementId, Event,
+    MountedAttribute, Properties, TaskId, Template, VNode,
 };
 use bumpalo::{boxed::Box as BumpBox, Bump};
 use std::{
@@ -348,17 +348,76 @@ impl<'src> ScopeState {
     /// }
     /// ```
     pub fn render(&'src self, rsx: LazyNodes<'src, '_>) -> Element<'src> {
-        let element = rsx.call(self);
-
+        // <<<<<<< HEAD
+        //         let element = rsx.call(self);
+
+        //         let mut listeners = self.attributes_to_drop_before_render.borrow_mut();
+        //         for attr in element.dynamic_attrs {
+        //             attr.ty.for_each(|attr| {
+        //                 match attr.value {
+        //                     // We need to drop listeners before the next render because they may borrow data from the borrowed props which will be dropped
+        //                     AttributeValue::Listener(_) => {
+        //                         let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) };
+        //                         listeners.push(unbounded);
+        //                     }
+        //                     // We need to drop any values manually to make sure that their drop implementation is called before the next render
+        //                     AttributeValue::Any(_) => {
+        //                         let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) };
+        //                         self.previous_frame().add_attribute_to_drop(unbounded);
+        //                     }
+
+        //                     _ => (),
+        //                 }
+        //             })
+        //         }
+
+        //         let mut props = self.borrowed_props.borrow_mut();
+        //         let mut drop_props = self
+        //             .previous_frame()
+        //             .props_to_drop_before_reset
+        //             .borrow_mut();
+        //         for node in element.dynamic_nodes {
+        //             if let DynamicNode::Component(comp) = node {
+        //                 let unbounded = unsafe { std::mem::transmute(comp as *const VComponent) };
+        //                 if !comp.static_props {
+        //                     props.push(unbounded);
+        //                 }
+        //                 drop_props.push(unbounded);
+        //             }
+        //         }
+
+        //         Some(element)
+        // =======
+        // Note: We can't do anything in this function related to safety because the user could always circumvent it by creating a VNode manually and return it without calling this function
+        Some(rsx.call(self))
+        // >>>>>>> 9fe172e9 (Fix leak in render macro)
+    }
+
+    /// Create a [`crate::VNode`] from the component parts
+    pub fn vnode(
+        &'src self,
+        parent: Cell<Option<ElementRef>>,
+        key: Option<&'src str>,
+        template: Cell<Template<'static>>,
+        root_ids: RefCell<bumpalo::collections::Vec<'src, ElementId>>,
+        dynamic_nodes: &'src [DynamicNode<'src>],
+        dynamic_attrs: &'src [MountedAttribute<'src>],
+    ) -> VNode<'src> {
         let mut listeners = self.attributes_to_drop_before_render.borrow_mut();
-        for attr in element.dynamic_attrs {
-            attr.ty.for_each(|attr| {
+        for attr in dynamic_attrs {
+            let attrs = match attr.ty {
+                AttributeType::Single(ref attr) => std::slice::from_ref(attr),
+                AttributeType::Many(attrs) => attrs,
+            };
+
+            for attr in attrs {
                 match attr.value {
                     // We need to drop listeners before the next render because they may borrow data from the borrowed props which will be dropped
                     AttributeValue::Listener(_) => {
                         let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) };
                         listeners.push(unbounded);
                     }
+
                     // We need to drop any values manually to make sure that their drop implementation is called before the next render
                     AttributeValue::Any(_) => {
                         let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) };
@@ -367,7 +426,7 @@ impl<'src> ScopeState {
 
                     _ => (),
                 }
-            })
+            }
         }
 
         let mut props = self.borrowed_props.borrow_mut();
@@ -375,7 +434,7 @@ impl<'src> ScopeState {
             .previous_frame()
             .props_to_drop_before_reset
             .borrow_mut();
-        for node in element.dynamic_nodes {
+        for node in dynamic_nodes {
             if let DynamicNode::Component(comp) = node {
                 let unbounded = unsafe { std::mem::transmute(comp as *const VComponent) };
                 if !comp.static_props {
@@ -385,45 +444,14 @@ impl<'src> ScopeState {
             }
         }
 
-        Some(element)
-    }
-
-    #[doc(hidden)]
-    /// Take a [`crate::VNode`] and link it to a [`ScopeState`]
-    /// 
-    /// This is used internally in the render macro
-    pub fn link_node(&'src self, element: &crate::VNode<'src>) {
-        let mut listeners = self.attributes_to_drop_before_render.borrow_mut();
-        for attr in element.dynamic_attrs {
-            match attr.value {
-                // We need to drop listeners before the next render because they may borrow data from the borrowed props which will be dropped
-                AttributeValue::Listener(_) => {
-                    let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) };
-                    listeners.push(unbounded);
-                }
-                // We need to drop any values manually to make sure that their drop implementation is called before the next render
-                AttributeValue::Any(_) => {
-                    let unbounded = unsafe { std::mem::transmute(attr as *const Attribute) };
-                    self.previous_frame().add_attribute_to_drop(unbounded);
-                }
-
-                _ => (),
-            }
-        }
-
-        let mut props = self.borrowed_props.borrow_mut();
-        let mut drop_props = self
-            .previous_frame()
-            .props_to_drop_before_reset
-            .borrow_mut();
-        for node in element.dynamic_nodes {
-            if let DynamicNode::Component(comp) = node {
-                let unbounded = unsafe { std::mem::transmute(comp as *const VComponent) };
-                if !comp.static_props {
-                    props.push(unbounded);
-                }
-                drop_props.push(unbounded);
-            }
+        VNode {
+            stable_id: Default::default(),
+            key,
+            parent,
+            template,
+            root_ids,
+            dynamic_nodes,
+            dynamic_attrs,
         }
     }
 

+ 28 - 7
packages/core/tests/fuzzing.rs

@@ -171,15 +171,27 @@ fn create_random_dynamic_node(cx: &ScopeState, depth: usize) -> DynamicNode {
     match rand::random::<u8>() % range {
         0 => DynamicNode::Placeholder(Default::default()),
         1 => cx.make_node((0..(rand::random::<u8>() % 5)).map(|_| {
-            VNode::new(
+            // <<<<<<< HEAD
+            //             VNode::new(
+            //                 None,
+            //                 Template {
+            // =======
+            cx.vnode(
                 None,
-                Template {
+                Default::default(),
+                Cell::new(Template {
+                    // >>>>>>> 9fe172e9 (Fix leak in render macro)
                     name: concat!(file!(), ":", line!(), ":", column!(), ":0"),
                     roots: &[TemplateNode::Dynamic { id: 0 }],
                     node_paths: &[&[0]],
                     attr_paths: &[],
-                },
-                bumpalo::collections::Vec::new_in(cx.bump()),
+                    // <<<<<<< HEAD
+                    //                 },
+                    //                 bumpalo::collections::Vec::new_in(cx.bump()),
+                    // =======
+                }),
+                bumpalo::collections::Vec::new_in(cx.bump()).into(),
+                // >>>>>>> 9fe172e9 (Fix leak in render macro)
                 cx.bump().alloc([cx.component(
                     create_random_element,
                     DepthProps { depth, root: false },
@@ -273,10 +285,19 @@ fn create_random_element(cx: Scope<DepthProps>) -> Element {
                 )
                 .into_boxed_str(),
             ));
-            let node = VNode::new(
+            // <<<<<<< HEAD
+            //             let node = VNode::new(
+            //                 None,
+            //                 template,
+            //                 bumpalo::collections::Vec::new_in(cx.bump()),
+            // =======
+            // println!("{template:#?}");
+            let node = cx.vnode(
+                None,
                 None,
-                template,
-                bumpalo::collections::Vec::new_in(cx.bump()),
+                Cell::new(template),
+                bumpalo::collections::Vec::new_in(cx.bump()).into(),
+                // >>>>>>> 9fe172e9 (Fix leak in render macro)
                 {
                     let dynamic_nodes: Vec<_> = dynamic_node_types
                         .iter()

+ 1 - 1
packages/dioxus-tui/examples/many_small_edit_stress.rs

@@ -83,7 +83,7 @@ fn Grid(cx: Scope<GridProps>) -> Element {
                                     let alpha = y as f32*100.0/size as f32 + counts.read()[x*size + y] as f32;
                                     let key = format!("{}-{}", x, y);
                                     rsx! {
-                                        Box{
+                                        Box {
                                             x: x,
                                             y: y,
                                             alpha: 100.0,

+ 42 - 14
packages/native-core/tests/fuzzing.rs

@@ -178,15 +178,27 @@ fn create_random_dynamic_node(cx: &ScopeState, depth: usize) -> DynamicNode {
     match rand::random::<u8>() % range {
         0 => DynamicNode::Placeholder(Default::default()),
         1 => cx.make_node((0..(rand::random::<u8>() % 5)).map(|_| {
-            VNode::new(
+            // <<<<<<< HEAD
+            //             VNode::new(
+            //                 None,
+            //                 Template {
+            // =======
+            cx.vnode(
                 None,
-                Template {
+                Default::default(),
+                Cell::new(Template {
+                    // >>>>>>> 9fe172e9 (Fix leak in render macro)
                     name: concat!(file!(), ":", line!(), ":", column!(), ":0"),
                     roots: &[TemplateNode::Dynamic { id: 0 }],
                     node_paths: &[&[0]],
                     attr_paths: &[],
-                },
-                dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()),
+                    // <<<<<<< HEAD
+                    //                 },
+                    //                 dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()),
+                    // =======
+                }),
+                dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()).into(),
+                // >>>>>>> 9fe172e9 (Fix leak in render macro)
                 cx.bump().alloc([cx.component(
                     create_random_element,
                     DepthProps { depth, root: false },
@@ -266,10 +278,18 @@ fn create_random_element(cx: Scope<DepthProps>) -> Element {
                 .into_boxed_str(),
             ));
             println!("{template:#?}");
-            let node = VNode::new(
+            // <<<<<<< HEAD
+            //             let node = VNode::new(
+            //                 None,
+            //                 template,
+            //                 dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()),
+            // =======
+            let node = cx.vnode(
+                None,
                 None,
-                template,
-                dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()),
+                Cell::new(template),
+                dioxus::core::exports::bumpalo::collections::Vec::new_in(cx.bump()).into(),
+                // >>>>>>> 9fe172e9 (Fix leak in render macro)
                 {
                     let dynamic_nodes: Vec<_> = dynamic_node_types
                         .iter()
@@ -284,13 +304,21 @@ fn create_random_element(cx: Scope<DepthProps>) -> Element {
                         .collect();
                     cx.bump().alloc(dynamic_nodes)
                 },
-                cx.bump()
-                    .alloc(
-                        (0..template.attr_paths.len())
-                            .map(|_| create_random_dynamic_mounted_attr(cx))
-                            .collect::<Vec<_>>(),
-                    )
-                    .as_slice(),
+                // <<<<<<< HEAD
+                //                 cx.bump()
+                //                     .alloc(
+                //                         (0..template.attr_paths.len())
+                //                             .map(|_| create_random_dynamic_mounted_attr(cx))
+                //                             .collect::<Vec<_>>(),
+                //                     )
+                //                     .as_slice(),
+                // =======
+                cx.bump().alloc(
+                    (0..template.attr_paths.len())
+                        .map(|_| create_random_dynamic_attr(cx))
+                        .collect::<Vec<_>>(),
+                ),
+                // >>>>>>> 9fe172e9 (Fix leak in render macro)
             );
             Some(node)
         }

+ 12 - 6
packages/rsx/src/lib.rs

@@ -139,9 +139,7 @@ impl ToTokens for RenderCallBody {
         out_tokens.append_all(quote! {
             Some({
                 let __cx = cx;
-                let __element = { #body };
-                cx.link_node(&__element);
-                __element
+                #body
             })
         })
     }
@@ -256,10 +254,18 @@ impl<'a> ToTokens for TemplateRenderer<'a> {
                 node_paths: &[ #(#node_paths),* ],
                 attr_paths: &[ #(#attr_paths),* ],
             };
-            ::dioxus::core::VNode::new(
+// <<<<<<< HEAD
+//             ::dioxus::core::VNode::new(
+//                 #key_tokens,
+//                 TEMPLATE,
+//                 dioxus::core::exports::bumpalo::collections::Vec::with_capacity_in(#root_count, __cx.bump()),
+// =======
+            __cx.vnode(
+                None,
                 #key_tokens,
-                TEMPLATE,
-                dioxus::core::exports::bumpalo::collections::Vec::with_capacity_in(#root_count, __cx.bump()),
+                std::cell::Cell::new(TEMPLATE),
+                dioxus::core::exports::bumpalo::collections::Vec::with_capacity_in(#root_count, __cx.bump()).into(),
+// >>>>>>> 9fe172e9 (Fix leak in render macro)
                 __cx.bump().alloc([ #( #node_printer ),* ]),
                 __cx.bump().alloc([ #( #dyn_attr_printer ),* ]),
             )