Fabric: Fixed object ownership problem in ImageManager

Summary:
Sometimes, when we deal with ImageRequest and ImageResponseObserverCoordinator we subscribe for status (or access the coordinator) without owning an ImageRequest. In those cases, we have to retain the coordinator explicitly.
For those cases, ImageRequest now exposes `ImageResponseObserverCoordinator` as a `std::shared_ptr`.

Eg, concretely in the code, `completionBlock` and `progressBlock` copied a raw pointer to the observer inside which can lead to a crash when ImageRequest is being deallocated before we received an image data.

Reviewed By: JoshuaGross

Differential Revision: D14072079

fbshipit-source-id: e10120bc05bf685e288f7b3d69092714dcd91d43

React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm

@@ -83,7 +83,7 @@
   bool havePreviousData = previousData != nullptr;
 
   if (!havePreviousData || _imageLocalData->getImageSource() != previousData->getImageSource()) {
-    self.coordinator = _imageLocalData->getImageRequest().getObserverCoordinator();
+    self.coordinator = &_imageLocalData->getImageRequest().getObserverCoordinator();
 
     // Loading actually starts a little before this, but this is the first time we know
     // the image is loading and can fire an event from this component

React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm

@@ -161,18 +161,18 @@ using namespace facebook::react;
   bool havePreviousData = previousData != nullptr;
 
   if (!havePreviousData || _sliderLocalData->getTrackImageSource() != previousData->getTrackImageSource()) {
-    self.trackImageCoordinator = _sliderLocalData->getTrackImageRequest().getObserverCoordinator();
+    self.trackImageCoordinator = &_sliderLocalData->getTrackImageRequest().getObserverCoordinator();
   }
   if (!havePreviousData ||
       _sliderLocalData->getMinimumTrackImageSource() != previousData->getMinimumTrackImageSource()) {
-    self.minimumTrackImageCoordinator = _sliderLocalData->getMinimumTrackImageRequest().getObserverCoordinator();
+    self.minimumTrackImageCoordinator = &_sliderLocalData->getMinimumTrackImageRequest().getObserverCoordinator();
   }
   if (!havePreviousData ||
       _sliderLocalData->getMaximumTrackImageSource() != previousData->getMaximumTrackImageSource()) {
-    self.maximumTrackImageCoordinator = _sliderLocalData->getMaximumTrackImageRequest().getObserverCoordinator();
+    self.maximumTrackImageCoordinator = &_sliderLocalData->getMaximumTrackImageRequest().getObserverCoordinator();
   }
   if (!havePreviousData || _sliderLocalData->getThumbImageSource() != previousData->getThumbImageSource()) {
-    self.thumbImageCoordinator = _sliderLocalData->getThumbImageRequest().getObserverCoordinator();
+    self.thumbImageCoordinator = &_sliderLocalData->getThumbImageRequest().getObserverCoordinator();
   }
 }
 

ReactCommon/fabric/imagemanager/ImageRequest.h

@@ -53,9 +53,18 @@ class ImageRequest final {
   void setCancelationFunction(std::function<void(void)> cancelationFunction);
 
   /*
-   * Get observer coordinator.
+   * Returns stored observer coordinator as a shared pointer.
+   * Retain this *or* `ImageRequest` to ensure a correct lifetime of the object.
    */
-  const ImageResponseObserverCoordinator *getObserverCoordinator() const;
+  const std::shared_ptr<const ImageResponseObserverCoordinator>
+      &getSharedObserverCoordinator() const;
+
+  /*
+   * Returns stored observer coordinator as a reference.
+   * Use this if a correct lifetime of the object is ensured in some other way
+   * (e.g. by retaining an `ImageRequest`).
+   */
+  const ImageResponseObserverCoordinator &getObserverCoordinator() const;
 
  private:
   /*

ReactCommon/fabric/imagemanager/platform/android/ImageRequest.cpp

@@ -25,11 +25,16 @@ ImageRequest::~ImageRequest() {
   // Not implemented.
 }
 
-const ImageResponseObserverCoordinator *ImageRequest::getObserverCoordinator()
+const ImageResponseObserverCoordinator &ImageRequest::getObserverCoordinator()
     const {
   // Not implemented
   abort();
 }
 
+const std::shared_ptr<const ImageResponseObserverCoordinator>
+    &ImageRequest::getSharedObserverCoordinator() const {
+  // Not implemented
+  abort();
+}
 } // namespace react
 } // namespace facebook

ReactCommon/fabric/imagemanager/platform/ios/ImageRequest.cpp

@@ -40,9 +40,14 @@ void ImageRequest::setCancelationFunction(
   cancelRequest_ = cancelationFunction;
 }
 
-const ImageResponseObserverCoordinator *ImageRequest::getObserverCoordinator()
+const ImageResponseObserverCoordinator &ImageRequest::getObserverCoordinator()
     const {
-  return coordinator_.get();
+  return *coordinator_;
+}
+
+const std::shared_ptr<const ImageResponseObserverCoordinator>
+    &ImageRequest::getSharedObserverCoordinator() const {
+  return coordinator_;
 }
 
 } // namespace react

ReactCommon/fabric/imagemanager/platform/ios/RCTImageManager.mm

@@ -30,11 +30,17 @@ using namespace facebook::react;
 - (ImageRequest)requestImage:(const ImageSource &)imageSource {
   auto imageRequest = ImageRequest(imageSource);
 
-  auto observerCoordinator = imageRequest.getObserverCoordinator();
+  auto weakObserverCoordinator =
+      (std::weak_ptr<const ImageResponseObserverCoordinator>)imageRequest.getSharedObserverCoordinator();
 
   NSURLRequest *request = NSURLRequestFromImageSource(imageSource);
 
   auto completionBlock = ^(NSError *error, UIImage *image) {
+    auto observerCoordinator = weakObserverCoordinator.lock();
+    if (!observerCoordinator) {
+      return;
+    }
+
     if (image && !error) {
       auto imageResponse = ImageResponse(
           std::shared_ptr<void>((__bridge_retained void *)image, CFRelease));
@@ -46,6 +52,11 @@ using namespace facebook::react;
   };
 
   auto progressBlock = ^(int64_t progress, int64_t total) {
+    auto observerCoordinator = weakObserverCoordinator.lock();
+    if (!observerCoordinator) {
+      return;
+    }
+
     observerCoordinator->nativeImageResponseProgress(progress / (float)total);
   };