Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to ARC #1541

Merged
merged 42 commits into from Aug 29, 2013
Merged

Switch to ARC #1541

merged 42 commits into from Aug 29, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Jul 26, 2013

Opening this so we will have a place to discuss issues we encounter.

  • With the shadow pref enabled, the interface sometimes appears without a shadow. (I'm not 100% sure that the problem is specific to this branch. I just happened to notice it around the same time.)
  • When double-clicking a plug-in, the file isn't moved to the PlugIns folder. You are asked to restart QS if replacing an older plug-in, so it's not ignoring the file completely.

/cc @craigotis

craigotis and others added 29 commits Mar 19, 2013
…App and Code-QuickStepInterface. Reverted the removal of the release method in NDHotKeyEvent. Altered a few allocWithZone: calls in Code-App, changing them to simple alloc calls.
…ng for Debug/Release from the Core and Interface projects.
…method was deprecated.

Included a subrepo commit for VDKQueue

Removed an unnecessary release call, and also an unused variable.
…-Core project to ignore ARC for the VDKQueue.m file.
…tain strong references to the currently-open large type windows. They are removed/released when they are closed.
…and autoreleased an NSImage in a NSBundle category that would cause a leak in some circumstances.
…App and Code-QuickStepInterface. Reverted the removal of the release method in NDHotKeyEvent. Altered a few allocWithZone: calls in Code-App, changing them to simple alloc calls.
…ng for Debug/Release from the Core and Interface projects.
…method was deprecated.

Included a subrepo commit for VDKQueue

Removed an unnecessary release call, and also an unused variable.
…-Core project to ignore ARC for the VDKQueue.m file.
…tain strong references to the currently-open large type windows. They are removed/released when they are closed.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Jul 29, 2013

Trying to track down the plug-in install problem, and now I can't reproduce it. Might have been user error.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 12, 2013

Maybe you should leave your changes in the core application to avoid calling those methods, but also fix them as described so we'll have more time to address this in the plug-ins.

My point was to just change the _BLTRExtension methods to use blocks. There's still no standard method (that I know of) to call a selector on objects in an array - blocks make it easier but it's still not a one liner.
I like your fix Craig, and thanks for the info. Interesting :)

On 12 Awst 2013, at 21:56, Rob McBroom notifications@github.com wrote:

I like the idea of replacing these methods with standard Cocoa stuff wherever possible. Maybe you should leave your changes in the core application to avoid calling those methods, but also fix them as described so we'll have more time to address this in the plug-ins.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 12, 2013

Since I don't have (and don't want) a local copy of every plug-in repo, I figured out how to search GitHub for calls to those methods.

We want .m files containing “onObjectsInArray” in repositories owned by the quicksilver organization.

Haven't gone through all of it, but the impact looks minimal.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 13, 2013

The BLTR extension methods have been updated to use block enumeration. I think a bug was also fixed in the insertObjectsFromArray:atIndex: method - it started inserting from the provided index, and then... continued inserting at the same index. I updated it to increment the index as it inserted, but perhaps I misunderstood its purpose?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 13, 2013

it started inserting from the provided index, and then... continued inserting at the same index. I updated it to increment the index as it inserted, but perhaps I misunderstood its purpose?

I suppose it worked before, but the inserted array would have gotten reversed in the process, which was surely an oversight. Your changes make sense to me.

The other changes look good, too. I'll test for a while.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 13, 2013

Saw one problem right off the bat. I had the (still in dev) Dropbox plug-in enabled. That prevents the launch process from ever finishing.

2013-08-13 10:10:47.120 Quicksilver[94119:303] An error ocurred while scanning "Dropbox": +[QSObject performSelector:onObjectsInArray:returnValues:]: unrecognized selector sent to class 0x100214938

I looked at the code, and the plug-in is calling that method on a QSObject instead of an array. That seems wrong and the plug-in probably needs to be changed, but I bring it up here because we need to figure out:

  1. Why didn't this cause a problem before?
  2. Are there other plug-ins doing something like this?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 14, 2013

If you look at the method in NSArray_BLTRExtension.h you'll see that it's actually in an NSObject category, so it's fine to call it on QSObject. What I did was right ;-)

On 13 Awst 2013, at 22:14, Rob McBroom notifications@github.com wrote:

Saw one problem right off the bat. I had the (still in dev) Dropbox plug-in enabled. That prevents the launch process from ever finishing.

2013-08-13 10:10:47.120 Quicksilver[94119:303] An error ocurred while scanning "Dropbox": +[QSObject performSelector:onObjectsInArray:returnValues:]: unrecognized selector sent to class 0x100214938
I looked at the code, and the plug-in is calling that method on a QSObject instead of an array. That seems wrong and the plug-in probably needs to be changed, but I bring it up here because we need to figure out:

Why didn't this cause a problem before?
Are there other plug-ins doing something like this?

Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 14, 2013

@skurfer @pjrobertson This is my fault. I must have had the wrong copypasta when I re-added the 3 performSelector: methods, because I added them as:

@implementation NSArray (BLTRExtensions)

When they should have been:

@implementation NSObject (BLTRArrayPerform)

I'll check in a fix for this shortly.

…category related to the performSelector methods, so that it matches the interface.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 14, 2013

you'll see that it's actually in an NSObject category

I was only looking at the differences in the header and there weren't any (of consequence). Didn't catch the change in the implementation. Should be good now. I'll test again.

BTW @pjrobertson, we really need to straighten out that string identifier stuff. Half my triggers were destroyed when this branch failed to load all the necessary catalog entries. 😃 If you're too busy, I can take a look at it.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 14, 2013

BTW @pjrobertson, we really need to straighten out that string identifier stuff.

I can see how switching between incompatible QS versions will break things, but is this not what we've said about v1.1.0?
I know it's annoying though - apologies ;-)
I won't have any time today to look at it so feel free if you get a moment

On 14 Awst 2013, at 21:14, Rob McBroom notifications@github.com wrote:

you'll see that it's actually in an NSObject category

I was only looking at the differences in the header and there weren't any (of consequence). Didn't catch the change in the implementation. Should be good now. I'll test again.

BTW @pjrobertson, we really need to straighten out that string identifier stuff. Half my triggers were destroyed when this branch failed to load all the necessary catalog entries. If you're too busy, I can take a look at it.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 15, 2013

Found an easily reproduced crash. If you enable the “Show Window” pref for a trigger, Quicksilver will crash after showing the window.

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                 0x00007fff933b6f5e objc_release + 14
1   libobjc.A.dylib                 0x00007fff933b6230 (anonymous namespace)::AutoreleasePoolPage::pop(void*) + 464
2   com.apple.CoreFoundation        0x00007fff93572d72 _CFAutoreleasePoolPop + 34
3   com.apple.Foundation            0x00007fff8e0d347a -[NSAutoreleasePool drain] + 154
4   com.apple.AppKit                0x00007fff910aa27e -[NSApplication run] + 736
5   com.apple.AppKit                0x00007fff9104ebd6 NSApplicationMain + 869
6   com.blacktree.Quicksilver       0x000000010cb470d4 0x10cb45000 + 8404

Based on this comment from @fheckl, I wonder if ARC agrees with the analyzer and thinks the window needs to be released.

…n the QSTriggerManager/QSWindow that would cause a crash in ARC.
@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 16, 2013

@skurfer The "Show Window" crash is now fixed in 79d5d65 as well. It was a similar fix as the "Large Type Window" crash.

However: Instruments is now reporting a leak when this window closes. I'm still investigating, but the strange thing is that the resource being leaked is only leaked once per application icon - so this seems to be very much related to either a caching operation in Quicksilver, or in the system.

screen shot 2013-08-16 at 6 58 59 am

So, if you set up a trigger to "Toggle Application", and enable the "Show Window" preference, you can toggle Chrome 20 times, and only leak once. (The first time.) Then when you run the trigger on Safari, you have one more leak.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 23, 2013

I'm voting we merge this into master at this point. I've been including it in my local build for some time and haven't seen any problems (that haven't been addressed). I'm sure it's not perfect, but neither is master. 😃 We can continue to do additional fixes in other pull requests.

The main reason I'm pushing for this is that I don't want to work on anything of consequence until after this is merged (and there are things I want to start on).

Thoughts?

@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 23, 2013

@skurfer I'm fine with that. Aside from the memory leak regarding icon caching, I haven't encountered any other issues in my ARC-enabled build. I've been keeping a close eye on the issues list and notifications, so if issues do pop up (and I'm sure they will) I'll try to get to them quickly.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 28, 2013

Aside from the memory leak regarding icon caching, I haven't encountered any other issues in my ARC-enabled build.

Yeah, and if it only leaks once per trigger for which "Show Window" is enabled, that's probably like 25 leaks per year world-wide. 😃

I mentioned a crash I saw on the 10.9 issue, and realized later that I was using an ARC-enabled build. If I have time today, I'll fire up the debugger under 10.9 and see if I can narrow it down.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 28, 2013

OK, narrowed it down some. It does appear to be something on this branch. Here are the conditions:

  1. Running under 10.9
  2. Release build (not under Debug) 😕
  3. Only if you enable the Catalog → System → Configuration → Preference Panes → System Preferences (System) preset

Today's crash report was a bit more detailed than previous ones, so it might help.

Crashed Thread:  3  Dispatch queue: QSCatalogEntry scanQueue: QSPresetSystemPreferencePanes

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Thread 3 Crashed:: Dispatch queue: QSCatalogEntry scanQueue: QSPresetSystemPreferencePanes
0   libobjc.A.dylib                 0x00007fff8c0f9096 objc_release + 22
1   com.blacktree.QSFoundation      0x0000000108b3f39b -[NSFileManager(Scanning) resolveAliasAtPath:usingUI:] + 130
2   com.blacktree.Quicksilver.QSCorePlugIn  0x000000010ebde1e8 0x10ebd4000 + 41448
3   com.blacktree.Quicksilver.QSCorePlugIn  0x000000010ebddee6 0x10ebd4000 + 40678
4   com.blacktree.Quicksilver.QSCorePlugIn  0x000000010ebdf85e 0x10ebd4000 + 47198
5   com.blacktree.QSCore            0x0000000108bc4e6e -[QSCatalogEntry scannedObjects] + 76
6   com.blacktree.QSCore            0x0000000108bc519e __30-[QSCatalogEntry scanAndCache]_block_invoke + 159
7   libdispatch.dylib               0x00007fff8315f2ad _dispatch_client_callout + 8
8   libdispatch.dylib               0x00007fff83160166 _dispatch_barrier_sync_f_invoke + 39
9   com.blacktree.QSCore            0x0000000108bc506a -[QSCatalogEntry scanAndCache] + 175
10  com.blacktree.QSCore            0x0000000108bc5758 -[QSCatalogEntry scanForced:] + 1025
11  com.blacktree.QSCore            0x0000000108bd1f44 __42-[QSLibrarian scanCatalogIgnoringIndexes:]_block_invoke + 340
12  libdispatch.dylib               0x00007fff831621d7 _dispatch_call_block_and_release + 12
13  libdispatch.dylib               0x00007fff8315f2ad _dispatch_client_callout + 8
14  libdispatch.dylib               0x00007fff8316109e _dispatch_root_queue_drain + 326
15  libdispatch.dylib               0x00007fff83162193 _dispatch_worker_thread2 + 40
16  libsystem_pthread.dylib         0x00007fff85f0af08 _pthread_wqthread + 314
17  libsystem_pthread.dylib         0x00007fff85f0dfc9 start_wqthread + 13

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 28, 2013

Removing CFRelease((__bridge CFTypeRef)(url)); seems to prevent the crash, but I don't know if that's the correct solution. Maybe by casting to NSURL two lines earlier, ARC takes control of it and does the release for us?

Just guessing. Knowing what CF stands for is about the extent of my knowlege in this area. 😳

@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 29, 2013

@skurfer Yes, removing that CFRelease is the correct solution. When we cast to NSURL * two lines earlier, we do so with CFBridgingRelease, which:

Moves a non-Objective-C pointer to Objective-C and also transfers ownership to ARC.
You use this function to cast a Core Foundation-style object as an Objective-C object and transfer ownership of the object to ARC such that you don’t have to release the object

+5 points to Apple for good ARC documentation

@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 29, 2013

OK, great. I'll do that (and maybe search the project for other occurrences of CFBridgingRelease), then merge this into master. Thanks.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 29, 2013

Thanks - I searched the project for all other CFBridgingRelease calls after taking a look at the erroneous one this morning, the rest look good.

Using `CFBridgingRelease` puts the object under ARC's control
skurfer added a commit that referenced this issue Aug 29, 2013
@skurfer skurfer merged commit 1575ce7 into master Aug 29, 2013
@skurfer skurfer deleted the ARC branch Aug 29, 2013
@skurfer
Copy link
Member Author

@skurfer skurfer commented Aug 29, 2013

Alright. It's merged.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Aug 30, 2013

Nice :)

I'll update my daily QS build to master+ARC now :)
Seems like most crashes are occurring for release builds, but just incase @craigotis do you know which exact setting was the one that you changed to reproduce crashes in Debug (from this post: #1541 (comment) )

On 30 Awst 2013, at 02:58, Rob McBroom notifications@github.com wrote:

Alright. It's merged.


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Aug 30, 2013

@pjrobertson Unfortunately I never narrowed it down to just one setting. I mainly just went through all the build settings, and changed anything that was different between Debug+Release, to the Release value.

Though, it is most likely one of the compiler optimization settings. I'm taking a look at Rob's stack trace right now to see if I can reproduce.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants