views:

1484

answers:

2

I have a method in a UIViewController of my iPhone application (inside a UINavigationController) that is called whenever a row is selected in the table in the ViewController's view. In this method, I access array of "Dream"'s stored in an instance field dreamsArray, which contains NSManagedObjects from my database. I can access objects from this array in other methods, but it seems that whenever I try to retrieve or modify retrieved objects from this array in this particular method, the program crashes.

Here is how dreamsArray is created:

    dreamsArray = [[NSMutableArray alloc] init];

    [self managedObjectContext];

    NSFetchRequest *request = [[NSFetchRequest alloc] init];
    NSEntityDescription *entity = [NSEntityDescription entityForName:@"Dream" inManagedObjectContext:managedObjectContext];
    [request setEntity:entity];

    NSSortDescriptor *sortDescriptor = [[NSSortDescriptor alloc] initWithKey:@"title" ascending:NO];
    NSArray *sortDescriptors = [[NSArray alloc] initWithObjects:sortDescriptor, nil];
    [request setSortDescriptors:sortDescriptors];
    [sortDescriptors release]; [sortDescriptor release];

    NSError *error;
    NSMutableArray *mutableFetchResults = [[managedObjectContext executeFetchRequest:request error:&error] mutableCopy];
    if ( mutableFetchResults == nil )
        NSLog(@"oh noes! no fetch results DreamsTabController:45");

    dreamsArray = [mutableFetchResults mutableCopy];
    [mutableFetchResults release];
    [request release];

An instance in which querying dreamsArray and its objects works:

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    static NSString *cellID = @"Cell";

    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:cellID];
    if ( cell == nil )
        cell = [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleSubtitle reuseIdentifier:cellID] autorelease];

    Dream *dream = (Dream *)[dreamsArray objectAtIndex:indexPath.row];

    cell.textLabel.text = [dream title];
    cell.detailTextLabel.text = @"foo!";

    [dream release];

    return cell;
}

And the method that has all the problems:

- (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath *)indexPath {
    Dream *dream = (Dream *)[dreamsArray objectAtIndex:indexPath.row];
    // BOOM - crashes right here
    EditDreamController *edit = [[EditDreamController alloc] initWithNibName:@"EditDream" bundle:nil];
    edit.dream = [[NSArray alloc] initWithObjects:dream.dreamContent, nil];
    [navigationController pushViewController:edit animated:YES];
    [dream release];
    [edit release];
}

The app crashes immediately after dreamsArray is queried.

Even calling a simple NSLog(@"%@", dream.title) in this method causes a crash. What could be going wrong here?

+7  A: 
Dream *dream = (Dream *)[dreamsArray objectAtIndex:indexPath.row];

cell.textLabel.text = [dream title];
cell.detailTextLabel.text = @"foo!";

[dream release];

You shouldn't be releasing -dream. The array has a hold of it. Same goes for the -tableView:didSelectRowAtIndexPath: method. Most likely, the object has been released enough times as to be deallocated, leaving behind a dangling reference in the array.

End result?

A crash.

Also, your first bit of code has:

dreamsArray = [[NSMutableArray alloc] init];

[self managedObjectContext];

NSFetchRequest *request = [[NSFetchRequest alloc] init];
NSEntityDescription *entity = [NSEntityDescription entityForName:@"Dream" inManagedObjectContext:managedObjectContext];
[request setEntity:entity];

NSSortDescriptor *sortDescriptor = [[NSSortDescriptor alloc] initWithKey:@"title" ascending:NO];
NSArray *sortDescriptors = [[NSArray alloc] initWithObjects:sortDescriptor, nil];
[request setSortDescriptors:sortDescriptors];
[sortDescriptors release]; [sortDescriptor release];

NSError *error;
NSMutableArray *mutableFetchResults = [[managedObjectContext executeFetchRequest:request error:&error] mutableCopy];
if ( mutableFetchResults == nil )
    NSLog(@"oh noes! no fetch results DreamsTabController:45");

dreamsArray = [mutableFetchResults mutableCopy];

There are several bits of confused code here.

(1) Why do you set dreamsArray to be an empty mutable array, then reset it to refer to a mutable copy of the results of the fetch request? You are leaking the empty mutable array.

(2) You call [self managedObjectContext], but don't do anything with the return value. Then you use managedObjectContext directly. Just use [self managedObjectContext] everywhere. The overhead is negligible.

(3) You create a retained fetch request and assign it to request, but never release it. Another memory leak.

(4) Why are you copying the mutableFetchResults twice? That doesn't make any sense (and is leading to another leak).

All in all, I would suggest revisiting the documentation on Objective-C memory management.

bbum
Yes, I think so now, too. This was my first adventure in memory management.. though the data-fetching code was taken from a tutorial. :/
hansengel
+21  A: 

The following code is shorter, more efficient, easier to read, and doesn't have the six or so memory leaks of your first block:

NSFetchRequest *request = [[NSFetchRequest alloc] init];
[request setEntity:[NSEntityDescription entityForName:@"Dream" inManagedObjectContext:[self managedObjectContext]]];

static NSArray *sortDescriptors = nil;
if (!sortDescriptors)
    sortDescriptors = [[NSArray alloc] initWithObject:[[[NSSortDescriptor alloc] initWithKey:@"title" ascending:NO] autorelease]];
[request setSortDescriptors:sortDescriptors];

NSError *error = nil;
NSArray *fetchResults = [managedObjectContext executeFetchRequest:request error:&error];
if (!fetchResults)
    NSLog(@"oh noes! no fetch results DreamsTabController:45, error %@", error);
[request release];

dreamsArray = [NSMutableArray arrayWithArray:fetchResults];

This method is rewritten to be smaller and not over-release dream, which results in crashers:

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath;
{
    static NSString *cellID = @"Cell";

    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:cellID] ? : [[[UITableViewCell alloc] initWithStyle:UITableViewCellStyleSubtitle reuseIdentifier:cellID] autorelease];

    Dream *dream = [dreamsArray objectAtIndex:indexPath.row];
    cell.textLabel.text = dream.title;
    cell.detailTextLabel.text = @"foo!";

    return cell;
}

This method had an over-release of 'dream' and a leak on an NSArray and thus a 'dream' instance, as well.

- (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath *)indexPath;
{
    EditDreamController *editDreamController = [[EditDreamController alloc] initWithNibName:@"EditDream" bundle:nil];

    Dream *dream = [dreamsArray objectAtIndex:indexPath.row];
    editDreamController.dream = [NSArray arrayWithObjects:dream.dreamContent];

    [navigationController pushViewController:editDreamController animated:YES];
    [editDreamController release];
}

It's not clear whey the instance variable on EditDreamController is singular when it takes an array - it should be 'dreams' if you really can set more than one of them.

-Wil

Wil Shipley
Well, gee, wil, if you are just gonna write his code for 'em, how will he ever learn? ;)
bbum
Minor point: I would personally use [[fetchResults mutableCopy] autorelease] rather than [NSMutableArray arrayWithArray:fetchResults] since, in theory, fetchResults could have an optimised mutableCopy method (in future, even if not at present).
Chris Suter
I don't ever use -mutableCopy, as I just don't trust it. One reason is that I can hardly ever remember if 'mutableCopy' is deep or not (it shouldn't be), and if *I* can't remember, how can I expect other people reading my code to remember?Another is because if it WERE optimized in this way, then using it to make fast-enumeration safe (because I'm modifying the base array) would still be unsafe.Also, if the base array was a wonky NSArray subclass, now I know it's just a normal one.
Wil Shipley
The argument that you can't remember whether it's a deep copy is a weak one because you can say the same about arrayWithArray. That's not to take anything away from your more general point: that if you can't easily remember something, you shouldn't do it (e.g. I forget the order of precedence between it's something you should know.
Chris Suter
Also, I don't understand your second point. If you couldn't use an optimised version of mutableCopy to make fast enumeration safe, it would be broken.One possible optimisation that comes to mind is that since fetch results could come from a database, it could be a "lazy" object and making a mutableCopy could perform a faster batch load of all the objects than you might otherwise have.And I'm sure there are others that the talented folks at Apple could come up with.
Chris Suter
I fixed all of the memory management problems that you described, however, the application now crashes after the third line of your tableView:didSelectRowAtIndexPath: function, where the Dream object is fetched from the array. I re-checked that I removed all of my mistakes where I was releasing elements retrieved from this array. If that is not the problem causing the crash, what else could it be?
hansengel
Run Instruments on your program, in Zombies mode. This should point you to what's going wrong, and also functions as a great tutorial on the whole retain/release/autorelease mess.One thing to remember is that usually where you crash with a memory bug isn't where the bug is - you crash when you access something that got released earlier, and often elsewhere.
Wil Shipley
Ah, I found another leak - I ran `dreamsArray = [[NSMutableArray alloc] init]` and later set `dreamsArray = [NSMutableArray arrayWithArray:...`. Even after fixing this, however, it's still crashing at the same spot.I'm not sure what you mean by "Zombie" mode. I ran my Instruments with the Leaks template to find this.. (Sorry for the noobiness, but I couldn't find anything on Google.)
hansengel
Yes! I figured it out. In the last line of my `viewDidLoad` method, I was using the convenience constructor `[NSMutableArray arrayWithArray:]`. I changed this to `[[[NSMutableArray alloc] initWithArray:fetchResults] retain`, and there are no longer crashes when accessing the dreamsArray object. Thank you to everyone for your help :)
hansengel