Fix race condition in ImageFetcher when registering/unregistering commitHooks (facebook#53862)

Summary:
Pull Request resolved: facebook#53862

Recently, we observed a `use-after-free race condition` where the ImageFetcher object was destroyed while it was still registered as a UIManagerCommitHook.

The crash occurred in `std::vector::size()` at line 635 when accessing corrupted memory.

Changelog: [Internal]

Reviewed By: lenaic

Differential Revision: D82846245

fbshipit-source-id: fe0d9156ec6bc004339ed987bb5937699ebdf23b

Author: christophpurrer

packages/react-native/ReactCommon/react/renderer/imagemanager/platform/android/react/renderer/imagemanager/ImageFetcher.cpp

@@ -10,9 +10,39 @@
 #include <glog/logging.h>
 #include <react/common/mapbuffer/JReadableMapBuffer.h>
 #include <react/renderer/imagemanager/conversions.h>
+#include <react/renderer/uimanager/UIManagerCommitHook.h>
 
 namespace facebook::react {
 
+class ImageFetcherCommitHook : public UIManagerCommitHook {
+ public:
+  explicit ImageFetcherCommitHook(ImageFetcher* fetcher) : fetcher_(fetcher) {}
+
+  RootShadowNode::Unshared shadowTreeWillCommit(
+      const ShadowTree& /*shadowTree*/,
+      const RootShadowNode::Shared& /*oldRootShadowNode*/,
+      const RootShadowNode::Unshared& newRootShadowNode,
+      const ShadowTree::CommitOptions& /*commitOptions*/) noexcept override {
+    if (fetcher_ != nullptr) {
+      fetcher_->flushImageRequests();
+    }
+    return newRootShadowNode;
+  }
+
+  void commitHookWasRegistered(
+      const UIManager& /*uiManager*/) noexcept override {}
+
+  void commitHookWasUnregistered(
+      const UIManager& /*uiManager*/) noexcept override {}
+
+  void invalidate() {
+    fetcher_ = nullptr;
+  }
+
+ private:
+  ImageFetcher* fetcher_;
+};
+
 ImageFetcher::ImageFetcher(
     std::shared_ptr<const ContextContainer> contextContainer)
     : contextContainer_(std::move(contextContainer)) {
@@ -22,20 +52,24 @@ ImageFetcher::ImageFetcher(
                 ->find<std::shared_ptr<UIManagerCommitHookManager>>(
                     std::string(UIManagerCommitHookManagerKey));
         uiManagerCommitHookManager.has_value()) {
-      (*uiManagerCommitHookManager)->registerCommitHook(*this);
+      commitHook_ = std::make_unique<ImageFetcherCommitHook>(this);
+      (*uiManagerCommitHookManager)->registerCommitHook(*commitHook_);
     }
   }
 }
 
 ImageFetcher::~ImageFetcher() {
-  if (ReactNativeFeatureFlags::enableImagePrefetchingJNIBatchingAndroid()) {
+  if (ReactNativeFeatureFlags::enableImagePrefetchingJNIBatchingAndroid() &&
+      commitHook_ != nullptr) {
+    commitHook_->invalidate();
     if (auto uiManagerCommitHookManager =
             contextContainer_
                 ->find<std::shared_ptr<UIManagerCommitHookManager>>(
                     std::string(UIManagerCommitHookManagerKey));
         uiManagerCommitHookManager.has_value()) {
-      (*uiManagerCommitHookManager)->unregisterCommitHook(*this);
+      (*uiManagerCommitHookManager)->unregisterCommitHook(*commitHook_);
     }
+    commitHook_ = nullptr;
   }
 }
 
@@ -58,15 +92,6 @@ ImageRequest ImageFetcher::requestImage(
   return {imageSource, telemetry};
 }
 
-RootShadowNode::Unshared ImageFetcher::shadowTreeWillCommit(
-    const ShadowTree& /*shadowTree*/,
-    const RootShadowNode::Shared& /*oldRootShadowNode*/,
-    const RootShadowNode::Unshared& newRootShadowNode,
-    const ShadowTree::CommitOptions& /*commitOptions*/) noexcept {
-  flushImageRequests();
-  return newRootShadowNode;
-}
-
 void ImageFetcher::flushImageRequests() {
   if (items_.empty()) {
     return;

packages/react-native/ReactCommon/react/renderer/imagemanager/platform/android/react/renderer/imagemanager/ImageFetcher.h

@@ -11,17 +11,19 @@
 #include <react/renderer/imagemanager/ImageRequest.h>
 #include <react/renderer/imagemanager/ImageRequestParams.h>
 #include <react/renderer/mounting/ShadowTree.h>
-#include <react/renderer/uimanager/UIManagerCommitHook.h>
 #include <react/utils/ContextContainer.h>
+#include <memory>
 #include <unordered_map>
 #include <vector>
 
 namespace facebook::react {
 
-class ImageFetcher : public UIManagerCommitHook {
+class ImageFetcherCommitHook;
+
+class ImageFetcher {
  public:
   ImageFetcher(std::shared_ptr<const ContextContainer> contextContainer);
-  ~ImageFetcher() override;
+  ~ImageFetcher();
   ImageFetcher(const ImageFetcher&) = delete;
   ImageFetcher& operator=(const ImageFetcher&) = delete;
   ImageFetcher(ImageFetcher&&) = delete;
@@ -33,21 +35,12 @@ class ImageFetcher : public UIManagerCommitHook {
       const ImageRequestParams& imageRequestParams,
       Tag tag);
 
-  void commitHookWasRegistered(const UIManager& uiManager) noexcept override {}
-
-  void commitHookWasUnregistered(const UIManager& uiManager) noexcept override {
-  }
-
-  RootShadowNode::Unshared shadowTreeWillCommit(
-      const ShadowTree& shadowTree,
-      const RootShadowNode::Shared& oldRootShadowNode,
-      const RootShadowNode::Unshared& newRootShadowNode,
-      const ShadowTree::CommitOptions& commitOptions) noexcept override;
-
  private:
+  friend class ImageFetcherCommitHook;
   void flushImageRequests();
 
   std::unordered_map<SurfaceId, std::vector<ImageRequestItem>> items_;
   std::shared_ptr<const ContextContainer> contextContainer_;
+  std::unique_ptr<ImageFetcherCommitHook> commitHook_;
 };
 } // namespace facebook::react