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

Replace [pool release] with [pool drain] and alloc an iVar #870

Merged
merged 2 commits into from May 10, 2012

Conversation

pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented May 9, 2012

Sending an NSAutorelease pool a release call should be exactly the same as sending it a drain call, but the Apple Documentation says "You should typically use drain instead of release."[1]
There has also been a report of another person receiving BAD_ACCESS calls when doing this (8 crash reports in 2 days for QS)
Another few crash reports indicated that the defaultSearchSet iVar is never alloc or inited, so was causing a crash when trying to release a non-existent iVar (again, see Crash reports)

Sending an NSAutorelease pool a release call should be exactly the same as sending it a drain call, but the Apple Documentation says "You should typically use drain instead of release."[1]
There has also been a report of another person receiving BAD_ACCESS calls when doing this (8 crash reports in 2 days for QS)
Another few crash reports indicated that the `defaultSearchSet` iVar is never alloc or inited, so was causing a crash when trying to release a non-existent iVar (again, see Crash reports)
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

@HenningJ - on the subject of autorelease pools and their efficiency, when changing all the release calls to drain I came across this use of NSAutoreleasePool. Do you think it's inefficient (following on from the discuss we had a few days back about using NSAutoReleasePools) and that it could be removed?

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

Actually, probably the main thing about that bit of code I linked to is that the for(j....) call does ABSOLUTELY nothing from what I can tell?! Although it is only for debug builds, so it doesn't really matter

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 9, 2012

There has also been a report of another person receiving BAD_ACCESS calls when doing this (8 crash reports in 2 days for QS)

That can't be related to using release instead of drain. Since we are not using GC, they should behave the same, and therefore changing it shouldn't make any difference. Right? So why do it? Are you sure you're reading the crash reports correctly? ;-)

Another few crash reports indicated that the defaultSearchSet iVar is never alloc or inited, so was causing a crash when trying to release a non-existent iVar (again, see Crash reports)

Now that sounds like it makes sense.

Actually, probably the main thing about that bit of code I linked to is that the for(j....) call does ABSOLUTELY nothing from what I can tell?!

It executes whatever happens inside the loop 25 times. But since I don't know what the whole method does, I don't know if that makes any sense.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

Here's the important bit from the crash report:

Thread 4 Crashed:
0   libobjc.A.dylib                 0x9792ec07 objc_msgSend + 23
1   libobjc.A.dylib                 0x979315b2 (anonymous namespace)::AutoreleasePoolPage::pop(void*) + 490
2   com.apple.CoreFoundation        0x93a5f613 _CFAutoreleasePoolPop + 51
3   com.apple.Foundation            0x9739f7b3 -[NSAutoreleasePool release] + 125
4   com.blacktree.QSCore            0x000aae9f -[QSLibrarian scanCatalogIgnoringIndexes:] + 561

We've had about 8 crash reports all exactly the same with these lines. Initially I thought that there was no difference between drain and release but reading some things has made me change my mind. I forgot to add links to the commit message, oops:

http://jongampark.wordpress.com/2008/07/30/when-pool-release-causes-bad_access-error/
https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Foundation/Classes/NSAutoreleasePool_Class/Reference/Reference.html#//apple_ref/doc/uid/20000051-SW5

Worst case scenario: I've wasted about 5 minutes of my time :)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

And looking explicitly at AutoreleasePoolPage::pop(void*) the problem could be because of creating autoreleasepools on multiple threads and messing up the stack order, that might be more likely

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 9, 2012

They first link talks about when GC is turned on. The second on says exactly what we both are saying: With GC off, they should behave exactly the same.

And looking explicitly at AutoreleasePoolPage::pop(void*) the problem could be because of creating autoreleasepools on multiple threads and messing up the stack order, that might be more likely

Now that sounds like it is worth looking into more.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

They first link talks about when GC is turned on. The second on says
exactly what we both are saying: With GC off, they should behave exactly
the same.

Fair enough :)
So it's a stylistic change, I prefer drain ;-)

On 9 May 2012 08:50, Henning Jungkurth <
reply@reply.github.com

wrote:

They first link talks about when GC is turned on. The second on says
exactly what we both are saying: With GC off, they should behave exactly
the same.

And looking explicitly at AutoreleasePoolPage::pop(void*) the problem
could be because of creating autoreleasepools on multiple threads and
messing up the stack order, that might be more likely

Now that sounds like it is worth looking into more.


Reply to this email directly or view it on GitHub:
#870 (comment)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 9, 2012

Fair enough :)
So it's a stylistic change, I prefer drain ;-)

I don't really care, I'm fine with drain. I just wanted to make sure you actually caught the bug you wanted to fix.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

I don't really care, I'm fine with drain. I just wanted to make sure
you actually caught the bug you wanted to fix.

Yeah, thanks. If it's related to multithreaded pools and draining the wrong
pool, then I think it's going to be harder to fix than I originally though
:(

In an ideal world we'd start using @autoreleasepool {} blocks, but we
have to look after those 10.6 developers... :P

If you have any suggestions on fixing the bug, then let me know. Since I
can't reproduce I'm not sure where to go from here.

On 9 May 2012 08:56, Henning Jungkurth <
reply@reply.github.com

wrote:

Fair enough :)
So it's a stylistic change, I prefer drain ;-)

I don't really care, I'm fine with drain. I just wanted to make sure you
actually caught the bug you wanted to fix.


Reply to this email directly or view it on GitHub:
#870 (comment)

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 9, 2012

In an ideal world we'd start using @autoreleasepool {} blocks, but we have to look after those 10.6 developers... :P

Would that change anything? I'm not really sure what's the difference between those and old style autoreleasepools.

If you have any suggestions on fixing the bug, then let me know. Since I can't reproduce I'm not sure where to go from here.

Not off the top of my head. But you could send me the crash reports, maybe I can come up with something.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

Would that change anything? I'm not really sure what's the difference between those and old style autoreleasepools.

From what I gather, @autoreleasepool {} blocks are more efficient, but it might be dubious. To be perfectly honest, we should be moving to @autoreleasepools anyway as ARC doesn't support the old type.

I think that in the future we should keep 10.6 support but drop support for compiling on 10.6, but that's a bigger topic requiring more discussion

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented May 9, 2012

I think that in the future we should keep 10.6 support but drop support for compiling on 10.6, but that's a bigger topic requiring more discussion

Meaning I can't compile anymore? That would hamper my development efforts a little. ;-)

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

Meaning I can't compile anymore? That would hamper my development efforts
a little. ;-)

Aye, that's the worry. I was just thinking about that now, and we could
possibly add a compiler #define such as this:

#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
#define ARPStart @autoreleasepool {
#else
#define ARPStart NSAutoreleasePool *pool = [[NSAutoReleasePool alloc] init];
#endif

and

#if MAC_OS_X_VERSION_MAX_ALLOWED >= 1070
#define ARPEnd }
#else
#define ARPEnd [pool drain];
#endif

then we'd replace all NSAutoreleasePool calls with
ARPStart
//do the code
ARPEnd

Don't worry, I've been thinking about you ;)

On 9 May 2012 10:07, Henning Jungkurth <
reply@reply.github.com

wrote:

I think that in the future we should keep 10.6 support but drop support
for compiling on 10.6, but that's a bigger topic requiring more discussion

Meaning I can't compile anymore? That would hamper my development efforts
a little. ;-)


Reply to this email directly or view it on GitHub:
#870 (comment)

@skurfer
Copy link
Member

@skurfer skurfer commented May 9, 2012

Another few crash reports indicated that the defaultSearchSet iVar is never alloc or inited, so was causing a crash when trying to release a non-existent iVar

I’m glad you tracked this down. I had started to see exceptions on that line, but (for me anyway) it was only with the Debug configuration, and only once out of 8 or so runs, so it was low priority to look into.

@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 9, 2012

Since it's so small, I've added another commit here that stops the version and date columns being editable in the plugin prefs as @lovequicksilver said here

skurfer added a commit that referenced this issue May 10, 2012
Replace [pool release] with [pool drain] and alloc an iVar
@skurfer skurfer merged commit 250aa7b into quicksilver:master May 10, 2012
@pjrobertson
Copy link
Member Author

@pjrobertson pjrobertson commented May 11, 2012

FWIW:

allocing and initing the defaultSearchSet object didn't fix the crash. I've seen it for user running 3927. :(

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