svg_img: Fix memory leak in SvgImg (#818)

crates/story/src/image_story.rs

@@ -1,5 +1,6 @@
 use gpui::{
-    px, App, AppContext, Entity, FocusHandle, Focusable, ParentElement as _, Render, Styled, Window,
+    px, App, AppContext, ElementId, Entity, FocusHandle, Focusable, ParentElement as _, Render,
+    Styled, Window,
 };
 use gpui_component::{dock::PanelControl, v_flex, SvgImg};
 
@@ -9,7 +10,6 @@ const GOOGLE_LOGO: &str = include_str!("./fixtures/google.svg");
 
 pub struct ImageStory {
     focus_handle: gpui::FocusHandle,
-    google_logo: SvgImg,
 }
 
 impl super::Story for ImageStory {
@@ -34,7 +34,6 @@ impl ImageStory {
     pub fn new(_: &mut Window, cx: &mut App) -> Self {
         Self {
             focus_handle: cx.focus_handle(),
-            google_logo: SvgImg::new().source(GOOGLE_LOGO.as_bytes(), px(300.), px(300.)),
         }
     }
 
@@ -57,11 +56,15 @@ impl Render for ImageStory {
     ) -> impl gpui::IntoElement {
         v_flex().gap_4().size_full().child(
             section("SVG Image")
-                .child(self.google_logo.clone().size(px(100.)).flex_grow())
+                .child(svg_img("logo1").size(px(100.)).flex_grow())
-                .child(self.google_logo.clone().size(px(100.)).flex_grow())
+                .child(svg_img("logo2").size(px(100.)).flex_grow())
-                .child(self.google_logo.clone().size_80().flex_grow())
+                .child(svg_img("logo3").size_80().flex_grow())
-                .child(self.google_logo.clone().size_12().flex_grow())
+                .child(svg_img("logo4").size_12().flex_grow())
-                .child(self.google_logo.clone().size(px(100.))),
+                .child(svg_img("logo5").size(px(100.))),
         )
     }
 }
+
+fn svg_img(id: impl Into<ElementId>) -> SvgImg {
+    SvgImg::new(id).source(GOOGLE_LOGO.as_bytes(), px(300.), px(300.))
+}

crates/ui/src/svg_img.rs

@@ -5,9 +5,9 @@ use std::{
 };
 
 use gpui::{
-    px, size, App, Asset, Bounds, Element, ElementId, GlobalElementId, Hitbox, ImageCacheError,
+    hash, px, size, App, Asset, Bounds, Element, ElementId, GlobalElementId, Hitbox,
-    InteractiveElement, Interactivity, IntoElement, IsZero, Pixels, RenderImage, SharedString,
+    ImageCacheError, InteractiveElement, Interactivity, IntoElement, IsZero, Pixels, RenderImage,
-    Size, StyleRefinement, Styled, Window,
+    SharedString, Size, StyleRefinement, Styled, Window,
 };
 use image::Frame;
 use smallvec::SmallVec;
@@ -15,6 +15,9 @@ use smallvec::SmallVec;
 use image::ImageBuffer;
 
 const SCALE: f32 = 2.;
+
+struct SvgImgState(Option<(u64, Arc<RenderImage>)>);
+
 static OPTIONS: LazyLock<usvg::Options> = LazyLock::new(|| {
     let mut options = usvg::Options::default();
     options.fontdb_mut().load_system_fonts();
@@ -56,6 +59,7 @@ impl From<&'static str> for SvgSource {
 impl Clone for SvgImg {
     fn clone(&self) -> Self {
         Self {
+            id: self.id.clone(),
             interactivity: Interactivity::default(),
             source: self.source.clone(),
             size: self.size,
@@ -146,6 +150,7 @@ impl Asset for Image {
 }
 
 pub struct SvgImg {
+    id: ElementId,
     interactivity: Interactivity,
     source: Option<ImageSource>,
     size: Size<Pixels>,
@@ -155,8 +160,9 @@ impl SvgImg {
     /// Create a new svg image element.
     ///
     /// The `src_width` and `src_height` are the original width and height of the svg image.
-    pub fn new() -> Self {
+    pub fn new(id: impl Into<ElementId>) -> Self {
         Self {
+            id: id.into(),
             interactivity: Interactivity::default(),
             source: None,
             size: Size::default(),
@@ -200,7 +206,7 @@ impl Element for SvgImg {
     type PrepaintState = (Option<Hitbox>, Option<Arc<RenderImage>>);
 
     fn id(&self) -> Option<ElementId> {
-        self.interactivity.element_id.clone()
+        Some(self.id.clone())
     }
 
     fn request_layout(
@@ -214,18 +220,37 @@ impl Element for SvgImg {
                 .request_layout(global_id, window, cx, |style, window, cx| {
                     window.request_layout(style, None, cx)
                 });
+        let global_id = global_id.unwrap();
 
-        let source = self.source.clone();
+        window.with_element_state::<SvgImgState, _>(global_id, |state, window| {
-        let data = if let Some(source) = source {
+            match (state, &self.source) {
-            match window.use_asset::<Image>(&source, cx) {
+                (_, None) => ((layout_id, None), SvgImgState(None)),
-                Some(Ok(data)) => Some(data),
+                (Some(SvgImgState(Some((prev_hash, image)))), Some(source))
-                _ => None,
+                    if hash(source) == prev_hash =>
+                {
+                    (
+                        (layout_id, Some(image.clone())),
+                        SvgImgState(Some((prev_hash, image))),
+                    )
+                }
+                (state, Some(source)) => {
+                    if let Some(SvgImgState(Some((_, prev_image)))) = state {
+                        // Drop the previous image from the cache
+                        _ = window.drop_image(prev_image);
                     }
-        } else {
-            None
-        };
 
-        (layout_id, data)
+                    let image = window
+                        .use_asset::<Image>(&source, cx)
+                        .transpose()
+                        .ok()
+                        .flatten();
+                    (
+                        (layout_id, image.clone()),
+                        SvgImgState(image.map(|image| (hash(source), image))),
+                    )
+                }
+            }
+        })
     }
 
     fn prepaint(