iOS Mem leak using NSMutableArray?

[Dirk the Daring]

The relevant code looks like this:

- (void) callingFunc;
	NSMutableArray *list = [[NSMutableArray alloc] init];
	[database getListNum:num ArrayOfNSIntegers:list];
	int val = [cards count];
	[cards release];
}

- (void) getListNum:(int) num ArrayOfNSIntegers:(NSMutableArray *) array
{

	// Match. add to array.
	NSNumber *num = [[NSNumber alloc] initWithInt:value];		 
        [array addObject:num]; <----
	[num release];
}

Any time getListNum is called, instruments complains about a memory leak in the line marked <---- above.

My understanding is that when I release the array, the members of that array all decrement their retain counter, and will then be freed. What did I miss?

 

[RonC]

NSMutableArray not leaking, you are.

Your callingFunc method creates the list using init, so it's been retained for you already. Then you post it to your database (which I presume also retains it) so now it's retain count is +2. Perhaps you want to release it locally (after posting) or get an autoreleased version by using [NSMutableArray array].

 

[Dirk the Daring]

Sorry, typo (fixed below)-- I believe I'm already doing the release.
the getListNum function does not explicitly retain list. Does Obj C do this automatically?

- (void) callingFunc;
	NSMutableArray *list = [[NSMutableArray alloc] init];
	[database getListNum:num ArrayOfNSIntegers:list];
	int val = [cards count];
	[list release];
}

// this is from the database class
- (void) getListNum:(int) num ArrayOfNSIntegers:(NSMutableArray *) array
{

	// Match. add to array.
	NSNumber *num = [[NSNumber alloc] initWithInt:value];		 
        [array addObject:num]; <----
	[num release];
}

 

[chown33]

- (void) callingFunc;
	NSMutableArray *list = [[NSMutableArray alloc] init];
	[database getListNum:num ArrayOfNSIntegers:list];
	int val = [cards count];
	[list release];
}

This isn't real code. It may be fragments of real code, but it won't compile.

- (void) callingFunc;
	NSMutableArray *list = [[NSMutableArray alloc] init];

You're missing the opening {, and you have a ; instead of {.

	int val = [[COLOR="Blue"]cards[/COLOR] count];
	[[COLOR="SeaGreen"]list[/COLOR] release];

Is the blue-hilited variable-name an instance-variable? Or should it really be the list local variable?

I'm guessing because the green-hilited variable was cards in the original post.


When you post fake or fragmented code, you can't really expect anyone to comment on it when it contains such serious errors. There's no way for us to guess what's wrong in your real code by looking at damaged or mistyped code fragments. All we can see is what's wrong (or right) in whatever fragments you posted.

Whatever context there was in the real code has been completely eliminated in the posted and damaged fragments. It is entirely possible that in the process of "simplifying" what you posted, you have actually removed the leak.

If you expect a real answer to your question about a memory leak, we have to see the real code. Accuracy is important.

 

[Sydde]

Sorry, typo (fixed below)-- I believe I'm already doing the release.
the getListNum function does not explicitly retain list. Does Obj C do this automatically?

Before you proceed, go to the documentation (XCode is very thorough) and read the section on memory management. This should help you understand things like "+alloc", "+new" and "-copy" and how long the objects they return will live.

As chown33 said, your source will not compile. This part will generate an error and a warning. If you cannot see that just by looking at it, you should brush up on your C. Are you deliberately posting bad code to hide what you are actually doing?

// this is from the database class
- (void) getListNum:(int) [COLOR="blue"]num[/COLOR] ArrayOfNSIntegers:(NSMutableArray *) array
{
        // Match. add to array.
	NSNumber *[COLOR="Blue"]num[/COLOR] = [[NSNumber alloc] initWithInt:[COLOR="Red"]value[/COLOR]];		 
        [array addObject:num]; <----
	[num release];
}

 

[Dirk the Daring]

I apologize. Yes, I was trying to modify the code for brevity and to make what it generic, and failed horribly. Here's the code, cut and pasted.

// The number of rows in section #section. 
- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section 
{	
	NSMutableArray *trans = [[NSMutableArray alloc] init];
	[database GetTransactionsForCardID:cardid ArrayOfNSIntegers:trans];
	int count = [trans count];
	[trans release];
	return count;
}

(database has the following method)

- (void) GetCardsForStoreID:(int) storeid ArrayOfNSIntegers:(NSMutableArray *) array
{
	//NSLog(@"GetCardsForStoreID: id %d\n",storeid);
	
	// Clear the array.
	[array removeAllObjects];

	NSString *deletestore = [[NSString alloc] initWithFormat:@"SELECT id, storeid FROM CARDS WHERE storeid=%d ORDER BY id;",storeid];
	sqlite3_stmt *statement;
	if (sqlite3_prepare_v2(sqldata, [deletestore UTF8String], -1, &statement, nil)!=SQLITE_OK)
	{
		[deletestore release];
		NSLog(@"     Failed to get card list data\n");
		return;
	}
	
	while (sqlite3_step(statement)==SQLITE_ROW)
	{
		// Found another one.
		int cardid = sqlite3_column_int(statement,0);
		int cardstoreid = sqlite3_column_int(statement,1);
		
		if (cardstoreid == storeid)
		{
			// Match. add to array.
			NSNumber *num = [[NSNumber alloc] initWithInt:cardid];
			[array addObject:num];
			[num release];
		}
	}
	sqlite3_finalize(statement); // We're done.
	[deletestore release];
	return ;
}

 

[robbieduncan]

I can't promise this will change anything but from a coding style point of view it would be much better to use the class-methods of NSString and NSNumber that return autoreleased instances than using alloc/init/release cycles. For example:

// Match. add to array.
NSNumber *num = [[NSNumber alloc] initWithInt:cardid];
[array addObject:num];
[num release];

would be better written as

[array addObject:[NSNumber numberWithInt:cardid]];

In general unnecessary retain/release cycles should be avoided.

 

[Sydde]

I can't promise this will change anything but from a coding style point of view it would be much better to use the class-methods of NSString and NSNumber that return autoreleased instances than using alloc/init/release cycles.

In general unnecessary retain/release cycles should be avoided.

Not sure I agree with you here. The class methods still have an -alloc, and the release cycle does still occur, just out of your view. In many cases, it is just simpler to use the class methods (less coding on your part), but if you want to keep your heap usage to a minimum, explicitly allocating and releasing objects can be more efficient. Granted, if you are that concerned about memory, your code could probably be revised to improve performance and efficiency elsewhere, but to say that the class methods eliminate unnecessary retain/release cycles is misleading.

Writing pretty or concise code is not always the best route.

 

[PhoneyDeveloper]

From a performance standpoint you can only determine that autoreleased objects in a loop are better/worse than alloc/init/release objects by using instruments. Sometimes one will be better, sometimes the other.

From a coding style standpoint they are equally good. Using autoreleased objects is not cleaner/neater/cooler/better than using alloc/init/released objects. They are the same. Both are correct code. Use whatever is better from a performance standpoint or matches your preferred style.

In the early days, 2008, using autoreleased objects was eschewed strongly by Apple for memory usage reasons. I think that advice has fallen by the wayside. I often use the alloc/init/release style but also often don't.

 

[chown33]

// The number of rows in section #section. 
- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section 
{	
	NSMutableArray *trans = [[NSMutableArray alloc] init];
	[database [COLOR="Red"]GetTransactionsForCardID:[/COLOR]cardid ArrayOfNSIntegers:trans];
	int count = [trans count];
	[trans release];
	return count;
}

(database has the following method)

- (void) [COLOR="Blue"]GetCardsForStoreID:[/COLOR](int) storeid ArrayOfNSIntegers:(NSMutableArray *) array
{
	//NSLog(@"GetCardsForStoreID: id %d\n",storeid);
	
	// Clear the array.
	[array removeAllObjects];

	NSString *deletestore = [[NSString alloc] initWithFormat:@"SELECT id, storeid FROM CARDS WHERE storeid=%d ORDER BY id;",storeid];
	sqlite3_stmt *statement;
	if (sqlite3_prepare_v2(sqldata, [deletestore UTF8String], -1, &statement, nil)!=SQLITE_OK)
	{
		[deletestore release];
		NSLog(@"     Failed to get card list data\n");
		return;
	}
	
	while (sqlite3_step(statement)==SQLITE_ROW)
	{
		// Found another one.
		int cardid = sqlite3_column_int(statement,0);
		int cardstoreid = sqlite3_column_int(statement,1);
		
		if (cardstoreid == storeid)
		{
			// Match. add to array.
			NSNumber *num = [[NSNumber alloc] initWithInt:cardid];
			[array addObject:num];
			[num release];
		}
	}
	sqlite3_finalize(statement); // We're done.
	[deletestore release];
	return ;
}

This still isn't right. The red-hilited name doesn't match the blue-hilited method name.

I see no obvious source of leaks in the GetCardsForStoreID method. Some perplexing things, yes, but no obvious leaks. There may well be an obvious leak in GetTransactionsForCardID, but since you haven't shown its code, AFAICT, all anyone can do is guess.

The first perplexing thing is why you'd pass an array, simply to clear it then fill it. As a design, it seems a lot clearer to me if it created an array, filled it, and returned it from GetCardsForStoreID. Passing by reference is an odd design choice here. If you were appending to an existing array, which might already contain other items, then passing the array to append to makes sense. As given, not so much.

Equally perplexing is that you don't seem to believe your database. The SELECT statement selects only rows where the 'storeid' column matches the value passed in. By nature, all other rows will be excluded. Yet you have an 'if' statement in the loop that checks whether the column value is identical to the passed-in value. That's redundant. Worse, if by some unfortunate circumstance you do happen to get a non-identical storeid value, it silently skips over all the erroneous data, as if everything was fine. Frankly, if you get results that are that far wrong, you probably have a damaged database, and the last thing you should do is silently ignore it. If storeid doesn't match, it should throw an exception or call abort() or immediately stop, rather than going on.

 

[Dirk the Daring]

This still isn't right. The red-hilited name doesn't match the blue-hilited method name.

I see no obvious source of leaks in the GetCardsForStoreID method. Some perplexing things, yes, but no obvious leaks. There may well be an obvious leak in GetTransactionsForCardID, but since you haven't shown its code, AFAICT, all anyone can do is guess.

The first perplexing thing is why you'd pass an array, simply to clear it then fill it. As a design, it seems a lot clearer to me if it created an array, filled it, and returned it from GetCardsForStoreID. Passing by reference is an odd design choice here. If you were appending to an existing array, which might already contain other items, then passing the array to append to makes sense. As given, not so much.

Equally perplexing is that you don't seem to believe your database. The SELECT statement selects only rows where the 'storeid' column matches the value passed in. By nature, all other rows will be excluded. Yet you have an 'if' statement in the loop that checks whether the column value is identical to the passed-in value. That's redundant. Worse, if by some unfortunate circumstance you do happen to get a non-identical storeid value, it silently skips over all the erroneous data, as if everything was fine. Frankly, if you get results that are that far wrong, you probably have a damaged database, and the last thing you should do is silently ignore it. If storeid doesn't match, it should throw an exception or call abort() or immediately stop, rather than going on.

#1: You're right. I completely failed to view instrument's backtrace properly, and followed the wrong path. Reading the correct information, the leak was obvious in the correct calling function.

#2: I chose to avoid returning an array out of frustration with the retain/release system-- Although I understand the concept of how it works, enough things happen behind the scenes that I'm not confident about having the correct reference count when I go to delete an object. (questions at end)

#3: I'm somewhat new to using SQL. I'm going to re-write the function.

Questions:
* Is there a way to print/view the current retain reference count of an object?
* Is there a way to free memory regardless of the reference count?
* Does the redundancy of the select/data check cause a problem? (The code will be changed regardless)

Thanks for looking over the code, particularly since I gave the wrong code multiple times.

 

[chown33]

#2: I chose to avoid returning an array out of frustration with the retain/release system-- Although I understand the concept of how it works, enough things happen behind the scenes that I'm not confident about having the correct reference count when I go to delete an object. (questions at end)

If you're thinking about reference count, you're doing it wrong. Seriously. You should be thinking about ownership: Do I own this object at this point? If I don't own it, how do I claim ownership? If I do own it, what are my responsibilities at this point?

Read the Memory Management Guide, and memorize the basic rules of ownership.
http://developer.apple.com/library/ios/#documentation/cocoa/Conceptual/MemoryMgmt/MemoryMgmt.html

Ownership is the fundamental principle behind memory management, and memory management is fundamental to every correctly working program. If you don't have a solid grasp of the fundamentals, then you're already beyond your skill level.

* Is there a way to print/view the current retain reference count of an object?

There is, but it almost always leads you astray. Because reference count doesn't directly indicate your ownership. It could be someone else's ownership, and you believe things are fine when they're not.

* Is there a way to free memory regardless of the reference count?

Why would you do that?

The reference count indicates the total of all ownership claims. If you free the object, you are revoking all ownership claims,, whether you know who the owners are or not, and whether you know what their ownership means or not.

Take care of your own claims of ownership, and let other's claims take care of themselves.

* Does the redundancy of the select/data check cause a problem? (The code will be changed regardless)

Does any redundant code cause a problem?

It might. For one thing, it's redundant. For another, it's entirely redundant. And thirdly, it's completely redundant. If the needless verbosity and logical conditions embodied in the superfluous redundancy isn't an immediate problem, it could become one as the code changes over time. Redundant code must be maintained (all code is subject to bugs being introduced; the more code you have, the more places for bugs to be introduced). Immediate problems may include subtle bugs (typically in the scope of a redundant conditional block), inefficiency (unnecessary checks and conditionals), unecessary code size (redundant code is still code), and a general absence of clarity for those who read the code. Reading redundant code always makes me wonder whether the redundancy was intentional or whether it's actually a latent bug that's testing the wrong condition, and then what the correct condition might actually be.

If you want to code in failsafes, then use assertions. The C assert() function works, as does NSAssert. You can google them. If you don't know what assertions are and why you'd use them, there's a page for that:
http://en.wikipedia.org/wiki/Assertion_(computing)

 

[chen8002004]

Very good link from chown33. However, still some questions about memory. Is code below enough for releasing memory? Is it necessary to release each item from the array?

NSMutableArray *array = [[NSMutableArray alloc] init];
NSUInteger i;
for (i = 0; i < 10; i++) {
NSNumber *allocedNumber = [[NSNumber alloc] initWithInteger: i];
[array addObject:allocedNumber];
[allocedNumber release];
}
[array release];

 

[chown33]

Is it necessary to release each item from the array?

Read the "Memory Management Programming Guide". All of it.

There is a section under "Practical Memory Management", heading "Cases Which Often Cause Confusion", subheading "Using Collections".

"When you add an object to a collection such as an array, dictionary, or set, the collection takes ownership of it. The collection will relinquish ownership when the object is removed from the collection or when the collection is itself released. ..."
​

http://developer.apple.com/library/...ical.html#//apple_ref/doc/uid/TP40004447-SW12