Fix memory management issue

[Kapeli]

_titleBarContainer does not get retained anywhere, so in manual memory management you end up with an EXC_BAD_ACCESS waiting to happen in _recalculateFrameForTitleBarContainer.

[indragiek]

When the container is added as a subview (https://github.com/indragiek/INAppStoreWindow/blob/master/INAppStoreWindow.m#L965) it's retained by the superview. Could you explain a little bit further about how this crash happens? As far as I can tell, the title bar container is added to a superview as soon as it is created, and is not removed for the entire lifecycle of the window.

[Kapeli]

I didn't notice the addSubview. That makes it really weird...

The crash only seems to happen on 10.7.

This is the crash log:

Process:         Dash [4408]
Path:            /Users/USER/Desktop/Dash.app/Contents/MacOS/Dash
Identifier:      com.kapeli.dash
Version:         1.8.9 (1.8.9)
Code Type:       X86-64 (Native)
Parent Process:  launchd [127]

Date/Time:       2013-09-11 14:57:37.235 +0300
OS Version:      Mac OS X 10.7.5 (11G63b)
Report Version:  9

Interval Since Last Report:          50411 sec
Crashes Since Last Report:           1
Per-App Interval Since Last Report:  3 sec
Per-App Crashes Since Last Report:   1
Anonymous UUID:                      8C3EFC63-8E34-40CB-B846-D8A61BD1E7DB

Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x000000080000002b

VM Regions Near 0x80000002b:
    CG shared images       00000001c2b3e000-00000001c2b46000 [   32K] rw-/rw- SM=SHM  
--> 
    JS JIT generated code  000052f249e00000-000052f249e01000 [    4K] ---/rwx SM=NUL  

Application Specific Information:
objc_msgSend() selector name: setFrame:
objc[4408]: garbage collection is OFF

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                 0x00007fff8d09ce90 objc_msgSend + 16
1   com.kapeli.dash                 0x000000010bc2800d -[INAppStoreWindow _recalculateFrameForTitleBarContainer] + 477
2   com.kapeli.dash                 0x000000010bc2612d -[INAppStoreWindow _layoutTrafficLightsAndContent] + 61
3   com.kapeli.dash                 0x000000010bc245dc -[INAppStoreWindow becomeKeyWindow] + 92
4   com.apple.AppKit                0x00007fff8bb4b6d6 -[NSWindow _changeKeyAndMainLimitedOK:] + 690
5   com.apple.AppKit                0x00007fff8bb4b296 -[NSWindow _makeKeyRegardlessOfVisibility] + 100
6   com.apple.AppKit                0x00007fff8bb4b207 -[NSWindow makeKeyAndOrderFront:] + 27
7   com.kapeli.dash                 0x000000010ba54547 -[DHWindowController show:] + 1479
8   com.kapeli.dash                 0x000000010ba4e726 -[DashAppDelegate applicationDidFinishLaunching:] + 198
9   com.apple.Foundation            0x00007fff87a23d0e __-[NSNotificationCenter addObserver:selector:name:object:]_block_invoke_1 + 47
10  com.apple.CoreFoundation        0x00007fff8aa497ba _CFXNotificationPost + 2634
11  com.apple.Foundation            0x00007fff87a0ffc3 -[NSNotificationCenter postNotificationName:object:userInfo:] + 65
12  com.apple.AppKit                0x00007fff8ba56e2b -[NSApplication _postDidFinishNotification] + 212
13  com.apple.AppKit                0x00007fff8ba56b91 -[NSApplication _sendFinishLaunchingNotification] + 78
14  com.apple.AppKit                0x00007fff8ba55858 -[NSApplication(NSAppleEventHandling) _handleAEOpenEvent:] + 242
15  com.apple.AppKit                0x00007fff8ba555b9 -[NSApplication(NSAppleEventHandling) _handleCoreEvent:withReplyEvent:] + 330
16  com.apple.CoreFoundation        0x00007fff8aa93541 -[NSObject performSelector:withObject:withObject:] + 65
17  com.apple.Foundation            0x00007fff87a467c7 __-[NSAppleEventManager setEventHandler:andSelector:forEventClass:andEventID:]_block_invoke_1 + 101
18  com.apple.Foundation            0x00007fff87a4574e -[NSAppleEventManager dispatchRawAppleEvent:withRawReply:handlerRefCon:] + 283
19  com.apple.Foundation            0x00007fff87a455dc _NSAppleEventManagerGenericHandler + 105
20  com.apple.AE                    0x00007fff8cc51c25 aeDispatchAppleEvent(AEDesc const*, AEDesc*, unsigned int, unsigned char*) + 200
21  com.apple.AE                    0x00007fff8cc51b03 _ZL25dispatchEventAndSendReplyPK6AEDescPS_ + 38
22  com.apple.AE                    0x00007fff8cc519f7 aeProcessAppleEvent + 250
23  com.apple.HIToolbox             0x00007fff8e96fb69 AEProcessAppleEvent + 102
24  com.apple.AppKit                0x00007fff8ba529c5 _DPSNextEvent + 1247
25  com.apple.AppKit                0x00007fff8ba5207d -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 135
26  com.apple.AppKit                0x00007fff8ba4e9b9 -[NSApplication run] + 470
27  com.apple.AppKit                0x00007fff8bccaeac NSApplicationMain + 867
28  com.kapeli.dash                 0x000000010ba4d3b2 main + 34
29  com.kapeli.dash                 0x000000010ba4d384 start + 52

[indragiek]

Do you know if explicitly retaining the _titleBarContainer instead of relying on -addSubview: to do it fixes the issue?

[Kapeli]

I investigated it a bit more.

This is what happens: I'm calling setStyleMask: on the window. On 10.8+, this is not an issue. On 10.7, calling setStyleMask also causes a dealloc of the NSThemeFrame, as seen here:

0   Dash                                0x0000000103b7ef32 -[INTitlebarContainer release] + 114
1   CoreFoundation                      0x00007fff8e010ab0 CFRelease + 176
2   CoreFoundation                      0x00007fff8e038f90 -[__NSArrayM dealloc] + 304
3   AppKit                              0x00007fff85439b85 -[NSView _finalizeWithReferenceCounting] + 1302
4   AppKit                              0x00007fff85439646 -[NSView dealloc] + 41
5   AppKit                              0x00007fff8555ccfc -[NSFrameView dealloc] + 131
6   AppKit                              0x00007fff8555cc72 -[NSTitledFrame dealloc] + 70
7   AppKit                              0x00007fff8555cc25 -[NSThemeFrame dealloc] + 67
8   AppKit                              0x00007fff854b2781 -[NSWindow setStyleMask:] + 352
9   Dash                                0x0000000103b80838 -[INAppStoreWindow setStyleMask:] + 88
10  Dash                                0x0000000103a86710 -[DHRegularWindow awakeFromNib] + 96

My hack of retaining the _titleBarContainer is wrong, as all it does is to avoid a crash, but the app is left with a _titleBarContainer that is not a subview of the theme frame.

I think the correct solution is this:

1. Retain the _titleBarContainer properly
2. After setStyleMask: is called, check the superview of the _titleBarContainer and if it's nil, re-add the _titleBarContainer to the themeFrameView.

However, given that themeFrameView seems to change during setStyleMask: on 10.7, it might mean that other bugs could be in there...

[Kapeli]

https://github.com/Kapeli/INAppStoreWindow/commit/1a8a8875f10430982ee585a096b7384252d3f2f5 seems to fix the moving window issue and memory management issue in 10.7.

Please note that you do have other views that you add to the themeFrameView (the window buttons). This means that when the themeFrameView is changed because of a setStyleMask, the buttons should be re-added as well.

I don't use the custom buttons so I did not make the changes for those as well as I would not be able to test properly.

[indragiek]

Sorry for not getting back to this sooner, just swamped with other stuff at the moment. You're correct, INAppStoreWindow's handling of -setStyleMask: is far from ideal. I think that behaviour needs a full audit to make sure that state is fully preserved when the theme frame is swapped. Difficult to test though since as you mentioned, it only affects older OS versions.

[jakepetroules]

Is any part of this still relevant now that we are ARC only?

[Kapeli]

Probably not. As no one else reported anything I think it's just an issue I'm having. Probably doing something wrong somewhere else. The fix I've applied seems to be ok, so as far as I'm concerned this can be closed.

[jakepetroules]

Re-adding the title bar container to the theme frame should probably be looked at, though. I have all versions of OS X available so I can test this later if necessary.

[jakepetroules]

Attempting to toggle NSTexturedBackgroundWindowMask on 10.7 seems to put the window in a totally corrupted state with or without the changes described here. I don't have time to debug this so I simply recommend not calling setStyleMask with NSTexturedBackgroundWindowMask if possible. Even on 10.9 it's quirky.