Add some defensive cleanup of cancelBlock in RCTImageLoader

Reviewed By: mmmulani Differential Revision: D3742177 fbshipit-source-id: 1cd16c2519052ec5811ecadf2530a5846b4ae1bc
2016-08-19 10:31:42 -07:00 · 2016-08-19 10:31:42 -07:00 · 14188289fc
--- a/Libraries/Image/RCTImageLoader.m
+++ b/Libraries/Image/RCTImageLoader.m
@ -294,10 +294,6 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
                                                    progressBlock:(RCTImageLoaderProgressBlock)progressHandler
                                                  completionBlock:(void (^)(NSError *error, id imageOrData, BOOL cacheResult, NSString *fetchDate))completionBlock
 {
-  __block volatile uint32_t cancelled = 0;
-  __block dispatch_block_t cancelLoad = nil;
-  __weak RCTImageLoader *weakSelf = self;
-
  {
    NSMutableURLRequest *mutableRequest = [request mutableCopy];
    [NSURLProtocol setProperty:@"RCTImageLoader"
@ -316,9 +312,14 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
  BOOL requiresScheduling = [loadHandler respondsToSelector:@selector(requiresScheduling)] ?
      [loadHandler requiresScheduling] : YES;

+  __block volatile uint32_t cancelled = 0;
+  __block dispatch_block_t cancelLoad = nil;
  void (^completionHandler)(NSError *, id, NSString *) = ^(NSError *error, id imageOrData, NSString *fetchDate) {
+    cancelLoad = nil;
+
    BOOL cacheResult = [loadHandler respondsToSelector:@selector(shouldCacheLoadedImages)] ?
      [loadHandler shouldCacheLoadedImages] : YES;
+
    // If we've received an image, we should try to set it synchronously,
    // if it's data, do decoding on a background thread.
    if (RCTIsMainQueue() && ![imageOrData isKindOfClass:[UIImage class]]) {
@ -352,6 +353,7 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
    [self setUp];
  }

+  __weak RCTImageLoader *weakSelf = self;
  dispatch_async(_URLRequestQueue, ^{
    __typeof(self) strongSelf = weakSelf;
    if (cancelled || !strongSelf) {
@ -364,7 +366,7 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
                                          scale:scale
                                     resizeMode:resizeMode
                                progressHandler:progressHandler
-                              completionHandler:^(NSError *error, UIImage *image){
+                              completionHandler:^(NSError *error, UIImage *image) {
                                completionHandler(error, image, nil);
                              }];
    } else {
@ -378,6 +380,7 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
  return ^{
    if (cancelLoad) {
      cancelLoad();
+      cancelLoad = nil;
    }
    OSAtomicOr32Barrier(1, &cancelled);
  };
@ -464,7 +467,6 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
      // Prepare for next task
      [strongSelf dequeueTasks];
    });
-
  }];

  task.downloadProgressBlock = progressHandler;
@ -488,13 +490,19 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
                                                     scale:(CGFloat)scale
                                                   clipped:(BOOL)clipped
                                                resizeMode:(RCTResizeMode)resizeMode
-                                             progressBlock:(RCTImageLoaderProgressBlock)progressHandler
+                                             progressBlock:(RCTImageLoaderProgressBlock)progressBlock
                                           completionBlock:(RCTImageLoaderCompletionBlock)completionBlock
 {
  __block volatile uint32_t cancelled = 0;
-  __block void(^cancelLoad)(void) = nil;
-  __weak RCTImageLoader *weakSelf = self;
+  __block dispatch_block_t cancelLoad = nil;
+  dispatch_block_t cancellationBlock = ^{
+    if (cancelLoad) {
+      cancelLoad();
+    }
+    OSAtomicOr32Barrier(1, &cancelled);
+  };

+  __weak RCTImageLoader *weakSelf = self;
  void (^completionHandler)(NSError *, id, BOOL, NSString *) = ^(NSError *error, id imageOrData, BOOL cacheResult, NSString *fetchDate) {
    __typeof(self) strongSelf = weakSelf;
    if (cancelled || !strongSelf) {
@ -502,6 +510,7 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
    }

    if (!imageOrData || [imageOrData isKindOfClass:[UIImage class]]) {
+      cancelLoad = nil;
      completionBlock(error, imageOrData);
      return;
    }
@ -509,27 +518,29 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
    // Check decoded image cache
    if (cacheResult) {
      UIImage *image = [[strongSelf imageCache] imageForUrl:imageURLRequest.URL.absoluteString
-                                                          size:size
-                                                         scale:scale
-                                                    resizeMode:resizeMode
-                                                  responseDate:fetchDate];
+                                                       size:size
+                                                      scale:scale
+                                                 resizeMode:resizeMode
+                                               responseDate:fetchDate];
      if (image) {
+        cancelLoad = nil;
        completionBlock(nil, image);
        return;
      }
    }

-    // Store decoded image in cache
-    RCTImageLoaderCompletionBlock cacheResultHandler = ^(NSError *error_, UIImage *image) {
-      if (image) {
+    RCTImageLoaderCompletionBlock decodeCompletionHandler = ^(NSError *error_, UIImage *image) {
+      if (cacheResult && image) {
+        // Store decoded image in cache
        [[strongSelf imageCache] addImageToCache:image
-                                                URL:imageURLRequest.URL.absoluteString
-                                               size:size
-                                              scale:scale
-                                         resizeMode:resizeMode
-                                       responseDate:fetchDate];
+                                             URL:imageURLRequest.URL.absoluteString
+                                            size:size
+                                           scale:scale
+                                      resizeMode:resizeMode
+                                    responseDate:fetchDate];
      }

+      cancelLoad = nil;
      completionBlock(error_, image);
    };

@ -538,21 +549,16 @@ static UIImage *RCTResizeImageIfNeeded(UIImage *image,
                                     scale:scale
                                   clipped:clipped
                                resizeMode:resizeMode
-                           completionBlock:cacheResult ? cacheResultHandler: completionBlock];
+                           completionBlock:decodeCompletionHandler];
  };

  cancelLoad = [self _loadImageOrDataWithURLRequest:imageURLRequest
                                               size:size
                                              scale:scale
                                         resizeMode:resizeMode
-                                      progressBlock:progressHandler
+                                      progressBlock:progressBlock
                                    completionBlock:completionHandler];
-  return ^{
-    if (cancelLoad) {
-      cancelLoad();
-    }
-    OSAtomicOr32Barrier(1, &cancelled);
-  };
+  return cancellationBlock;
 }

 - (RCTImageLoaderCancellationBlock)decodeImageData:(NSData *)data
--- a/Libraries/Image/RCTImageView.m
+++ b/Libraries/Image/RCTImageView.m
@ -273,8 +273,11 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init)
    }

    RCTImageSource *source = _imageSource;
-    CGFloat blurRadius = _blurRadius;
    __weak RCTImageView *weakSelf = self;
+    RCTImageLoaderCompletionBlock completionHandler = ^(NSError *error, UIImage *loadedImage) {
+      [weakSelf imageLoaderLoadedImage:loadedImage error:error forImageSource:source];
+    };
+
    _reloadImageCancellationBlock =
    [_bridge.imageLoader loadImageWithURLRequest:source.request
                                            size:imageSize
@ -282,57 +285,64 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init)
                                         clipped:NO
                                      resizeMode:_resizeMode
                                   progressBlock:progressHandler
-                                 completionBlock:^(NSError *error, UIImage *loadedImage) {
-
-      RCTImageView *strongSelf = weakSelf;
-      void (^setImageBlock)(UIImage *) = ^(UIImage *image) {
-        if (![source isEqual:strongSelf.imageSource]) {
-          // Bail out if source has changed since we started loading
-          return;
-        }
-        if (image.reactKeyframeAnimation) {
-          [strongSelf.layer addAnimation:image.reactKeyframeAnimation forKey:@"contents"];
-        } else {
-          [strongSelf.layer removeAnimationForKey:@"contents"];
-          strongSelf.image = image;
-        }
-        if (error) {
-          if (strongSelf->_onError) {
-            strongSelf->_onError(@{ @"error": error.localizedDescription });
-          }
-        } else {
-          if (strongSelf->_onLoad) {
-            strongSelf->_onLoad(nil);
-          }
-        }
-        if (strongSelf->_onLoadEnd) {
-          strongSelf->_onLoadEnd(nil);
-        }
-      };
-
-      if (blurRadius > __FLT_EPSILON__) {
-        // Blur on a background thread to avoid blocking interaction
-        dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-          UIImage *image = RCTBlurredImageWithRadius(loadedImage, blurRadius);
-          RCTExecuteOnMainQueue(^{
-            setImageBlock(image);
-          });
-        });
-      } else {
-        // No blur, so try to set the image on the main thread synchronously to minimize image
-        // flashing. (For instance, if this view gets attached to a window, then -didMoveToWindow
-        // calls -reloadImage, and we want to set the image synchronously if possible so that the
-        // image property is set in the same CATransaction that attaches this view to the window.)
-        RCTExecuteOnMainQueue(^{
-          setImageBlock(loadedImage);
-        });
-      }
-    }];
+                                 completionBlock:completionHandler];
  } else {
    [self clearImage];
  }
 }

+- (void)imageLoaderLoadedImage:(UIImage *)loadedImage error:(NSError *)error forImageSource:(RCTImageSource *)source
+{
+  if (![source isEqual:_imageSource]) {
+    // Bail out if source has changed since we started loading
+    return;
+  }
+
+  if (error) {
+    if (_onError) {
+      _onError(@{ @"error": error.localizedDescription });
+    }
+    if (_onLoadEnd) {
+      _onLoadEnd(nil);
+    }
+    return;
+  }
+
+  void (^setImageBlock)(UIImage *) = ^(UIImage *image) {
+    if (image.reactKeyframeAnimation) {
+      [self.layer addAnimation:image.reactKeyframeAnimation forKey:@"contents"];
+    } else {
+      [self.layer removeAnimationForKey:@"contents"];
+      self.image = image;
+    }
+
+    if (self->_onLoad) {
+      self->_onLoad(nil);
+    }
+    if (self->_onLoadEnd) {
+      self->_onLoadEnd(nil);
+    }
+  };
+
+  if (_blurRadius > __FLT_EPSILON__) {
+    // Blur on a background thread to avoid blocking interaction
+    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+      UIImage *blurredImage = RCTBlurredImageWithRadius(loadedImage, self->_blurRadius);
+      RCTExecuteOnMainQueue(^{
+        setImageBlock(blurredImage);
+      });
+    });
+  } else {
+    // No blur, so try to set the image on the main thread synchronously to minimize image
+    // flashing. (For instance, if this view gets attached to a window, then -didMoveToWindow
+    // calls -reloadImage, and we want to set the image synchronously if possible so that the
+    // image property is set in the same CATransaction that attaches this view to the window.)
+    RCTExecuteOnMainQueue(^{
+      setImageBlock(loadedImage);
+    });
+  }
+}
+
 - (void)reactSetFrame:(CGRect)frame
 {
  [super reactSetFrame:frame];
--- a/Libraries/Image/RCTImageViewManager.m
+++ b/Libraries/Image/RCTImageViewManager.m
@ -49,13 +49,13 @@ RCT_EXPORT_METHOD(getSize:(NSURLRequest *)request
                  errorBlock:(RCTResponseErrorBlock)errorBlock)
 {
  [self.bridge.imageLoader getImageSizeForURLRequest:request
-                                  block:^(NSError *error, CGSize size) {
-                                    if (error) {
-                                      errorBlock(error);
-                                    } else {
-                                      successBlock(@[@(size.width), @(size.height)]);
-                                    }
-                                  }];
+                                               block:^(NSError *error, CGSize size) {
+                                                 if (error) {
+                                                   errorBlock(error);
+                                                 } else {
+                                                   successBlock(@[@(size.width), @(size.height)]);
+                                                 }
+                                               }];
 }

 RCT_EXPORT_METHOD(prefetchImage:(NSURLRequest *)request
--- a/Libraries/Network/RCTNetworkTask.m
+++ b/Libraries/Network/RCTNetworkTask.m
@ -78,9 +78,9 @@ RCT_NOT_IMPLEMENTED(- (instancetype)init)
 - (void)cancel
 {
  _status = RCTNetworkTaskFinished;
-  __strong id strongToken = _requestToken;
-  if (strongToken && [_handler respondsToSelector:@selector(cancelRequest:)]) {
-    [_handler cancelRequest:strongToken];
+  id token = _requestToken;
+  if (token && [_handler respondsToSelector:@selector(cancelRequest:)]) {
+    [_handler cancelRequest:token];
  }
  [self invalidate];
 }