Fix use-after-free when an Image is destroyed mid-download

[gmacmaster]

Description

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Why

ImageResponseObserverCoordinator (in react-native core) copies its observer list under a lock, releases the lock, and then dereferences the raw observer pointers:

mutex_.lock();
auto observers = observers_;
mutex_.unlock();

for (auto observer : observers) {
  observer->didReceiveProgress(progress, loaded, total);
}

WindowsImageManager notifies that coordinator from the download thread and from the IAsyncOperation completion handler. Observers, however, are added and removed on the UI thread (ImageComponentView::setStateAndResubscribeImageResponseObserver), and the UI thread is also where the owning ImageComponentView — and with it the std::shared_ptr<WindowsImageResponseObserver> — is destroyed.

So if a view is torn down while its download is still streaming, the observer can be freed between the snapshot and the dereference. The first thing WindowsImageResponseObserver::didReceiveProgress touches is its winrt::weak_ref member, copied into the dispatcher lambda, so the crash surfaces as an EXCEPTION_ACCESS_VIOLATION_READ inside winrt::com_ptr<T>::add_ref:

at Microsoft::ReactNative::WindowsImageManager::GetImageRandomAccessStreamAsync$_ResumeCoro$1 (WindowsImageManager.cpp:172)
at Microsoft::ReactNative::WindowsImageManager::requestImage::__l7::<T>::operator() (WindowsImageManager.cpp:208)
at facebook::react::ImageResponseObserverCoordinator::nativeImageResponseProgress (ImageResponseObserverCoordinator.cpp:85)
at ...ImageComponentView::WindowsImageResponseObserver::didReceiveProgress (ImageComponentView.cpp:40)
at winrt::com_ptr<T>::{ctor} (base.h:2385)
at winrt::com_ptr<T>::add_ref (base.h:2561)

The weak_ref doesn't help here — it guards the ImageComponentView, but the object that was freed is the observer that holds it.

This is the Fabric recurrence of a lifetime bug that was fixed in the Paper ReactImage back in #2820 and #2822. The scenario described there is the same one that reproduces today:

Scenario wise we are hitting this when going to a screen with a bunch of images being downloaded, if you navigate away so all those Image elements are destructed, we crash in a weakref dtor with invalid heap free stuff happening

The Fabric completion handler has always notified the coordinator off the UI thread, but the window was narrow because it fired once per request. #14493 ([Fabric] Implement onProgress for Image) added a progressCallback that calls nativeImageResponseProgress from the download loop on every 16 KB chunk, which widened that window enormously and is what makes this reachable in practice. #13440 is where the observer gained its winrt::weak_ref, which resolved an unrelated reference cycle but does not protect against the observer itself being freed.

We hit this in production on RNW 0.84 — a screen of remote thumbnails, navigated away from mid-load.

Resolves

N/A

What

All coordinator notifications are now marshalled onto the UI thread, so notification and subscription are serialized on the same thread and the coordinator can no longer iterate a snapshot while another thread frees an entry. Changes are confined to requestImage in vnext/Microsoft.ReactNative/Fabric/WindowsImageManager.cpp:

• progressCallback computes the progress fraction on the download thread and posts only the nativeImageResponseProgress call.
• The Completed handler keeps ResolveImage() on the completion thread, so WIC decoding stays off the UI thread, and posts only the nativeImageResponseComplete / nativeImageResponseFailed call via two small postComplete / postFailure helpers.
• The weak coordinator pointer is locked at delivery time rather than at call time, which is the correct liveness check now that notification is deferred.

This incidentally moves didReceiveFailure onto the UI thread as well. It previously resolved the weak ref and touched ImageComponentView state directly on the calling thread, which was a separate off-thread access to view state.

Note for reviewers: the inner UIDispatcher().Post calls in ImageComponentView::didReceiveProgress and didReceiveImage are now a redundant second hop. They're harmless and FIFO ordering through the serial dispatcher is preserved, so I left them out of this change to keep the diff to a single file. Happy to fold the cleanup in if you'd prefer.

Screenshots

N/A — crash fix with no visual change.

Testing

No automated tests added. The failure is a data race on object lifetime, so a deterministic unit test would require injecting a teardown between the coordinator's snapshot and its dereference, which isn't reachable from RNW without changing core.

Changelog

Yes.

Fixed a crash that could occur when an Image was destroyed while its download was still in flight, such as navigating away from a screen before its images finished loading.

Microsoft Reviewers: Open in CodeFlow

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.