1

I am using Three20/TTThumbsviewcontroller to load photos. I am struggling since quite a some time now to fix memory leak in setting photosource. I am beginner in Object C & iOS memory management. Please have a look at following code and suggest any obvious mistakes or any errors in declaring and releasing variables.

-- PhotoViewController.h

 @interface PhotoViewController : TTThumbsViewController <UIPopoverControllerDelegate,CategoryPickerDelegate,FilterPickerDelegate,UISearchBarDelegate>{
......
NSMutableArray *_photoList;

......
@property(nonatomic,retain) NSMutableArray *photoList;

-- PhotoViewController.m

@implementation PhotoViewController
....

@synthesize photoList;
.....

- (void)LoadPhotoSource:(NSString *)query:(NSString *)title:(NSString* )stoneName{


NSLog(@"log- in loadPhotosource method");


if (photoList == nil)
    photoList = [[NSMutableArray alloc] init ];

[photoList removeAllObjects];



@try {
    sqlite3 *db;
    NSFileManager *fileMgr = [NSFileManager defaultManager];
    NSString* documentsPath = [NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES) objectAtIndex:0];

    NSString *dbPath = [documentsPath stringByAppendingPathComponent: @"DB.s3db"];
    BOOL success = [fileMgr fileExistsAtPath:dbPath];
    if(!success)
    {
        NSLog(@"Cannot locate database file '%@'.", dbPath);
    }

    if(!(sqlite3_open([dbPath UTF8String], &db) == SQLITE_OK))
    {
        NSLog(@"An error has occured.");
    }


    NSString *_sql = query;
    const char *sql = [_sql UTF8String];
    sqlite3_stmt *sqlStatement;
    if(sqlite3_prepare(db, sql, -1, &sqlStatement, NULL) != SQLITE_OK)
    {
        NSLog(@"Problem with prepare statement");
    }

    if ([stoneName length] != 0)
    {
        NSString *wildcardSearch = [NSString stringWithFormat:@"%@%%",[stoneName stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]]];

        sqlite3_bind_text(sqlStatement, 1, [wildcardSearch UTF8String], -1, SQLITE_STATIC);
    }


    while (sqlite3_step(sqlStatement)==SQLITE_ROW) {

        NSString* urlSmallImage = @"Mahallati_NoImage.png";
        NSString* urlThumbImage = @"Mahallati_NoImage.png";


        NSString *designNo = [NSString stringWithUTF8String:(char *) sqlite3_column_text(sqlStatement,2)];
        designNo = [designNo stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

        NSString *desc = [NSString stringWithUTF8String:(char *) sqlite3_column_text(sqlStatement,7)];
        desc = [desc stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

        NSString *caption = designNo;//[designNo stringByAppendingString:desc];
        caption = [caption stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];



        NSString *smallFilePath = [documentsPath stringByAppendingPathComponent: [NSString stringWithFormat:@"Small%@.JPG",designNo] ];
        smallFilePath = [smallFilePath stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
        if ([fileMgr fileExistsAtPath:smallFilePath]){
            urlSmallImage = [NSString stringWithFormat:@"Small%@.JPG",designNo];
        }

        NSString *thumbFilePath = [documentsPath stringByAppendingPathComponent: [NSString stringWithFormat:@"Thumb%@.JPG",designNo] ];
        thumbFilePath = [thumbFilePath stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

        if ([fileMgr fileExistsAtPath:thumbFilePath]){
            urlThumbImage = [NSString stringWithFormat:@"Thumb%@.JPG",designNo];
        }




        NSNumber *photoProductId = [NSNumber numberWithInt:(int)sqlite3_column_int(sqlStatement, 0)];
        NSNumber *photoPrice = [NSNumber numberWithInt:(int)sqlite3_column_int(sqlStatement, 6)];

        char *productNo1 = sqlite3_column_text(sqlStatement, 3);
        NSString* productNo;
        if (productNo1 == NULL)
            productNo = nil;
        else
            productNo = [NSString stringWithUTF8String:productNo1];

        Photo *jphoto = [[[Photo alloc] initWithCaption:caption
                                               urlLarge:[NSString stringWithFormat:@"documents://%@",urlSmallImage] 
                                               urlSmall:[NSString stringWithFormat:@"documents://%@",urlSmallImage] 
                                               urlThumb:[NSString stringWithFormat:@"documents://%@",urlThumbImage]
                                                   size:CGSizeMake(123, 123) 
                                              productId:photoProductId
                                                  price:photoPrice
                                            description:desc
                                               designNo:designNo 
                                              productNo:productNo
                          ]  autorelease];



        [photoList addObject:jphoto];
        [jphoto release];



    }


}
@catch (NSException *exception) {
    NSLog(@"An exception occured: %@", [exception reason]);
}



self.photoSource = [[[MockPhotoSource alloc]
                     initWithType:MockPhotoSourceNormal
                     title:[NSString stringWithFormat: @"%@",title]
                     photos: photoList
                     photos2:nil] autorelease];

}

Memory leaks happen when calling above LoadPhotosource method again with different query... I feel its something wrong in declaring NSMutableArray (photoList), but can't figure out how to fix memory leak. Any suggestion is really appreciated.

Rob
  • 415,655
  • 72
  • 787
  • 1,044
Davin
  • 105
  • 12
  • I don't think it's related, but I think you want `@synthesize photoList = _photoList`. Right now, you have two ivars, the one you declared explicitly, `_photoList`, and one that is created implicitly from your photoList property, `photoList`. – Rob Jun 17 '12 at 15:47
  • Also, again unrelated, but I don't understand how jphoto is not over released, because you're issuing both an `autorelease` as well as a `release`. – Rob Jun 17 '12 at 15:48
  • Have you run this through the leaks tool in the profiler? This will tell you precisely which object is being leaked, which makes this diagnosis a lot easier. – Rob Jun 17 '12 at 15:50
  • 1
    Also, I don't see your `sqlite3_finalize()` and `sqlite3_close()` statements. Where are they? According to the [docs](http://www.sqlite.org/cintro.html), "Every prepared statement must be destroyed using a call to [`sqlite3_finalize()`] in order to avoid memory leaks." – Rob Jun 17 '12 at 16:01

2 Answers2

2

I havent seen any leaked objects in your code, but actually a double released object that will eventually crash your app:

In your last lines, you have this:

Photo *jphoto = [[[Photo alloc] initWithCaption:caption
                                               urlLarge:[NSString stringWithFormat:@"documents://%@",urlSmallImage] 
                                               urlSmall:[NSString stringWithFormat:@"documents://%@",urlSmallImage] 
                                               urlThumb:[NSString stringWithFormat:@"documents://%@",urlThumbImage]
                                                   size:CGSizeMake(123, 123) 
                                              productId:photoProductId
                                                  price:photoPrice
                                                description:desc
                          designNo:designNo 
                                              productNo:productNo
                          ]  autorelease];



        [photoList addObject:jphoto];
        [jphoto release];

If you take a closer look, you have a double release (autorelease in alloc and release after the addObject). You should remove one of them.

Besides that, I would also recommend you a few things:

  1. Switch to ARC: If you are new, you will definetly take advantage of it because you will not have to worry about almost any of the memory management anymore. Your code will be free of leaked objects and memory crashes. Just be careful with the memory retain cycles and with variable ownerships and you are done.
  2. Remove NSMutableArray *_photoList;, you are not using it since you declare your property syntetizer as @synthesize photoList;. In addition, as Robert commented below, you should use this form to clearly differentiate when you use the property: @synthesize photoList = photoList_; The underscore is better at the end because Apple uses it at the beginning and you dont want to accidentally use the same variable name.

  3. Use property accessors to manage ivars. Instead of doing this:

    if (photoList == nil) photoList = [[NSMutableArray alloc] init ];

try this:

if (photoList == nil)
    self.photoList = [[[NSMutableArray alloc] init] autorelease];

In your example both lines have the same behaviour, but the second one is preferable because it will rely on your variable memory policy. If you change your retain by a copy or an assign one day, your code will still work without any problem. In the same way, release your memory by assigning your ivar a nil like this self.photoList = nil

Why do you think you have a leaked object?

Angel G. Olloqui
  • 8,045
  • 3
  • 33
  • 31
  • 1
    In addition to your suggestion of removing the _photoList explicit declaration, I think he should change his synthesize statement to `@synthesize photoList = _photoList` so that you can always be clear when he's accessing the ivar directly, and when he's using the property's getters and setters. – Rob Jun 17 '12 at 15:54
  • Angel & Robert. Thanks a lot guys, you are amazing. I followed both of your suggestion and there is no memory leak in LoadPhotoSource method anymore. I didn't realize there should be only one release or autorelease in case of jPhoto. I removed NSMutableArray *_photoList too. Thanks for detailed explanation and suggestion. – Davin Jun 17 '12 at 16:22
  • Your welcome. Then, maybe you were not asking by a memory leak, which is memory not released in your code, but by a memory crash due to an over released object. – Angel G. Olloqui Jun 17 '12 at 16:27
  • @AngelGarcíaOlloqui actually in profile it was showing memory leak in LoadPhotosource method and in Ipad1 it crashed but worked fine in ipad2. I think it caused due to to photoList and over released of jphoto too. Anyway its not showing memory leak in profile anymore and I will try installing in iPad1 and 2, hopefully it will work. so, if I synthesize the property I don't need to declare in hidden file as _photoList... I will follow this advice otherwise too. Thanks – Davin Jun 17 '12 at 17:14
0

Just to recap on my observations:

  1. I don't think it's related, but I think you want @synthesize photoList = _photoList. Right now, you have two ivars, the one you declared explicitly, _photoList, and one that is created implicitly from your photoList property, photoList. I'd also get rid of the existing explicit declaration of the _photoList ivar, as Apple current advises that you let the synthesize statement do that for you. (And it prevents situations like this, where you ended up with additional ivars accidentally.)

  2. Also, again unrelated to a leak (but rather the converse problem), but jphoto is being over released, because you're issuing both an autorelease as well as a release. The latter is sufficient.

  3. Also, I don't see your sqlite3_finalize() and sqlite3_close() statements. Where are they? According to the docs, "Every prepared statement must be destroyed using a call to [sqlite3_finalize()] in order to avoid memory leaks."

  4. Finally, if you still have a leak after adding the finalize/close statements for your database, I'd suggest running your code through the leaks tool in the profiler to find the leak. This will help you identify precisely what's leaking.

Rob
  • 415,655
  • 72
  • 787
  • 1,044
  • Thanks a lot. You are right. It fixes memory leak issues and I will close connection also. – Davin Jun 18 '12 at 10:48