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 AFNetworking/AFNetworking#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

Author: nicklockwood

Libraries/Image/RCTImageLoader.m

@@ -37,7 +37,8 @@ @implementation RCTImageLoader
 {
   NSArray<id<RCTImageURLLoader>> *_loaders;
   NSArray<id<RCTImageDataDecoder>> *_decoders;
-  NSURLCache *_cache;
+  dispatch_queue_t _URLCacheQueue;
+  NSURLCache *_URLCache;
 }
 
 @synthesize bridge = _bridge;
@@ -87,9 +88,10 @@ - (void)setBridge:(RCTBridge *)bridge
   _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<RCTImageURLLoader>)imageURLLoaderForURL:(NSURL *)URL
@@ -185,12 +187,10 @@ - (RCTImageLoaderCancellationBlock)loadImageWithTag:(NSString *)imageTag
                                       progressBlock:(RCTImageLoaderProgressBlock)progressHandler
                                     completionBlock:(RCTImageLoaderCompletionBlock)completionBlock
 {
-  if (imageTag.length == 0) {
-    RCTLogWarn(@"source.uri should not be an empty string <Native>");
-    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 @@ - (RCTImageLoaderCancellationBlock)loadImageWithTag:(NSString *)imageTag
     }
   };
 
-  // Find suitable image URL loader
-  NSURLRequest *request = [RCTConvert NSURLRequest:imageTag];
-  id<RCTImageURLLoader> 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 ^{};
-  }
+  // All access to URL cache must be serialized
+  dispatch_async(_URLCacheQueue, ^{
+    RCTImageLoader *strongSelf = weakSelf;
+    if (cancelled || !strongSelf) {
+      return;
+    }
 
-  // Use networking module to load image
-  __weak RCTImageLoader *weakSelf = self;
-  __block RCTImageLoaderCancellationBlock decodeCancel = nil;
-  RCTURLRequestCompletionBlock processResponse =
-  ^(NSURLResponse *response, NSData *data, NSError *error) {
+    // Find suitable image URL loader
+    NSURLRequest *request = [RCTConvert NSURLRequest:imageTag];
+    id<RCTImageURLLoader> loadHandler = [strongSelf imageURLLoaderForURL:request.URL];
+    if (loadHandler) {
+      cancelLoad = [loadHandler loadImageForURL:request.URL
+                                           size:size
+                                          scale:scale
+                                     resizeMode:resizeMode
+                                progressHandler:progressHandler
+                              completionHandler:completionHandler] ?: ^{};
+      return;
+    }
 
-    // Check for system errors
-    if (error) {
-      completionHandler(error, nil);
+    // 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;
-    } else if (!data) {
-      completionHandler(RCTErrorWithMessage(@"Unknown image download error"), nil);
+    }
+
+    // Check if networking module can load image
+    if (RCT_DEBUG && ![_bridge.networking canHandleRequest:request]) {
+      RCTLogError(@"No suitable image URL loader found for %@", imageTag);
       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);
+    // 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;
       }
-    }
 
-    // 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 ^{};
-  }
+      // 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;
+        }
+      }
 
-  // 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
+      cancelDecode = [strongSelf decodeImageData:data
+                                            size:size
+                                           scale:scale
+                                      resizeMode:resizeMode
+                                 completionBlock:completionHandler];
+    };
 
-  // Download image
-  RCTNetworkTask *task = [_bridge.networking networkTaskWithRequest:request completionBlock:
-                          ^(NSURLResponse *response, NSData *data, NSError *error) {
-    if (error) {
-      completionHandler(error, 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;
     }
 
-    // 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 @@ - (RCTImageLoaderCancellationBlock)decodeImageData:(NSData *)data
       if (cancelled) {
         return;
       }
-      UIImage *image = [UIImage imageWithData:data scale:scale];
+      UIImage *image = RCTDecodeImageWithData(data, size, scale, resizeMode);
       if (image) {
         completionHandler(nil, image);
       } else {

Libraries/Image/RCTImageStoreManager.m

@@ -213,9 +213,8 @@ - (void)getImageForTag:(NSString *)imageTag withBlock:(void (^)(UIImage *image))
   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]);
     });
   });
 }

Libraries/Image/RCTImageUtils.m

@@ -255,7 +255,8 @@ CGSize RCTSizeInPixels(CGSize pointSize, CGFloat scale)
 
   // 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

React/Base/RCTConvert.m

@@ -429,7 +429,18 @@ + (UIImage *)UIImage:(id)json
     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 @@ + (UIImage *)UIImage:(id)json
     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 {