Fix an over-release and stop stacking web views

AppDelegate.m

@@ -15,7 +15,8 @@ @implementation AppDelegate
 # pragma mark - Initialization
 
 - (id)init {
-	[super init];
+	self = [super init];
+	if(!self) return nil;
 	timer = [[NSTimer scheduledTimerWithTimeInterval:10.0 target:self selector:@selector(timer:) userInfo:nil repeats:YES] retain];
 	webBaseURL = @"https://www.intelliscapesolutions.com/apps/caffeine";
 
@@ -170,22 +171,28 @@ - (BOOL)screensaverIsRunning {
     return activeAppID && [bundleIDs containsObject:activeAppID];
 }
 
-- (LSSharedFileListItemRef)applicationItemInList:(LSSharedFileListRef)list {
+// Hands back a +1 reference, which is what both callers already release.
+// CF_RETURNS_RETAINED states that so the compiler checks it, since the name
+// does not follow the copy/create convention.
+- (LSSharedFileListItemRef)applicationItemInList:(LSSharedFileListRef)list CF_RETURNS_RETAINED {
 	NSString *appPath = [[NSBundle mainBundle] bundlePath];
-	
+
 	NSArray *items = (id)LSSharedFileListCopySnapshot(list, NULL);
-	for(id item in items) {    
+	LSSharedFileListItemRef found = NULL;
+	for(id item in items) {
 		LSSharedFileListItemRef itemRef = (LSSharedFileListItemRef)item;
 		CFURLRef URL = NULL;
 		if(LSSharedFileListItemResolve(itemRef, 0, &URL, NULL)) continue;
-		
+
 		BOOL matches = [[(NSURL*)URL path] isEqual:appPath];
 		CFRelease(URL);
-		if(matches)
-			return itemRef;
+		if(matches) {
+			found = (LSSharedFileListItemRef)CFRetain(itemRef);
+			break;
+		}
 	}
-	CFRelease(items);
-	return NULL;
+	if(items) CFRelease(items);
+	return found;
 }
 
 - (BOOL)startsAtLogin {
@@ -205,7 +212,10 @@ - (void)setStartsAtLogin:(BOOL)start {
 	if(start) {
 		NSString *appPath = [[NSBundle mainBundle] bundlePath];
 		CFURLRef appURL = CFURLCreateWithFileSystemPath(NULL, (CFStringRef)appPath, kCFURLPOSIXPathStyle, YES);
-		LSSharedFileListInsertItemURL(loginItems, kLSSharedFileListItemLast, NULL, NULL, appURL, NULL, NULL);
+		// Declared CF_RETURNS_RETAINED in LSSharedFileList.h, so the item it
+		// hands back has to be released too.
+		LSSharedFileListItemRef inserted = LSSharedFileListInsertItemURL(loginItems, kLSSharedFileListItemLast, NULL, NULL, appURL, NULL, NULL);
+		if(inserted) CFRelease(inserted);
 		CFRelease(appURL);
 	}else{
 		LSSharedFileListItemRef item = [self applicationItemInList:loginItems];
@@ -302,6 +312,27 @@ - (IBAction)showPreferences:(id)sender {
 
 # pragma mark - Help & Feedback Window Utility Methods
 
+// Each open used to allocate another WKWebView and stack it on the one already
+// there, so a window accumulated a full web view, and its WebContent process,
+// per visit. Reuse the one the window already holds.
+//
+// The window's content view owns the web view, so nothing here retains it. Both
+// of these windows are releasedWhenClosed="NO" in the nib, so it survives being
+// closed. The frame is refreshed on reuse in case the window ever becomes
+// resizable.
+- (WKWebView *)webViewForWindow:(NSWindow *)window {
+    NSView *contentView = [window contentView];
+    for(NSView *subview in [contentView subviews]) {
+        if([subview isKindOfClass:[WKWebView class]]) {
+            [subview setFrame:[contentView frame]];
+            return (WKWebView *)subview;
+        }
+    }
+    WKWebView *webView = [[[WKWebView alloc] initWithFrame:[contentView frame]] autorelease];
+    [contentView addSubview:webView];
+    return webView;
+}
+
 -(IBAction)launchHelpCenter:(id)sender {
     [helpCenterWindow center];
     [helpCenterWindow setIsVisible:YES];
@@ -322,9 +353,7 @@ -(IBAction)launchFeedback:(id)sender {
     NSURL *nsurl=[NSURL URLWithString:[NSString stringWithFormat:@"%@%@", webBaseURL, @"/feedback"]];
     if (NSClassFromString(@"WKWebView")) {
         NSURLRequest *nsrequest=[NSURLRequest requestWithURL:nsurl];
-        WKWebView *feedbackWebView = [[WKWebView alloc] initWithFrame:[[feedbackWindow contentView] frame]];
-        [feedbackWebView loadRequest:nsrequest];
-        [[feedbackWindow contentView] addSubview:feedbackWebView];
+        [[self webViewForWindow:feedbackWindow] loadRequest:nsrequest];
         [feedbackWindow center];
         [feedbackWindow setIsVisible:YES];
         [feedbackWindow makeKeyAndOrderFront:nil];
@@ -337,9 +366,7 @@ -(IBAction)launchDonate:(id)sender {
     NSURL *nsurl=[NSURL URLWithString:[NSString stringWithFormat:@"%@%@", webBaseURL, @"/donate"]];
     if (NSClassFromString(@"WKWebView")) {
         NSURLRequest *nsrequest=[NSURLRequest requestWithURL:nsurl];
-        WKWebView *donateWebView = [[WKWebView alloc] initWithFrame:[[donateWindow contentView] frame]];
-        [donateWebView loadRequest:nsrequest];
-        [[donateWindow contentView] addSubview:donateWebView];
+        [[self webViewForWindow:donateWindow] loadRequest:nsrequest];
         [donateWindow center];
         [donateWindow setIsVisible:YES];
         [donateWindow makeKeyAndOrderFront:nil];
@@ -354,10 +381,17 @@ -(IBAction)showProblemReportInfoPopoverButton:(id)sender {
 
 # pragma mark - Maintenance & Memory Management
 
+// Note this does not currently run. init schedules a repeating timer whose
+// target is self, and the run loop holds that timer, so retainCount never
+// reaches zero. Measured: 2 right after init, 1 after releasing the only
+// reference the app holds. Correct teardown regardless, and it is what
+// osx.cocoa.Dealloc is asking for.
 - (void)dealloc {
+    [[[NSWorkspace sharedWorkspace] notificationCenter] removeObserver:self];
     [timer invalidate];
     [timer release];
     [menuView release];
+    [timeoutTimer invalidate];
     [timeoutTimer release];
     [super dealloc];
 }

LCMenuIconView.m

@@ -12,6 +12,7 @@ @implementation LCMenuIconView
 
 - (id)initWithFrame:(NSRect)r {
     self = [super initWithFrame:r];
+    if(!self) return nil;
     statusItem = [[[NSStatusBar systemStatusBar] statusItemWithLength:30] retain];
     [statusItem setVisible:YES];
     [statusItem setView:self];
@@ -124,4 +125,27 @@ - (void)observeValueForKeyPath:(NSString *)keyPath
     [self setNeedsDisplay];
 }
 
+# pragma mark - Memory Management
+
+// Note this does not currently run either. setView: makes the status item
+// retain this view while the view retains the status item, and the status item
+// is held by the system status bar on top of that; retainCount right after
+// initWithFrame: measures 9, and releasing the app's only reference does not
+// deallocate. Exactly one of these is created, in awakeFromNib, and it lives
+// for the life of the process, which for a menu bar icon is the intended
+// behavior rather than a leak.
+//
+// It is here because the class holds retained ivars and osx.cocoa.Dealloc is
+// right to ask for a correct teardown, not because it fixes an observed fault.
+- (void)dealloc {
+    if (@available(macOS 10.14, *)) {
+        [statusItem removeObserver:self forKeyPath:@"view.effectiveAppearance"];
+    }else{
+        [[NSUserDefaults standardUserDefaults] removeObserver:self forKeyPath:@"AppleInterfaceStyle"];
+    }
+    [statusItem release];
+    [menu release];
+    [super dealloc];
+}
+
 @end