Xcode - iOS Memory leak that is driving me crazy I think it's NSNotificationCenter, but hoping fresh eyes can see what I can't

Question

The code here is a modal view launched from the RootViewController and is to display a video with a thumbnail filmstrip below the movie and then timed instructions bound to the movie.

It all works, but there is a memory leak / lack of release that I just can't see for looking and having spent three days trying to fix it, the time has come to ask for help...

If I disable the NSNotificationCenter by commenting it out (highlighted in the .m) I don't have any issues regarding memory and keep the timed text. But I also don't have any thumbnails. I have tried inserting [[NSNotificationCenter alloc] removeObserver:self]; at numerous points to see if that will get rid of it for me. But alas, to no avail.

I have also tried releasing the 'backgroundTimer' but it's not overly impressed when I try to compile and run.

In essence, the first time I load the modal view, there are no issues whatsoever and all seems great - However, if I close it with the -(IBAction)close:(id)sender; it seems something isn't releasing as the next time I launch the same page the memory usage increases by about 30% (roughly the amount that is used by the thumbnail generation) and increases by roughly the same amount each time I re-launch the modal view.

Please bare in mind I am a newbie to this and the error is likely to a bloody stupid one to those of you in the know. But in the interest of getting this project done, I'll gladly take any abuse you fancy throwing at me.

Here's the code:

.h

#import <UIKit/UIKit.h>
#import <MediaPlayer/MPMoviePlayerController.h>
#import "ImageViewWithTime.h"
#import "CommentView.h"

@interface SirloinVideoViewController_iPad : UIViewController {
    UIView *landscapeView;
    UIView *viewForMovie;
    MPMoviePlayerController *player;
    UILabel *onScreenDisplayLabel;
    UIScrollView *myScrollView;
    NSMutableArray *keyframeTimes;
    NSArray *shoutOutTexts;
    NSArray *shoutOutTimes;
    NSTimer *backgroundTimer;
    UIView *instructions;
}

-(IBAction)close:(id)sender;
-(IBAction)textInstructions:(id)sender;

@property (nonatomic, retain) IBOutlet UIView *instructions;
@property (nonatomic, retain) NSTimer *theTimer;
@property (nonatomic, retain) NSTimer *backgroundTimer;
@property (nonatomic, retain) IBOutlet UIView *viewForMovie;
@property (nonatomic, retain) MPMoviePlayerController *player;
@property (nonatomic, retain) IBOutlet UILabel *onScreenDisplayLabel;
@property (nonatomic, retain) IBOutlet UIScrollView *myScrollView;
@property (nonatomic, retain) NSMutableArray *keyframeTimes;

-(NSURL *)movieURL;
- (void) playerThumbnailImageRequestDidFinish:(NSNotification*)notification;
- (ImageViewWithTime *)makeThumbnailImageViewFromImage:(UIImage *)image andTimeCode:(NSNumber *)timecode;
- (void)handleTapFrom:(UITapGestureRecognizer *)recognizer;
@end

.m

#import "SirloinVideoViewController_iPad.h"
#import "SirloinTextViewController.h"

@implementation SirloinVideoViewController_iPad
@synthesize theTimer, backgroundTimer, viewForMovie, player,
   onScreenDisplayLabel, myScrollView, keyframeTimes, instructions; 

- (id)initWithNibName:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil
{
    self = [super initWithNibName:nibNameOrNil bundle:nibBundleOrNil];
    if (self) {

    }
    return self;
    [nibNameOrNil release];
    [nibBundleOrNil release];
}

- (IBAction)close:(id)sender{
    [self.parentViewController dismissModalViewControllerAnimated:YES];
    [player stop];
    [player release];
    [theTimer invalidate];
    [theTimer release];
    [backgroundTimer invalidate];
    [SirloinVideoViewController_iPad release];
}

—

-(IBAction)textInstructions:(id)sender {

    SirloinTextViewController *vController = [[SirloinTextViewController alloc] initWithNibName:nil bundle:nil];
    [self presentModalViewController:vController animated:YES];
    [vController release];
}

- (void)viewDidLoad {
    [super viewDidLoad];
    keyframeTimes = [[NSMutableArray alloc] init];
    shoutOutTexts = [[NSArray 
                      arrayWithObjects:
                      @"1. XXXXXXXXXXXX",
                      @"2. XXXXXXXXXXXX",
                      @"3. XXXXXXXXXXXX",
                      @"4. XXXXXXXXXXXX",
                      @"5. XXXXXXXXXXXX",
                      @"6. XXXXXXXXXXXX"
                      @"7. XXXXXXXXXXXX",
                      @"8. XXXXXXXXXXXX",                     
                      @"9. XXXXXXXXXXXX",                    
                      @"10. XXXXXXXXXXXX",                      
                      @"11. XXXXXXXXXXXX",                      
                      @"12. XXXXXXXXXXXX",                      
                      @"13. XXXXXXXXXXXX",
                      @"14. XXXXXXXXXXXX",                     
                      @"15. XXXXXXXXXXXX",
                      nil] retain];

    shoutOutTimes = [[NSArray 
                      arrayWithObjects:
                      [[NSNumber alloc] initWithInt: 1], 
                      [[NSNumber alloc] initWithInt: 73],
                      [[NSNumber alloc] initWithInt: 109],
                      [[NSNumber alloc] initWithInt: 131],
                      [[NSNumber alloc] initWithInt: 205],
                      [[NSNumber alloc] initWithInt: 250],
                      [[NSNumber alloc] initWithInt: 337],
                      [[NSNumber alloc] initWithInt: 378],
                      [[NSNumber alloc] initWithInt: 402],
                      [[NSNumber alloc] initWithInt: 420],
                      [[NSNumber alloc] initWithInt: 448],
                      [[NSNumber alloc] initWithInt: 507],
                      [[NSNumber alloc] initWithInt: 531],
                      [[NSNumber alloc] initWithInt: 574],
                      nil] retain];

    self.player = [[MPMoviePlayerController alloc] init];
    self.player.contentURL = [self movieURL];

    self.player.view.frame = self.viewForMovie.bounds;
    self.player.view.autoresizingMask = 
    UIViewAutoresizingFlexibleWidth |
    UIViewAutoresizingFlexibleHeight;

    [self.viewForMovie addSubview:player.view];

    backgroundTimer = [NSTimer scheduledTimerWithTimeInterval:0.5f target:self selector:@selector(timerAction:) userInfo:nil repeats:YES];

    [self.view addSubview:self.myScrollView];

    //I am pretty sure that this is the culprit - Just not sure why...

    [[NSNotificationCenter defaultCenter] 
     addObserver:self
     selector:@selector(movieDurationAvailable:)
     name:MPMovieDurationAvailableNotification
     object:theTimer];

    //Could be wrong, but when commented out I don't have the memory issues
}

—

- (NSInteger)positionFromPlaybackTime:(NSTimeInterval)playbackTime
{
    NSInteger position = 0;
    for (NSNumber *startsAt in shoutOutTimes)
    {
        if (playbackTime > [startsAt floatValue])
        {
            ++position;
        }
    }
    return position;
}

-(NSURL *)movieURL
{
    NSBundle *bundle = [NSBundle mainBundle];
    NSString *moviePath = 
    [bundle 
     pathForResource:@"sirloin" 
     ofType:@"m4v"];

    if (moviePath) {
        return [NSURL fileURLWithPath:moviePath];
    } else {
        return nil;
    }
}

NSTimeInterval lastCheckAt = 0.0;

---

- (void)timerAction: theTimer 
{
    int count = [shoutOutTimes count];

    NSInteger position = [self positionFromPlaybackTime:self.player.currentPlaybackTime];

   NSLog(@"position is at %d", position);
    if (position > 0)
    {
        --position;
    }
    if (position < count) 
    {
        NSNumber *timeObj = [shoutOutTimes objectAtIndex:position];
        int time = [timeObj intValue];

        NSLog(@"shout scheduled for %d", time);
        NSLog(@"last check was at %g", lastCheckAt);
        NSLog(@"current playback time is %g", self.player.currentPlaybackTime);

        if (lastCheckAt < time && self.player.currentPlaybackTime >= time)
        {
            NSString *shoutString = [shoutOutTexts objectAtIndex:position];

            NSLog(@"shouting: %@", shoutString);

            CommentView *cview = [[CommentView alloc] initWithText:shoutString];
            [self.instructions addSubview:cview];
            [shoutString release];
        }
    }
    lastCheckAt = self.player.currentPlaybackTime;
}

---

// Override to allow orientations other than the default portrait orientation.
- (BOOL)shouldAutorotateToInterfaceOrientation:(UIInterfaceOrientation)interfaceOrientation {
    return YES;
}

-(void)removeObserver:(NSObject *)observer forKeyPath:(NSString *)keyPath {
    [[NSNotificationCenter defaultCenter] removeObserver:MPMovieDurationAvailableNotification];
    [[NSNotificationCenter defaultCenter] removeObserver:MPMoviePlayerThumbnailImageRequestDidFinishNotification];
    [keyPath release];
}

- (void) movieDurationAvailable:(NSNotification*)notification {
    float duration = [self.player duration];

    [[NSNotificationCenter defaultCenter] 
     addObserver:self 
     selector:@selector(playerThumbnailImageRequestDidFinish:)
     name:MPMoviePlayerThumbnailImageRequestDidFinishNotification
     object:nil];

    NSMutableArray *times = [[NSMutableArray alloc] init];
    for(int i = 0; i < 20; i++) {
        float playbackTime = i * duration/20;
        [times addObject:[NSNumber numberWithInt:playbackTime]];
    }
    [self.player 
     requestThumbnailImagesAtTimes:times 
     timeOption: MPMovieTimeOptionExact];
}

---

- (void) playerThumbnailImageRequestDidFinish:(NSNotification*)notification {
    NSDictionary *userInfo = [notification userInfo];
    NSNumber *timecode = 
    [userInfo objectForKey: MPMoviePlayerThumbnailTimeKey]; 
    UIImage *image = 
    [userInfo objectForKey: MPMoviePlayerThumbnailImageKey];
    ImageViewWithTime *imageView = 
    [self makeThumbnailImageViewFromImage:image andTimeCode:timecode];

    [myScrollView addSubview:imageView];

    UITapGestureRecognizer *tapRecognizer = 
    [[UITapGestureRecognizer alloc] 
     initWithTarget:self action:@selector(handleTapFrom:)];
    [tapRecognizer setNumberOfTapsRequired:1];

    [imageView addGestureRecognizer:tapRecognizer];

    [tapRecognizer release];
    [image release];
    [imageView release];
}

- (void)handleTapFrom:(UITapGestureRecognizer *)recognizer {
    ImageViewWithTime *imageView = (ImageViewWithTime *) recognizer.view;
    self.player.currentPlaybackTime = [imageView.time floatValue];
}

---

- (ImageViewWithTime *)makeThumbnailImageViewFromImage:(UIImage *)image andTimeCode:(NSNumber *)timecode {
    float timeslice = self.player.duration / 3.0;
    int pos = [timecode intValue] / (int)timeslice;

    float width = 75 * 
    ((float)image.size.width / (float)image.size.height);

    self.myScrollView.contentSize = 
    CGSizeMake((width + 2) * 13, 75);

    ImageViewWithTime *imageView = 
    [[ImageViewWithTime alloc] initWithImage:image];
    [imageView setUserInteractionEnabled:YES];

    [imageView setFrame:CGRectMake(pos * width + 2, 0, width, 75.0f)];

    imageView.time = [[NSNumber alloc] initWithFloat:(pos * timeslice)];
    return imageView;

    [myScrollView release];
}

- (void)dealloc {
    [player release];
    [viewForMovie release];
    [onScreenDisplayLabel release];
    [keyframeTimes release];
    [instructions release];
    [shoutOutTexts release];
    [shoutOutTimes release];
    [super dealloc];
}

@end

This app is already out there heavily using UIWebView (which just plain sux) so I am trying to things right and do it properly.

Answer 1

You do have a couple more issues than one leak.

The first one

is in your initWithNibName:bundle: as you aren't doing anything useful there: get rid of it, entirely! (Besides: don't release arguments, that are passed to your methods! Luckily, you've placed those releases in lines that are unreachable, i.e. after the return statement...)

Next method, next problems

1. Why are you sending release to a class object? Don't! That's wrong on many levels.
2. You have decided to create properties for your timers. That's nothing bad per se. But why then, are you using the ivars directly here? I'd strongly encourage you to implement setTheTimer: and setBackgroundTimer: to handle the invalidation and release properly and simply do self.theTimer = nil; self.backgroundTimer = nil; here. That would fix the asymmetry in handling those things, as well. (By the way: theTimer isn't such a great name for an ivar...especially when there is another ivar that is a timer!)

textInstructions: looks unsuspicious but...

viewDidLoad has some more issues

1. It leaks an MPMoviePlayerController:
   The @property will retain it, so you need to balance the alloc here.
2. backgroundTimer has a corresponding @property that is declared to be retaining: You are violating this API contract here, since you only assign the timer to the ivar. Use self.backgroundTimer = ... instead.
3. From all the code you posted, it seems to me that passing theTimer as the last argument in your call to -[NSNotificationCenter addObserver:selector:name:object:] is a fancy way of passing in nil as that parameter. Which is kind of good, because usually NSTimer doesn't post too many MPMovieDurationAvailableNotifications. In fact, as I can't see theTimer being used except in close:: Could it be that this is just a useless remnant from before you introduced the backgroundTimer ivar/@property? (Well there is another occurrence of a variable of that name, but it should be accompanied by a big fat compiler warning...)
4. Do you, in any way, implement viewDidUnload? If so, does it:
   1. self.player = nil;?
   2. [shoutOutTexts release], shoutOutTexts = nil;?
   3. [shoutOutTimes release], shoutOutTimes = nil;?
   4. self.keyframeTimes = nil;?
   5. [[NSNotificationCenter defaultCenter] removeObserver:self name: MPMovieDurationAvailableNotification object:nil];?
   6. self.backgroundTimer = nil;? (Assuming, setBackgroundTimer: releases and invalidates the old value)
5. Update I've missed this one on the first go: You are leaking 15 NSNumbers here. Use [NSNumber numberWithInt:] instead of alloc/init in the setup of shoutOutTimes.

A minor remark on movieURL, which you can turn into the following one-liner:

-(NSURL*)movieURL {
    return [[NSBundle mainBundle] URLForResource:@"sirloin" withExtension:@"m4v"];
}

And then this

NSTimeInterval lastCheckAt = 0.0; within the global scope. From your usage of it: ivar PLZ?!?one?

More issues later. I gotta get myself something to eat first.

---

Part Two

Now let's get into timerAction:

The first issue is not too grave, — especially in this particular context — but you should be aware that -[NSArray count] returns an NSUInteger and that the U is not a typo, but a designation that this value is unsigned. You certainly won't run into problems with the signedness in this App and rarely do on other occasions, but when you do, they make up for really funky bugs and you should be aware of the implications...
The real issue with this method is, however, that you are leaking one CommentView per iteration while — at the same time — overreleasing one NSString. The fact, that you were using string literals (which will never be dealloced) in the first place, (i.e. when you were initializing shoutOutTimes) totally saves your butt, here.

Next up: removeObserver:forKeyPath:

You really should get rid of that really bad habit of releasing parameters, that are passed to your methods!

That being said, get rid of the entirety of this method!

First and foremost removeObserver:forKeyPath: is a method from the NSKeyValueObserving informal protocol and plays a totally different role than what you are (ab-)using it to accomplish here. Secondly, it is one of those methods where it is essential to call through to super if — by any means — you really need to override it. (Well, except, when you were overriding addObserver:forKeyPath:options:context: as well and-it-should-go-without-saying-that-you-shouldn't-do-that-unless-you-really-know-what-you-are-doing-if-you-ever-planned-on-using-KVO.)

movieDurationAvailable:

Like Evan said, you're leaking times here. Go for his suggestion or — instead — make it NSMutableArray *times = [NSMutableArray array]; and you are done here.

playerThumbnailImageRequestDidFinish:

You don't own image, so don't release it!
Personally, I would finish setting up the view (i.e. add the recognizer and do stuff like that) before adding it into the view-hierarchy, but that's completely a matter of taste...

makeThumbnailImageViewFromImage:andTimeCode:

...leaks an NSNumber (use [NSNumber numberWithFloat:(pos * timeslice)] instead of the alloc/initWithFloat:-dance) and prevents you from crashing due to overreleasing myScrollView by the unconditional return statement that directly precedes it (phew!). While we're at it: rename this method to either newThumbnailImageView... so that when you'll revisit this code in a year or so, you instantly know that [imageView release]; at the bottom of playerThumbnailImageRequestDidFinish: really is necessary, without having to look at the implementation of this method.
Alternatively, you could rename it to thumbnailImageView... and change the return statement to return [imageView autorelease];. Bonus: one fewer line in playerThumbnailImageRequestDidFinish: as the [imageView release]; there becomes obsolete then.

dealloc

Add [[NSNotificationCenter defaultCenter] removeObserver:self]; at the very top.

The rest of it looks okay. (Although I find it odd, that the ivar landscapeView is never/nowhere mentioned apart from its declaration.)

Summary

Read the sections Memory Management Rules and Autorelease from Apple's "Memory Management Programming Guide" once again. They're pure gold!

Answer 2

You never release times in movieDurationAvailable::

NSMutableArray *times = [[NSMutableArray alloc] init];

You should use autorelease when you pass it to the method:

[self.player requestThumbnailImagesAtTimes:[times autorelease] timeOption: MPMovieTimeOptionExact];