From ebbd22c9d84800c11715662e2342cd6a7daae848 Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Tue, 27 Jun 2023 09:01:29 -0700 Subject: [PATCH] Add nullptr check to SharedFunction (#38075) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/38075 changelog: [internal] `SharedFunction<>` is created with nullptr for its internal `std::function`. If called after created with default constructor, it crashes app. It also does not have API to check if its internal function is not nullptr. With image cancelation, there is a race between when native component calls `imageRequest.cancel()` and when cancelation function is set in `RCTImageManager`. To fix this, this diff adds a nullptr check inside SharedFunction. So it is always safe to call. Reviewed By: javache Differential Revision: D47022957 fbshipit-source-id: 0a04a87cd1ffe6bf3ca2fded38f689f06cc92ca9 --- .../Image/RCTImageComponentView.mm | 3 +++ .../renderer/components/image/ImageShadowNode.h | 2 +- .../react/renderer/imagemanager/ImageRequest.h | 13 +++++-------- .../react/renderer/imagemanager/ImageManager.cpp | 2 +- .../react/renderer/imagemanager/ImageRequest.cpp | 7 +++++-- .../react/renderer/imagemanager/ImageRequest.cpp | 16 ++++++---------- .../renderer/imagemanager/RCTImageManager.mm | 6 ++---- .../renderer/imagemanager/RCTSyncImageManager.mm | 6 ++---- .../ReactCommon/react/utils/SharedFunction.h | 10 ++++++---- 9 files changed, 31 insertions(+), 34 deletions(-) diff --git a/packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm b/packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm index 472d83e45b12e0..65cea1460fc76e 100644 --- a/packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm +++ b/packages/react-native/React/Fabric/Mounting/ComponentViews/Image/RCTImageComponentView.mm @@ -109,6 +109,9 @@ - (void)_setStateAndResubscribeImageResponseObserver:(ImageShadowNode::ConcreteS // This will only become issue if we decouple life cycle of a // ShadowNode from ComponentView, which is not something we do now. imageRequest.cancel(); + imageRequest.cancel(); + imageRequest.cancel(); + imageRequest.cancel(); } } diff --git a/packages/react-native/ReactCommon/react/renderer/components/image/ImageShadowNode.h b/packages/react-native/ReactCommon/react/renderer/components/image/ImageShadowNode.h index 55d56df694b3e3..0ffc2f22a2ee23 100644 --- a/packages/react-native/ReactCommon/react/renderer/components/image/ImageShadowNode.h +++ b/packages/react-native/ReactCommon/react/renderer/components/image/ImageShadowNode.h @@ -46,7 +46,7 @@ class ImageShadowNode final : public ConcreteViewShadowNode< ShadowNodeFamily::Shared const & /*family*/, ComponentDescriptor const &componentDescriptor) { auto imageSource = ImageSource{ImageSource::Type::Invalid}; - return {imageSource, {imageSource, nullptr}, 0}; + return {imageSource, {imageSource, nullptr, {}}, 0}; } #pragma mark - LayoutableShadowNode diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/ImageRequest.h b/packages/react-native/ReactCommon/react/renderer/imagemanager/ImageRequest.h index 78c1b4c8112704..5853c97cd8ef1a 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/ImageRequest.h +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/ImageRequest.h @@ -12,6 +12,7 @@ #include #include #include +#include namespace facebook::react { @@ -29,7 +30,8 @@ class ImageRequest final { */ ImageRequest( ImageSource imageSource, - std::shared_ptr telemetry); + std::shared_ptr telemetry, + SharedFunction<> cancelationFunction); /* * The move constructor. @@ -41,11 +43,6 @@ class ImageRequest final { */ ImageRequest(const ImageRequest &other) = delete; - /** - * Set cancelation function. - */ - void setCancelationFunction(std::function cancelationFunction); - /* * Calls cancel function if one is defined. Should be when downloading * image isn't needed anymore. E.g. was removed. @@ -94,9 +91,9 @@ class ImageRequest final { std::shared_ptr coordinator_{}; /* - * Function we can call to cancel image request (see destructor). + * Function we can call to cancel image request. */ - std::function cancelRequest_; + SharedFunction<> cancelRequest_; }; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp index 30bc153fc0d4e4..d9a382125e606c 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageManager.cpp @@ -24,7 +24,7 @@ ImageRequest ImageManager::requestImage( const ImageSource &imageSource, SurfaceId /*surfaceId*/) const { // Not implemented. - return {imageSource, nullptr}; + return {imageSource, nullptr, {}}; } } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp index a502f65d9c305c..6a5c1341830b40 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/cxx/react/renderer/imagemanager/ImageRequest.cpp @@ -13,8 +13,11 @@ namespace facebook::react { ImageRequest::ImageRequest( ImageSource imageSource, - std::shared_ptr telemetry) - : imageSource_(std::move(imageSource)), telemetry_(std::move(telemetry)) { + std::shared_ptr telemetry, + SharedFunction<> cancelationFunction) + : imageSource_(std::move(imageSource)), + telemetry_(std::move(telemetry)), + cancelRequest_(std::move(cancelationFunction)) { // Not implemented. } diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/ImageRequest.cpp b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/ImageRequest.cpp index cca7918e2d6d2c..2c1e009a58e47c 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/ImageRequest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/ImageRequest.cpp @@ -11,20 +11,16 @@ namespace facebook::react { ImageRequest::ImageRequest( ImageSource imageSource, - std::shared_ptr telemetry) - : imageSource_(std::move(imageSource)), telemetry_(std::move(telemetry)) { + std::shared_ptr telemetry, + SharedFunction<> cancelationFunction) + : imageSource_(std::move(imageSource)), + telemetry_(std::move(telemetry)), + cancelRequest_(std::move(cancelationFunction)) { coordinator_ = std::make_shared(); } -void ImageRequest::setCancelationFunction( - std::function cancelationFunction) { - cancelRequest_ = cancelationFunction; -} - void ImageRequest::cancel() const { - if (cancelRequest_) { - cancelRequest_(); - } + cancelRequest_(); } const ImageSource &ImageRequest::getImageSource() const { diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTImageManager.mm b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTImageManager.mm index 1f4b4eabaa2ce2..036607666424c8 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTImageManager.mm +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTImageManager.mm @@ -48,13 +48,11 @@ - (ImageRequest)requestImage:(ImageSource)imageSource surfaceId:(SurfaceId)surfa telemetry = nullptr; } - auto imageRequest = ImageRequest(imageSource, telemetry); + auto sharedCancelationFunction = SharedFunction<>(); + auto imageRequest = ImageRequest(imageSource, telemetry, sharedCancelationFunction); auto weakObserverCoordinator = (std::weak_ptr)imageRequest.getSharedObserverCoordinator(); - auto sharedCancelationFunction = SharedFunction<>(); - imageRequest.setCancelationFunction(sharedCancelationFunction); - /* * Even if an image is being loaded asynchronously on some other background thread, some other preparation * work (such as creating an `NSURLRequest` object and some obscure logic inside `RCTImageLoader`) can take a couple diff --git a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTSyncImageManager.mm b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTSyncImageManager.mm index dd7c9936ac9562..71a1e882856626 100644 --- a/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTSyncImageManager.mm +++ b/packages/react-native/ReactCommon/react/renderer/imagemanager/platform/ios/react/renderer/imagemanager/RCTSyncImageManager.mm @@ -37,13 +37,11 @@ - (instancetype)initWithImageLoader:(id)i - (ImageRequest)requestImage:(ImageSource)imageSource surfaceId:(SurfaceId)surfaceId { auto telemetry = std::make_shared(surfaceId); - auto imageRequest = ImageRequest(imageSource, telemetry); + auto sharedCancelationFunction = SharedFunction<>(); + auto imageRequest = ImageRequest(imageSource, telemetry, sharedCancelationFunction); auto weakObserverCoordinator = (std::weak_ptr)imageRequest.getSharedObserverCoordinator(); - auto sharedCancelationFunction = SharedFunction<>(); - imageRequest.setCancelationFunction(sharedCancelationFunction); - dispatch_group_t imageWaitGroup = dispatch_group_create(); dispatch_group_enter(imageWaitGroup); diff --git a/packages/react-native/ReactCommon/react/utils/SharedFunction.h b/packages/react-native/ReactCommon/react/utils/SharedFunction.h index 70db3e378bda1f..d44d8102fdc668 100644 --- a/packages/react-native/ReactCommon/react/utils/SharedFunction.h +++ b/packages/react-native/ReactCommon/react/utils/SharedFunction.h @@ -21,9 +21,9 @@ namespace facebook::react { * - When captured by `std::function` arguments are not copyable; * - When we need to replace the content of the callable later on the go. */ -template +template class SharedFunction { - using T = ReturnT(ArgumentT...); + using T = void(ArgumentT...); struct Pair { Pair(std::function &&function) : function(std::move(function)) {} @@ -46,9 +46,11 @@ class SharedFunction { pair_->function = function; } - ReturnT operator()(ArgumentT... args) const { + void operator()(ArgumentT... args) const { std::shared_lock lock(pair_->mutex); - return pair_->function(args...); + if (pair_->function) { + pair_->function(args...); + } } private: