RMCachedTileSource initWithSource: allocation/deallocation bug

[GoogleCodeExporter]

Code correctness issue here that is going to cause a reference counting
problem.

The case where initWithSource: is called with an RMCachedTileSource
class is intended to cause the sender to release.

In the case below, -dealloc; is being called directly. This has four major
errors as a result.

(1) the superclass is going to enter dealloc before ever entering init. If
the code is encapsulated, it is possible that the superclass will attempt
to free unallocated memory as a result. dealloc is not supposed to be
entered without init being entered first. Subclass does not know what super
is doing so subclass has to obey the rules.

(2) dealloc is being entered into in self without init ever being entered.
While this is within scope for encapsulation, it is still erroneous and
could cause an issue down the line.

(3) reference count is bad exiting this method. Since +alloc; has created
this object and incremented its reference count, calling -dealloc; directly
is skipping over the inverse reference count. What happens then is that
this object will attempt to deallocate memory it never allocated, and then
this object will leak.

(4) After leaking, this object returns the passed in argument in place of
itself. The object passed in has a retain count that balances its release
count in its previous role. It may now be exiting in place of an allocated
object, an object which should have a +1 to the retain count due to alloc.
Therefore the recipient down the road in the new role will be expecting to
have one extra release to balance the alloc; Since this object was already
balanced in its previous role, the new role will be out of balance
generating the possibility of an extra release. If this is being balanced
by using the dealloc direct call, it's probably bad news. 

The suggestion here is to clean up the allocation and deallocation issues,
follow the reference counting standard, as it will guarantee correctness
and minimize the chance of funky heisenbug type behavior and/or memory leaks.

Corrected code in this case would be to initialize super, check for failure
there first, and return nil if failed. Continuing pass to the second case,
checking for equivalency to the given superclass, if so, [self release]
should be called to decrement the reference count and fix the leak. The
returned object should have a reference count added to it and then returned. 

I'm a bit uneasy about the second part, and hoping that it isn't
programming by side effect somehow...






- (id) initWithSource: (id<RMTileSource>) _source
{
    if ([_source isKindOfClass:[RMCachedTileSource class]])
    {
        [self dealloc];
        return _source;
    }

    if (![super init])
        return nil;

    tileSource = [_source retain];

    cache = [[RMTileCache alloc] initWithTileSource:tileSource];

    return self;
}

Original issue reported on code.google.com by dbx.gt...@gmail.com on 29 Jan 2009 at 3:52

[GoogleCodeExporter]

Original comment by halmuel...@gmail.com on 4 Mar 2009 at 6:07

• Added labels: Milestone-Release0.5

[GoogleCodeExporter]

I agree with the comments. I don't understand the RMTileSource class cluster 
well enough to start editing this 
code, though. Since Darcy seems to be making great strides in a new cache 
implementation, I'm marking this 
issue Won't Fix.

Original comment by halmuel...@gmail.com on 8 Mar 2009 at 6:41

• Changed state: WontFix