Memory leak whilst reading from disk

[Rheeseyb]

There seems to be an issue whilst reading responses cached on disk and it appears to be on this line:

response = [NSKeyedUnarchiver unarchiveObjectWithFile:[_diskCachePath stringByAppendingPathComponent:cacheKey]]

I've split it out into 2 stages (reading the data from file, and then calling unarchiveObjectWithData), and it appears that the leak occurs when unarchiving the data.

This isn't helped by the fact that with iOS5 the following line does nothing for objects over about 52kB in size:

[super storeCachedResponse:response forRequest:request]

meaning objects over that size will be read from disk every time. (I've tested against iOS4, and this line does cache objects over 52kB in memory).

[bogardon]

I too would like to know what is causing this

[icoigo]

Same, iOS 4.3 does not have leak, and iOS 5.0+ has leak as Rheeseyb said. And from the stack trace, it seems the leak is from:

"- (void)storeCachedResponse:(NSCachedURLResponse *)cachedResponse forRequest:(NSURLRequest *)request",

every time the cachedResponse.data is call, that is get data from cachedResponse, will cause the leak sometimes. Also, in:

"- (void)storeRequestToDisk:(NSURLRequest *)request response:(NSCachedURLResponse *)cachedResponse",

the line:
"if (![NSKeyedArchiver archiveRootObject:cachedResponse toFile:cacheFilePath]) "

will cause the leak as well.

Just do not know why.

[bogardon]

yeah this is literally causing memory crashes in my app, i have no choice but to use the original version. @saurik doesn't actually see a point to converting SDURLCache to gcd either...hmm...

[steipete]

I haven't looked into this bug since I currently don't have a project that uses SDURLCache heavily.

It's a long shot, but since 1e889b6 SDURLCache is now ARCified, which might fix this issue. if not, we're in bigger trouble (maybe a retain cycle?). So any help on fixing this or reporting if this is still there is appreciated.

[bogardon]

@steipete

I've looked into this a bit more, and I wasn't able to come to any conclusions.

However, I can make this leak disappear by simply calling

[super removeCachedResponseForRequest:request]; 

right before

[super storeCachedResponseForRequest:request];

Another thing to note is that, this leak IS NOT unique to your fork, and it actually exists in the original as well.

I'll post more on this later, but for now this is all I have.

[steipete]

Ahh, I was afraid that the easy solution isn't the fix. So, should we add the remove* call? Since you're saying this fixes the leak, and it's a pure memory operation up there, it shouldn't do any harm.

[bogardon]

@steipete, if we can't figure out the real problem, I suppose I would be OK with putting that in.

So a little back story on how I even discovered this leak. By the way this leak is very hard to tease out.

I'm implementing a caching behavior where you return the cached result and then the origin result. I do this by making two different mutable copies of a NSURLRequest.

1. One request with cachePolicy NSURLRequestReturnCacheDataDontLoad
2. Another one with cachePolicy NSURLRequestReloadIgnoringLocalCacheData

But I forgot to modify the cache policy of the 2nd copy, and hence that request was checking for the cached response as well (as per the default NSURLRequestUseProtocolCachePolicy).

I could make the leak disappear by either

1. doing what I did two posts above
2. setting NSURLRequestReloadIgnoringLocalCacheData on the 2nd request object, which means it won't go into the method - (NSCachedURLResponse *)cachedResponseForRequest:(NSURLRequest *)request

I thought for sure it had to do with [super storeCachedResponse:diskResponse forRequest:request], and so I tried to call this method a couple times in a row to see if I could create a reproducer, but no luck there.

Just want to rope in @rs and @saurik to see if they may offer any assistance.

[saurik]

@bonchon Can you provide more details for how this leak can be detected, and in what circumstances it comes up?

I am personally somewhat surprised that there would be a memory leak in the upstream SDURLCache (which you are now claiming also has this bug) at this point. Cydia's usage of it is pretty hard core, and I cache immense numbers of giant files.

Cydia is pretty much a giant web browser, you spend your entire time staring at web pages, and users download tons of images, such as icons and even entire screenshots: many of these files are marked as cacheable (and get cached).

In fact, I'm storing files that are so large in my cache that NSKeyedArchiver sometimes stores corrupt data to disk (apparently, a known bug), so I had to patch SDURLCache to throw out and delete corrupted cache entries (a patch I haven't asked to be merged yet, as the way I delete the corrupted entry is really crude and doesn't update anything else). I also have the hilarious experience of having to wait sufficiently long to rm -rf my cache folder that I sometimes wonder if the device has locked up.

(This experience under heavy usage, for the record, is why I like to point out that the scalability constraint of SDURLCache has nothing to do with operation queues, and has everything to do with the performance of using a single flat directory with thousands of files with long names, each a property list dictionary representing an object serialized with NSKeyedArchiver.)

It thereby seems very very unlikely that I would not be getting tons of reports from people telling me that Cydia was suddenly using much more memory on 5.x and causing out of memory conditions; it seems much more likely that, instead, there is a bug in your code somewhere that is causing the leak. (Maybe gist up your test code?)

[saurik]

So, I'm actually really glad this is coming up, as apparently in Cydia I clear the entire cache, including the disk cache (which is arguably kind of catastrophic for me, given the reasons why I added SDURLCache in the first place), if I'm running low on memory (which I imagine happens quite often on some of my target platforms, such as the iPhone 3G)...

- (void) applicationDidReceiveMemoryWarning:(UIApplication *)application {
    [[NSURLCache sharedURLCache] removeAllCachedResponses];
}

This could also explain how I'm being isolated from reports of this problem? (As, if it is not a "true leak", then maybe by blowing away everything in the cache when the device starts running low on memory, maybe even more so than if you tried to remove things one by one from the memory cache due to possible bugs in Apple's logic, I avoid the issue?)

[bogardon]

@saurik For sure this is highly related to my own code. And will not happen under the standard use case.

I feel like I may have screwed something by entering - (NSCachedURLResponse *)cachedResponseForRequest:(NSURLRequest *)request twice in close vicinity with the same URL. Both requests are eventually used in a NSOperationQueue, the order in which the request enters this method is non-deterministic, but I haven't ran into a case where 2nd request to enter makes it pass the in-memory check before the other first one finishes storing.

Give me a sec and I'll upload my instrument trace. ( http://dl.dropbox.com/u/3119094/leak.trace.zip )

So direct your attention to CFCachedURLResponse ( 0xb2c3ea0 ), click the arrow and view the history of its life cycle.
As you can see, it gets created when we decode the NSCachedURLResponse. Look near the bottom, you'll see that NSCachedURLResponse gets a dealloc, but somehow CFCachedURLResponse lives on with a single retain count in the very end. I'm assuming all the other entries you see are the result of leaking this object. It is completely possible that the fault may actually be the networking library I'm using (AFNetworking), which kind of makes sense since everyone was directed to this particular fork via AFNetworking.

Again, I just want to emphasize that this is really a non-issue, unless @Rheeseyb and @icoigo you guys want to jump in and elaborate on this. Sorry if I've wasted more brain power than necessary.

[saurik]

@bonchon Is this trace using rs/SDURLCache, or steipete/SDURLCache? If possible, can I get one that demonstrates the leak with rs/SDURLCache? I will admit to never having used Instruments before... I'm somewhat confused as to why the #1 event of Sharkfin (I presume your app) has SDURLCache's code somehow directly retaining the CFCachedURLResponse... that should be encapsulated in the NSCachedURLResponse.

[saurik]

After having spent too much time staring at Instruments (which could be much better designed... I'd honestly have much preferred if it just gave me a text file with all of the data in it...) I am not even certain there really is a leak (there are tons of live objects, including the URL cache... why do we have any reason to believe this object isn't simple in the memory cache? that seems like a much more reasonable explanation), and if there is I'm going to ask if you are using the simulator (there at least used to be known memory leaks in the networking system of the simulator; never test anything on the simulator), and if not then I'm going to ask (assuming you want to keep invoking my help, which again is only going to apply to rs/SDURLCache, as that's what I use) that you come up with better evidence than this Instruments thing. In particular, I'd recommend setting some conditional breakpoints on the retain/release mechanisms to catch this object, and then verify yourself exactly where and why it is being stored. You should also make 100% certain that the cache itself is deallocated and that WebKit no longer has the object in the "page cache" (which may or may not be enabled for the embedded target anyway, and if it is maybe it is off by default or something with only private APIs for enabling) before calling the leak ;P.

[bogardon]

@saurik sorry I'm back home now and can't do more work on this til tomorrow.

The trace is done with rs/SDURLCache. And though this particular trace is done with a 4.0 simulator, I have produced the same results running on a 3GS or a 4th Gen iPod Touch.

My base assumption is that the Leaks instrument is reliable when it claims to have found a leak. That said, I admit that I am not quite sure how my app (Sharkfin) can directly retain that CF object.

I should also mention that at no point in that trace did I instantiate a UIWebview, does that exclude me from the WebKit possibility? Everything is pulled down as JSON and represented natively.

The fact that I still cannot create a scenario to reliably cause this leak is a sign that I need to do some more work before continuing this discussion.

[bogardon]

@saurik This is a tangent (a secant rather) from the above discussion.

Take a look at this gist https://gist.github.com/2323766/00856c9a08ff1f04ede2cd9cd02f3bcc874ab083

I assert that, in iOS5 only, [cachedResponse data] returns a retained data object, and hence leaks.

This was shown to be true in the Leaks instrument.

To be extra sure, I ran that gist in a loop (and I create and drain an autorelease pool in every iteration) on my iphone 3gs, it crashed out and created a low memory log in ~/library/logs/crashreporter/mobiledevice/name_of_your_phone/

If this were truly a leak, then any iOS 5 device using SDURLCache is in for some trouble. A quick file search shows that the data method is called in two different places

- (void)storeCachedResponse:(NSCachedURLResponse *)cachedResponse forRequest:(NSURLRequest *)request
- (void)encodeWithCoder:(NSCoder *)coder

[saurik]

So you did the synchronous request in your loop? I am much more willing to believe there is a memory leak in the synchronous request system (as people have been reporting memory issues with that disfavored API, especially with regards to usage in the iPhone simulator, for many years now) than that there is a leak in the seemingly boring URL response cache object.

Put simply, if there were a memory leak getting the data from the cached response, you should be able to see the leak by just calling that method over and over again. Otherwise, it it is much more likely that some other block of code is retaining the data when it shouldn't: you keep making the same incorrect leap of faith that the place where an object is allocated is where it "leaked".

Regardless, can you please provide your exact test code, not simply the code you ran "in a loop", so there can be independent verification of implementation mistakes in the memory management of the loop itself? Additionally, can you isolate the synchronous request from the rest of the trial, in order to demonstrate that the synchronous request itself isn't responsible for the leaks?

FWIW, I this is how -[NSCachedResponse data] is implemented. As you can see, unless there is a memory management issue with core types like CFData, NSMutableData, or CFArray, there doesn't seem to be any opportunity for there to be a memory leak caused by getting the data from a cached response: while the memory management isn't exception-safe, it is clearly otherwise correct.

(I will, though, note that the implementation is also not thread-safe: if the receiver is still adding to the data array at the same time that this function is looping over it, the underlying CFArray might reallocate its storage and cause unfortunate behavior during a concurrent call to CFArrayGetValueAtIndex from the code below; I imagine you simply aren't able or supposed to have a CFCachedURLResponse for a live request.)

// iOS 4.3
- (NSData *) data {
    return (NSData *) CFCachedURLResponseGetReceiverData(response_);
}

CFDataRef CFCachedURLResponseGetReceiverData(CFCachedURLResponseRef response) {
    return response->receiverData;
}

// iOS 5.0
- (NSData *) data {
    // as of iOS 5.0, cache data is stored as a sequential array of segments
    CFArrayRef *array(CFCachedURLResponseCopyReceiverDataArray(response_));
    if (array == NULL)
        return nil;

    CFIndex count(CFArrayGetCount(array));

    // if there is only a single segment, we can just return it directly
    if (count == 1) { 
        CFDataRef data((CFDataRef) CFArrayGetValueAtIndex(array, 0));
        // the moral equivalent of: data = [[data retain] autorelease];
        data = (CFDataRef) CFMakeCollectable(CFRetain(data));

        CFRelease(array);
        return (NSData *) data;
    }

    // we should not get to this point if there are not multiple segments
    if (count < 2) {
        CFRelease(array);
        return nil;
    }

    // allocate a single mutable data to concatenate the data array into
    NSMutableData *data([NSMutableData data]);

    for (CFIndex index(0); index != count; ++index)
        [data appendData:(NSData *) CFArrayGetValueAtIndex(array, index)];

    CFRelease(array);
    return data;
}

CFArrayRef CFCachedURLResponseCopyReceiverDataArray(CFCachedURLResponseRef response) {
    if (response == NULL)
        return NULL;

    CFDataRef data(response->receiverDataArray);
    if (data == NULL)
        return NULL;

    return CFRetain(data);
}

[saurik]

For the hell of it, I spent some more time looking into my assertion this code looks like it isn't thread-safe, and found something infuriating: CFCachedURLResponseGetReceiverData still exists, so Apple didn't need to do this loop up at the NSCachedURLResponse level, and in fact shouldn't, as they now have the same code in multiple places that have different result caching behavior. :(

In this case, I assert that this code also isn't thread-safe, and is in fact much worse: the other code had some chance of weird behavior, but if this code is called concurrently from multiple places it will simply leak memory. Specifically, Apple has a lock around setting receiverData, but they failed to get the value in the same lock, so there is a race condition that may lose track of one of the copies.

Thankfully, while CFCachedURLResponseGetReceiverData still exists, duplicates the above functionality, and seems to have this egregious multi-threading mistake, it also doesn't seem to actually be used anymore: there are no calls left without CFNetwork, and doing a quick grep shows shows the symbol not being referenced by any other frameworks; this is thereby just for exposition.

(So, to be 100% clear: I don't currently believe there to be a memory leak in cached URL responses themselves.)

CFDataRef CFCachedURLResponseGetReceiverData(CFCachedURLResponseRef response) {
    if (response == NULL)
        return NULL;

    return response->GetReceiverData();
}

// this function is only called by CFCachedURLResponseGetReceiverData()
CFDataRef CFCachedURLResponse::GetReceiverData() {
    if (receiverData != NULL)
        return receiverData;

    if (receiverDataArray == NULL)
        return NULL;

    // I am not 100% certain, but this lock seems to /only/ be used here
    OSSpinLockLock(receiverDataLock);
    receiverData = CopyAllDataFromDataArray(receiverDataArray);
    OSSpinLockUnlock(receiverDataLock);

    return receiverData;
}

// this function just concatenates up a data array: nothing wrong with it
CFDataRef CopyAllDataFromDataArray(CFArrayRef array) {
    if (array == NULL)
        return NULL;

    CFIndex count(CFArrayGetCount(array));

    // if there is only a single segment, we can just return it directly
    if (count == 1)
        return CFRetain(CFArrayGetValueAtIndex(array));

    // we should not get to this point if there are not multiple segments
    if (count < 2)
        return NULL;

    // allocate a single mutable data to concatenate the data array into
    CFMutableDataRef data(CFDataCreateMutable(CFGetAllocator(array), 0));

    for (CFIndex index(0); index != count; ++index) {
        CFDataRef value((CFDataRef) CFArrayGetValueAtIndex(array, index));
        CFDataAppendBytes(CFDataGetBytePtr(value), CFDataGetLength(value));
    }

    return data;
}

[bogardon]

@saurik

Where did you obtain the implementation details?

Anyways, I used a breakpoint and started repeating these calls in LLDB:

po [NSNumber numberWithInt:(int)cachedResponse.data.retainCount]

the number returned increments with each call, is this sufficient proof yet? Just to be extra sure, I took LLDB out of the equation and simply called cachedResponse.data a couple times in succession, the retain count still increments without fail.

I encourage you to try this yourself on an iOS 5 device, it should be extremely self-evident. Furthermore, by using a hacky patch inside - (void)encodeWithCoder:(NSCoder *)coder, I eliminated every single leak Instruments was reporting.

To back up your claim about thread safety, it is true that this steady increase is not deterministic with every request, but will happen frequently enough to be alarming.

At this point, I am fairly confident in the leak's existence.

[icoigo]

The leak only happens on iOS 5.0+, not for iOS 4.3

As what I said before, it will cause the leak even I call
cachedResponse.data

[bogardon]

@icoigo

Ah I see, I must of missed it in your post. I couldn't infer that you meant the data object was leaking or that it caused a leak. Thanks for confirming my suspicion though.

[steipete]

To get a little off-topic, I just discovered that since iOS5 NSURLCache saves files to disk per default. Check out Caches//Cache.db to see the sqlite file with the responses+metadata saved.

[icoigo]

@steipete Does that mean NSURLCache has on disk cache capability on iOS5 by default?

[steipete]

Yes. http://petersteinberger.com/blog/2012/nsurlcache-uses-a-disk-cache-as-of-ios5/

[saurik]

@bonchon I can read assembly, so I simply translated the implementation in the binary (pulled from the SDK, disassembled with otool and gdb, and with the few selector references looked up using a live running binary) to Objective-C (all your code are belong to me ;P). The comments (and this, we will see, is crucial) were added by me.

(Quick aside: I had already described in my comments that the multi-threading issues that I found were not actually important at the level of NSCachedURLResponse (as the CFCachedURLResponse has an immutable receiverDataArray), and the real bugs in the lower-level won't happen because that code is never used. I maintain that there is not actually proof of a memory leak here yet.)

Ok, on to the meat of my comment: with the clear "yes, I tried just calling .data in a loop and that was sufficient to cause the memory leak", I sat around with gdb+cycript and managed to find both the mistake in my earlier analysis--and the bug in Apple's code--that is causing this to actually be a real memory leak (yes: you are right!).

// the moral equivalent of: data = [[data retain] autorelease];
data = (CFDataRef) CFMakeCollectable(CFRetain(data));

It turns out that my comment there is simply not true: CFMakeCollectable "is a no-op in a reference counted environment" (according to Apple's documentation on "Using CoreFoundation with Garbage Collection"). I have verified that this is truly the case on my iPhone (which I felt the need to do, as the documentation-described behavior that was overly complex ;P).

(gdb) disassemble CFMakeCollectable
Dump of assembler code for function CFMakeCollectable:
0x36e5c680 <CFMakeCollectable+0>:   bx  lr
0x36e5c682 <CFMakeCollectable+2>:   nop
End of assembler dump.

(In fact, the corrected comment would state that CFMakeCollectable(CFRetain(data)) is the moral equivalent of CFRelease([CFRetain(data) retain]), which is then clearly incorrect ;P. In a garbage-collected environment, the retain in CFMakeCollectable's retain/CFRelease is a nop; the CFRelease then pushes the retainCount closer to 0, and if it hits the object just gets to keep existing until it is later collected, as it no longer has any strong references.)

So, now, looking at the reversed implementations I had posted before, we irritatingly see that the extra CFRetain will then only happen in the case where the receiverDataArray has only the single element: in other cases, a new NSMutableData is allocated, and it will correctly end up in the current NSAutoreleasePool (as it was allocated using the static data selector).

That means that it is not sufficient to just check the version of the firmware to see if it is one that has the bug, as the incorrect behavior only happens in some specific circumstances. For me (in Cydia) the solution is quite simple: use dataArray (whenever available) instead of data. This is actually better anyway, as the usage in SDURLCache is to get the total size, and it is silly to actually build and then immediately throw away a stitched data.

However, that solution will not work for many developers (as that's not a public API) and I don't think CFCachedURLResponse is part of the public API at all (so we can't dip lower). I thereby don't actually see a "sane" way to detect whether or not to do an extra release on the result; best I've come up with (and this is really sketchy) is to call .data twice to see if the retainCount seems to demonstrate the bug, and then compensate (praying that it is not used concurrently). It might simply be saner (albeit really irritating) to just go ahead and try to save the file, and then deal with files that were "too large" after-the-fact.

Regardless, @steipete's discovery that the default NSURLCache supports caching to disk is downright epic (like, seriously: wow, thanks so much for noticing and reporting that), and so it might simply be more useful to not fix this bug and instead encourage people to remove the usage of SDURLCache on iOS 5.0+ (I'm going to go ahead and do that to the next release of Cydia).

[steveriggins]

I have an arcified SDURLCache. Is there a fix for this issue? I'm a bit late in the game to dump SDURLCache and I do not know how we never saw this issue before (we've done plenty of memory tests since December)

[bogardon]

Have any of you tried using the ios5 disk cache yet? It is extremely inconsistent. The issue is described here
http://openradar.appspot.com/11877993
The included fix did not work for me though. Anyone have any insights?

[plamenterziev]

I am experiencing the same memory leak in [NSCachedURLResponse data]. It works fine on iOS < 5 and leaks in iOS 5 and 6 beta 4. I've implemented my own NSURLCache subclass. I traced it with the Heapshop analyzer and [NSCachedURLResponse data] calls extra CFRetain, which is never released. Seems though that what leaks is not the whole data, because if I try to explicitly release it, my app crashes because of release on a deallocated instance. Also the data I get from the cached response is about 30MB, but the leak itself is 2MB.

I am starting to think that what leaks is not the data from the response but the data that forms the NSURLResponse object itself, i.e. HTTP headers, etc.

The leak still exists even if I call [super removeAllCachedResponses]

[bogardon]

@plamenterziev check out this gist

https://gist.github.com/3245415

You have to download FMDB, but this works pretty well for me.

[asarazan]

Just adding my 2c here-- NSURLCache is completely unusable for disk caching. As far as I can tell, @bonchon's solution with FMDB still triggers the memory leak that was showing up in SDURLCache.

From what I've seen, any code that instantiates an NSCachedURLResponse will trigger the leak. Should I give up trying to work around this? It's causing an unacceptable number of crashes in my app.

Maybe there's some clever swizzling solution? I don't know if there's some workaround to reference the class' private variables and swizzle the data accessor that Saurik disassembled, but that would be pretty amazing if so.

[asarazan]

I noticed that there were leaks in Instruments that didn't list SDURLCache in the stack trace, so I implemented a more general swizzled solution for the over-retain problem:

https://gist.github.com/4265969

It's a little scary, and the @synchronized might hurt performance a bit, but my Leaks instrument is now 100% clean, and I haven't seen a crash yet.

Thoughts?

[bogardon]

sigh...did someone file a bug about this yet? i guess i'm just going to check retain counts and autorelease to balance manually...

    NSData *d1 = cachedURLResponse.data;
    NSUInteger d1c = d1.retainCount;
    NSData *d2 = cachedURLResponse.data;
    NSUInteger d2c = d2.retainCount;
    if (d1 == d2 && d1c < d2c) {
        [d1 release];
        [d2 release];
    }

super hacky but atleast instruments is happy XD

[asarazan]

If somebody has filed a report, I'll be happy to +1 it. Otherwise it will be a while before I have time to file one.

@bonchon I'd strongly recommend you take the swizzling route linked above, as I believe there are leaky objects being instantiated outside of SDURLCache's stack, and this solution fixes those as well.

I've confirmed that the bug exists all the way up until the latest <redacted>, so I'm just setting that as the max version to get the fix. Subsequent os versions will just be leaky until we can verify and push a new app build. It's not a completely unmanageable flow, and at this point I'm just glad to finally have a working solution.

[asarazan]

According to the developer forums, the bug has been filed as Bug ID# 12459632. Unfortunately, trying to follow that link ends up with a server error. It'd be funny if it weren't completely ruining my life right now.

Furthermore, we ran my fix through QA last night, and we received one crash somewhere deep in the CFNetwork stack. I'm going to try to tighten up the code and see if we can't get it airtight. Stack trace for crash thread is here

[jkubicek]

The issue has been fixed in iOS 7 by adding the returned data object to the autorelease pool, which has the pleasant side-effect of still triggering any if statements that check retain count, then crashing when the autorelease pool is eventually drained. Hacks upon hacks upon hacks solution:

    int retainCount1, retainCount2;
    @autoreleasepool {
        retainCount1 = self.data.retainCount;
    }
    @autoreleasepool {
        retainCount2 = self.data.retainCount;
    }
    if (retainCount1 == retainCount2)
    {
        data = [self data];
    }
    else
    {
        data = [self data].autorelease.autorelease.autorelease;
    }

[asarazan]

@jkubicek The simplest to handle this is probably to do a check at app launch to see if the hack should be disabled altogether, then run your hack as normal during runtime. You can see how we do it at https://gist.github.com/asarazan/4265969 which was referenced in our blog post about the subject: http://tech.cueup.com/blog/2013/06/14/how-apples-garbage-collector-trashed-our-ios-app