Fix/textinput reliability

[gmacmaster]

No description provided.

[greptile-apps]

Greptile Summary

This PR fixes a use-after-free reliability issue in WindowsTextInputComponentView by introducing a Detach() mechanism on CompTextHost, adding a destructor with explicit teardown ordering, threading m_outer null-guards through every ITextHost callback method, and making the one-time DLL load of Msftedit.dll thread-safe via std::call_once. All findings are P2 style/clarity suggestions — the core fix is sound.

Confidence Score: 5/5

Safe to merge — all remaining findings are P2 style suggestions that do not affect correctness.

The core UAF fix is correctly structured: DrawText() already guards !m_textServices so the destructor ordering is safe in practice; the three P2 comments are about clarity, a dead code guard, and a non-retry-on-failure behaviour that appears intentional.

No files require special attention — only the destructor ordering in WindowsTextInputComponentView.cpp warrants a second read.

Important Files Changed

Filename	Overview
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.cpp	Adds destructor with m_textServices-first teardown ordering and CompTextHost::Detach(), null-guards every ITextHost callback, and makes DLL loading thread-safe via std::call_once. Destructor ordering may expose half-torn-down members to callbacks before Detach() is called; the post-as() null check in createVisual() is dead code.
vnext/Microsoft.ReactNative/Fabric/Composition/TextInput/WindowsTextInputComponentView.h	Adds destructor declaration to match the new definition in the .cpp file — no issues.

Sequence Diagram

sequenceDiagram
    participant WTIV as WindowsTextInputComponentView
    participant CTH as CompTextHost
    participant TS as ITextServices2

    Note over WTIV: createVisual()
    WTIV->>CTH: winrt::make<CompTextHost>(this)
    WTIV->>TS: g_pfnCreateTextServices(textHost)
    TS-->>WTIV: m_textServices

    Note over WTIV: ~WindowsTextInputComponentView()
    WTIV->>TS: m_textServices = nullptr (Release)
    TS-->>CTH: TxInvalidateRect / TxViewChange callbacks
    CTH->>WTIV: DrawText() guarded by !m_textServices early return
    WTIV->>CTH: Detach() sets m_outer = nullptr
    Note over CTH: All subsequent ITextHost calls early return (m_outer == nullptr)

Loading

_{Reviews (1): Last reviewed commit: "Fix potential use-after-free in CompText..." | Re-trigger Greptile}

[greptile-apps]

Dead null-check after throwing QI

winrt::com_ptr::as() throws an hresult_error on QueryInterface failure rather than returning a null pointer, so m_textServices will never be null at this point — the if (!m_textServices) return nullptr; guard is unreachable. If you want to tolerate a failed QI gracefully instead of propagating an exception, use spUnk.try_as(m_textServices) and keep the null check; otherwise the block can be removed.

Suggested change

	if (!m_textServices) {
	return nullptr;
	}
	spUnk.as(m_textServices);

[greptile-apps]

Callbacks during m_textServices release may reach live member functions

When m_textServices = nullptr triggers COM Release, the text-services object can still fire callbacks (e.g., TxInvalidateRect → DrawText(), or TxNotify) before the destructor returns. At that moment m_outer is still set (Detach hasn't run yet), so those callbacks enter real member functions on the partially-destroyed object. DrawText() is safe because it already guards !m_textServices (line 1781), but methods like TxSetFocus and TxSetCapture do more work. Consider calling Detach() before nulling out m_textServices so every in-flight callback immediately returns at the null-outer guard:

Suggested change

	WindowsTextInputComponentView::~WindowsTextInputComponentView() {
	// Release text services first to prevent further callbacks into CompTextHost.
	m_textServices = nullptr;
	// Detach the CompTextHost so any lingering references cannot use a dangling pointer.
	if (m_textHost) {
	winrt::get_self<CompTextHost>(m_textHost)->Detach();
	}
	}
	WindowsTextInputComponentView::~WindowsTextInputComponentView() {
	// Detach first so any callbacks fired during text-services teardown
	// are short-circuited by the !m_outer guards.
	if (m_textHost) {
	winrt::get_self<CompTextHost>(m_textHost)->Detach();
	}
	// Now safe to release: all callbacks will see m_outer == nullptr.
	m_textServices = nullptr;
	}

[greptile-apps]

call_once permanently marks failure — DLL load is never retried

std::call_once marks the flag as "done" even when the callable exits normally with hr = E_FAIL. If LoadLibrary fails on the first call, every subsequent call returns E_FAIL for the process lifetime. This is likely intentional, but a clarifying comment would help, and the return hr at the end is redundant (the globals check above already handles the failure path):

Suggested change

	HRESULT HrEnsureRichEd20Loaded() noexcept {
	if (g_hInstRichEdit == nullptr) {
	HRESULT hr = S_OK;
	std::call_once(g_richEditLoadedFlag, [&hr]() {
	g_hInstRichEdit = LoadLibrary(L"Msftedit.dll");
	if (!g_hInstRichEdit)
	return E_FAIL;
	if (!g_hInstRichEdit) {
	hr = E_FAIL;
	return;
	}
	// Create the windowless control (text services object)
	g_pfnCreateTextServices = (PCreateTextServices)GetProcAddress(g_hInstRichEdit, "CreateTextServices");
	if (!g_pfnCreateTextServices)
	return E_FAIL;
	/*
	// Calling the REExtendedRegisterClass() function is required for
	// registering the REComboboxW and REListBoxW window classes.
	PFNREGISTER pfnRegister = (PFNREGISTER)GetProcAddress(g_hInstRichEdit, "REExtendedRegisterClass");
	if (pfnRegister) {
	pfnRegister();
	return S_OK;
	} else
	return E_FAIL;
	*/
	}
	return NOERROR;
	if (!g_pfnCreateTextServices) {
	hr = E_FAIL;
	return;
	}
	});
	if (!g_hInstRichEdit || !g_pfnCreateTextServices)
	return E_FAIL;
	return hr;
	HRESULT HrEnsureRichEd20Loaded() noexcept {
	// Loading is attempted exactly once per process; on failure all subsequent
	// callers receive E_FAIL without retrying.
	HRESULT hr = S_OK;
	std::call_once(g_richEditLoadedFlag, [&hr]() {