Crash when invoking stringWithString on NSMutableString object

I have the following code running when the user clicks the "Save" button:

- (IBAction) onSaveChangesClick:(id)sender  {

NSMutableString *newGroups = [[NSMutableString alloc] init];


for (int i = 0; i < [self.isInGroupArr count]; i++) {
    if ([[self.isInGroupArr objectAtIndex:i] boolValue] == YES) {
        [newGroups appendString:[[AppDelegate arrayGroups] objectAtIndex:i]];
        [newGroups appendString:@","];
    }
}

//  remove last : ","
if ([newGroups length] > 0)
    newGroups = [NSMutableString stringWithString:[newGroups substringToIndex:[newGroups length] - 1]];


self.contact.groups = newGroups;
[newGroups release];
//[[self navigationController] popViewControllerAnimated:YES];
}

self.IsInGroups is BOOL array and arrayGroups is (NSString *) array that holds groups names. I would like to add the newGroups string to th开发者_如何学编程e arrayGroups[i] only if (IsInGroups[i] == YES).

This piece of code generates EXC_BAD_ACCESS. WHY?

Thanks.

---

newGroups = [NSMutableString stringWithString:[newGroups substringToIndex:[newGroups length] - 1]];

This line generates a leak, then is the cause of the crash.

After this executes, you no longer have a reference on your alloc/inited mutable string, and you have an autoreleased string. So calling release on that string causes a double release somewhere.

EDIT: with solution

Simplest solution: do not add the last ','.

for (int i = 0; i < [self.isInGroupArr count]; i++) {
    if ([[self.isInGroupArr objectAtIndex:i] boolValue] == YES) {
        [newGroups appendString:[[AppDelegate arrayGroups] objectAtIndex:i]];
        if (i != ([self.isInGroupArr count] - 1))
            [newGroups appendString:@","];
    }
}

Not very elegant, but quite efficient (could avoid taking the count each time though).

---

Here you create autoreleased instance of NSMutable string.

newGroups = [NSMutableString stringWithString:[newGroups substringToIndex:[newGroups length] - 1]];

so you shouldn't release it, and all will be fine.

Here is improved code:

- (IBAction) onSaveChangesClick:(id)sender  {

NSMutableString *newGroups = [[[NSMutableString alloc] init] autorelease];


for (int i = 0; i < [self.isInGroupArr count]; i++) {
    if ([[self.isInGroupArr objectAtIndex:i] boolValue] == YES) {
        [newGroups appendString:[[AppDelegate arrayGroups] objectAtIndex:i]];
        [newGroups appendString:@","];
    }
}

//  remove last : ","
if ([newGroups length] > 0)
    newGroups = [NSMutableString stringWithString:[newGroups substringToIndex:[newGroups length] - 1]];


self.contact.groups = newGroups;

//[[self navigationController] popViewControllerAnimated:YES];

}

Explanation:

1. Here you allocate memory and retain it.

   [[NSMutableString alloc] init]

2. [NSMutableString stringWithString: returns autoreleased instance of NSMutable string, that we shouldn't release(it does the same as [[[NSMutableString alloc] init] autorelease] + smth more). and you assign it to variable newGroups(so your old value that was stored in this variable lost)

if ([newGroups length] > 0) newGroups = [NSMutableString stringWithString:[newGroups substringToIndex:[newGroups length] - 1]];

1. newGroups here is autoreleased, you release it, and it destroys. But as it was autoreleased, autorelease pool tries to release it again and gets exception(because memory is allready free)

   [newGroups release];

---

You have allocated a string(NSMutableString *newGroups = [[NSMutableString alloc] init]; )

and then assigning it with an autorelease string(newGroups = [NSMutableString stringWithString:[newGroups substringToIndex:[newGroups length] - 1]]; ).

Which you should never do. See this blog - http://andycodes.tumblr.com/post/947927280/difficult-bug-finally-solved

Comment out [newGroups release]; and code should work fine.

Also always set NSZombieEnabled environment variable and run the code again, have a look at the crash log, you will get exactly which object is causing the crash.