Drop `Box<dyn PlatformWindow>` when the OS closes the native window (#8016)

Antonio Scandurra and Thorsten created 2 years ago

Closes #7973 

This fixes a leak in GPUI when the user didn't override
`on_should_close_window`.

Release Notes:

- N/A

---------

Co-authored-by: Thorsten <[e-mail]>

Change summary

crates/gpui/src/platform/mac/window.rs  | 28 ++++++++++++++++----------
crates/gpui/src/platform/test/window.rs |  4 --
crates/gpui/src/window.rs               | 18 ++++++----------
3 files changed, 25 insertions(+), 25 deletions(-)

Detailed changes

crates/gpui/src/platform/mac/window.rs

@@ -319,6 +319,7 @@ struct MacWindowState {
     handle: AnyWindowHandle,
     executor: ForegroundExecutor,
     native_window: id,
+    native_window_was_closed: bool,
     native_view: NonNull<Object>,
     display_link: Option<DisplayLink>,
     renderer: renderer::Renderer,
@@ -565,6 +566,7 @@ impl MacWindow {
                 handle,
                 executor,
                 native_window,
+                native_window_was_closed: false,
                 native_view: NonNull::new_unchecked(native_view),
                 display_link: None,
                 renderer: renderer::new_renderer(
@@ -732,13 +734,18 @@ impl Drop for MacWindow {
         this.renderer.destroy();
         let window = this.native_window;
         this.display_link.take();
-        this.executor
-            .spawn(async move {
-                unsafe {
-                    window.close();
-                }
-            })
-            .detach();
+        unsafe {
+            this.native_window.setDelegate_(nil);
+        }
+        if !this.native_window_was_closed {
+            this.executor
+                .spawn(async move {
+                    unsafe {
+                        window.close();
+                    }
+                })
+                .detach();
+        }
     }
 }
 
@@ -1511,10 +1518,9 @@ extern "C" fn close_window(this: &Object, _: Sel) {
     unsafe {
         let close_callback = {
             let window_state = get_window_state(this);
-            window_state
-                .as_ref()
-                .try_lock()
-                .and_then(|mut window_state| window_state.close_callback.take())
+            let mut lock = window_state.as_ref().lock();
+            lock.native_window_was_closed = true;
+            lock.close_callback.take()
         };
 
         if let Some(callback) = close_callback {

crates/gpui/src/platform/test/window.rs

@@ -272,9 +272,7 @@ impl PlatformWindow for TestWindow {
         self.0.lock().should_close_handler = Some(callback);
     }
 
-    fn on_close(&self, _callback: Box<dyn FnOnce()>) {
-        unimplemented!()
-    }
+    fn on_close(&self, _callback: Box<dyn FnOnce()>) {}
 
     fn on_appearance_changed(&self, _callback: Box<dyn FnMut()>) {}
 

crates/gpui/src/window.rs

@@ -341,6 +341,12 @@ impl Window {
         let next_frame_callbacks: Rc<RefCell<Vec<FrameCallback>>> = Default::default();
         let last_input_timestamp = Rc::new(Cell::new(Instant::now()));
 
+        platform_window.on_close(Box::new({
+            let mut cx = cx.to_async();
+            move || {
+                let _ = handle.update(&mut cx, |_, cx| cx.remove_window());
+            }
+        }));
         platform_window.on_request_frame(Box::new({
             let mut cx = cx.to_async();
             let dirty = dirty.clone();
@@ -1595,17 +1601,7 @@ impl<'a> WindowContext<'a> {
         let mut this = self.to_async();
         self.window
             .platform_window
-            .on_should_close(Box::new(move || {
-                this.update(|cx| {
-                    // Ensure that the window is removed from the app if it's been closed
-                    // by always pre-empting the system close event.
-                    if f(cx) {
-                        cx.remove_window();
-                    }
-                    false
-                })
-                .unwrap_or(true)
-            }))
+            .on_should_close(Box::new(move || this.update(|cx| f(cx)).unwrap_or(true)))
     }
 
     pub(crate) fn parent_view_id(&self) -> EntityId {