Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834102:
mac: Let gcapi install chrome as the CGSession owner (Closed)
|
Created:
8 years, 4 months ago by Nico Modified:
8 years, 4 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionmac: Let gcapi install chrome as the CGSession owner
This way, chrome should be able to set up user keystone
on first run in most cases.
Also don't offer installation if /Applications/Google Chrome.app already
exists.
BUG=128462
TEST=
1.) Run as user.
Have chrome with system ticket and user keystone installed anywhere.
-> Shouldn't offer installation.
Have chrome with user ticket and user keystone installed anywhere.
-> Shouldn't offer installation.
Have chrome with system ticket and system keystone installed anywhere.
-> Shouldn't offer installation.
Have chrome with system ticket and system keystone installed anywhere.
-> Shouldn't offer installation.
Have chrome in /Applications, no matter owned by whom and no matter
if with keystone set up.
-> Shouldn't offer installation.
Don't have chrome installed
-> Offers to install, and creates /Applications/Google Chrome.app which
is owned by the current user. On first launch, Chrome registers itself
successfully for a user ticket without user interaction.
2.) Same as with 1, but run as root. Should have EXACTLY the same results,
i.e. chrome is still installed as current user, and when run as the current
user (not root) successfully registers itself.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151324
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : . #Patch Set 4 : foo #
Total comments: 25
Patch Set 5 : comments1 #Patch Set 6 : comments2 #
Total comments: 20
Patch Set 7 : . #
Total comments: 6
Patch Set 8 : . #
Total comments: 2
Patch Set 9 : . #
Messages
Total messages: 12 (0 generated)
I have a few questions that I have marked with XXX. The interesting questions are: 1.) Does ksadmin need to be invoked with `sudo -u consoleuser` when asking user keystone and running euid? (My guess is no, user ksadmin shouldn't be confused by running seteuid) 2.) Do we still want to try and write brand code and master prefs to a system location first? I'm not sure about this one, but deleting that code is easy.
https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... File chrome/installer/gcapi_mac/OWNERS (right): https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/OWNERS:2: thakis@chromium.org That’s nice, git seems to think this is a copy. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:20: NSString* const kSystemBrandPath = @"/Library/Google/Google Chrome Brand.plist"; Provided we’re just letting Chrome set up a user ticket, we no longer need to bother with system brand file locations at all. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:74: CFNumberRef userUID = (CFNumberRef)CFDictionaryGetValue(sessionInfoDict, Can you use something like CFCast here? https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:91: NSString* homeDir = [NSString stringWithUTF8String:homedir]; Naming nit: home_dir https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:92: return [s stringByReplacingOccurrencesOfString:@"~~" I’d be a little more comfortable if this only operated on ~~ at the beginning of the string. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:98: NSMutableArray* keystonePaths = Naming nit: keystone_paths. Line 109 too. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:112: // XXX does this need to run as other user? or can admin read the user store? What do you mean? That you want to check the console user’s Keystone tickets from some other UID? https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:166: // Do set permissions explicitly; for admin paths all admins can write, for You haven’t made this so that all admins can write to admin paths. Not clear if you intended to based on this comment now. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:194: // XXX do we still want to try to write this to a system path? probably? Brand file, no. The brand file should only be written where the ticket is written. Master prefs file, sure, you can write that to a system path. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:279: local_reasons |= GCCC_ERROR_ACCESSDENIED; Is this the right “reason” or should you add a new “reason” to cover this?
https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:112: // XXX does this need to run as other user? or can admin read the user store? On 2012/08/13 13:14:42, Mark Mentovai wrote: > What do you mean? That you want to check the console user’s Keystone tickets > from some other UID? Right, I imagine this code will usually run with euid 0, but I want to check for the console user's user tickets, not for root's user tickets.
https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:20: NSString* const kSystemBrandPath = @"/Library/Google/Google Chrome Brand.plist"; On 2012/08/13 13:14:42, Mark Mentovai wrote: > Provided we’re just letting Chrome set up a user ticket, we no longer need to > bother with system brand file locations at all. Done. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:91: NSString* homeDir = [NSString stringWithUTF8String:homedir]; On 2012/08/13 13:14:42, Mark Mentovai wrote: > Naming nit: home_dir Done. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:92: return [s stringByReplacingOccurrencesOfString:@"~~" On 2012/08/13 13:14:42, Mark Mentovai wrote: > I’d be a little more comfortable if this only operated on ~~ at the beginning of > the string. Done. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:98: NSMutableArray* keystonePaths = On 2012/08/13 13:14:42, Mark Mentovai wrote: > Naming nit: keystone_paths. Line 109 too. Done. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:166: // Do set permissions explicitly; for admin paths all admins can write, for On 2012/08/13 13:14:42, Mark Mentovai wrote: > You haven’t made this so that all admins can write to admin paths. Not clear if > you intended to based on this comment now. For admins, the group is set to wheel and permissions are 755, so this should work (?) https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:194: // XXX do we still want to try to write this to a system path? probably? On 2012/08/13 13:14:42, Mark Mentovai wrote: > Brand file, no. The brand file should only be written where the ticket is > written. > > Master prefs file, sure, you can write that to a system path. Done. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:279: local_reasons |= GCCC_ERROR_ACCESSDENIED; On 2012/08/13 13:14:42, Mark Mentovai wrote: > Is this the right “reason” or should you add a new “reason” to cover this? How would the new reason be called? Or should there be just a generic ALREADYPRESENT that's returned for user/system/this case?
https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:112: // XXX does this need to run as other user? or can admin read the user store? Nico wrote: > On 2012/08/13 13:14:42, Mark Mentovai wrote: > > What do you mean? That you want to check the console user’s Keystone tickets > > from some other UID? > > Right, I imagine this code will usually run with euid 0, but I want to check for > the console user's user tickets, not for root's user tickets. Root will be able to read a user store from a filesystem permission perspective (except for corner cases like FileVault 1), BUT: How will ksadmin know which user store to read? If you run ksadmin as root, it shows you the system store (even if you tell it to do --user-store). To check the user store, you want this to check the console user’s store, don’t you? https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:166: // Do set permissions explicitly; for admin paths all admins can write, for Nico wrote: > On 2012/08/13 13:14:42, Mark Mentovai wrote: > > You haven’t made this so that all admins can write to admin paths. Not clear > if > > you intended to based on this comment now. > > For admins, the group is set to wheel and permissions are 755, so this should > work (?) How do you figure? Admins aren’t a member of group wheel and in any case, you haven’t set the group write bit. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:279: local_reasons |= GCCC_ERROR_ACCESSDENIED; Nico wrote: > On 2012/08/13 13:14:42, Mark Mentovai wrote: > > Is this the right “reason” or should you add a new “reason” to cover > this? > > How would the new reason be called? > > Or should there be just a generic ALREADYPRESENT that's returned for > user/system/this case? Yeah, that’s probably best. ACCESSDENIED doesn’t necessarily fit this case at all, absent any test to see if it’s writeable—that’s overkilll, since we don’t want to overwrite it if it’s present.
https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:74: CFNumberRef userUID = (CFNumberRef)CFDictionaryGetValue(sessionInfoDict, On 2012/08/13 13:14:42, Mark Mentovai wrote: > Can you use something like CFCast here? Doneish. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:112: // XXX does this need to run as other user? or can admin read the user store? On 2012/08/13 15:04:34, Mark Mentovai wrote: > Nico wrote: > > On 2012/08/13 13:14:42, Mark Mentovai wrote: > > > What do you mean? That you want to check the console user’s Keystone tickets > > > from some other UID? > > > > Right, I imagine this code will usually run with euid 0, but I want to check > for > > the console user's user tickets, not for root's user tickets. > > Root will be able to read a user store from a filesystem permission perspective > (except for corner cases like FileVault 1), BUT: > > How will ksadmin know which user store to read? If you run ksadmin as root, it > shows you the system store (even if you tell it to do --user-store). To check > the user store, you want this to check the console user’s store, don’t you? Done. https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:166: // Do set permissions explicitly; for admin paths all admins can write, for On 2012/08/13 15:04:34, Mark Mentovai wrote: > Nico wrote: > > On 2012/08/13 13:14:42, Mark Mentovai wrote: > > > You haven’t made this so that all admins can write to admin paths. Not clear > > if > > > you intended to based on this comment now. > > > > For admins, the group is set to wheel and permissions are 755, so this should > > work (?) > > How do you figure? Admins aren’t a member of group wheel and in any case, you > haven’t set the group write bit. Done. (?) https://chromiumcodereview.appspot.com/10834102/diff/5001/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:279: local_reasons |= GCCC_ERROR_ACCESSDENIED; On 2012/08/13 15:04:34, Mark Mentovai wrote: > Nico wrote: > > On 2012/08/13 13:14:42, Mark Mentovai wrote: > > > Is this the right “reason” or should you add a new “reason” to > cover > > this? > > > > How would the new reason be called? > > > > Or should there be just a generic ALREADYPRESENT that's returned for > > user/system/this case? > > Yeah, that’s probably best. > > ACCESSDENIED doesn’t necessarily fit this case at all, absent any test to see if > it’s writeable—that’s overkilll, since we don’t want to overwrite it if it’s > present. Done.
https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.h (right): https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.h:10: #define GCCC_ERROR_ACCESSDENIED (1 << 2) You can renumber these now. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:76: return NULL; But you haven’t released sessionInfoDict (which should be session_info_dict anyway) and thus leak it. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:108: id ksPath; Naming again. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:115: // XXX does this need to run as other user? or can admin read the user store? This XXX is now obsolete? https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:129: arguments = [ Weird indentation. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:182: // XXX do this? not needed i think? Right, shouldn’t be needed. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:222: // files are created with group of parent dir (good), owner of euid (good). You have no assurance what the group of the parent directory is, though. You only have this assurance if you created it in CreatePathToFile, but if it already existed, it could be anything. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:293: if (!local_reasons && [[NSFileManager defaultManager] Why are you checking !local_reasons here, when in other cases you let the different reasons accumulate? You should explain the rationale in a comment, since it’s not really obvious to me. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:337: @[@"-u", run_as, install_script, app_path, kChromeInstallPath]]; But if run_as is not an admin user, it won’t be able to make the app admin-writable at all. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:372: //passwd* user = GetRealUserId(); Not needed?
https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.h (right): https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.h:10: #define GCCC_ERROR_ACCESSDENIED (1 << 2) On 2012/08/13 15:59:25, Mark Mentovai wrote: > You can renumber these now. I guess nobody links to gcapi in dylib form. Done. (Nobody links to gcapi yet at all :-P) https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:76: return NULL; On 2012/08/13 15:59:25, Mark Mentovai wrote: > But you haven’t released sessionInfoDict (which should be session_info_dict > anyway) and thus leak it. Done. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:108: id ksPath; On 2012/08/13 15:59:25, Mark Mentovai wrote: > Naming again. Done. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:115: // XXX does this need to run as other user? or can admin read the user store? On 2012/08/13 15:59:25, Mark Mentovai wrote: > This XXX is now obsolete? Done. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:129: arguments = [ On 2012/08/13 15:59:25, Mark Mentovai wrote: > Weird indentation. Made it weird in a different way. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:222: // files are created with group of parent dir (good), owner of euid (good). On 2012/08/13 15:59:25, Mark Mentovai wrote: > You have no assurance what the group of the parent directory is, though. You > only have this assurance if you created it in CreatePathToFile, but if it > already existed, it could be anything. The directory is /Library/Google -- if that exist and isn't owned by group admin, I suppose it doesn't really matter if the master prefs file is? I can change it if you like though. (I'd call getgrnam() to get the gid of admin and then chown(geteuid(), admin_gid), so if you like me to change this and don't like the code I'd write you can comment on that code) https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:293: if (!local_reasons && [[NSFileManager defaultManager] On 2012/08/13 15:59:25, Mark Mentovai wrote: > Why are you checking !local_reasons here, when in other cases you let the > different reasons accumulate? You should explain the rationale in a comment, > since it’s not really obvious to me. Done. I thought that ACCESSDENIED shouldn't be set if one of the ALREADYPRESENTS is already there, but now that this is ALREADPRESENT too it no longer makes sense. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:337: @[@"-u", run_as, install_script, app_path, kChromeInstallPath]]; On 2012/08/13 15:59:25, Mark Mentovai wrote: > But if run_as is not an admin user, it won’t be able to make the app > admin-writable at all. Right, the new approach won't be able to install chrome for users who can't write to /Applications. That's why the compatibility check checks if it can write to /Applications. https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:372: //passwd* user = GetRealUserId(); On 2012/08/13 15:59:25, Mark Mentovai wrote: > Not needed? Done.
https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/1005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:222: // files are created with group of parent dir (good), owner of euid (good). Nico wrote: > On 2012/08/13 15:59:25, Mark Mentovai wrote: > > You have no assurance what the group of the parent directory is, though. You > > only have this assurance if you created it in CreatePathToFile, but if it > > already existed, it could be anything. > > The directory is /Library/Google -- if that exist and isn't owned by group > admin, I suppose it doesn't really matter if the master prefs file is? I can > change it if you like though. (I'd call getgrnam() to get the gid of admin and > then chown(geteuid(), admin_gid), so if you like me to change this and don't > like the code I'd write you can comment on that code) If /Library/Google exists and isn’t group admin, the master prefs file can still be made group admin and admin-writeable. getgrnam and chown is fine. Optionally, you can write a constant 0 inline instead of geteuid() in this case if you know you’re always running as uid 0 here. https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:70: //[reinterpret_cast<NSDictionary*>(session_info_dict) autorelease]; How is this supposed to work when it’s commented out like this? https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:127: arguments = [@[@"-u", run_as, ks_path] The indentation is still weird here. In case it wasn’t clear, I’m complaining that the 'a' in arguments is one space in from the '[' on the line above. https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:270: NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init]; You used @autoreleasepool elsewhere in this file. You can do that here too.
https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:70: //[reinterpret_cast<NSDictionary*>(session_info_dict) autorelease]; On 2012/08/13 18:43:24, Mark Mentovai wrote: > How is this supposed to work when it’s commented out like this? I had a vague memory that I wanted to do something before uploading again :-D Done. (Went with a c-style cast, as c++ casts need both reinterpret_cast and const_cast) https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:127: arguments = [@[@"-u", run_as, ks_path] On 2012/08/13 18:43:24, Mark Mentovai wrote: > The indentation is still weird here. > > In case it wasn’t clear, I’m complaining that the 'a' in arguments is one space > in from the '[' on the line above. Oh, you meant the "arguments" line, not the continuation. Done. https://chromiumcodereview.appspot.com/10834102/diff/3003/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:270: NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init]; On 2012/08/13 18:43:24, Mark Mentovai wrote: > You used @autoreleasepool elsewhere in this file. You can do that here too. Done.
LGTM with this one last change. I think that’s about as productive as we can be in this changelist without crossing over the “diminishing returns” line, time to let you test and deal with more fallout in a separate change. https://chromiumcodereview.appspot.com/10834102/diff/5005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:71: [(NSDictionary*)session_info_dict autorelease]; This might run under GC. Might not, but also might. We’ve got no control over that. If it runs in a GC environment, you need to call NSMakeCollectable(session_info_dict), otherwise it’s a leak (the autorelease won’t do anything). See CFTypeRefToNSObjectAutorelease from base/mac/foundation_util.mm.
Thanks! https://chromiumcodereview.appspot.com/10834102/diff/5005/chrome/installer/gc... File chrome/installer/gcapi_mac/gcapi.mm (right): https://chromiumcodereview.appspot.com/10834102/diff/5005/chrome/installer/gc... chrome/installer/gcapi_mac/gcapi.mm:71: [(NSDictionary*)session_info_dict autorelease]; On 2012/08/13 18:59:51, Mark Mentovai wrote: > This might run under GC. Might not, but also might. We’ve got no control over > that. > > If it runs in a GC environment, you need to call > NSMakeCollectable(session_info_dict), otherwise it’s a leak (the autorelease > won’t do anything). > > See CFTypeRefToNSObjectAutorelease from base/mac/foundation_util.mm. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
