diff --git a/change/react-native-windows-d68e4c7a-72b0-4475-8aea-5b88fa568ee6.json b/change/react-native-windows-d68e4c7a-72b0-4475-8aea-5b88fa568ee6.json new file mode 100644 index 00000000000..b77a8e029a4 --- /dev/null +++ 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 index 9cad2dc47c7..6e38462ac2f 100644 --- 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(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 index ce641873c34..471bd9eebcd 100644 --- 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 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(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(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(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( - *componentViewDescriptor.view - .as()); + m_imageResponseObserver = std::make_shared(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 index 0195a48be65..829aca4f383 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h @@ -47,6 +47,7 @@ struct ImageComponentView : ImageComponentViewT 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 m_image; + winrt::weak_ref m_wkImage; }; void ensureDrawingSurface() noexcept; diff --git a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp index dff12e4c588..58df7c80555 100644 --- 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 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 index 7995b199975..12bfbb099c4 100644 --- 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(std::move(timerRegistry));