Objective-C: Fixing memory management in a method

Hi all,

I'm almost there understanding simple reference counting / memory management in Objective-C, however I'm having a difficult time with the following code. I'm releasing mutableDict (commented in the code below) and it's causing detrimental behavior in my code. If I let the memory leak, it works as expected, but that's clearly not the answer here. ;-) Would any of you more experienced folks be kind enough to point me in the right direction as how I can re-write any of this method to better handle my memory footprint? Mainly with how I'm managing NSMutableDictionary *mutableDict, as that is the big culprit here. I'd like to understand the problem, and not just copy/paste code -- so some comments/feedback is ideal. Thanks all.

- (NSArray *)createArrayWithDictionaries:(NSString *)xmlDocument 
             withXPath:(NSString *)XPathStr {

    NSError *theError = nil;
    NSMutableArray *mutableArray = [[[NSMutableArray alloc] init] autorelease];
    //NSMutableDictionary *mutableDict = [[NSMutableDictionary alloc] init];
    CXMLDocument *theXMLDocument = [[[CXMLDocument alloc] initWithXMLString:xmlDocument options:0 error:&theError] retain]; 
    NSArray *nodes = [theXMLDocument nodesForXPath:XPathStr error:&theError];
    int i, j, cnt = [nodes count];
    for(i=0; i < cnt; i++) {
     CXMLElement *xmlElement = [nodes objectAtIndex:i];
     if(nil != xmlElement) {
      NSArray *attributes = [NSArray array];
      attributes = [xmlElement attributes];
      int attrCnt = [attributes count];
      NSMutableDictionary *mutableDict = [[NSMutableDictionary alloc] init];
      for(j = 0; j < attrCnt; j++) {
       if([[[attributes objectAtIndex:j] name] isKindOfClass:[NSString class]]) 
        [mutableDict setValue:[[attributes objectAtIndex:j] stringValue] forKey:[[attributes objectAtIndex:j] name]];
       else 
        continue;
      }
      if(nil != mutableDict) {
       [mutableArray addObject:mutableDict];
      }
      [mutableDict release];  // This is causing bad things to happen.
     }
    }

    return (NSArray *)mutableArray;
}

From stackoverflow

• Well, releasing mutableDict really shouldn't be causing any problems because the line above it (adding mutableDict to mutableArray) will retain it automatically. While I'm not sure what exactly is going wrong with your code (you didn't specify what "bad things" means), there's a few general things I would suggest:

  1. Don't autorelease mutableArray right away. Let it be a regular alloc/init statement and autorelease it when you return it ("return [mutableArray autorelease];").

  2. theXMLDocument is leaking, be sure to release that before returning. Also, you do not need to retain it like you are. alloc/init does the job by starting the object retain count at 1, retaining it again just ensures it leaks forever. Get rid of the retain and release it before returning and it won't leak.

  3. Just a tip: be sure that you retain the return value of this method when using it elsewhere - the result has been autoreleased as isn't guaranteed to be around when you need it unless you explicitly retain/release it somewhere.

  Otherwise, this code should work. If it still doesn't, one other thing I would try is maybe doing [mutableArray addObject:[mutableDict copy]] to ensure that mutableDict causes you no problems when it is released.

  Peter Hosey : “Don't autorelease mutableArray right away… autorelease it when you return it ….” I disagree. If I don't see the autorelease on the same line, I have to go look for it to make sure you didn't leak it.

• Here's an equivalent rewrite of your code:

  - (NSArray *)attributeDictionaries:(NSString *)xmlDocument withXPath:(NSString *)XPathStr {
      NSError *theError = nil;
      NSMutableArray *dictionaries = [NSMutableArray array];
      CXMLDocument *theXMLDocument = [[CXMLDocument alloc] initWithXMLString:xmlDocument options:0 error:&theError]; 
      NSArray *nodes = [theXMLDocument nodesForXPath:XPathStr error:&theError];
  
      for (CXMLElement *xmlElement in nodes) {
          NSArray *attributes = [xmlElement attributes];
          NSMutableDictionary *attributeDictionary = [NSMutableDictionary dictionary];
          for (CXMLNode *attribute in attributes) {
              [attributeDictionary setObject:[attribute stringValue] forKey:[attribute name]];
          }
  
          [dictionaries addObject:attributeDictionary];
      }
  
      [theXMLDocument release];
      return attributeDictionaries;
  }
  

  Notice I only did reference counting on theXMLDocument. That's because the arrays and dictionaries live beyond the scope of this method. The array and dictionary class methods create autoreleased instances of NSArray and NSMutableDictionary objects. If the caller doesn't explicitly retain them, they'll be automatically released on the next go-round of the application's event loop.

  • I also removed code that was never going to be executed. The CXMLNode name method says it returns a string, so that test will always be true.
  • If mutableDict is nil, you have bigger problems. It's better that it throws an exception than silently fail, so I did away with that test, too.
  • I also used the relatively new for enumeration syntax, which does away with your counter variables.
  • I renamed some variables and the method to be a little bit more Cocoa-ish. Cocoa is different from most languages in that it's generally considered incorrect to use a verb like "create" unless you specifically want to make the caller responsible for releasing whatever object you return.
  • You didn't do anything with theError. You should either check it and report the error, or else pass in nil if you're not going to check it. There's no sense in making the app build an error object you're not going to use.

  I hope this helps get you pointed in the right direction.

  erikprice : I think he just made a typo (and I don't have enough rep to edit it) - I think the last line should be "return dictionaries". The attributeDictionary instance is added to this array, so it won't go out of scope.

  Alex : Yup. Erik is right. I didn't catch that soon enough.

• In Memory Management Programming Guide under the topic Returning Objects from Methods (scroll down a bit), there are a few simple examples on how to return objects from a method with the correct memory managment.