groue / GRMustache

Flexible and production-ready Mustache templates for MacOS Cocoa and iOS
http://mustache.github.com/
MIT License
1.44k stars 190 forks source link

Over-released NSError in renderObject:fromContentsOfFile:error: #6

Closed pix0r closed 13 years ago

pix0r commented 13 years ago

I'm having an issue getting the output NSError from GRMustacheTemplate +renderObject:fromContentsOfFile:error:. When an error is encountered, the NSError** I passed in is over-released.

The offending code, I believe, is the explicit autorelease pool set up in https://github.com/groue/GRMustache/blob/master/Classes/GRMustacheRendering.m#L201

When an error is encountered, GRMustacheTemplateParser calls -finishWithError: which sets its error ivar (retained).

The problem is that the template/parser is released when the NSAutoreleasePool is drained, thus the error is released. When execution returns back to my code, the error has already been released and if I'll get a violation if I attempt to access it.

groue commented 13 years ago

This issue has been closed thanks to your pull request. Thank you very much!

pawan1011 commented 12 years ago

Hi, We are facing a issue with this method in Mustache. [GRMustacheTemplate renderObject: fromString: error: ]; So whenever a error is encountered am printing it NSLog(@"error is %@",[err userInfo]); But a get a BAD_ACCESS and the app crashes here. Am using the latest version of Mustache. What exactly is the issue here?

groue commented 12 years ago

Oops. Investigating.

groue commented 12 years ago

@pawan1011 I can not reproduce your issue.

Here is some code that works fine, for instance:

#import <Foundation/Foundation.h>
#import "GRMustache.h"

int main(int argc, const char * argv[])
{
    @autoreleasepool {
        NSError *error;
        id data = @{};
        NSString *rendering = [GRMustacheTemplate renderObject:data fromString:@"{{fail" error:&error];
        if (!rendering) {
            // works fine
            NSLog(@"error is %@",[error userInfo]);
        }
    }
    return 0;
}

Please attach some minimal code that exhibits this crash, so that we figure out where is your problem.

pawan1011 commented 12 years ago

NSString tempStr= [[NSString alloc ]initWithString:@"Replacable String"]; NSMutableDictionary data = [NSMutableDictionary dictionaryWithObject:tempStr forKey:@"context"]; NSString *rendering = [GRMustacheTemplate renderObject:data fromString:@"Test {{contextPath}} "
error:&err]; if (!rendering) { NSLog(@"error is %@",[err userInfo]); }

I assumed GRMustache will throw a error for the above code since the Templates do not match. But it did not..!! My question here is when will I get a error message from Mustache? Can you clarify.

groue commented 12 years ago

Sure I can.

Errors are documented at https://github.com/groue/GRMustache/blob/master/Guides/templates.md :

GRMustache methods may return errors whose domain is GRMustacheErrorDomain, and error codes interpreted with the GRMustacheErrorCode enumeration:

extern NSString* const GRMustacheErrorDomain;

typedef enum {
    GRMustacheErrorCodeParseError,
    GRMustacheErrorCodeTemplateNotFound,
} GRMustacheErrorCode;

That means that the only errors you get are parse errors (for unparsable templates), and template not found errors (when loading a template file that does not exist, for instance).

Your template, Test {{contextPath}}, is perfectly valid Mustache, and does not load any partial template that could be missing, so you won't get any error.

The mismatch you are experiencing is between the contextPath key in the template, and the context key in your data.

GRMustache has absolutely no opinion on keys that should be provided by data. If data provides contextPath, GRMustache will render the associated value. If data does not provide contextPath, nothing will be rendered. And actually, your rendering variable contains a string: Test. The {{contextPath}} was not tied to any value, and has been replaced by an empty string.

This is the way GRMustache works: missing keys are not errors. They are just not rendered at all. You may find this behavior annoying. Actually, it is quite useful: check https://github.com/groue/GRMustache/blob/master/Guides/runtime/context_stack.md for the big picture.

But let's get back to your problem: since your sample template is quite tiny, it's not very difficult to "fix". However, real-life templates can get huge. It may become tedious to spot this kind of errors. If this is your case, I encourage you reading https://github.com/groue/GRMustache/blob/master/Guides/delegate.md : you may find it useful.

Last point:

GRMustache returns errors in the conventional Objective-C way ( https://developer.apple.com/library/ios/#documentation/Cocoa/Conceptual/ErrorHandlingCocoa/CreateCustomizeNSError/CreateCustomizeNSError.html). Particularly (emphasis mine):

Important: Success or failure is indicated by the return value of the method. Although Cocoa methods that indirectly return error objects in the Cocoa error domain are guaranteed to return such objects if the method indicates failure by directly returning nil or NO, you should always check that the return value is nil or NO before attempting to do anything with the NSError object.

In your particular case, since renderObject:fromString:error did return a rendering, checking the error was a mistake.

I hope things are clearer, now :-)

pawan1011 commented 12 years ago

Yes.. Now I am able to get 'parse errors ' when the template is of type {{contextPath. But the second error type template not found errors (when loading a template file that does not exist, for instance) that you mentioned above is not working. I have given wrong template file path in my code. But still no error message. Can you clarify on the second error condition.

groue commented 12 years ago

Please attach some code again.

pawan1011 commented 12 years ago
NSString *tempStr= [[NSString alloc ]initWithString:@"Rendering String"];
NSError *err ;
NSString *templateString = [NSString stringWithContentsOfFile:[NSString stringWithFormat:@"%@/Desktop/mustache.xml",NSHomeDirectory()] encoding:NSUTF8StringEncoding error:NULL];
NSMutableDictionary *data = [NSMutableDictionary dictionaryWithObject:tempStr forKey:@"contextPath"];
NSString *rendering = [GRMustacheTemplate renderObject:data
                                            fromString:templateString                
                                                 error:&err];

if (!rendering) {
    NSLog(@"error is %@",[err localizedDescription]);
}

Here, I do not have any file by name mustache.xml in that file path specified. But still, Mustache is not throwing any error as expected.

groue commented 12 years ago

Look a little closely, with focus and dedication, at the way you invoke GRMustache: you use the renderObject:fromString:error method. This method does absolutely not talk about files. It talks about strings. Do you know the -[NSString originalFile] method? No, you don't, because it doesn't exist. So GRMustache can't tell you that the file mustache.xml does not exist. And it doesn't, as you can see.

Let's go further: since the file does not exist, templateString is nil. Did you notice? No, because you haven't tested the result of stringWithContentsOfFile:encoding:error:. That's unfortunate, because now you are invoking [GRMustacheTemplate renderObject:data fromString:nil error:&err] (nil string) without even knowing it. The program fails again, because your control over it is lacking.

Your two options now:

  1. either test the result of stringWithContentsOfFile:encoding:error: : you'll get an early error, and you'll know that it's useless to invoke GRMustache.
  2. use the [GRMustacheTemplate renderObject:fromContentsOfFile:error:] method, which takes a path, not a string, and that will return you an error when the file does not exist.
pawan1011 commented 12 years ago

Yes groue..I did test and notice that since file does not exist in the path specified ,templateString will be nil. But what I misunderstood was [GRMustacheTemplate renderObject: fromString: error:] method will check if the file exists, if not throw an error, since we are reading its contents and passing the NSString as a parameter to that method.

Well, Thanks for clarifying that only [GRMustacheTemplate renderObject:fromContentsOfFile:error:] method will throw an error if the file does not exist in the path specified.

groue commented 12 years ago

You're welcome. Happy Mustache!