How do i resolve EXC_BAD_ACCESS errors encountered in iphone development

Question

I'm trying to do a simple thing; read an image from the internet, save it to the app's documents directory on the iphone, and read it back from that file so that i can do other things with it later. Writing the file works fine but when i try to read it back i get an EXC_BAD_ACCESS error in GDB that i have no idea how to resolve. Here is what my code basically looks like:

-(UIImage *) downloadImageToFile {
NSURL * url = [[NSURL alloc] initWithString: self.urlField.text];

NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);

NSString *documentsDirectory = [paths objectAtIndex:0];

[paths release]
NSString * path = [documentsDirectory stringByAppendingString:@"/testimg.png"];

NSData * data = [[NSData alloc] initWithContentsOfURL:url];

[data writeToFile:path atomically:YES];

return [[UIImage alloc] initWithContentsOfFile:path];
}

The code fails in the return statement when i try to initialize the UIImage from the file. Any ideas?

Edit: neglected to add a release that was the problem in the code originally.

Answer 1

Note: This applies specifically to non-ARC memory management.

Since this has had so many views and the checked answer appropriately states that "code shows a severe lack of knowledge of how memory management works in Objective-C," yet no one has pointed the specific errors out, I figure I'd add an answer that touched on them.

The baseline-level rule that we have to remember about calling methods:

• If the method call includes the words alloc, new, copy, or retain, we have ownership of the object created.¹ If we have ownership of an object, it's our responsibility to release it.

• If the method call does not contain those words, we do not have ownership of the object created.¹ If we don't have ownership of an object, releasing it is not our responsibility, and we therefore should never do it.

Let's look at each line the OP's code:

-(UIImage *) downloadImageToFile {

We started a new method. In doing so we have started a new context in which each of the created objects lives. Keep this in mind for a bit. The next line:

    NSURL * url = [[NSURL alloc] initWithString: self.urlField.text];

We own url: the word alloc there tells us that we have ownership of the object and that we will need to release it ourselves. If we do not then the code will leak memory.

    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);

We do not own paths: no use of the four magic words, so we do not have ownership and must never release it ourselves.

    NSString *documentsDirectory = [paths objectAtIndex:0];

We do not own documentsDirectory: no magic words = no ownership.

    [paths release]

Going back a couple of lines we see that we don't own paths, so this release will cause an EXC_BAD_ACCESS crash as we try to access something that no longer exists.

    NSString * path = [documentsDirectory stringByAppendingString:@"/testimg.png"];

We do not own path: no magic words = no ownership.

    NSData * data = [[NSData alloc] initWithContentsOfURL:url];

We own data: the word alloc there tells use that we have ownership of the object and that we will need to release it ourselves. If we do not then the code will leak memory.

The following two lines don't create or release anything. Then comes the last line:

}

The method is over, so the context for the variables has ended. Looking at the code we can see that we owned both url and data, but didn't release either of them. As a result our code will leak memory every time this method is called.

The NSURL object url isn't very big, so it's possible that we might never notice the leak, though it should still be cleaned up, there's no reason to leak it.

The NSData object data is a png image, and could be very large; we are leaking the entire size of the object every time this method is called. Imagine this was called every time a table cell was drawn: it wouldn't take long at all to crash the whole app.

So what do we need to do to fix the problems? It's pretty simple, we simply need to release the objects as soon as we no longer need them, commonly right after the last time they're used:

-(UIImage *) downloadImageToFile {

    // We own this object due to the alloc
    NSURL * url = [[NSURL alloc] initWithString: self.urlField.text];

    // We don't own this object
    NSArray *paths = NSSearchPathForDirectoriesInDomains(NSDocumentDirectory, NSUserDomainMask, YES);

    // We don't own this object
    NSString *documentsDirectory = [paths objectAtIndex:0];

    //[paths release] -- commented out, we don't own paths so can't release it

    // We don't own this object
    NSString * path = [documentsDirectory stringByAppendingString:@"/testimg.png"];

    // We own this object due to the alloc
    NSData * data = [[NSData alloc] initWithContentsOfURL:url];

    [url release]; //We're done with the url object so we can release it

    [data writeToFile:path atomically:YES];

    [data release]; //We're done with the data object so we can release it

    return [[UIImage alloc] initWithContentsOfFile:path];

    //We've released everything we owned so it's safe to leave the context
}

Some people prefer to release everything at once, right before the context closes at the end of the method. In that case both [url release]; and [data release]; would appear right before the closing } brace. I find that if I release them as soon as I can the code is clearer, making it clear when I go over it later exactly where I'm done with objects.

To summarize: we own objects created with alloc, new, copy, or retain in the method calls so must release them before the context ends. We don't own anything else and must never release them.

---

¹There's nothing actually magical in the four words, they're just a consistently-used reminder by the folks at Apple who created the methods in question. If we create our own initialization or copying methods for a class of our own then the inclusion of the words alloc, new, copy, or retain in its appropriate methods is our responsibility, and if we don't use them in our names then we'll need to remember for ourselves whether ownership has passed.