views:

472

answers:

1

I have created a grid view that displays six "cells" of content. In each cell, an image is loaded from the web. There are a multiple pages of this grid (the user moves through them by swiping up / down to see the next set of cells).

Each cell has its own view controller. When these view controllers load, they use an ImageLoader class that I made to load and display an image. These view controllers implement an ImageLoaderDelegate that has a single method that gets called when the image is finished loading.

ImageLoader does its work on a background thread and then simply notifies its delegate when it is done loading, passing the image to the delegate method.

Trouble is that if the user moves on to the next page of grid content before the image has finished loading (releasing the GridCellViewControllers that use the ImageLoaders), the app crashes. I suspect that this is because along the line, an asynchronous method finishes and attempts to notify its delegate but can't because it's been released.

Here's some code to give a better picture:

GridCellViewController.m:

- (void)viewDidLoad {
    [super viewDidLoad];

    // ImageLoader  
    _loader = [[ProductImageLoader alloc] init];
    _loader.delegate = self;

    if(_boundObject)
        [_loader loadImageForProduct:_boundObject]; 
}

//ImageLoaderDelegate method
- (void) imageDidFinishLoading: (UIImage *)image {
    [_imgController setImage:image];
}

ProductImageLoader.m

- (void) loadImageForProduct: (Product *) product {
    // Get image  on another thread
    [NSThread detachNewThreadSelector:@selector(getImageForProductInBackground:) toTarget:self withObject:product];
}

- (void) getImageForProductInBackground: (Product *) product {
    NSAutoreleasePool *tempPool = [[NSAutoreleasePool alloc] init];

    HttpRequestLoader *tempLoader = [[HttpRequestLoader alloc] init];

    NSURL *tempUrl = [product getImageUrl];

    NSData *imageData = tempUrl ? [tempLoader loadSynchronousDataFromAddress:[tempUrl absoluteString]] : nil;

    UIImage *image = [[UIImage alloc] initWithData:imageData];

    [tempPool release];

    if(delegate)
        [delegate imageDidFinishLoading:image];
}

The app crashes with EXC_BAD_ACCESS.

Disclaimer: The code has been slightly modified to focus on the issue at hand.

+1  A: 

Half the time I ask a question on here, I come up with a solution shortly after. Writing down the issue is probably 90% of the solution.

Anyway, here's what I did, and there is probably an improvement to be made to this as I'm still suspicious of the threads.

As it turns out the problem was that the delegate method would still fire (imageDidFinishLoading), but the delegate had been released. By explicitly setting the delegate to nil in GridCellViewController.m when it is released, the message never fires.

GridCellViewController.m

- (void) dealloc {
    if(_loader) {
        _loader.delegate = nil;
        [_loader release];
    }
}
retailevolved
This is the correct approach. If you set yourself to be the delegate on an object, you should nil out that delegate when you are released.
Mike Weller
Great, thank you. I learn something new every day.
retailevolved
Check out this blog post from Jeff Lamarche: http://iphonedevelopment.blogspot.com/2010/05/downloading-images-for-table-without.html if you're still suspicious of using threads, don't use them. And your app will be better for it ;)
Jasarien
@Mike Weller: s/released/deallocated/
JeremyP
@Jasarien - Good article link. I actually originally intended to use an asynchronous URL connection. The reason I opted not to is that I would have to make two asynchronous calls in a row. What you don't see there is that [product getImageUrl] goes to the web to get the URL. Rather than set up a complicated chain of delegates, I opted for using one delegate. That said, you have made me reconsider my choice :)
retailevolved