-
Notifications
You must be signed in to change notification settings - Fork 85
Helpers for Darwin Autorelease stuff #1776
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
base: master
Are you sure you want to change the base?
Conversation
|
One thing that I found confusing or non-obvious, is that this code crashes (but it also crashes if I use {
hsAutoreleasingScope;
NSString* s = [NSString stringWithFormat:@"Hello %s", "world"];
hsAutorelease(s);
}I guess the EDIT: Okay, I looked into this further and apparently |
Sources/Plasma/CoreLib/hsDarwin.h
Outdated
| SEL autorelease = sel_registerName("autorelease"); | ||
| IMP imp = class_getMethodImplementation(object_getClass(bridge_cast<id>(const_cast<void*>(ptr))), autorelease); | ||
| ((CFTypeRef (*)(CFTypeRef, SEL))imp)(ptr, autorelease); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is jumping through several hoops to void the case where ARC tries to be overly helpful and causes double-free issues: https://www.mikeash.com/pyblog/friday-qa-2014-05-09-when-an-autorelease-isnt.html
The rule is if the function starts with init/new/copy (working from memory here) it's retained. If it doesn't, it's autoreleased. |
Autorelease pools were not supposed to be part of CoreFoundation. CoreFoundation shipped on Mac OS 8 under Carbon - before Cocoa - so it's not 1:1. Autorelease pools were a uniquely Cocoa concept. Casting to an NSObject was usually the best way this was handled. But pure CoreFoundation code should probably avoid Autorelease pools. |
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
| TEST(hsDarwin_Foundation, converts_to_ST_string) | ||
| { | ||
| NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init]; | ||
| hsAutoreleasingScope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option with this scope type is we could just turn off ARC in places where the code needs to be shared. I'm not sure what your plan is with the client in since that targets a newer SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the client, but I was thinking this would replace the hacking around pools I did in hsMessageBox
This is a bit outside the scope of this PR, but trying to wrap my head about this: Does our |
8bff8ee to
f3b1568
Compare
No - ARC will automatically apply the autorelease. ARC reads the function names and automatically applies the correct ruleset. When calling ARC from ARC it doesn't really matter, the rule set application is only done so non-ARC code can call ARC code and get the matching behavior. Autorelease should never be called directly on an Obj-C type in ARC. Hacking around that with CoreFoundation will lead to a bad time. |
|
I'm going to do a deeper review on this tonight, leaving a few comments in the meantime... |
What about non-ARC because it sounds like With |
Both should return an autoreleased reference in since neither function starts with the magic init/create/copy words. For completeness I looked it up here:
|
f3b1568 to
ec66a26
Compare
Okay, so in ARC this already worked because we return a |
dgelessus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @colincornaby that if we can completely avoid autoreleases in the non-Objective-C code, we should do that rather than introducing a custom compatibility/abstraction layer like this. (In that case, most of my comments below would become obsolete.)
But assuming that we do want this abstraction: the autoreleasepool code could be simplified a lot by implementing it in Objective-C. This would require making the functions not inline and moving the implementation out of the header, so that the header remains plain C++. (That may reduce performance, but if you're creating autoreleasepools in a tight loop, you're doing something wrong.)
e83cc16 to
9d9e1ec
Compare
This tests both the "native" password store as well as the fallback file-based one.
9d9e1ec to
5f60239
Compare
| + (id)stringWithSTString:(const ST::string&)string | ||
| { | ||
| return NSStringCreateWithSTString(string); | ||
| return hsAutorelease(NSStringCreateWithSTString(string)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards this file just turning off ARC instead of adding hsAutorelease.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes look good.
In older version of Cocoa with manual memory management, you'd create
NSAutoreleasePool*objects in code and drain the pool at the end of the function/loop/whatever. You'd call[obj autorelease]to associate an object with a pool and it would automatically be destroyed when the pool was cleared.In mac OS X 10.7, Apple introduced Automatic Reference Counting which forbids calling
[obj autorelease]in Objective-C and also forbids creatingNSAutoreleasePoolobjects manually. Instead, syntax sugar was introduced to the Clang compiler to automatically add autorelease pools using@autoreleasepool { }blocks. Internally, Clang turns these into calls toobjc_autoreleasePoolPush()andobjc_autoreleasePoolPop().That left CoreFoundation objects (which are C API but also bridged with Objective-C) out of participating in autorelease pools, so in OS X 10.9 they introduced
CFAutorelease()for CF objects.To this day, you have the choice of compiling with Automatic Reference Counting or without Automatic Reference Counting for Objective-C objects, so we need to handle both cases.
This PR introduces:
bridge_cast<T>()to cast to Objective-C types (without changing the reference count if compiled with ARC)hsAutorelease()to call eitherCFAutorelease()or[obj autorelease](or nothing, in ARC mode) depending on the object type, with a polyfill forCFAutorelease()for older OS X versionshsAutoreleasePoolas a RAII type that will push an autorelease pool when it is created and drain the pool when it leaves scopehsAutoreleasingScopeas a quick way to create an anonymoushsAutoreleasePoolfor the duration of a blockIt's somewhat weird to me that
CFAutoreleasewas introduced, but there is no public documented API for creating autorelease pools in C. Supposedly there are_CFAutoreleasePoolPush()and_CFAutoreleasePoolPop()private functions that just call the Clang helpers directly, but they're also only available from OS X 10.7 onwards, so I opted to just useobjc_msgSendto create anNSAutoreleasePoolif we're on an older version or not using a compiler with the syntax sugar.