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

Fix some leaks #1683

Merged
merged 12 commits into from Dec 5, 2013
Merged

Fix some leaks #1683

merged 12 commits into from Dec 5, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Nov 20, 2013

I wanted to fix all of them, but was only able to fix one, and introduced a warning in the process. Maybe there’s a better solution? I tried various changes to the prototype including extern void as described by CocoaDev, thinking maybe Apple had changed it at some point and ours needed to match, but in the end, just removing it was the only thing that stopped the leaking. So now there’s the warning. 😕

There are some other leaks, but they only show up a handful of times. The one fixed here was responsible for hundreds of leaks, so the majority are fixed by far.

The other leaks I found, but wasn’t able to fix:

  1. Line 379 of QSAppleScriptActions.m. Looks right to me. Could the leak be down in the framework somewhere?
  2. In -[QSRegistry getClassInstance:], depending on the conditions, the variable instance might be “owned” by the method, but most of the time, it shouldn’t be. ARC doesn’t seem to know what to do. I tried various things to separate that condition out with the hope that ARC would then know to release it, but no dice.

There are more leaks, but I think they’re all in plug-ins. I’ll be looking at some of those next.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 22, 2013

I think it's because ARC assumes the returned NSArray is autoreleased, so ownership is not being transferred to ARC.
In reality, the object is a CF object, so the private function should probably be declared as such
This page probably gives a better representation of the actual declaration: http://recon-mac.googlecode.com/svn/trunk/recon-mac/Vendor/Path%20Finder%20SDK/Source/CocoatechFile/NTLaunchServices.m
Note the use of the CFArrayRef and the return of an OSStatus

Here's what I did before seeing that URL above (you can probably now swap the bool for an osstatus:

bool _LSCopyAllApplicationURLs(CFArrayRef *array);

- (NSArray *)allApplications {
    CFArrayRef appURLs = NULL;
    _LSCopyAllApplicationURLs(&appURLs);
    NSMutableArray *apps = nil;
    if (appURLs != NULL) {
        apps = [NSMutableArray arrayWithCapacity:[(__bridge NSArray *)appURLs count]];
        for (id loopItem in (__bridge NSArray *)appURLs) {
            [apps addObject:[loopItem path]];
        }
        CFRelease(appURLs);
    }
    return apps;
}

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 22, 2013

There's probably some more fancy thing you can do with (__bridge transfer) but what I did is more explicit.

Oh - and I think that awake line can be removed right. Is it that useful?! ;-)

About the other leaks:

  • Have you tried releasing the returned errorDict from line 379? Does it crash? Maybe the method is returning a retained dict. My guess is that that framework hasn't been updated for ages since Apple don't care about AS anymore, so it could be leaky (but I still think it's unlikely)
  • Yuck, getClassInstance is horrible. I think we need to be very careful about any places where when switching to ARC we've just removed the autorelease from a returning method (@craigotis). We've probably got quite a few more leaks somewhere.
    See the "Transitioning to ARC" release notes for a possible solution (search for "Variable Qualifiers")
    What I think we need to do is declare two variables as follows:
id __strong instanceStrong = nil;
id __autoreleasing instance = nil;

In all cases but one (the alloc] init] line) we assign whatever to the instance var. But in the case of the one alloc] init] line we set instanceStrong, then later we do:

    if (instanceStrong) {
        instance = instanceStrong;
    }

There may be a cleaner way, but I think this'll work

...and I've just run the analyzer in Xcode 5. It brought some useful things (mainly with the ND classes, we really need to ditch them I think...), one of which is our stuff:

  • QSObject_FileHandling.m:L800 add CFRelease(mdItem)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 23, 2013

Also, I think the same leak will apply to QSObject_Pasteboard.m +[QSObject objectWithPasteboard:] since in that case the returned type is sometimes an autoreleased var and others a retained var.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 23, 2013

Your version of allApplications doesn’t leak, so I’ll do that and add the prototype back. Thanks.

Don’t have time to look at the other stuff right now, but I found another problem with getInstance: when looking at it again the other day. The sharedInstance line will almost always get something that isn’t autoreleased, but since it doesn’t start with “init”, “copy”, etc. ARC has no way of knowing that. I guess it’s not a big deal since using sharedInstance prevents more than one such object from existing, and they stay around for the life of the application, so leaks are unlikely. But should we do something about it? I tried adding NS_RETURNS_RETAINED to our sharedInstance prototypes, but since other classes have a method by that name, it wouldn’t build.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 23, 2013

Your version of allApplications doesn’t leak, so I’ll do that and add the prototype back. Thanks.

Cool, no problem :)

The sharedInstance line will almost always get something that isn’t autoreleased, but since it doesn’t start with “init”, “copy”, etc. ARC has no way of knowing that.

See one of my comments about using __autoreleasing and __strong. I think that’s the right way of doing things

On 24 Nov 2013, at 05:18, Rob McBroom notifications@github.com wrote:

Your version of allApplications doesn’t leak, so I’ll do that and add the prototype back. Thanks.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 23, 2013

Hey guys! Sorry I'm getting to this PR a little late, trying to catch up on all the side projects. Do you want me to take a look at getClassInstance, or has that one been fixed up? I'll pull down the leaks branch and take a peek in Instruments.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 23, 2013

I found an interesting one that should be easy to knock out, but I thought I'd check here first. It seems to happen when opening, then closing, the preferences window for the first time:

screen shot 2013-11-23 at 6 51 14 pm

Taking a look at the docs for that method call (instantiateNibWith...), it has been deprecated, and recommends you instead use (instantiateWith...), which mentions:

Unlike legacy methods, the objects adhere to standard Cocoa memory management rules; it is necessary to keep a strong reference to the objects or the array to prevent the nib contents from being deallocated.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

Ah, nevermind - that newer method was introduced in 10.8, I thought it was in 10.7. Back to the drawing board.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

Nice find, since we’re still supporting 10.7, then we can’t really drop that. Not sure how to make the outlets strong references though?

FYI - about using __autoreleasing: the docs say:

__autoreleasing is used to denote arguments that are passed by reference (id *) and are autoreleased on return.

so perhaps we shouldn’t use it in any other place?

On 24 Nov 2013, at 07:57, Craig Otis notifications@github.com wrote:

I found an interesting one that should be easy to knock out, but I thought I'd check here first. It seems to happen when opening the preferences window for the first time:

Taking a look at the docs for that method call (instantiateNibWith...), it has been deprecated, and recommends you instead use (instantiateWith...), which mentions:

Unlike legacy methods, the objects adhere to standard Cocoa memory management rules; it is necessary to keep a strong reference to the objects or the array to prevent the nib contents from being deallocated.


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

Fortunately the easy solution to that is to simply maintain the top level objects, which we're currently just discarding, as an ivar of the QSPreferencePane. You can take a look at this commit, and I can push it to upstream to include it here. It does squelch the leak.

craigotis@2fc6a6b

I'm still doing a bit of research into this NIB instantiation to ensure we're not creating any retain cycles. Best stuff I've found so far:

http://lists.apple.com/archives/cocoa-dev/2012/Aug/msg00074.html

http://www.tangent405.com/2012/11/20/commonNibLeaks.html

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

Ah, nevermind - that newer method was introduced in 10.8, I thought it was in 10.7. Back to the drawing board.

Yeah, we’ve discussed dropping 10.7 support (well, I have!) so we could potentially do it - especially since Mavericks was a free upgrade

Some stats frmo # of requests to the update system:

OS Version %age requests
10.9 69%
10.8 16%
10.7 7%
10.6 4%
Other 4%

Only 7% of users are using 10.7... that's enough reason to drop it right? :)

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

Ehh... Yea, that's a tough call, and one I'll leave up to you guys. I plan on supporting 10.7 until 10.10 is at least in developer hands. I still get emails from 10.6.8 folks who are upset that I set the minimum bar at 10.7.

As for the QSPreferencePane leak, this mailing list mentions dropping to Core Foundation and releasing each object:

http://markmail.org/message/d7yzsqzz5p3md3mm

Sneaking a peek at the Chromium code (https://src.chromium.org/chrome/trunk/src/ui/base/cocoa/nib_loading.mm), and a couple other examples, I pushed another commit that releases them manually immediately after instantiating the NIB, rather than maintaining an ivar, which didn't actually help, which I blame on it being Saturday evening. Here's the better way:

craigotis@b26e955

…se the top-level NIB objects, to avoid using the improved 10.8-only NIB-loading method, while still ensuring the top-level objects are properly released when using ARC.
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

Oh and getClassInstance hasn’t yet been fixed, but see my comment above. I've done a bit more reading and apparently __autoreleasing is only for returning objects by reference...
There's also a leak in QSObject_Pasteboard.m +[QSObject objectWithPasteboard:] as I said.

It's probably worth searching for every place where we've removed autorelease and make sure that the method always returns an autoreleased object. The problem is that sometimes it returns a __strong var and other times an __autoreleasing var.

I meant to give a link to the "Transitioning to ARC" docs. Here it is (search for "You also need to take care with objects passed by reference").

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

The thing about removing the autorelease calls is that in ARC, the objects that you create are by design, automatically released. The compiler inserts the release calls when the instances are no longer referenced by any strong references, so when you're returning:

NSObject *myObject = [[NSObject alloc] init];
return [myObject autorelease];

Changing this to use ARC and:

NSObject *myObject = [[NSObject alloc] init];
return myObject;

Is cool beans - the compiler will examine all the places where this returned instance is referenced, and will automatically insert the release calls: http://stackoverflow.com/questions/8292060/arc-equivalent-of-autorelease

But I feel like I'm misunderstanding what you're pointing out - feel free to correct me. :-) As you mention - the __autoreleasing is only used for reference references, which is commonly NSError ** and similar. Are there any common places we do this in Quicksilver? I'll go back and re-examine the original ARC commits to see if I missed anything.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

I think you're probably right, but just to clarify, in the getClassInstance method, we have:

id instance;

which is the same as

id __strong instance; (not the position of __strong - I just learnt that apparently __strong id instance is wrong, but the compiler allows it)

now in some places we set instance to an autoreleased variable, and in other places to a retained variable. If ARC only autoreleases at the end of the method (just before it returns) but it sees that in most cases instance is already an autoreleased var, then will it tack on the autorelease at the end?

Now I say that though... my guess is that ARC does this for the autoreleased vars (since instance is __strong)

we write:

instance = [classInstances objectForKey:className];

To adhere to the __strong, ARC does this:

instance = [[classInstances objectForKey:className] retain];

then at the end instance gets autoreleased... although that doesn't help with the leak :/

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

You're right - it will tack on the release in either case, because when the value that's being assigned to instance is assigned, it will have received a retain call. This is because, as you mention, instance is implicitly a __strong variable. So the instance does have a +1 retain count, which needs to be decremented.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

@skurfer Can you identify a particular scenario or steps that I can run on my machine while profiling that will cause a leak to occur in getClassInstance:?

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

OK, cool. But we still have the problem of the leak :(

http://d.pr/i/rU40

Perhaps it's a bug with ARC? I've just made a test project trying to replicate the getClassInstance: method and it doesn't leak. Hmm...

Oh about reproducing the leak: I can reproduce it, and I'm not doing anything. It's coming from one of the plugins that uses Scripting Bridge. Try installing these if you don't already have them:

  • iTunes plugin
  • Mail plugin
  • Chrome plugin
  • Transmit plugin

Maybe Rob is right and the issue is actually down in the SB Framework

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

OK no, I’ve just seen QSTextActions leak the exact same leak

P.S. I've added a commit that fixes another leak

On 24 Nov 2013, at 09:53, Craig Otis notifications@github.com wrote:

@skurfer Can you identify a particular scenario or steps that I can run on my machine while profiling that will cause a leak to occur in getClassInstance:?


Reply to this email directly or view it on GitHub.

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 24, 2013

Guys, are you sure the leak is in getClassInstance: and not somewhere below ? I can't see anything wrong in that method.

  • instance RC (=retain count) starts at 0, doesn't exists.
  • if sharedInstance is supported, it's used. instance has RC = 2, owned by itself and the autorelease pool (because ARC in sharedInstance ought to put it there).
  • else, instance is created by us. instance has RC = 1, "owned" by us.
  • we add instance to our cache. RC++, owned by our cache.
  • instance is returned as a an RC++ autoreleased object.

Just after the return, we'll be left with either :

  • a singleton, RC = 4 owned by itself and our cache and doubly autoreleased which will go down to RC = 2,
  • or a normal object RC = 2 owned by us and our cache but autoreleased, which will go down to RC = 1.

I can't see anything wrong here ;-). Can we start suspecting Apple stuff ?

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

I'm with @tiennou - that method looks fine.

The leak highlighted in your screenshot, @pjrobertson, doesn't place blame on the method, it just indicates where the leaked piece of memory was originally allocated. And as @tiennou mentions, classInstances maintains a reference to the allocated memory - which will be severed (and release'd) when the QSRegistry is deallocated and the dictionary is purged.

Aside from the classInstances dictionary, after ..alloc] init] is called on that leaky line, we're not doing anything else with the memory, just returning it - which ARC knows exactly how to handle.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

P.S. Pushed my two commits for the QSPreferencePane leak.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

I can’t reproduce that leak for some reason. Just out of interest, would just doing:

[nib instantiateNibWithOwner:self topLevelObjects:nil]; not be OK?!

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

Nope, that will leak the top level objects. They're being created regardless of whether you provide that pointer reference to get access to them afterwards. Since QSPreferencePane is neither an NSViewController nor an NSWindowController, you have to manually release those top level objects. (As those two classes will do it for you.)

You can reproduce the leak by removing the CFRelease() loop, then opening the preference pane and closing it. It won't leak until the pane is closed.

Edit: I just noticed it will only leak if you have the "Actions" category open at some point. If you open the preferences pane to the "Handlers" category, then click on "Appearance", then close it, it won't leak. But if you click on "Actions" at any point, that will trigger the leak once you finally close the window.

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 24, 2013

I just noticed it will only leak if you have the "Actions" category open at some point.

Maybe only the Actions prefpane has a leak (or uses top-level objects for which we keep no reference and thus leak when the window's closed). Anyway, keeping the topLevelObjects: reference is fine (as would have been making sure every TLO in the "Actions" nib gets released, but the former is more future-proof).

classInstances maintains a reference to the allocated memory - which will be severed (and release'd) when the QSRegistry is deallocated and the dictionary is purged.

Just to point that out, this can never happen, as QSReg is a singleton.

Aside from the classInstances dictionary, after ..alloc] init] is called on that leaky line, we're not doing anything else with the memory, just returning it - which ARC knows exactly how to handle.

Ooooh I think I saw it : Imagine instance fails to initialize by raising an exception, you'd have a valid pointer to some half-working object — because alloc fails only in case there's no memory, but init raised, failing to do its part. Does returning nil from the @catch block makes the leak go away ?

I'm really not sure it's the exact cause, but :

  • the iTunes plugin loves to raises exceptions there (anyone who debugs QS with "Break on exception" set knows that, and I think it's in line with us suspecting SB.framework).
  • the setObject:forKey: call just below would have done weird things when passed a half-object. So maybe init was (almost) fine in that it did its part and returned something that won't crash when accessed (I mean, self = [super init] is usually the first thing you do in there, and once it's done, it doesn't matter if init returns cleanly or via exception-raising — self is already valid), but raising the exception leaked something.

Better check those init methods ;-).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 24, 2013

Ooooh I think I saw it : Imagine instance fails to initialize by raising an exception, you'd have a valid pointer to some half-working object — because alloc fails only in case there's no memory, but init raised, failing to do its part. Does returning nil from the @catch block makes the leak go away ?

As an FYI - when I see the leak, I don’t see the "Failed to instantiate provider for class…” NSLog, so I’m not sure.
I too was worried that we weren’t using an @finally block/using the @catch block for something more useful than just an NSLog, maybe we should?

On 24 Nov 2013, at 23:23, Etienne Samson notifications@github.com wrote:

I just noticed it will only leak if you have the "Actions" category open at some point.

Maybe only the Actions prefpane has a leak (or uses top-level objects for which we keep no reference and thus leak when the window's closed). Anyway, keeping the topLevelObjects: reference is fine (as would have been making sure every TLO in the "Actions" nib gets released, but the former is more future-proof).

classInstances maintains a reference to the allocated memory - which will be severed (and release'd) when the QSRegistry is deallocated and the dictionary is purged.

Just to point that out, this can never happen, as QSReg is a singleton.

Aside from the classInstances dictionary, after ..alloc] init] is called on that leaky line, we're not doing anything else with the memory, just returning it - which ARC knows exactly how to handle.

Ooooh I think I saw it : Imagine instance fails to initialize by raising an exception, you'd have a valid pointer to some half-working object — because alloc fails only in case there's no memory, but init raised, failing to do its part. Does returning nil from the @catch block makes the leak go away ?

I'm really not sure it's the exact cause, but :

the iTunes plugin loves to raises exceptions there (anyone who debugs QS with "Break on exception" set knows that, and I think it's in line with us suspecting SB.framework).
the setObject:forKey: call just below would have done weird things when passed a half-object. So maybe init was (almost) fine in that it did its part and returned something that won't crash when accessed (I mean, self = [super init] is usually the first thing you do in there, and once it's done, it doesn't matter if init returns cleanly or via exception-raising — self is already valid), but raising the exception leaked something.
Better check those init methods ;-).


Reply to this email directly or view it on GitHub.

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 24, 2013

Yeah, wasn't sure from the comments, so it might not be that. (Unrelated note, be careful with those @ keywords, unless you plan on pinging unrelated people ;-)).

@craigotis
Copy link
Contributor

@craigotis craigotis commented Nov 24, 2013

Just to point that out, this can never happen, as QSReg is a singleton.

Right - I had a feeling it was a singleton, but at least it's still going to (eventually) release the memory it's holding on to. So no leak there.

Anyway, keeping the topLevelObjects: reference is fine (as would have been making sure every TLO in the "Actions" nib gets released, but the former is more future-proof).

We don't actually keep the reference - not for very long, anyway. We just get the reference, manually CFRelease() every one of the top-level objects, then continue on as before, ditching the reference.

Better check those init methods ;-).

A half-initialized object could be causing the leak. If an object fails to initialize, it is the responsibility of that object to do two things (assuming the object is not using ARC, if it is, it only has to adhere to # 2):

  1. Release self
  2. Return nil

See: http://stackoverflow.com/a/4337133/88111

So whether the init method returns self or nil, ARC will handle the memory properly on the receiving end of things. However, if the object returns nil without remembering to call [self release], that memory could indeed be lost forever. So it might be best to add an NSLog() after line 164 to check and see if there was no exception thrown, but the init method returned nil. Just a hunch.

I still haven't been able to reproduce this leak locally though - @pjrobertson, do you have any additional screenshots from Instruments with the alloc/retain/release lifecycle of the leaked memory, or additional tips for reproducing? I installed iTunes and Chrome plugins, but still can't get it to pop up.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 25, 2013

I’ll address the speculation about assigning a value to instance inside the try/catch: That’s not the problem.

One of the things I tried was creating an entirely new variable inside the try block and returning it from there. Something like:

id newInstance = [[providerClass alloc] init];
[classInstances setObject:newInstance forKey:className];
return newInstance;

Instruments still complained of a leak, even though that variable doesn’t exist outside the scope of try and ARC should have no trouble figuring out that it needs to be released. So it’s probably what @craigotis is suggesting: An init doing the wrong thing somewhere.

I can reproduce this every time by right-arrowing into an address book contact. You may or may not need to arrow down through the items until you hit an e-mail address.

tiennou added 2 commits Nov 28, 2013
```
Leak: 0x10e2df330  size=48  zone: DefaultMallocZone_0x100319000   __NSCFString  ObjC  CoreFoundation  "/Volumes/Quicksilver"
	Call stack: [thread 0x106122000]: | start_wqthread | _pthread_wqthread | _dispatch_worker_thread2 | _dispatch_client_callout | _dispatch_call_block_and_release | __42-[QSLibrarian scanCatalogIgnoringIndexes:]_block_invoke QSLibrarian.m:531 | -[QSCatalogEntry scanForced:] QSCatalogEntry.m:577 | -[QSCatalogEntry scanAndCache] QSCatalogEntry.m:526 | QSGCDQueueSync QSGCD.h:33 | _dispatch_barrier_sync_f_invoke | _dispatch_client_callout | __30-[QSCatalogEntry scanAndCache]_block_invoke QSCatalogEntry.m:532 | -[QSCatalogEntry scannedObjects] QSCatalogEntry.m:502 | -[QSDefaultsObjectSource objectsForEntry:] QSDefaultsObjectSource.m:85 | -[QSDefaultsObjectSource addObjectsForKeyList:keyNumber:ofType:inObject:toArray:] QSDefaultsObjectSource.m:102 | -[QSDefaultsObjectSource addObjectsForKeyList:keyNumber:ofType:inObject:toArray:] QSDefaultsObjectSource.m:95 | -[QSDefaultsObjectSource addObjectsForKeyList:keyNumber:ofType:inObject:toArray:] QSDefaultsObjectSource.m:102 | -[QSDefaultsObjectSource addObjectsForKeyList:keyNumber:ofType:inObject:toArray:] QSDefaultsObjectSource.m:118 | -[NDAlias(QSMods) quickPath] NDAlias+QSMods.m:15 | FSCopyAliasInfo | _FSCopyExtendedAliasInfoFromAliasPtr | CFStringCreateWithCString | __CFStringCreateImmutableFunnel3 | _CFRuntimeCreateInstance | malloc_zone_malloc
```
@tiennou
Copy link
Member

@tiennou tiennou commented Nov 28, 2013

Here's some more. I can't reproduce the leak how you described though @skurfer. I'm not sure how is it possible yet (still on 10.8 here, running Xcode 5.0.2)... The only one I got left is that :

Leak: 0x108bbd7c0  size=16  zone: DefaultMallocZone_0x100319000   NSFileManager  ObjC  Foundation
    0x7cc6e3e0 0x00007fff 0x00000000 0x00000000     ...|............
    Call stack: [thread 0x101781000]: | 0x1030eea22 | _objc_rootAllocWithZone | class_createInstance | calloc | malloc_zone_calloc

which from the binary images list seems to point to

0x1030eb000 -        0x1030f3fff +com.robertson.quicksilver.Dropbox-Plugin (1.0.0 - 7F) <E83FD210-2581-3B52-8731-F36487498643> /Users/tiennou/Library/Application Support/Quicksilver/PlugIns/com.robertson.quicksilver.Dropbox-Plugin.7F.qsplugin/Contents/MacOS/Dropbox Plugin

most likely here... Okay, tried to put a link to QSDropboxPluginController.m:135, but I can't find the repo on GitHub...

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 30, 2013

I was happily trying to kill the NSWorkspace deprecations when I stumbled on that one, ouch ! But there, I fixed it ;-).

About NSWorkspace, I think most of the category can be deleted : the Carbon ProcessManager we could have used to workaround the deprecation notices is now marked as such in the 10.9 SDK, so... I guess a rewrite is in order ;-).

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 1, 2013

most likely here... Okay, tried to put a link to QSDropboxPluginController.m:135, but I can't find the repo on GitHub...

I haven’t wanted to make the code public (yet). Do you still have access to it? - My free private repos from GitHub were revoked recently because I’m no longer a student :’(

On 30 Nov 2013, at 23:34, Etienne Samson notifications@github.com wrote:

I was happily trying to kill the NSWorkspace deprecations when I stumbled on that one, ouch ! But there, I fixed it ;-).

About NSWorkspace, I think most of the category can be deleted : the Carbon ProcessManager we could have used to workaround the deprecation notices is now marked as such in the 10.9 SDK, so... I guess a rewrite is in order ;-).


Reply to this email directly or view it on GitHub.

@tiennou
Copy link
Member

@tiennou tiennou commented Dec 1, 2013

I have access to my local clone, but your private repo is gone.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 2, 2013

OK we're still stuck with the getInstance: thing, but shall we merge this?

FWIW there is definitely a leak in the Dropbox plugin (Etienne - the line you said, not releasing NSFileManager *fm). That may well be where the leak is appearing. So it seems like it is just plugins leaking, not getInstance: itself?

@craigotis
Copy link
Contributor

@craigotis craigotis commented Dec 2, 2013

That would be my guess (that it's the plugins leaking) - I don't see anything wrong with getInstance:.

As for the leak in the Dropbox plugin, if you want to send me a copy (or is there a cloneable repo somewhere?) I'd be happy to take a look. I find it hard to believe an NSFileManager is being leaked, unless one is being explicitly created on a secondary thread to avoid race conditions. Otherwise, you'd almost always want to use defaultManager.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 2, 2013

I find it hard to believe an NSFileManager is being leaked, unless one is being explicitly created on a secondary thread to avoid race conditions.

I did indeed use alloc, init to create NSFileManager, and forgot to release. Not a biggy :)
I’ll try and figure out a way of getting a new repo online, but I’d rather keep it closed source for now. (I seem to recall bitbucket gives unlimited private repos so I may use that if you still want to have a look - the leaks not interesting :P)

On 2 Dec 2013, at 20:21, Craig Otis notifications@github.com wrote:

That would be my guess. I don't see anything wrong with getInstance:.

As for the leak in the Dropbox plugin, if you want to send me a copy (or is there a cloneable repo somewhere?) I'd be happy to take a look. I find it hard to believe an NSFileManager is being leaked, unless one is being explicitly created on a secondary thread to avoid race conditions. Otherwise, you'd almost always want to use defaultManager.


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Dec 4, 2013

This can probably be merged - I think all the commits here just fix leaks - they don't cause any new ones?

pjrobertson added a commit that referenced this issue Dec 5, 2013
@pjrobertson pjrobertson merged commit 61acca9 into master Dec 5, 2013
1 check passed
@pjrobertson pjrobertson deleted the leaks branch Dec 5, 2013
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 5, 2013

Done

@craigotis
Copy link
Contributor

@craigotis craigotis commented Dec 5, 2013

Thanks @pjrobertson. High five!

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 5, 2013

hehe! Thank you :)

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

4 participants