From 78c6ea357b9ed95b359e7ac4fa3afd37286a1bd9 Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 11:08:40 -0700 Subject: [PATCH 1/8] [Fabric] Fix image component reference cycle --- .../Fabric/Composition/ImageComponentView.cpp | 27 +++++++++---------- .../Fabric/Composition/ImageComponentView.h | 5 ++-- .../Fabric/FabricUIManagerModule.cpp | 23 ++++++++++++---- .../ReactHost/ReactInstanceWin.cpp | 5 ++-- 4 files changed, 36 insertions(+), 24 deletions(-) diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp index ce641873c34..73ebb1e6987 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,18 @@ 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, imgComponentView]() { imgComponentView->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 +150,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); diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h index 0195a48be65..08dfdafd1bb 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.h @@ -69,13 +69,14 @@ 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..0b42fe2b897 100644 --- a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp @@ -200,11 +200,24 @@ 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); +#ifdef DEBUG + winrt::weak_ref wkView; +#endif + { + auto &oldChildShadowView = mutation.oldChildShadowView; + auto &oldChildViewDescriptor = m_registry.componentViewDescriptorWithTag(oldChildShadowView.tag); + // observerCoordinator.unregisterViewComponentDescriptor(oldChildViewDescriptor, surfaceId); +#ifdef DEBUG + wkView = winrt::make_weak(oldChildViewDescriptor.view); +#endif + m_registry.enqueueComponentViewWithComponentHandle( + oldChildShadowView.componentHandle, oldChildShadowView.tag, oldChildViewDescriptor); + } +#ifdef DEBUG + // 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)); From c0e9ea903e7ef5e7280f5766ead6933cdb312d03 Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 11:08:48 -0700 Subject: [PATCH 2/8] Change files --- ...ative-windows-d68e4c7a-72b0-4475-8aea-5b88fa568ee6.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/react-native-windows-d68e4c7a-72b0-4475-8aea-5b88fa568ee6.json 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" +} From d9ebc9d2d37656f694111b13679b81c8a047c7f4 Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 11:57:49 -0700 Subject: [PATCH 3/8] format --- vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp index 0b42fe2b897..f8088bd6c41 100644 --- a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp @@ -214,8 +214,8 @@ void FabricUIManager::RCTPerformMountInstructions( oldChildShadowView.componentHandle, oldChildShadowView.tag, oldChildViewDescriptor); } #ifdef DEBUG - // 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 + // 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; From f589227c205919a1298989f331e9a9597f2601b0 Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 14:08:31 -0700 Subject: [PATCH 4/8] disable aggressive component deleted assert --- .../Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp index f8088bd6c41..58df7c80555 100644 --- a/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/FabricUIManagerModule.cpp @@ -200,20 +200,21 @@ void FabricUIManager::RCTPerformMountInstructions( } case facebook::react::ShadowViewMutation::Delete: { -#ifdef DEBUG +// #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 DEBUG +#ifdef DETECT_COMPONENT_OUTLIVE_DELETE_MUTATION wkView = winrt::make_weak(oldChildViewDescriptor.view); #endif m_registry.enqueueComponentViewWithComponentHandle( oldChildShadowView.componentHandle, oldChildShadowView.tag, oldChildViewDescriptor); } -#ifdef DEBUG +#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()); From b8a264a29371b05bac05a5b07f90d5ef0df93a22 Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 14:08:52 -0700 Subject: [PATCH 5/8] Use weak_ref for image didReceiveImage callback --- .../Fabric/Composition/ImageComponentView.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp index 73ebb1e6987..866ba48fb26 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp @@ -43,8 +43,11 @@ void ImageComponentView::WindowsImageResponseObserver::didReceiveImage( facebook::react::ImageResponse const &imageResponse) const { if (auto imgComponentView{m_wkImage.get()}) { auto imageResponseImage = std::static_pointer_cast(imageResponse.getImage()); - imgComponentView->m_reactContext.UIDispatcher().Post( - [imageResponseImage, imgComponentView]() { imgComponentView->didReceiveImage(imageResponseImage); }); + imgComponentView->m_reactContext.UIDispatcher().Post([imageResponseImage, wkImage = m_wkImage]() { + if (auto image{m_wkImage.get()}) { + image->didReceiveImage(imageResponseImage); + } + }); } } From 72471ae425743b1a98f5fee7e47226dfc82f3dbe Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 14:11:14 -0700 Subject: [PATCH 6/8] typo --- .../Fabric/Composition/ImageComponentView.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp index 866ba48fb26..f721281ef00 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp @@ -44,7 +44,7 @@ void ImageComponentView::WindowsImageResponseObserver::didReceiveImage( 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{m_wkImage.get()}) { + if (auto image{wkImage.get()}) { image->didReceiveImage(imageResponseImage); } }); From cb3689460beed288dde13b2212d2c987af58494d Mon Sep 17 00:00:00 2001 From: Andrew <30809111+acoates-ms@users.noreply.github.com> Date: Fri, 12 Jul 2024 16:29:34 -0700 Subject: [PATCH 7/8] Unsubscribe from imageresponseobserver when deleted - aligns with core --- .../Fabric/Composition/ComponentViewRegistry.cpp | 2 ++ .../Fabric/Composition/ImageComponentView.cpp | 4 ++++ .../Fabric/Composition/ImageComponentView.h | 1 + 3 files changed, 7 insertions(+) diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp index 9cad2dc47c7..5f2f9ecaaee 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp @@ -120,6 +120,8 @@ 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 f721281ef00..471bd9eebcd 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ImageComponentView.cpp @@ -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 08dfdafd1bb..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 Date: Fri, 12 Jul 2024 16:30:02 -0700 Subject: [PATCH 8/8] format --- .../Fabric/Composition/ComponentViewRegistry.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp b/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp index 5f2f9ecaaee..6e38462ac2f 100644 --- a/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/Composition/ComponentViewRegistry.cpp @@ -120,7 +120,8 @@ void ComponentViewRegistry::enqueueComponentViewWithComponentHandle( ComponentViewDescriptor componentViewDescriptor) noexcept { assert(m_registry.find(tag) != m_registry.end()); - winrt::get_self(componentViewDescriptor.view)->prepareForRecycle(); + winrt::get_self(componentViewDescriptor.view) + ->prepareForRecycle(); m_registry.erase(tag); }