Pull request: Fix MRU eviction bugs that render the wrong contents for images

Edit
------
Introduce the concept of in use entries.

Entries that are in use are immune from eviction. This concept is needed
to protect the bits that are currently being referenced by a UIImage/CGImageRef
from being overwritten.

Note also that inUse is a refcounted concept since multiple FICImageTableEntry's
can be allocated against the same entityUUID/bits in the backing file.

A side effect of this change is that the imageFormat's maximumCount is no
longer a hard limit since it is possible that all maximumCount entries could
be in use.

Author: epatey

FastImageCache/FICImageTable.m

@@ -59,6 +59,7 @@ @interface FICImageTable () {
     NSMutableDictionary *_sourceImageMap;   // Key: entity UUID, value: source image UUID
     NSMutableIndexSet *_occupiedIndexes;
     NSMutableOrderedSet *_MRUEntries;
+    NSCountedSet *_inUseEntries;
     NSDictionary *_imageFormatDictionary;
 }
 
@@ -144,6 +145,8 @@ - (instancetype)initWithFormat:(FICImageFormat *)imageFormat {
         _occupiedIndexes = [[NSMutableIndexSet alloc] init];
 
         _MRUEntries = [[NSMutableOrderedSet alloc] init];
+        _inUseEntries = [NSCountedSet set];
+
         _sourceImageMap = [[NSMutableDictionary alloc] init];
 
         _recentChunks = [[NSMutableArray alloc] init];
@@ -163,6 +166,10 @@ - (instancetype)initWithFormat:(FICImageFormat *)imageFormat {
             NSInteger goalChunkLength = 2 * (1024 * 1024);
             NSInteger goalEntriesPerChunk = goalChunkLength / _entryLength;
             _entriesPerChunk = MAX(4, goalEntriesPerChunk);
+            if ([self _maximumCount] > [_imageFormat maximumCount]) {
+                NSString *message = [NSString stringWithFormat:@"*** FIC Warning: growing desired maximumCount (%d) for format %@ to fill a chunk (%d)", [_imageFormat maximumCount], [_imageFormat name], [self _maximumCount]];
+                [[FICImageCache sharedImageCache] _logMessage:message];
+            }
             _chunkLength = (size_t)(_entryLength * _entriesPerChunk);
 
             _fileLength = lseek(_fileDescriptor, 0, SEEK_END);
@@ -263,8 +270,7 @@ - (void)setEntryForEntityUUID:(NSString *)entityUUID sourceImageUUID:(NSString *
             newEntryIndex = [self _nextEntryIndex];
 
             if (newEntryIndex >= _entryCount) {
-                NSInteger maximumEntryCount = [_imageFormat maximumCount];
-                NSInteger newEntryCount = MIN(maximumEntryCount, _entryCount + MAX(_entriesPerChunk, newEntryIndex - _entryCount + 1));
+                NSInteger newEntryCount = _entryCount + MAX(_entriesPerChunk, newEntryIndex - _entryCount + 1);
                 [self _setEntryCount:newEntryCount];
             }
         }
@@ -330,6 +336,12 @@ - (UIImage *)newImageForEntityUUID:(NSString *)entityUUID sourceImageUUID:(NSStr
                 // Create CGImageRef whose backing store *is* the mapped image table entry. We avoid a memcpy this way.
                 CGDataProviderRef dataProvider = CGDataProviderCreateWithData((__bridge_retained void *)entryData, [entryData bytes], [entryData imageLength], _FICReleaseImageData);
 
+                [_inUseEntries addObject:entityUUID];
+                __weak FICImageTable *weakSelf = self;
+                [entryData executeBlockOnDealloc:^{
+                    [weakSelf removeInUseForEntityUUID:entityUUID];
+                }];
+
                 CGSize pixelSize = [_imageFormat pixelSize];
                 CGBitmapInfo bitmapInfo = [_imageFormat bitmapInfo];
                 NSInteger bitsPerComponent = [_imageFormat bitsPerComponent];
@@ -357,7 +369,15 @@ - (UIImage *)newImageForEntityUUID:(NSString *)entityUUID sourceImageUUID:(NSStr
 }
 
 static void _FICReleaseImageData(void *info, const void *data, size_t size) {
-    CFRelease(info);
+    if (info) {
+        CFRelease(info);
+    }
+}
+
+- (void)removeInUseForEntityUUID:(NSString *)entityUUID {
+    [_lock lock];
+    [_inUseEntries removeObject:entityUUID];
+    [_lock unlock];
 }
 
 - (void)deleteEntryForEntityUUID:(NSString *)entityUUID {
@@ -411,6 +431,10 @@ - (BOOL)entryExistsForEntityUUID:(NSString *)entityUUID sourceImageUUID:(NSStrin
 
 #pragma mark - Working with Entries
 
+- (int)_maximumCount {
+    return MAX([_imageFormat maximumCount], _entriesPerChunk);
+}
+
 - (void)_setEntryCount:(NSInteger)entryCount {
     if (entryCount != _entryCount) {        
         off_t fileLength = entryCount * _entryLength;
@@ -460,15 +484,37 @@ - (NSInteger)_nextEntryIndex {
         index = _entryCount;
     }
 
-    if (index >= [_imageFormat maximumCount] && [_MRUEntries count]) {
+    if (index >= [self _maximumCount] && [_MRUEntries count]) {
         // Evict the oldest/least-recently accessed entry here
-        [self deleteEntryForEntityUUID:[_MRUEntries lastObject]];
-        index = [self _nextEntryIndex];
+
+        NSString *oldestEvictableEntityUUID = [self oldestEvictableEntityUUID];
+        if (oldestEvictableEntityUUID) {
+            [self deleteEntryForEntityUUID:oldestEvictableEntityUUID];
+            index = [self _nextEntryIndex];
+        }
+    }
+
+    if (index >= [self _maximumCount]) {
+        NSString *message = [NSString stringWithFormat:@"FICImageTable - unable to evict entry from table '%@' to make room. New index %d, desired max %d", [_imageFormat name], index, [self _maximumCount]];
+        [[FICImageCache sharedImageCache] _logMessage:message];
     }
 
     return index;
 }
 
+- (NSString *)oldestEvictableEntityUUID {
+    NSString *uuid = nil;
+    for (NSInteger i = [_MRUEntries count] - 1; i >= 0; i--) {
+        NSString *candidateUUID = [_MRUEntries objectAtIndex:i];
+        if (![_inUseEntries containsObject:candidateUUID]) {
+            uuid = candidateUUID;
+            break;
+        }
+    }
+
+    return uuid;
+}
+
 - (NSInteger)_indexOfEntryForEntityUUID:(NSString *)entityUUID {
     NSInteger index = NSNotFound;
     if (_indexMap != nil && entityUUID != nil) {
@@ -570,6 +616,7 @@ - (void)reset {
 
     [_indexMap removeAllObjects];
     [_occupiedIndexes removeAllIndexes];
+    [_inUseEntries removeAllObjects];
     [_MRUEntries removeAllObjects];
     [_sourceImageMap removeAllObjects];
 

FastImageCache/FICImageTableEntry.h

@@ -69,6 +69,8 @@ typedef struct {
  */
 - (instancetype)initWithImageTableChunk:(FICImageTableChunk *)imageTableChunk bytes:(void *)bytes length:(size_t)length;
 
+- (void)executeBlockOnDealloc:(dispatch_block_t)block;
+
 ///--------------------------------------------
 /// @name Flushing a Modified Image Table Entry
 ///--------------------------------------------

FastImageCache/FICImageTableEntry.m

@@ -21,6 +21,7 @@ @interface FICImageTableEntry () {
     FICImageTableChunk *_imageTableChunk;
     void *_bytes;
     size_t _length;
+    dispatch_block_t _deallocBlock;
 }
 
 @end
@@ -68,6 +69,16 @@ - (id)initWithImageTableChunk:(FICImageTableChunk *)imageTableChunk bytes:(void
     return self;
 }
 
+- (void)executeBlockOnDealloc:(dispatch_block_t)block {
+    _deallocBlock = [block copy];
+}
+
+- (void)dealloc {
+    if (_deallocBlock) {
+        _deallocBlock();
+    }
+}
+
 #pragma mark - Other Accessors
 
 + (NSInteger)metadataVersion {