views:

569

answers:

3

I have a line of UILabel text in a UIView which is regularly updated via NSTimer. This code is supposed to write a status item near the bottom of the screen every so often. The data comes from outside of its control.

My app runs out of memory really fast because it seems the UILabel is not being released. It seems that dealloc is never called.

Here is a very compressed version of my code (error checking etc removed for clarity.):

File:SbarLeakAppDelegate.h

#import <UIKit/UIKit.h>
#import "Status.h"

@interface SbarLeakAppDelegate : NSObject 
{
    UIWindow *window;
Model *model;
}
@end

File:SbarLeakAppDelegate.m

#import "SbarLeakAppDelegate.h"

@implementation SbarLeakAppDelegate
- (void)applicationDidFinishLaunching:(UIApplication *)application 
{       
    model=[Model sharedModel];

    Status * st=[[Status alloc] initWithFrame:CGRectMake(0.0, 420.0, 320.0, 12.0)];
    [window addSubview:st];
    [st release];

    [window makeKeyAndVisible];
}

- (void)dealloc 
{
    [window release];
    [super dealloc];
}
@end

File:Status.h

#import <UIKit/UIKit.h>
#import "Model.h"

@interface Status : UIView 
{
    Model *model;
    UILabel * title;
}
@end

File:Status.m This is the where the problem lies. UILabel just does not seem to be released, and quite possible the string as well.

#import "Status.h"

@implementation Status

- (id)initWithFrame:(CGRect)frame 
{
self=[super initWithFrame:frame];
model=[Model sharedModel];
[NSTimer scheduledTimerWithTimeInterval:.200 target:self  selector:@selector(setNeedsDisplay) userInfo:nil repeats:YES];
return self;
}

- (void)drawRect:(CGRect)rect 
{
title =[[UILabel alloc] initWithFrame:CGRectMake(0.0f, 0.0f, 320.0f, 12.0f)];
title.text = [NSString stringWithFormat:@"Tick  %d", [model n]] ;
[self addSubview:title];
[title release];
}

- (void)dealloc 
{
    [super dealloc];
}
@end

File: Model.h (this and the next are the data sources, so included only for completeness.) All it does is update a counter every second.

#import <Foundation/Foundation.h>
@interface Model : NSObject 
{
int n;
}

@property int n;
+(Model *) sharedModel;
-(void) inc;
@end

File: Model.m

#import "Model.h"


@implementation Model

static Model * sharedModel = nil;

+ (Model *) sharedModel
{
if (sharedModel == nil)
 sharedModel = [[self alloc] init];
return sharedModel; 
}

@synthesize n;
-(id) init
{
self=[super init];
[NSTimer scheduledTimerWithTimeInterval:1 target:self selector:@selector(inc) userInfo:nil repeats:YES];
return self;
}

-(void) inc
{
n++;
}
@end
+1  A: 

Instead of calling -setNeedsDisplay with your NSTimer in the view controller, why not create a method that calls "title.text = [NSString stringWithFormat:@"Tick %d", [model n]] ;"? That way instead of re-creating the label every time the timer fires you can just update the value displayed.

Jeff Kelley
Thank you very much. This seems to have done the trick. I moved the title=[[UILabel alloc].... line to the init part.
John Smith
+4  A: 

The problem is that you are never removing the UILabel from the Status UIView. Let's take a look at your retain counts in drawRect:

(void)drawRect:(CGRect)rect {
   title =[[UILabel alloc] initWithFrame:CGRectMake(0.0f, 0.0f, 320.0f, 12.0f)];

Here, you have created a UILabel with alloc, which creates an object with a retain count of 1.

[self addSubview:title];
[title release];

Adding the UILabel to Status view increases title's retain count to 2. The following release results in a final retain count of 1. Since the object is never removed from its superview, the object is never deallocated.

Essentially, you are adding one UILabel on top of another, each time the timer is fired, until memory runs out.

As suggested below, you should probably create the UILabel once when the view loads, and just update the UILabel's text with [model n].

As a housekeeping note, you might also want to make sure that you are properly deallocating any left over objects in your dealloc methods. 'model' and 'title' should be released in Status' dealloc, just as 'model' should be in SbarLeakAppDelegate.

Hope this helps.

Edit [1]:

It sounds like you have the memory issue pretty well handled at this point. I just wanted to suggest another alternative to the two timers you are using.

The timer you have running in your Status object fires every .2 seconds. The timer which actually increments the 'model' value, n, fires only once each second. While I believe you are doing this to ensure a more regular "refresh rate" of the Status view, you are potentially re-drawing the view 4 or 5 times per second without the data changing. While this is may not be noticeable because the view is fairly simple, you might want to consider something like NSNotification.

With NSNotification, you can have the Status object "observe" a particular kind of notification that will be fired by the Model whenever the value 'n' changes. (in this case, approximately 1 per second).

You can also specify a callback method to handle the notification when it is received. This way, you would only call -setNeedsDisplay when the model data was actually changed.

thauburger
I tried to add autorelease to title and the program crashed.How would you suggest I get it released?
John Smith
Since you have now moved [[UILabel alloc]init] to the init method, the only release you need to do for the title is in Status's dealloc. Just put [title release] before [super dealloc]. You can remove all other release/autorelease calls on title. What's happening here is you are allocating the UILabel once in init, which means a retain count of 1. Since you want that single UILabel around for the program's entire life cycle, the only place you need to release the UILabel is when the whole view is deallocated.
thauburger
+1  A: 

There are 2 problems with your code.

Problem 1

In -drawRect you add a subview to the view hierarchy every time the view is drawn. This is wrong for 2 reasons:

  • Every time the view is drawn, the number of subviews increases by 1
  • You are modifying the view hierarchy at draw time - this is incorrect.

Problem 2

Timers retain their targets. In the initializer for your Status object, you create a timer which targets self. Until the timer is invalidated, there is a retain cycle between the timer and the view, so the view will not be deallocated.

If the approach of using a timer to invalidate the view is really the correct solution to your problem, you need to take explicit steps to break the retain cycle.

One approach to doing this is to schedule the timer in -viewDidMoveToWindow: when the view is being put in a window [1], and invalidate the timer when the view is being removed from a window.

[1] Having the view invalidate itself periodically when it isn't displayed in any window is otherwise pointless.

Jim Correia
Thanks Jim. I realised problem 1. I believe I have fixed both issues now. For problem 2 do I need to occasionally kill the timer?
John Smith
I've updated the answer with a solution for breaking the retain cycle if this timer approach is really the correct one.
Jim Correia