From 6cfb845bab6d52ede2a9139885b86e799f15a5cf Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 21 Apr 2018 21:26:59 +0200 Subject: [PATCH 1/8] adding a multiwindow example --- examples/recreate_window_leak.rs | 77 ++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 examples/recreate_window_leak.rs diff --git a/examples/recreate_window_leak.rs b/examples/recreate_window_leak.rs new file mode 100644 index 0000000000..b4108d865d --- /dev/null +++ b/examples/recreate_window_leak.rs @@ -0,0 +1,77 @@ +extern crate winit; + +use winit::{Event, WindowEvent, EventsLoop, ControlFlow}; + +fn create_window(title: &str, events_loop: &EventsLoop) -> winit::Window { + let window_builder = winit::WindowBuilder::new() + .with_title(title) + .with_dimensions(300, 300) + .with_maximized(false); + window_builder + .build(events_loop) + .unwrap() +} + +fn main() { + let mut events_loop = EventsLoop::new(); + + let mut n_window = 0; + let mut is_maximized = false; + let mut decorations = true; + + let mut window = create_window("Hello world", &events_loop); + + let mut should_create_new_window = false; + loop { + let mut should_break = false; + events_loop.run_forever(|event| { + match event { + Event::WindowEvent { event, .. } => match event { + WindowEvent::Closed => { + should_break = true; + ControlFlow::Break + }, + WindowEvent::KeyboardInput { + input: + winit::KeyboardInput { + virtual_keycode: Some(virtual_code), + state, + .. + }, + .. + } => match (virtual_code, state) { + (winit::VirtualKeyCode::Escape, _) => { + should_break = true; + ControlFlow::Break + }, + (winit::VirtualKeyCode::Return, winit::ElementState::Pressed) => { + should_create_new_window = true; + ControlFlow::Break + } + (winit::VirtualKeyCode::M, winit::ElementState::Pressed) => { + is_maximized = !is_maximized; + window.set_maximized(is_maximized); + ControlFlow::Continue + } + (winit::VirtualKeyCode::D, winit::ElementState::Pressed) => { + decorations = !decorations; + window.set_decorations(decorations); + ControlFlow::Continue + } + _ => ( ControlFlow::Continue ), + }, + _ => ( ControlFlow::Continue ), + }, + _ => { ControlFlow::Continue } + } + }); + if should_create_new_window { + window = create_window(&format!("Hello world! {}", n_window), &events_loop); + n_window += 1; + should_create_new_window = false; + } + if should_break { + break; + } + } +} From 1d6d7393ffae82d10c7584fd21dbccd9280c919b Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 21 Apr 2018 21:46:18 +0200 Subject: [PATCH 2/8] Added NSAutoReleasepool for WindowDelegate::Drop as setDelegate:nil autoreleases WindowDelegate during work. Added NSAutoReleasepool for Window2::Create, as it uses autorelease on objects while doing work. Added NSAutoreleasepool for Window2::Drop as nswindow::close uses autorelease on objects. Added NSAutoreleasepool for IdRef. Moved Window2 WinitWindow objc class to a static var, as we are creating multiple windows. --- src/platform/macos/window.rs | 95 ++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 15 deletions(-) diff --git a/src/platform/macos/window.rs b/src/platform/macos/window.rs index 986814dff9..ff54327b35 100644 --- a/src/platform/macos/window.rs +++ b/src/platform/macos/window.rs @@ -14,7 +14,7 @@ use cocoa; use cocoa::appkit::{self, NSApplication, NSColor, NSScreen, NSView, NSWindow, NSWindowButton, NSWindowStyleMask}; use cocoa::base::{id, nil}; -use cocoa::foundation::{NSDictionary, NSPoint, NSRect, NSSize, NSString}; +use cocoa::foundation::{NSDictionary, NSPoint, NSRect, NSSize, NSString, NSAutoreleasePool}; use core_graphics::display::CGDisplay; @@ -452,9 +452,15 @@ impl WindowDelegate { unsafe { let delegate = IdRef::new(msg_send![WindowDelegate::class(), new]); + // setDelegate uses autorelease on objects, + // so need autorelease + let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); + (&mut **delegate).set_ivar("winitState", state_ptr as *mut ::std::os::raw::c_void); let _: () = msg_send![*state.window, setDelegate:*delegate]; + msg_send![autoreleasepool, drain]; + WindowDelegate { state: state, _this: delegate } } } @@ -464,7 +470,11 @@ impl Drop for WindowDelegate { fn drop(&mut self) { unsafe { // Nil the window's delegate so it doesn't still reference us + // NOTE: setDelegate:nil at first retains the previous value, + // and then autoreleases it, so autorelease pool is needed + let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); let _: () = msg_send![*self.state.window, setDelegate:nil]; + msg_send![autoreleasepool, drain]; } } } @@ -505,6 +515,19 @@ unsafe fn get_current_monitor() -> RootMonitorId { impl Drop for Window2 { fn drop(&mut self) { + // Remove this window from the `EventLoop`s list of windows. + let id = self.id(); + if let Some(shared) = self.delegate.state.shared.upgrade() { + shared.find_and_remove_window(id); + } + + + // nswindow::close uses autorelease + // so autorelease pool + let autoreleasepool = unsafe { + NSAutoreleasePool::new(cocoa::base::nil) + }; + // Close the window if it has not yet been closed. let nswindow = *self.window; if nswindow != nil { @@ -512,6 +535,10 @@ impl Drop for Window2 { let () = msg_send![nswindow, close]; } } + + unsafe { + msg_send![autoreleasepool, release]; + } } } @@ -538,20 +565,35 @@ impl Window2 { panic!("Windows can only be created on the main thread on macOS"); } } + let autoreleasepool = unsafe { + NSAutoreleasePool::new(cocoa::base::nil) + }; let app = match Window2::create_app(pl_attribs.activation_policy) { Some(app) => app, - None => { return Err(OsError(format!("Couldn't create NSApplication"))); }, + None => { + unsafe { + msg_send![autoreleasepool, drain]; + } + return Err(OsError(format!("Couldn't create NSApplication"))); }, }; let window = match Window2::create_window(win_attribs, pl_attribs) { Some(window) => window, - None => { return Err(OsError(format!("Couldn't create NSWindow"))); }, + None => { + unsafe { + msg_send![autoreleasepool, drain]; + } + return Err(OsError(format!("Couldn't create NSWindow"))); }, }; let view = match Window2::create_view(*window) { Some(view) => view, - None => { return Err(OsError(format!("Couldn't create NSView"))); }, + None => { + unsafe { + msg_send![autoreleasepool, drain]; + } + return Err(OsError(format!("Couldn't create NSView"))); }, }; unsafe { @@ -618,6 +660,10 @@ impl Window2 { window.delegate.state.perform_maximized(win_attribs.maximized); } + unsafe { + msg_send![autoreleasepool, drain]; + } + Ok(window) } @@ -638,11 +684,30 @@ impl Window2 { } } + fn class() -> *const Class { + + static mut WINDOW2_CLASS: *const Class = 0 as *const Class; + static INIT: std::sync::Once = std::sync::ONCE_INIT; + + INIT.call_once(|| unsafe { + let window_superclass = Class::get("NSWindow").unwrap(); + let mut decl = ClassDecl::new("WinitWindow", window_superclass).unwrap(); + decl.add_method(sel!(canBecomeMainWindow), yes as extern fn(&Object, Sel) -> BOOL); + decl.add_method(sel!(canBecomeKeyWindow), yes as extern fn(&Object, Sel) -> BOOL); + WINDOW2_CLASS = decl.register(); + }); + + unsafe { + WINDOW2_CLASS + } + } + fn create_window( attrs: &WindowAttributes, pl_attrs: &PlatformSpecificWindowBuilderAttributes) -> Option { unsafe { + let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); let screen = match attrs.fullscreen { Some(ref monitor_id) => { let monitor_screen = monitor_id.inner.get_nsscreen(); @@ -685,14 +750,7 @@ impl Window2 { NSWindowStyleMask::NSTitledWindowMask }; - let winit_window = Class::get("WinitWindow").unwrap_or_else(|| { - let window_superclass = Class::get("NSWindow").unwrap(); - let mut decl = ClassDecl::new("WinitWindow", window_superclass).unwrap(); - decl.add_method(sel!(canBecomeMainWindow), yes as extern fn(&Object, Sel) -> BOOL); - decl.add_method(sel!(canBecomeKeyWindow), yes as extern fn(&Object, Sel) -> BOOL); - decl.register(); - Class::get("WinitWindow").unwrap() - }); + let winit_window = Window2::class(); let window: id = msg_send![winit_window, alloc]; @@ -702,7 +760,7 @@ impl Window2 { appkit::NSBackingStoreBuffered, NO, )); - window.non_nil().map(|window| { + let res = window.non_nil().map(|window| { let title = IdRef::new(NSString::alloc(nil).init_str(&attrs.title)); window.setReleasedWhenClosed_(NO); window.setTitle_(*title); @@ -735,7 +793,9 @@ impl Window2 { window.center(); window - }) + }); + msg_send![autoreleasepool, drain]; + res } } @@ -1109,7 +1169,12 @@ impl IdRef { impl Drop for IdRef { fn drop(&mut self) { if self.0 != nil { - let _: () = unsafe { msg_send![self.0, release] }; + let _: () = unsafe { + let autoreleasepool = + NSAutoreleasePool::new(cocoa::base::nil); + msg_send![self.0, release]; + msg_send![autoreleasepool, release]; + }; } } } From ab4e7a827a60cda5f44f721a3b5b385a492fad4b Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Tue, 24 Apr 2018 10:50:11 +0200 Subject: [PATCH 3/8] specifying return type for msg_send! --- src/platform/macos/window.rs | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/platform/macos/window.rs b/src/platform/macos/window.rs index ff54327b35..60c0713732 100644 --- a/src/platform/macos/window.rs +++ b/src/platform/macos/window.rs @@ -459,7 +459,7 @@ impl WindowDelegate { (&mut **delegate).set_ivar("winitState", state_ptr as *mut ::std::os::raw::c_void); let _: () = msg_send![*state.window, setDelegate:*delegate]; - msg_send![autoreleasepool, drain]; + let _: () = msg_send![autoreleasepool, drain]; WindowDelegate { state: state, _this: delegate } } @@ -474,7 +474,7 @@ impl Drop for WindowDelegate { // and then autoreleases it, so autorelease pool is needed let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); let _: () = msg_send![*self.state.window, setDelegate:nil]; - msg_send![autoreleasepool, drain]; + let _: () = msg_send![autoreleasepool, drain]; } } } @@ -536,9 +536,7 @@ impl Drop for Window2 { } } - unsafe { - msg_send![autoreleasepool, release]; - } + let _: () = unsafe { msg_send![autoreleasepool, drain] }; } } @@ -572,9 +570,7 @@ impl Window2 { let app = match Window2::create_app(pl_attribs.activation_policy) { Some(app) => app, None => { - unsafe { - msg_send![autoreleasepool, drain]; - } + let _: () = unsafe { msg_send![autoreleasepool, drain] }; return Err(OsError(format!("Couldn't create NSApplication"))); }, }; @@ -582,17 +578,13 @@ impl Window2 { { Some(window) => window, None => { - unsafe { - msg_send![autoreleasepool, drain]; - } + let _: () = unsafe { msg_send![autoreleasepool, drain] }; return Err(OsError(format!("Couldn't create NSWindow"))); }, }; let view = match Window2::create_view(*window) { Some(view) => view, None => { - unsafe { - msg_send![autoreleasepool, drain]; - } + let _: () = unsafe { msg_send![autoreleasepool, drain] }; return Err(OsError(format!("Couldn't create NSView"))); }, }; @@ -660,9 +652,7 @@ impl Window2 { window.delegate.state.perform_maximized(win_attribs.maximized); } - unsafe { - msg_send![autoreleasepool, drain]; - } + let _: () = unsafe { msg_send![autoreleasepool, drain] }; Ok(window) } @@ -794,7 +784,7 @@ impl Window2 { window.center(); window }); - msg_send![autoreleasepool, drain]; + let _: () = msg_send![autoreleasepool, drain]; res } } @@ -1169,11 +1159,11 @@ impl IdRef { impl Drop for IdRef { fn drop(&mut self) { if self.0 != nil { - let _: () = unsafe { + unsafe { let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); - msg_send![self.0, release]; - msg_send![autoreleasepool, release]; + let _ : () = msg_send![self.0, release]; + let _ : () = msg_send![autoreleasepool, release]; }; } } From 6ea3736c9e4125e9b8fb79f0e438170c36dcb831 Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 28 Apr 2018 13:09:27 +0200 Subject: [PATCH 4/8] removing example/recreate_window_leak.rs --- examples/recreate_window_leak.rs | 77 -------------------------------- 1 file changed, 77 deletions(-) delete mode 100644 examples/recreate_window_leak.rs diff --git a/examples/recreate_window_leak.rs b/examples/recreate_window_leak.rs deleted file mode 100644 index b4108d865d..0000000000 --- a/examples/recreate_window_leak.rs +++ /dev/null @@ -1,77 +0,0 @@ -extern crate winit; - -use winit::{Event, WindowEvent, EventsLoop, ControlFlow}; - -fn create_window(title: &str, events_loop: &EventsLoop) -> winit::Window { - let window_builder = winit::WindowBuilder::new() - .with_title(title) - .with_dimensions(300, 300) - .with_maximized(false); - window_builder - .build(events_loop) - .unwrap() -} - -fn main() { - let mut events_loop = EventsLoop::new(); - - let mut n_window = 0; - let mut is_maximized = false; - let mut decorations = true; - - let mut window = create_window("Hello world", &events_loop); - - let mut should_create_new_window = false; - loop { - let mut should_break = false; - events_loop.run_forever(|event| { - match event { - Event::WindowEvent { event, .. } => match event { - WindowEvent::Closed => { - should_break = true; - ControlFlow::Break - }, - WindowEvent::KeyboardInput { - input: - winit::KeyboardInput { - virtual_keycode: Some(virtual_code), - state, - .. - }, - .. - } => match (virtual_code, state) { - (winit::VirtualKeyCode::Escape, _) => { - should_break = true; - ControlFlow::Break - }, - (winit::VirtualKeyCode::Return, winit::ElementState::Pressed) => { - should_create_new_window = true; - ControlFlow::Break - } - (winit::VirtualKeyCode::M, winit::ElementState::Pressed) => { - is_maximized = !is_maximized; - window.set_maximized(is_maximized); - ControlFlow::Continue - } - (winit::VirtualKeyCode::D, winit::ElementState::Pressed) => { - decorations = !decorations; - window.set_decorations(decorations); - ControlFlow::Continue - } - _ => ( ControlFlow::Continue ), - }, - _ => ( ControlFlow::Continue ), - }, - _ => { ControlFlow::Continue } - } - }); - if should_create_new_window { - window = create_window(&format!("Hello world! {}", n_window), &events_loop); - n_window += 1; - should_create_new_window = false; - } - if should_break { - break; - } - } -} From dc0295016b7c0e3ca4f257e79e2899f458fdf4e0 Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 28 Apr 2018 13:50:16 +0200 Subject: [PATCH 5/8] EventLoop, Shared, no need to retain dead weak ptr --- src/platform/macos/events_loop.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/macos/events_loop.rs b/src/platform/macos/events_loop.rs index 323b681ae1..6621793099 100644 --- a/src/platform/macos/events_loop.rs +++ b/src/platform/macos/events_loop.rs @@ -94,7 +94,7 @@ impl Shared { if let Ok(mut windows) = self.windows.lock() { windows.retain(|w| match w.upgrade() { Some(w) => w.id() != id, - None => true, + None => false, }); } } From 69096b6e0bd23ebad0e0ae09684fbdea299d0881 Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 28 Apr 2018 14:03:47 +0200 Subject: [PATCH 6/8] Change log entry added --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6367aa667c..2ff09d6a54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased +- On Mac, `NSWindow` and supporting objects might be alive long after they were `closed` which resulted in apps consuming more heap then needed. Mainly it was affecting multi window applications. Not expecting any user visible change of behaviour after the fix. - Fix regression of Window platform extensions for macOS where `NSFullSizeContentViewWindowMask` was not being correctly applied to `.fullsize_content_view`. - Corrected `get_position` on Windows to be relative to the screen rather than to the taskbar. - Corrected `Moved` event on Windows to use position values equivalent to those returned by `get_position`. It previously supplied client area positions instead of window positions, and would additionally interpret negative values as being very large (around `u16::MAX`). From ebe131234a638a54a0cd4febc3acd116ca49854b Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 28 Apr 2018 15:09:37 +0200 Subject: [PATCH 7/8] added comment about Shared.find_and_remove_window --- src/platform/macos/window.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/platform/macos/window.rs b/src/platform/macos/window.rs index 60c0713732..a5ddd06312 100644 --- a/src/platform/macos/window.rs +++ b/src/platform/macos/window.rs @@ -516,6 +516,11 @@ unsafe fn get_current_monitor() -> RootMonitorId { impl Drop for Window2 { fn drop(&mut self) { // Remove this window from the `EventLoop`s list of windows. + // The destructor order is: + // Window -> + // Rc (makes Weak<..> in shared.windows none) -> + // Window2 + // needed to remove the element from array let id = self.id(); if let Some(shared) = self.delegate.state.shared.upgrade() { shared.find_and_remove_window(id); From b11259f585308df5e8de3b1b68952ccff5383346 Mon Sep 17 00:00:00 2001 From: Vladimir Shakhov Date: Sat, 28 Apr 2018 17:50:59 +0200 Subject: [PATCH 8/8] fixed code style errors --- src/platform/macos/window.rs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/platform/macos/window.rs b/src/platform/macos/window.rs index a5ddd06312..6f2d00dec5 100644 --- a/src/platform/macos/window.rs +++ b/src/platform/macos/window.rs @@ -454,7 +454,7 @@ impl WindowDelegate { // setDelegate uses autorelease on objects, // so need autorelease - let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); + let autoreleasepool = NSAutoreleasePool::new(nil); (&mut **delegate).set_ivar("winitState", state_ptr as *mut ::std::os::raw::c_void); let _: () = msg_send![*state.window, setDelegate:*delegate]; @@ -472,7 +472,7 @@ impl Drop for WindowDelegate { // Nil the window's delegate so it doesn't still reference us // NOTE: setDelegate:nil at first retains the previous value, // and then autoreleases it, so autorelease pool is needed - let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); + let autoreleasepool = NSAutoreleasePool::new(nil); let _: () = msg_send![*self.state.window, setDelegate:nil]; let _: () = msg_send![autoreleasepool, drain]; } @@ -518,7 +518,7 @@ impl Drop for Window2 { // Remove this window from the `EventLoop`s list of windows. // The destructor order is: // Window -> - // Rc (makes Weak<..> in shared.windows none) -> + // Rc (makes Weak<..> in shared.windows None) -> // Window2 // needed to remove the element from array let id = self.id(); @@ -526,11 +526,10 @@ impl Drop for Window2 { shared.find_and_remove_window(id); } - // nswindow::close uses autorelease // so autorelease pool let autoreleasepool = unsafe { - NSAutoreleasePool::new(cocoa::base::nil) + NSAutoreleasePool::new(nil) }; // Close the window if it has not yet been closed. @@ -569,14 +568,15 @@ impl Window2 { } } let autoreleasepool = unsafe { - NSAutoreleasePool::new(cocoa::base::nil) + NSAutoreleasePool::new(nil) }; let app = match Window2::create_app(pl_attribs.activation_policy) { Some(app) => app, None => { let _: () = unsafe { msg_send![autoreleasepool, drain] }; - return Err(OsError(format!("Couldn't create NSApplication"))); }, + return Err(OsError(format!("Couldn't create NSApplication"))); + }, }; let window = match Window2::create_window(win_attribs, pl_attribs) @@ -584,13 +584,15 @@ impl Window2 { Some(window) => window, None => { let _: () = unsafe { msg_send![autoreleasepool, drain] }; - return Err(OsError(format!("Couldn't create NSWindow"))); }, + return Err(OsError(format!("Couldn't create NSWindow"))); + }, }; let view = match Window2::create_view(*window) { Some(view) => view, None => { let _: () = unsafe { msg_send![autoreleasepool, drain] }; - return Err(OsError(format!("Couldn't create NSView"))); }, + return Err(OsError(format!("Couldn't create NSView"))); + }, }; unsafe { @@ -680,7 +682,6 @@ impl Window2 { } fn class() -> *const Class { - static mut WINDOW2_CLASS: *const Class = 0 as *const Class; static INIT: std::sync::Once = std::sync::ONCE_INIT; @@ -702,7 +703,7 @@ impl Window2 { pl_attrs: &PlatformSpecificWindowBuilderAttributes) -> Option { unsafe { - let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil); + let autoreleasepool = NSAutoreleasePool::new(nil); let screen = match attrs.fullscreen { Some(ref monitor_id) => { let monitor_screen = monitor_id.inner.get_nsscreen(); @@ -1088,7 +1089,7 @@ impl Window2 { // Convert the `cocoa::base::id` associated with a window to a usize to use as a unique identifier // for the window. -pub fn get_window_id(window_cocoa_id: cocoa::base::id) -> Id { +pub fn get_window_id(window_cocoa_id: id) -> Id { Id(window_cocoa_id as *const objc::runtime::Object as usize) } @@ -1166,7 +1167,7 @@ impl Drop for IdRef { if self.0 != nil { unsafe { let autoreleasepool = - NSAutoreleasePool::new(cocoa::base::nil); + NSAutoreleasePool::new(nil); let _ : () = msg_send![self.0, release]; let _ : () = msg_send![autoreleasepool, release]; };