fix(gui): free the photo viewer and the share dialog when they close

The viewer's handlers held strong references to the viewer and its
window, which own them, so every closed viewer stayed in memory with its
full-size photo. The share dialog's row handlers had the same cycle with
the dialog state. The handlers now hold weak references, and the viewer's
one strong handle is dropped on close with the photo (B241).

crates/pdfs-gui/src/app/pages/photo_viewer.rs

@@ -1260,8 +1260,11 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     // Any real pointer movement brings the controls back; resting over one of
     // them keeps them up.
     let motion = gtk4::EventControllerMotion::new();
-    let viewer_motion = viewer.clone();
+    let viewer_motion = Rc::downgrade(&viewer);
     motion.connect_motion(move |_, x, y| {
+        let Some(viewer_motion) = viewer_motion.upgrade() else {
+            return;
+        };
         let (last_x, last_y) = viewer_motion.pointer.get();
         viewer_motion.pointer.set((x, y));
         if (x - last_x).abs() > 1.0 || (y - last_y).abs() > 1.0 {
@@ -1271,10 +1274,17 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     body.add_controller(motion);
     for revealer in [&top_reveal, &prev_reveal, &next_reveal] {
         let hover = gtk4::EventControllerMotion::new();
-        let viewer_enter = viewer.clone();
-        hover.connect_enter(move |_, _, _| viewer_enter.chrome_hover.set(true));
-        let viewer_leave = viewer.clone();
+        let viewer_enter = Rc::downgrade(&viewer);
+        hover.connect_enter(move |_, _, _| {
+            if let Some(viewer) = viewer_enter.upgrade() {
+                viewer.chrome_hover.set(true);
+            }
+        });
+        let viewer_leave = Rc::downgrade(&viewer);
         hover.connect_leave(move |_| {
+            let Some(viewer_leave) = viewer_leave.upgrade() else {
+                return;
+            };
             viewer_leave.chrome_hover.set(false);
             show_chrome(&viewer_leave);
         });
@@ -1284,8 +1294,11 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     // Ctrl+scroll zooms around the pointer; a plain scroll pans a zoomed photo.
     let wheel = gtk4::EventControllerScroll::new(gtk4::EventControllerScrollFlags::VERTICAL);
     wheel.set_propagation_phase(gtk4::PropagationPhase::Capture);
-    let viewer_wheel = viewer.clone();
+    let viewer_wheel = Rc::downgrade(&viewer);
     wheel.connect_scroll(move |controller, _, dy| {
+        let Some(viewer_wheel) = viewer_wheel.upgrade() else {
+            return glib::Propagation::Proceed;
+        };
         if !controller
             .current_event_state()
             .contains(gtk4::gdk::ModifierType::CONTROL_MASK)
@@ -1305,14 +1318,20 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     scroller.add_controller(wheel);
 
     let pinch = gtk4::GestureZoom::new();
-    let viewer_pinch = viewer.clone();
+    let viewer_pinch = Rc::downgrade(&viewer);
     pinch.connect_begin(move |_, _| {
+        let Some(viewer_pinch) = viewer_pinch.upgrade() else {
+            return;
+        };
         viewer_pinch
             .pinch_start
             .set(current_scale(&viewer_pinch).unwrap_or(1.0));
     });
-    let viewer_pinch = viewer.clone();
+    let viewer_pinch = Rc::downgrade(&viewer);
     pinch.connect_scale_changed(move |gesture, delta| {
+        let Some(viewer_pinch) = viewer_pinch.upgrade() else {
+            return;
+        };
         let start = viewer_pinch.pinch_start.get();
         zoom_to(
             &viewer_pinch,
@@ -1324,8 +1343,11 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
 
     // Dragging a zoomed photo pans it.
     let drag = gtk4::GestureDrag::new();
-    let viewer_drag = viewer.clone();
+    let viewer_drag = Rc::downgrade(&viewer);
     drag.connect_drag_begin(move |_, _, _| {
+        let Some(viewer_drag) = viewer_drag.upgrade() else {
+            return;
+        };
         let scroller = &viewer_drag.scroller;
         viewer_drag.drag_start.set((
             scroller.hadjustment().value(),
@@ -1335,8 +1357,11 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
             viewer_drag.picture.set_cursor_from_name(Some("grabbing"));
         }
     });
-    let viewer_drag = viewer.clone();
+    let viewer_drag = Rc::downgrade(&viewer);
     drag.connect_drag_update(move |_, dx, dy| {
+        let Some(viewer_drag) = viewer_drag.upgrade() else {
+            return;
+        };
         if viewer_drag.zoom.get().is_none() {
             return;
         }
@@ -1344,39 +1369,58 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
         viewer_drag.scroller.hadjustment().set_value(x - dx);
         viewer_drag.scroller.vadjustment().set_value(y - dy);
     });
-    let viewer_drag = viewer.clone();
+    let viewer_drag = Rc::downgrade(&viewer);
     drag.connect_drag_end(move |_, _, _| {
+        let Some(viewer_drag) = viewer_drag.upgrade() else {
+            return;
+        };
         if viewer_drag.zoom.get().is_some() {
             viewer_drag.picture.set_cursor_from_name(Some("grab"));
         }
     });
     scroller.add_controller(drag);
 
-    let w_close = window.clone();
+    let w_close = window.downgrade();
     close_btn.connect_clicked(move |_| {
+        let Some(w_close) = w_close.upgrade() else {
+            return;
+        };
         w_close.close();
     });
 
-    let viewer_closing = viewer.clone();
+    // The one strong handle on the viewer. Every handler holds a weak one: a
+    // strong one in a handler of a widget the viewer holds is a cycle, which
+    // kept every closed lightbox, its window and its full-size photo in memory
+    // for the life of the app.
+    let keep = RefCell::new(Some(viewer.clone()));
     window.connect_close_request(move |_| {
-        stop_video(&viewer_closing);
-        disarm_slide(&viewer_closing);
-        if let Some(source) = viewer_closing.chrome_source.borrow_mut().take() {
-            source.remove();
+        if let Some(viewer) = keep.borrow_mut().take() {
+            stop_video(&viewer);
+            disarm_slide(&viewer);
+            if let Some(source) = viewer.chrome_source.borrow_mut().take() {
+                source.remove();
+            }
+            viewer.picture.set_paintable(gtk4::gdk::Paintable::NONE);
         }
         glib::Propagation::Proceed
     });
 
-    let viewer_info = viewer.clone();
+    let viewer_info = Rc::downgrade(&viewer);
     info_toggle.connect_toggled(move |toggle| {
+        let Some(viewer_info) = viewer_info.upgrade() else {
+            return;
+        };
         viewer_info
             .info_revealer
             .set_reveal_child(toggle.is_active());
     });
 
     let ui_slides = ui.clone();
-    let viewer_slides = viewer.clone();
+    let viewer_slides = Rc::downgrade(&viewer);
     slideshow_btn.connect_toggled(move |toggle| {
+        let Some(viewer_slides) = viewer_slides.upgrade() else {
+            return;
+        };
         if toggle.is_active() {
             start_slideshow(&ui_slides, &viewer_slides);
         } else {
@@ -1388,9 +1432,15 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     info_close.connect_clicked(move |_| toggle_off.set_active(false));
 
     let ui_map = ui.clone();
-    let viewer_map = viewer.clone();
-    let w_map = window.clone();
+    let viewer_map = Rc::downgrade(&viewer);
+    let w_map = window.downgrade();
     info_map.connect_clicked(move |_| {
+        let Some(w_map) = w_map.upgrade() else {
+            return;
+        };
+        let Some(viewer_map) = viewer_map.upgrade() else {
+            return;
+        };
         let coords = *viewer_map.coords.borrow();
         if let Some((lat, lon)) = coords {
             w_map.close();
@@ -1399,8 +1449,11 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     });
 
     let ui_fav = ui.clone();
-    let viewer_fav = viewer.clone();
+    let viewer_fav = Rc::downgrade(&viewer);
     favorite_btn.connect_toggled(move |btn| {
+        let Some(viewer_fav) = viewer_fav.upgrade() else {
+            return;
+        };
         if viewer_fav.favorite_suppress.get() {
             return;
         }
@@ -1444,17 +1497,26 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     // gone, and leaving it open on a trashed photo invites a second Delete on
     // something the user cannot see.
     let ui_delete = ui.clone();
-    let viewer_delete = viewer.clone();
-    let w_delete = window.clone();
+    let viewer_delete = Rc::downgrade(&viewer);
+    let w_delete = window.downgrade();
     delete_btn.connect_clicked(move |_| {
+        let Some(w_delete) = w_delete.upgrade() else {
+            return;
+        };
+        let Some(viewer_delete) = viewer_delete.upgrade() else {
+            return;
+        };
         let uid = viewer_delete.uid.borrow().clone();
         w_delete.close();
         trash_photos(&ui_delete, vec![uid], false);
     });
 
     let ui_group = ui.clone();
-    let viewer_group = viewer.clone();
+    let viewer_group = Rc::downgrade(&viewer);
     group_btn.connect_clicked(move |_| {
+        let Some(viewer_group) = viewer_group.upgrade() else {
+            return;
+        };
         let uid = viewer_group.uid.borrow().clone();
         let next =
             next_group_member(&viewer_group.group.borrow(), &uid).map(|item| item.uid.clone());
@@ -1465,9 +1527,15 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     });
 
     let ui_download = ui.clone();
-    let w_download = window.clone();
-    let viewer_download = viewer.clone();
+    let w_download = window.downgrade();
+    let viewer_download = Rc::downgrade(&viewer);
     download_btn.connect_clicked(move |_| {
+        let Some(w_download) = w_download.upgrade() else {
+            return;
+        };
+        let Some(viewer_download) = viewer_download.upgrade() else {
+            return;
+        };
         if let Some(path) = viewer_download.path.borrow().as_deref() {
             let name = viewer_download.name.borrow();
             save_photo_to_disk(
@@ -1479,23 +1547,35 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
         }
     });
 
-    let w_ext = window.clone();
-    let viewer_ext = viewer.clone();
+    let w_ext = window.downgrade();
+    let viewer_ext = Rc::downgrade(&viewer);
     open_ext_btn.connect_clicked(move |_| {
+        let Some(w_ext) = w_ext.upgrade() else {
+            return;
+        };
+        let Some(viewer_ext) = viewer_ext.upgrade() else {
+            return;
+        };
         if let Some(path) = viewer_ext.path.borrow().as_deref() {
             open_with(&w_ext, path, viewer_ext.name.borrow().as_deref());
         }
     });
 
     let ui_prev = ui.clone();
-    let viewer_prev = viewer.clone();
+    let viewer_prev = Rc::downgrade(&viewer);
     prev_btn.connect_clicked(move |_| {
+        let Some(viewer_prev) = viewer_prev.upgrade() else {
+            return;
+        };
         navigate_photo(&ui_prev, &viewer_prev, -1);
     });
 
     let ui_next = ui.clone();
-    let viewer_next = viewer.clone();
+    let viewer_next = Rc::downgrade(&viewer);
     next_btn.connect_clicked(move |_| {
+        let Some(viewer_next) = viewer_next.upgrade() else {
+            return;
+        };
         navigate_photo(&ui_next, &viewer_next, 1);
     });
 
@@ -1504,8 +1584,11 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
     // reaching for the image doesn't fling the window shut; a zoomed photo has
     // no backdrop to speak of. Double-click toggles between fitted and 100 %.
     let backdrop = gtk4::GestureClick::new();
-    let viewer_click = viewer.clone();
+    let viewer_click = Rc::downgrade(&viewer);
     backdrop.connect_pressed(move |_, n_press, x, y| {
+        let Some(viewer_click) = viewer_click.upgrade() else {
+            return;
+        };
         if n_press != 2 {
             return;
         }
@@ -1522,9 +1605,15 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
             zoom_to(&viewer_click, Some((fit * 2.0).max(1.0)), Some(anchor));
         }
     });
-    let viewer_click = viewer.clone();
-    let w_click = window.clone();
+    let viewer_click = Rc::downgrade(&viewer);
+    let w_click = window.downgrade();
     backdrop.connect_released(move |_, n_press, x, y| {
+        let Some(w_click) = w_click.upgrade() else {
+            return;
+        };
+        let Some(viewer_click) = viewer_click.upgrade() else {
+            return;
+        };
         if n_press == 1
             && viewer_click.zoom.get().is_none()
             && !over_photo(&viewer_click.picture, x, y)
@@ -1536,9 +1625,15 @@ pub(crate) fn open_photo_viewer(ui: &Rc<Ui>, initial_uid: String) {
 
     let key_controller = gtk4::EventControllerKey::new();
     let ui_key = ui.clone();
-    let viewer_key = viewer.clone();
-    let w_key = window.clone();
+    let viewer_key = Rc::downgrade(&viewer);
+    let w_key = window.downgrade();
     key_controller.connect_key_pressed(move |_, key, _keycode, state| {
+        let Some(w_key) = w_key.upgrade() else {
+            return glib::Propagation::Proceed;
+        };
+        let Some(viewer_key) = viewer_key.upgrade() else {
+            return glib::Propagation::Proceed;
+        };
         let ctrl = state.contains(gtk4::gdk::ModifierType::CONTROL_MASK);
         let playing_video = viewer_key.media.visible_child_name().as_deref() == Some("video");
         match key.name().as_deref() {

crates/pdfs-gui/src/app/widgets/share_dialog.rs

@@ -507,10 +507,16 @@ pub(crate) fn repaint_share_people(state: &Rc<ShareDialog>, entries: &[ShareEntr
                 .selected(role_wire_to_index(&entry.role))
                 .valign(gtk4::Align::Center)
                 .build();
-            let state_role = state.clone();
+            // Weak, here and in every row below: the rows belong to groups the
+            // dialog state holds, so a strong handle in a row's handler is a
+            // cycle that kept each closed dialog in memory.
+            let state_role = Rc::downgrade(state);
             let id = entry.id.clone();
             let kind = entry.kind;
             drop.connect_selected_notify(move |d| {
+                let Some(state_role) = state_role.upgrade() else {
+                    return;
+                };
                 share_dialog_op(
                     &state_role,
                     state_role.target.update_role(
@@ -540,12 +546,14 @@ pub(crate) fn repaint_share_people(state: &Rc<ShareDialog>, entries: &[ShareEntr
             .valign(gtk4::Align::Center)
             .build();
         remove.add_css_class("flat");
-        let state_rm = state.clone();
+        let state_rm = Rc::downgrade(state);
         let id = entry.id.clone();
         let kind = entry.kind;
         let who = entry.email.clone();
         remove.connect_clicked(move |btn| {
-            let state = state_rm.clone();
+            let Some(state) = state_rm.upgrade() else {
+                return;
+            };
             let id = id.clone();
             let button = btn.clone();
             confirm_destructive(
@@ -617,12 +625,12 @@ pub(crate) fn repaint_share_link(state: &Rc<ShareDialog>, link: Option<&PublicLi
                     .valign(gtk4::Align::Center)
                     .build();
                 copy.add_css_class("flat");
-                let state_copy = state.clone();
+                let ui_copy = state.ui.clone();
                 let url_copy = url.clone();
                 copy.connect_clicked(move |btn| {
                     btn.clipboard().set_text(&url_copy);
                     flash_copied(btn, "edit-copy-symbolic");
-                    toast(&state_copy.ui, &gettext("Link copied"));
+                    toast(&ui_copy, &gettext("Link copied"));
                 });
                 row.add_suffix(&copy);
             }
@@ -633,10 +641,12 @@ pub(crate) fn repaint_share_link(state: &Rc<ShareDialog>, link: Option<&PublicLi
                 .valign(gtk4::Align::Center)
                 .build();
             remove.add_css_class("flat");
-            let state_rm = state.clone();
+            let state_rm = Rc::downgrade(state);
             let id = link.id.clone();
             remove.connect_clicked(move |btn| {
-                let state = state_rm.clone();
+                let Some(state) = state_rm.upgrade() else {
+                    return;
+                };
                 let id = id.clone();
                 let button = btn.clone();
                 confirm_destructive(
@@ -690,9 +700,12 @@ pub(crate) fn repaint_share_link(state: &Rc<ShareDialog>, link: Option<&PublicLi
             let btn = create.clone();
             pw_row.connect_entry_activated(move |_| btn.emit_clicked());
 
-            let state_c = state.clone();
+            let state_c = Rc::downgrade(state);
             let pw_for = pw_row.clone();
             create.connect_clicked(move |btn| {
+                let Some(state_c) = state_c.upgrade() else {
+                    return;
+                };
                 if !btn.is_sensitive() {
                     return;
                 }

docs/BUGS.md

@@ -100,6 +100,7 @@ a live account since.
 
 | Entry | Title |
 |---|---|
+| [B241](#b241--every-closed-photo-viewer-stays-in-memory-with-its-photo) | Every closed photo viewer stays in memory with its photo |
 | [B240](#b240--pdfs-mount-can-be-core-dumped-with-the-keys-in-memory) | pdfs mount can be core-dumped with the keys in memory |
 | [B239](#b239----wait-reports-success-for-imports-and-re-dates-that-failed) | --wait reports success for imports and re-dates that failed |
 | [B238](#b238--draining-a-large-offline-batch-is-quadratic-in-its-size) | Draining a large offline batch is quadratic in its size |
@@ -343,6 +344,32 @@ a live account since.
 
 Newest first.
 
+### B241 — Every closed photo viewer stays in memory with its photo
+
+**Status:** Fixed, not verified live
+**Found:** 2026-10-16, architecture audit (`FINDINGS.md` 2.5.2).
+**Where:** `open_photo_viewer` in `crates/pdfs-gui/src/app/pages/photo_viewer.rs`;
+`repaint_share_people` and `repaint_share_link` in
+`crates/pdfs-gui/src/app/widgets/share_dialog.rs`
+
+**Repro:** Open and close the photo viewer twenty times on large photos, and watch the app's
+memory. Or open and close the Share dialog of a shared folder many times.
+
+**Cause.** Every handler of the viewer held a strong `Rc<Viewer>`, and the viewer holds the
+widgets those handlers belong to. Several handlers also held the window, which owns them. Each
+pair was a reference cycle, so a closed viewer was never freed: not its window, and not the
+full-size photo its picture showed. The Share dialog had the same cycle between its state and
+the handlers of the rows inside the groups that state holds.
+
+**Fix.** The handlers hold weak handles to the viewer and its window. The one strong handle sits
+in the window's close handler, which drops it on close and clears the picture. The Share
+dialog's row handlers hold the state weakly too.
+
+**Test:** none automated; the GUI tests run without a display. Checked by reading every handler
+`open_photo_viewer` and the share rows connect.
+
+---
+
 ### B240 — pdfs mount can be core-dumped with the keys in memory
 
 **Status:** Fixed, not verified live