From 0fe074acbd460fc34aa8f43e51d30ed7e149cebb Mon Sep 17 00:00:00 2001 From: Nick Lockwood Date: Fri, 20 Nov 2015 05:15:49 -0800 Subject: [PATCH] Removed all calls to [UIImage imageWithData:] on a background thread Summary: public I had previously assumed (based on past experience and common wisdom) that `[UIImage imageWithData:]` was safe to call concurrently and/or off the main thread, but it seems that may not be the case (see https://github.com/AFNetworking/AFNetworking/pull/2815). This diff replaces `[UIImage imageWithData:]` with ImageIO-based decoding wherever possible, and ensures that it is called on the main thread wherever that's not possible/convenient. I've also serialized access to the `NSURLCache` inside `RCTImageLoader`, which was causing a separate-but-similar crash when loading images. Reviewed By: fkgozali Differential Revision: D2678369 fb-gh-sync-id: 74d033dafcf6c412556e4c96f5ac5d3432298b18 --- Libraries/Image/RCTImageLoader.m | 222 ++++++++++++++----------- Libraries/Image/RCTImageStoreManager.m | 3 +- Libraries/Image/RCTImageUtils.m | 3 +- React/Base/RCTConvert.m | 14 +- 4 files changed, 137 insertions(+), 105 deletions(-) diff --git a/Libraries/Image/RCTImageLoader.m b/Libraries/Image/RCTImageLoader.m index b345591a3..8c77828b0 100644 --- a/Libraries/Image/RCTImageLoader.m +++ b/Libraries/Image/RCTImageLoader.m @@ -37,7 +37,8 @@ { NSArray> *_loaders; NSArray> *_decoders; - NSURLCache *_cache; + dispatch_queue_t _URLCacheQueue; + NSURLCache *_URLCache; } @synthesize bridge = _bridge; @@ -87,9 +88,10 @@ RCT_EXPORT_MODULE() _bridge = bridge; _loaders = loaders; _decoders = decoders; - _cache = [[NSURLCache alloc] initWithMemoryCapacity:5 * 1024 * 1024 // 5MB - diskCapacity:200 * 1024 * 1024 // 200MB - diskPath:@"React/RCTImageDownloader"]; + _URLCacheQueue = dispatch_queue_create("com.facebook.react.ImageLoaderURLCacheQueue", DISPATCH_QUEUE_SERIAL); + _URLCache = [[NSURLCache alloc] initWithMemoryCapacity:5 * 1024 * 1024 // 5MB + diskCapacity:200 * 1024 * 1024 // 200MB + diskPath:@"React/RCTImageDownloader"]; } - (id)imageURLLoaderForURL:(NSURL *)URL @@ -185,12 +187,10 @@ RCT_EXPORT_MODULE() progressBlock:(RCTImageLoaderProgressBlock)progressHandler completionBlock:(RCTImageLoaderCompletionBlock)completionBlock { - if (imageTag.length == 0) { - RCTLogWarn(@"source.uri should not be an empty string "); - return ^{}; - } - __block volatile uint32_t cancelled = 0; + __block void(^cancelLoad)(void) = nil; + __weak RCTImageLoader *weakSelf = self; + RCTImageLoaderCompletionBlock completionHandler = ^(NSError *error, UIImage *image) { if ([NSThread isMainThread]) { @@ -206,111 +206,133 @@ RCT_EXPORT_MODULE() } }; - // Find suitable image URL loader - NSURLRequest *request = [RCTConvert NSURLRequest:imageTag]; - id loadHandler = [self imageURLLoaderForURL:request.URL]; - if (loadHandler) { - return [loadHandler loadImageForURL:request.URL - size:size - scale:scale - resizeMode:resizeMode - progressHandler:progressHandler - completionHandler:completionHandler] ?: ^{}; - } - - // Check if networking module is available - if (RCT_DEBUG && ![_bridge respondsToSelector:@selector(networking)]) { - RCTLogError(@"No suitable image URL loader found for %@. You may need to " - " import the RCTNetworking library in order to load images.", - imageTag); + if (imageTag.length == 0) { + completionHandler(RCTErrorWithMessage(@"source.uri should not be an empty string"), nil); return ^{}; } - // Check if networking module can load image - if (RCT_DEBUG && ![_bridge.networking canHandleRequest:request]) { - RCTLogError(@"No suitable image URL loader found for %@", imageTag); - return ^{}; - } - - // Use networking module to load image - __weak RCTImageLoader *weakSelf = self; - __block RCTImageLoaderCancellationBlock decodeCancel = nil; - RCTURLRequestCompletionBlock processResponse = - ^(NSURLResponse *response, NSData *data, NSError *error) { - - // Check for system errors - if (error) { - completionHandler(error, nil); - return; - } else if (!data) { - completionHandler(RCTErrorWithMessage(@"Unknown image download error"), nil); + // All access to URL cache must be serialized + dispatch_async(_URLCacheQueue, ^{ + RCTImageLoader *strongSelf = weakSelf; + if (cancelled || !strongSelf) { return; } - // Check for http errors - if ([response isKindOfClass:[NSHTTPURLResponse class]]) { - NSInteger statusCode = ((NSHTTPURLResponse *)response).statusCode; - if (statusCode != 200) { - completionHandler([[NSError alloc] initWithDomain:NSURLErrorDomain - code:statusCode - userInfo:nil], nil); + // Find suitable image URL loader + NSURLRequest *request = [RCTConvert NSURLRequest:imageTag]; + id loadHandler = [strongSelf imageURLLoaderForURL:request.URL]; + if (loadHandler) { + cancelLoad = [loadHandler loadImageForURL:request.URL + size:size + scale:scale + resizeMode:resizeMode + progressHandler:progressHandler + completionHandler:completionHandler] ?: ^{}; + return; + } + + // Check if networking module is available + if (RCT_DEBUG && ![_bridge respondsToSelector:@selector(networking)]) { + RCTLogError(@"No suitable image URL loader found for %@. You may need to " + " import the RCTNetworking library in order to load images.", + imageTag); + return; + } + + // Check if networking module can load image + if (RCT_DEBUG && ![_bridge.networking canHandleRequest:request]) { + RCTLogError(@"No suitable image URL loader found for %@", imageTag); + return; + } + + // Use networking module to load image + __block RCTImageLoaderCancellationBlock cancelDecode = nil; + RCTURLRequestCompletionBlock processResponse = + ^(NSURLResponse *response, NSData *data, NSError *error) { + + // Check for system errors + if (error) { + completionHandler(error, nil); + return; + } else if (!data) { + completionHandler(RCTErrorWithMessage(@"Unknown image download error"), nil); return; } + + // Check for http errors + if ([response isKindOfClass:[NSHTTPURLResponse class]]) { + NSInteger statusCode = ((NSHTTPURLResponse *)response).statusCode; + if (statusCode != 200) { + completionHandler([[NSError alloc] initWithDomain:NSURLErrorDomain + code:statusCode + userInfo:nil], nil); + return; + } + } + + // Decode image + cancelDecode = [strongSelf decodeImageData:data + size:size + scale:scale + resizeMode:resizeMode + completionBlock:completionHandler]; + }; + + // Add missing png extension + if (request.URL.fileURL && request.URL.pathExtension.length == 0) { + NSMutableURLRequest *mutableRequest = [request mutableCopy]; + mutableRequest.URL = [NSURL fileURLWithPath:[request.URL.path stringByAppendingPathExtension:@"png"]]; + request = mutableRequest; } - // Decode image - decodeCancel = [weakSelf decodeImageData:data - size:size - scale:scale - resizeMode:resizeMode - completionBlock:completionHandler]; - }; - - // Check for cached response before reloading - // TODO: move URL cache out of RCTImageLoader into its own module - NSCachedURLResponse *cachedResponse = [_cache cachedResponseForRequest:request]; - if (cachedResponse) { - processResponse(cachedResponse.response, cachedResponse.data, nil); - return ^{}; - } - - // Add missing png extension - if (request.URL.fileURL && request.URL.pathExtension.length == 0) { - NSMutableURLRequest *mutableRequest = [request mutableCopy]; - mutableRequest.URL = [NSURL fileURLWithPath:[request.URL.path stringByAppendingPathExtension:@"png"]]; - request = mutableRequest; - } - - // Download image - RCTNetworkTask *task = [_bridge.networking networkTaskWithRequest:request completionBlock: - ^(NSURLResponse *response, NSData *data, NSError *error) { - if (error) { - completionHandler(error, nil); - return; - } - - // Cache the response + // Check for cached response before reloading // TODO: move URL cache out of RCTImageLoader into its own module - RCTImageLoader *strongSelf = weakSelf; - BOOL isHTTPRequest = [request.URL.scheme hasPrefix:@"http"]; - [strongSelf->_cache storeCachedResponse: - [[NSCachedURLResponse alloc] initWithResponse:response - data:data - userInfo:nil - storagePolicy:isHTTPRequest ? NSURLCacheStorageAllowed: NSURLCacheStorageAllowedInMemoryOnly] - forRequest:request]; + NSCachedURLResponse *cachedResponse = [_URLCache cachedResponseForRequest:request]; + if (cachedResponse) { + processResponse(cachedResponse.response, cachedResponse.data, nil); + } - // Process image data - processResponse(response, data, nil); + // Download image + RCTNetworkTask *task = [_bridge.networking networkTaskWithRequest:request completionBlock: + ^(NSURLResponse *response, NSData *data, NSError *error) { + if (error) { + completionHandler(error, nil); + return; + } - }]; - task.downloadProgressBlock = progressHandler; - [task start]; + dispatch_async(_URLCacheQueue, ^{ + + // Cache the response + // TODO: move URL cache out of RCTImageLoader into its own module + BOOL isHTTPRequest = [request.URL.scheme hasPrefix:@"http"]; + [strongSelf->_URLCache storeCachedResponse: + [[NSCachedURLResponse alloc] initWithResponse:response + data:data + userInfo:nil + storagePolicy:isHTTPRequest ? NSURLCacheStorageAllowed: NSURLCacheStorageAllowedInMemoryOnly] + forRequest:request]; + + // Process image data + processResponse(response, data, nil); + + }); + + }]; + task.downloadProgressBlock = progressHandler; + [task start]; + + cancelLoad = ^{ + [task cancel]; + if (cancelDecode) { + cancelDecode(); + } + }; + + }); return ^{ - [task cancel]; - if (decodeCancel) { - decodeCancel(); + if (cancelLoad) { + cancelLoad(); } OSAtomicOr32Barrier(1, &cancelled); }; @@ -342,7 +364,7 @@ RCT_EXPORT_MODULE() if (cancelled) { return; } - UIImage *image = [UIImage imageWithData:data scale:scale]; + UIImage *image = RCTDecodeImageWithData(data, size, scale, resizeMode); if (image) { completionHandler(nil, image); } else { diff --git a/Libraries/Image/RCTImageStoreManager.m b/Libraries/Image/RCTImageStoreManager.m index f34a13e8d..fe080101e 100644 --- a/Libraries/Image/RCTImageStoreManager.m +++ b/Libraries/Image/RCTImageStoreManager.m @@ -213,9 +213,8 @@ RCT_EXPORT_METHOD(addImageFromBase64:(NSString *)base64String RCTAssertParam(block); dispatch_async(_methodQueue, ^{ NSData *imageData = _store[imageTag]; - UIImage *image = [UIImage imageWithData:imageData]; dispatch_async(dispatch_get_main_queue(), ^{ - block(image); + block([UIImage imageWithData:imageData]); }); }); } diff --git a/Libraries/Image/RCTImageUtils.m b/Libraries/Image/RCTImageUtils.m index c05bf526b..3146a5b56 100644 --- a/Libraries/Image/RCTImageUtils.m +++ b/Libraries/Image/RCTImageUtils.m @@ -255,7 +255,8 @@ UIImage *RCTDecodeImageWithData(NSData *data, // adjust scale size_t actualWidth = CGImageGetWidth(imageRef); - CGFloat scale = actualWidth / targetSize.width; + CGFloat scale = actualWidth / targetSize.width * destScale; + // return image UIImage *image = [UIImage imageWithCGImage:imageRef scale:scale diff --git a/React/Base/RCTConvert.m b/React/Base/RCTConvert.m index 2531b69b9..53c6b9cad 100644 --- a/React/Base/RCTConvert.m +++ b/React/Base/RCTConvert.m @@ -429,7 +429,18 @@ RCT_CGSTRUCT_CONVERTER(CGAffineTransform, (@[ return nil; } - UIImage *image; + __block UIImage *image; + if (![NSThread isMainThread]) { + // It seems that none of the UIImage loading methods can be guaranteed + // thread safe, so we'll pick the lesser of two evils here and block rather + // than run the risk of crashing + RCTLogWarn(@"Calling [RCTConvert UIImage:] on a background thread is not recommended"); + dispatch_sync(dispatch_get_main_queue(), ^{ + image = [self UIImage:json]; + }); + return image; + } + NSString *path; CGFloat scale = 0.0; BOOL isPackagerAsset = NO; @@ -452,7 +463,6 @@ RCT_CGSTRUCT_CONVERTER(CGAffineTransform, (@[ if (RCTIsXCAssetURL(URL)) { // Image may reside inside a .car file, in which case we have no choice // but to use +[UIImage imageNamed] - but this method isn't thread safe - RCTAssertMainThread(); NSString *assetName = RCTBundlePathForURL(URL); image = [UIImage imageNamed:assetName]; } else {