From 13d87e9ad290f2ea55d9eca2c6426b4d215b5c3e Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 14 Feb 2019 00:37:39 -0800 Subject: [PATCH] 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 --- .../ComponentViews/Image/RCTImageComponentView.mm | 2 +- .../ComponentViews/Slider/RCTSliderComponentView.mm | 8 ++++---- ReactCommon/fabric/imagemanager/ImageRequest.h | 13 +++++++++++-- .../imagemanager/platform/android/ImageRequest.cpp | 7 ++++++- .../imagemanager/platform/ios/ImageRequest.cpp | 9 +++++++-- .../imagemanager/platform/ios/RCTImageManager.mm | 13 ++++++++++++- 6 files changed, 41 insertions(+), 11 deletions(-) diff --git a/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm b/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm index 0174d94a0d7..fe77fd4a957 100644 --- a/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm +++ b/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 diff --git a/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm b/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm index 8b90f3b4441..2fc56881ee8 100644 --- a/React/Fabric/Mounting/ComponentViews/Slider/RCTSliderComponentView.mm +++ b/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(); } } diff --git a/ReactCommon/fabric/imagemanager/ImageRequest.h b/ReactCommon/fabric/imagemanager/ImageRequest.h index 0ccc95a6e62..1f2ae5ed8a2 100644 --- a/ReactCommon/fabric/imagemanager/ImageRequest.h +++ b/ReactCommon/fabric/imagemanager/ImageRequest.h @@ -53,9 +53,18 @@ class ImageRequest final { void setCancelationFunction(std::function 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 + &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: /* diff --git a/ReactCommon/fabric/imagemanager/platform/android/ImageRequest.cpp b/ReactCommon/fabric/imagemanager/platform/android/ImageRequest.cpp index 06678387283..c3bd7a71988 100644 --- a/ReactCommon/fabric/imagemanager/platform/android/ImageRequest.cpp +++ b/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 + &ImageRequest::getSharedObserverCoordinator() const { + // Not implemented + abort(); +} } // namespace react } // namespace facebook diff --git a/ReactCommon/fabric/imagemanager/platform/ios/ImageRequest.cpp b/ReactCommon/fabric/imagemanager/platform/ios/ImageRequest.cpp index 19555035218..81f00b726de 100644 --- a/ReactCommon/fabric/imagemanager/platform/ios/ImageRequest.cpp +++ b/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 + &ImageRequest::getSharedObserverCoordinator() const { + return coordinator_; } } // namespace react diff --git a/ReactCommon/fabric/imagemanager/platform/ios/RCTImageManager.mm b/ReactCommon/fabric/imagemanager/platform/ios/RCTImageManager.mm index 94e3a26a430..5f6f3db1bcc 100644 --- a/ReactCommon/fabric/imagemanager/platform/ios/RCTImageManager.mm +++ b/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)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((__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); };