diff --git a/change/react-native-windows-eba3d592-2ea0-4067-8880-2beea6638837.json b/change/react-native-windows-eba3d592-2ea0-4067-8880-2beea6638837.json new file mode 100644 index 00000000000..7e7640aaeb8 --- /dev/null +++ b/change/react-native-windows-eba3d592-2ea0-4067-8880-2beea6638837.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "Fix use-after-free crash when an Image is destroyed while its download is still in flight", + "packageName": "react-native-windows", + "email": "gordomacmaster@gmail.com", + "dependentChangeType": "patch" +} diff --git a/vnext/Microsoft.ReactNative/Fabric/WindowsImageManager.cpp b/vnext/Microsoft.ReactNative/Fabric/WindowsImageManager.cpp index 045b3eb1ed8..ac05ea87127 100644 --- a/vnext/Microsoft.ReactNative/Fabric/WindowsImageManager.cpp +++ b/vnext/Microsoft.ReactNative/Fabric/WindowsImageManager.cpp @@ -186,6 +186,16 @@ facebook::react::ImageRequest WindowsImageManager::requestImage( auto weakObserverCoordinator = (std::weak_ptr) imageRequest.getSharedObserverCoordinator(); + // ImageResponseObserverCoordinator copies its observer list under a lock but dereferences the raw + // observer pointers after releasing it. Observers are added and removed on the UI thread (from + // ImageComponentView::setStateAndResubscribeImageResponseObserver), and that is also where the + // owning ImageComponentView - and with it the WindowsImageResponseObserver - is destroyed. Notifying + // the coordinator from the download/completion threads therefore races that teardown and can call + // into a freed observer. Marshal every notification onto the UI thread so subscription and + // notification are serialized on the same thread. Image decoding deliberately stays off the UI + // thread; only the notification itself is posted. + auto uiDispatcher = m_reactContext.UIDispatcher(); + auto rnImageSource = winrt::Microsoft::ReactNative::Composition::implementation::MakeImageSource(imageSource); auto provider = m_uriImageManager->TryGetUriImageProvider(m_reactContext.Handle(), rnImageSource); @@ -202,21 +212,39 @@ facebook::react::ImageRequest WindowsImageManager::requestImage( source.sourceType = ImageSourceType::Download; source.body = imageSource.body; - auto progressCallback = [weakObserverCoordinator](int64_t loaded, int64_t total) { - if (auto observerCoordinator = weakObserverCoordinator.lock()) { - float progress = total > 0 ? static_cast(loaded) / static_cast(total) : 1.0f; - observerCoordinator->nativeImageResponseProgress(progress, loaded, total); - } + auto progressCallback = [weakObserverCoordinator, uiDispatcher](int64_t loaded, int64_t total) { + float progress = total > 0 ? static_cast(loaded) / static_cast(total) : 1.0f; + uiDispatcher.Post([weakObserverCoordinator, progress, loaded, total]() { + if (auto observerCoordinator = weakObserverCoordinator.lock()) { + observerCoordinator->nativeImageResponseProgress(progress, loaded, total); + } + }); }; imageResponseTask = GetImageRandomAccessStreamAsync(source, progressCallback); } - imageResponseTask.Completed([weakObserverCoordinator](auto asyncOp, auto status) { - auto observerCoordinator = weakObserverCoordinator.lock(); - if (!observerCoordinator) { + imageResponseTask.Completed([weakObserverCoordinator, uiDispatcher](auto asyncOp, auto status) { + if (weakObserverCoordinator.expired()) { return; } + auto postComplete = [weakObserverCoordinator, uiDispatcher](auto image) { + uiDispatcher.Post([weakObserverCoordinator, image = std::move(image)]() { + if (auto observerCoordinator = weakObserverCoordinator.lock()) { + observerCoordinator->nativeImageResponseComplete(facebook::react::ImageResponse(image, nullptr /*metadata*/)); + } + }); + }; + + auto postFailure = [weakObserverCoordinator, + uiDispatcher](std::shared_ptr errorInfo) { + uiDispatcher.Post([weakObserverCoordinator, errorInfo = std::move(errorInfo)]() { + if (auto observerCoordinator = weakObserverCoordinator.lock()) { + observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(errorInfo)); + } + }); + }; + switch (status) { case winrt::Windows::Foundation::AsyncStatus::Completed: { auto imageResponse = asyncOp.GetResults(); @@ -224,23 +252,22 @@ facebook::react::ImageRequest WindowsImageManager::requestImage( winrt::get_self(imageResponse); auto imageResultOrError = selfImageResponse->ResolveImage(); if (imageResultOrError.image) { - observerCoordinator->nativeImageResponseComplete( - facebook::react::ImageResponse(imageResultOrError.image, nullptr /*metadata*/)); + postComplete(std::move(imageResultOrError.image)); } else { - observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(imageResultOrError.errorInfo)); + postFailure(std::move(imageResultOrError.errorInfo)); } break; } case winrt::Windows::Foundation::AsyncStatus::Canceled: { auto errorInfo = std::make_shared(); errorInfo->error = FormatHResultError(winrt::hresult_error(asyncOp.ErrorCode())); - observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(errorInfo)); + postFailure(std::move(errorInfo)); break; } case winrt::Windows::Foundation::AsyncStatus::Error: { auto errorInfo = std::make_shared(); errorInfo->error = FormatHResultError(winrt::hresult_error(asyncOp.ErrorCode())); - observerCoordinator->nativeImageResponseFailed(facebook::react::ImageLoadError(errorInfo)); + postFailure(std::move(errorInfo)); break; } }