[Fabric] Fix image component reference cycle (#13440)

acoates-ms · web-flow · commit 244d85aa9705 · 2024-07-15T16:12:36.000Z
* [Fabric] Fix image component reference cycle

* Change files

* format

* disable aggressive component deleted assert

* Use weak_ref for image didReceiveImage callback

* typo

* Unsubscribe from imageresponseobserver when deleted - aligns with core

* format
diff --git a/change/react-native-windows-d68e4c7a-72b0-4475-8aea-5b88fa568ee6.json b/change/react-native-windows-d68e4c7a-72b0-4475-8aea-5b88fa568ee6.json
@@ -0,0 +1,7 @@
+{
+  "type": "prerelease",
+  "comment": "[Fabric] Fix image component reference cycle",
+  "packageName": "react-native-windows",
+  "email": "30809111+acoates-ms@users.noreply.github.com",
+  "dependentChangeType": "patch"
+}
diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp
@@ -120,6 +120,9 @@ void ComponentViewRegistry::enqueueComponentViewWithComponentHandle(
     ComponentViewDescriptor componentViewDescriptor) noexcept {
   assert(m_registry.find(tag) != m_registry.end());
 
+  winrt::get_self<winrt::Microsoft::ReactNative::implementation::ComponentView>(componentViewDescriptor.view)
+      ->prepareForRecycle();
+
   m_registry.erase(tag);
 }
 } // namespace Microsoft::ReactNative
diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp
@@ -30,9 +30,9 @@ extern "C" HRESULT WINAPI WICCreateImagingFactory_Proxy(UINT SDKVersion, IWICIma
 
 namespace winrt::Microsoft::ReactNative::Composition::implementation {
 
-ImageComponentView::WindowsImageResponseObserver::WindowsImageResponseObserver(ImageComponentView &image) {
-  m_image.copy_from(&image);
-}
+ImageComponentView::WindowsImageResponseObserver::WindowsImageResponseObserver(
+    winrt::weak_ref<winrt::Microsoft::ReactNative::Composition::implementation::ImageComponentView> wkImage)
+    : m_wkImage(std::move(wkImage)) {}
 
 void ImageComponentView::WindowsImageResponseObserver::didReceiveProgress(float progress, int64_t loaded, int64_t total)
     const {
@@ -41,14 +41,21 @@ void ImageComponentView::WindowsImageResponseObserver::didReceiveProgress(float
 
 void ImageComponentView::WindowsImageResponseObserver::didReceiveImage(
     facebook::react::ImageResponse const &imageResponse) const {
-  auto imageResponseImage = std::static_pointer_cast<ImageResponseImage>(imageResponse.getImage());
-  m_image->m_reactContext.UIDispatcher().Post(
-      [imageResponseImage, image = m_image]() { image->didReceiveImage(imageResponseImage); });
+  if (auto imgComponentView{m_wkImage.get()}) {
+    auto imageResponseImage = std::static_pointer_cast<ImageResponseImage>(imageResponse.getImage());
+    imgComponentView->m_reactContext.UIDispatcher().Post([imageResponseImage, wkImage = m_wkImage]() {
+      if (auto image{wkImage.get()}) {
+        image->didReceiveImage(imageResponseImage);
+      }
+    });
+  }
 }
 
 void ImageComponentView::WindowsImageResponseObserver::didReceiveFailure(
     facebook::react::ImageLoadError const &error) const {
-  m_image->didReceiveFailureFromObserver(error);
+  if (auto imgComponentView{m_wkImage.get()}) {
+    imgComponentView->didReceiveFailureFromObserver(error);
+  }
 }
 
 facebook::react::SharedViewProps ImageComponentView::defaultProps() noexcept {
@@ -146,14 +153,7 @@ void ImageComponentView::updateState(
   auto newImageState = std::static_pointer_cast<facebook::react::ImageShadowNode::ConcreteState const>(state);
 
   if (!m_imageResponseObserver) {
-    // Should ViewComponents enable_shared_from_this? then we don't need this dance to get a shared_ptr
-    std::shared_ptr<::Microsoft::ReactNative::FabricUIManager> fabricuiManager =
-        ::Microsoft::ReactNative::FabricUIManager::FromProperties(m_reactContext.Properties());
-    auto componentViewDescriptor = fabricuiManager->GetViewRegistry().componentViewDescriptorWithTag(m_tag);
-
-    m_imageResponseObserver = std::make_shared<WindowsImageResponseObserver>(
-        *componentViewDescriptor.view
-             .as<winrt::Microsoft::ReactNative::Composition::implementation::ImageComponentView>());
+    m_imageResponseObserver = std::make_shared<WindowsImageResponseObserver>(get_weak());
   }
 
   setStateAndResubscribeImageResponseObserver(newImageState);
@@ -182,6 +182,10 @@ void ImageComponentView::setStateAndResubscribeImageResponseObserver(
   }
 }
 
+void ImageComponentView::prepareForRecycle() noexcept {
+  setStateAndResubscribeImageResponseObserver(nullptr);
+}
+
 winrt::Microsoft::ReactNative::ImageProps ImageComponentView::ImageProps() noexcept {
   // We do not currently support custom ImageComponentView's
   // If we did we would need to create a AbiImageProps and possibly return them here
diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h
@@ -47,6 +47,7 @@ struct ImageComponentView : ImageComponentViewT<ImageComponentView, ViewComponen
       override;
   void updateState(facebook::react::State::Shared const &state, facebook::react::State::Shared const &oldState) noexcept
       override;
+  void prepareForRecycle() noexcept override;
   void OnRenderingDeviceLost() noexcept override;
   void onThemeChanged() noexcept override;
 
@@ -69,13 +70,14 @@ struct ImageComponentView : ImageComponentViewT<ImageComponentView, ViewComponen
  private:
   struct WindowsImageResponseObserver : facebook::react::ImageResponseObserver {
    public:
-    WindowsImageResponseObserver(ImageComponentView &image);
+    WindowsImageResponseObserver(
+        winrt::weak_ref<winrt::Microsoft::ReactNative::Composition::implementation::ImageComponentView> wkImage);
     void didReceiveProgress(float progress, int64_t loaded, int64_t total) const override;
     void didReceiveImage(const facebook::react::ImageResponse &imageResponse) const override;
     void didReceiveFailure(const facebook::react::ImageLoadError &error) const override;
 
    private:
-    winrt::com_ptr<ImageComponentView> m_image;
+    winrt::weak_ref<winrt::Microsoft::ReactNative::Composition::implementation::ImageComponentView> m_wkImage;
   };
 
   void ensureDrawingSurface() noexcept;
diff --git a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp
@@ -200,11 +200,25 @@ void FabricUIManager::RCTPerformMountInstructions(
       }
 
       case facebook::react::ShadowViewMutation::Delete: {
-        auto &oldChildShadowView = mutation.oldChildShadowView;
-        auto &oldChildViewDescriptor = m_registry.componentViewDescriptorWithTag(oldChildShadowView.tag);
-        // observerCoordinator.unregisterViewComponentDescriptor(oldChildViewDescriptor, surfaceId);
-        m_registry.enqueueComponentViewWithComponentHandle(
-            oldChildShadowView.componentHandle, oldChildShadowView.tag, oldChildViewDescriptor);
+// #define DETECT_COMPONENT_OUTLIVE_DELETE_MUTATION
+#ifdef DETECT_COMPONENT_OUTLIVE_DELETE_MUTATION
+        winrt::weak_ref<winrt::Microsoft::ReactNative::ComponentView> wkView;
+#endif
+        {
+          auto &oldChildShadowView = mutation.oldChildShadowView;
+          auto &oldChildViewDescriptor = m_registry.componentViewDescriptorWithTag(oldChildShadowView.tag);
+          // observerCoordinator.unregisterViewComponentDescriptor(oldChildViewDescriptor, surfaceId);
+#ifdef DETECT_COMPONENT_OUTLIVE_DELETE_MUTATION
+          wkView = winrt::make_weak(oldChildViewDescriptor.view);
+#endif
+          m_registry.enqueueComponentViewWithComponentHandle(
+              oldChildShadowView.componentHandle, oldChildShadowView.tag, oldChildViewDescriptor);
+        }
+#ifdef DETECT_COMPONENT_OUTLIVE_DELETE_MUTATION
+        // After handling a delete mutation, nothing should be holding on to the view.  If there is thats an indication
+        // of a leak, or at least something holding on to a view longer than it should
+        assert(!wkView.get());
+#endif
         break;
       }
 
diff --git a/vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp b/vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp
@@ -337,7 +337,7 @@ void ReactInstanceWin::LoadModules(
   };
 
 #ifdef USE_FABRIC
-  if (!m_options.UseWebDebugger()) {
+  if (Microsoft::ReactNative::IsFabricEnabled(m_reactContext->Properties())) {
     registerTurboModule(
         L"FabricUIManagerBinding",
         winrt::Microsoft::ReactNative::MakeModuleProvider<::Microsoft::ReactNative::FabricUIManager>());
@@ -602,7 +602,8 @@ void ReactInstanceWin::InitializeBridgeless() noexcept {
 
             m_jsMessageThread.Load()->runOnQueueSync([&]() {
               ::SetThreadDescription(GetCurrentThread(), L"React-Native JavaScript Thread");
-              auto timerRegistry = ::Microsoft::ReactNative::TimerRegistry::CreateTimerRegistry(m_options.Properties);
+              auto timerRegistry =
+                  ::Microsoft::ReactNative::TimerRegistry::CreateTimerRegistry(m_reactContext->Properties());
               auto timerRegistryRaw = timerRegistry.get();
 
               auto timerManager = std::make_shared<facebook::react::TimerManager>(std::move(timerRegistry));

Original file line number	Diff line number	Diff line change
`@@ -120,6 +120,9 @@ void ComponentViewRegistry::enqueueComponentViewWithComponentHandle(`
`120`	`120`	`ComponentViewDescriptor componentViewDescriptor) noexcept {`
`121`	`121`	`assert(m_registry.find(tag) != m_registry.end());`
`122`	`122`
	`123`	`+ winrt::get_self<winrt::Microsoft::ReactNative::implementation::ComponentView>(componentViewDescriptor.view)`
	`124`	`+ ->prepareForRecycle();`
	`125`	`+`
`123`	`126`	`m_registry.erase(tag);`
`124`	`127`	`}`
`125`	`128`	`} // namespace Microsoft::ReactNative`