From 498e0bdc4b222367587d5cfb38969b8cf969f771 Mon Sep 17 00:00:00 2001 From: Alexander Sklar <22989529+asklar@users.noreply.github.com> Date: Wed, 8 Jul 2026 10:49:34 -0700 Subject: [PATCH] Replace manual COM pointers with WIL Use wil::com_ptr for owned COM references in the XAML, WPF, and WinForms TAP paths while preserving XAML diagnostics raw ABI lifetime behavior where ownership is unclear. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- CMakeLists.txt | 6 +- src/tap/lvt_tap.cpp | 90 +++++++++++++----------- src/tap_winforms/winforms_tap_native.cpp | 30 ++++---- src/tap_wpf/wpf_tap_native.cpp | 30 ++++---- 4 files changed, 76 insertions(+), 80 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 1ccdfd0..53d4833 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -156,7 +156,7 @@ add_library(lvt_tap SHARED ) target_compile_definitions(lvt_tap PRIVATE WIN32_LEAN_AND_MEAN NOMINMAX WINRT_LEAN_AND_MEAN) target_include_directories(lvt_tap PRIVATE src src/tap/winui3) -target_link_libraries(lvt_tap PRIVATE ole32) +target_link_libraries(lvt_tap PRIVATE WIL::WIL ole32) set_property(TARGET lvt_tap PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>") # Place lvt_tap_{arch}.dll next to lvt.exe @@ -173,7 +173,7 @@ add_library(lvt_wpf_tap SHARED ) target_compile_definitions(lvt_wpf_tap PRIVATE WIN32_LEAN_AND_MEAN NOMINMAX) target_include_directories(lvt_wpf_tap PRIVATE src) -target_link_libraries(lvt_wpf_tap PRIVATE ole32) +target_link_libraries(lvt_wpf_tap PRIVATE WIL::WIL ole32) set_property(TARGET lvt_wpf_tap PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>") set_target_properties(lvt_wpf_tap PROPERTIES @@ -201,7 +201,7 @@ add_library(lvt_winforms_tap SHARED ) target_compile_definitions(lvt_winforms_tap PRIVATE WIN32_LEAN_AND_MEAN NOMINMAX) target_include_directories(lvt_winforms_tap PRIVATE src) -target_link_libraries(lvt_winforms_tap PRIVATE ole32) +target_link_libraries(lvt_winforms_tap PRIVATE WIL::WIL ole32) set_property(TARGET lvt_winforms_tap PROPERTY MSVC_RUNTIME_LIBRARY "MultiThreaded$<$:Debug>") set_target_properties(lvt_winforms_tap PROPERTIES diff --git a/src/tap/lvt_tap.cpp b/src/tap/lvt_tap.cpp index 096976a..9fb556a 100644 --- a/src/tap/lvt_tap.cpp +++ b/src/tap/lvt_tap.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -95,8 +96,8 @@ static LRESULT CALLBACK LvtTapMsgWndProc(HWND hwnd, UINT msg, WPARAM wParam, LPA class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { LONG m_refCount = 1; - IUnknown* m_site = nullptr; - IXamlDiagnostics* m_diag = nullptr; + wil::com_ptr m_site; + wil::com_ptr m_diag; HWND m_msgWnd = nullptr; // Message-only window for UI thread dispatch std::map m_nodes; std::vector m_roots; @@ -104,7 +105,7 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { bool m_collectProps = false; public: - IVisualTreeService* m_vts = nullptr; + wil::com_ptr m_vts; static constexpr UINT WM_COLLECT_BOUNDS = WM_USER + 100; public: @@ -137,11 +138,17 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { HRESULT STDMETHODCALLTYPE SetSite(IUnknown* pSite) override { LogMsg("SetSite called, pSite=%p", pSite); - if (m_site) { m_site->Release(); m_site = nullptr; } - if (m_vts) { m_vts->Release(); m_vts = nullptr; } + m_site.reset(); + m_vts.reset(); + m_diag.reset(); - m_site = pSite; - if (m_site) m_site->AddRef(); + if (pSite) { + HRESULT hr = pSite->QueryInterface(IID_PPV_ARGS(m_site.put())); + if (FAILED(hr)) { + LogMsg("QI for IUnknown failed: 0x%08X", hr); + return S_OK; + } + } if (!pSite) return S_OK; @@ -159,8 +166,8 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { } HRESULT SetSiteImpl(IUnknown* pSite) { - IXamlDiagnostics* diag = nullptr; - HRESULT hr = pSite->QueryInterface(__uuidof(IXamlDiagnostics), (void**)&diag); + wil::com_ptr diag; + HRESULT hr = pSite->QueryInterface(IID_PPV_ARGS(diag.put())); if (FAILED(hr) || !diag) { LogMsg("QI for IXamlDiagnostics failed: 0x%08X", hr); return S_OK; @@ -183,14 +190,13 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { LogMsg("Pipe name: %ls, collectProps: %d", m_pipeName.c_str(), m_collectProps); } - hr = diag->QueryInterface(__uuidof(IVisualTreeService), (void**)&m_vts); + hr = diag->QueryInterface(IID_PPV_ARGS(m_vts.put())); if (FAILED(hr) || !m_vts) { LogMsg("QI for IVisualTreeService failed: 0x%08X", hr); - diag->Release(); return S_OK; } - m_diag = diag; + m_diag = std::move(diag); // Create a message-only window on the UI thread for dispatching // GetPropertyValuesChain calls (which have thread affinity). @@ -209,58 +215,64 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { // AdviseVisualTreeChange hangs if called on the SetSite thread. // Fire-and-forget a worker thread (same as Windhawk). AddRef(); - HANDLE hThread = CreateThread(nullptr, 0, &AdviseThreadProc, this, 0, nullptr); + wil::com_ptr threadSelf; + threadSelf.attach(this); + HANDLE hThread = CreateThread(nullptr, 0, &AdviseThreadProc, threadSelf.get(), 0, nullptr); if (hThread) { + (void)threadSelf.detach(); CloseHandle(hThread); // Don't wait — return immediately to unblock UI thread } else { LogMsg("CreateThread failed: %lu", GetLastError()); - Release(); } return S_OK; } static DWORD WINAPI AdviseThreadProc(LPVOID param) { - auto* self = reinterpret_cast(param); + wil::com_ptr self; + self.attach(reinterpret_cast(param)); + return self->AdviseThreadProcImpl(); + } + + DWORD AdviseThreadProcImpl() { LogMsg("AdviseThread starting"); __try { IVisualTreeServiceCallback* cb = static_cast( - static_cast(self)); + static_cast(this)); - HRESULT hr = self->m_vts->AdviseVisualTreeChange(cb); + HRESULT hr = m_vts->AdviseVisualTreeChange(cb); LogMsg("AdviseVisualTreeChange returned 0x%08X, nodes=%zu, roots=%zu", - hr, self->m_nodes.size(), self->m_roots.size()); + hr, m_nodes.size(), m_roots.size()); if (SUCCEEDED(hr)) { - if (self->m_nodes.empty()) { + if (m_nodes.empty()) { Sleep(500); - LogMsg("After sleep: nodes=%zu", self->m_nodes.size()); + LogMsg("After sleep: nodes=%zu", m_nodes.size()); } // Dispatch GetPropertyValuesChain to UI thread via message window. // SendMessage blocks until the UI thread processes the message. - if (self->m_msgWnd) { + if (m_msgWnd) { LogMsg("Dispatching CollectBounds to UI thread via SendMessage"); - SendMessageW(self->m_msgWnd, WM_COLLECT_BOUNDS, 0, - reinterpret_cast(self)); + SendMessageW(m_msgWnd, WM_COLLECT_BOUNDS, 0, + reinterpret_cast(this)); } // Get element positions via TransformToVisual (works around broken // ActualOffset serialization in WinUI3). Must run on the UI thread. #if LVT_HAS_XAML_PROJECTION - if (self->m_msgWnd) { - SendMessageW(self->m_msgWnd, WM_COLLECT_BOUNDS + 1, 0, - reinterpret_cast(self)); + if (m_msgWnd) { + SendMessageW(m_msgWnd, WM_COLLECT_BOUNDS + 1, 0, + reinterpret_cast(this)); } #endif - self->SerializeAndSend(); - self->m_vts->UnadviseVisualTreeChange(cb); + SerializeAndSend(); + m_vts->UnadviseVisualTreeChange(cb); } } __except(EXCEPTION_EXECUTE_HANDLER) { LogMsg("AdviseThread crashed: 0x%08X", GetExceptionCode()); } - self->Release(); return 0; } @@ -454,6 +466,7 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { for (auto& [handle, node] : m_nodes) { if (!node.hasBounds) continue; + // Keep raw to preserve XAML diagnostics' existing ABI lifetime behavior. ::IInspectable* raw = nullptr; HRESULT hr = m_diag->GetIInspectableFromHandle(handle, &raw); if (FAILED(hr) || !raw) continue; @@ -514,7 +527,7 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { // Called on the UI thread via SendMessage from the worker thread public: void CollectBoundsOnUIThread() { - CollectBounds(m_vts); + CollectBounds(m_vts.get()); } #if LVT_HAS_XAML_PROJECTION void CollectPositionsOnUIThread() { @@ -621,9 +634,6 @@ class LvtTap : public IObjectWithSite, public IVisualTreeServiceCallback2 { ~LvtTap() { if (m_msgWnd) DestroyWindow(m_msgWnd); - if (m_vts) m_vts->Release(); - if (m_diag) m_diag->Release(); - if (m_site) m_site->Release(); } }; @@ -670,11 +680,10 @@ class LvtTapFactory : public IClassFactory { } HRESULT STDMETHODCALLTYPE CreateInstance(IUnknown* pOuter, REFIID riid, void** ppv) override { if (pOuter) return CLASS_E_NOAGGREGATION; - auto* tap = new (std::nothrow) LvtTap(); + wil::com_ptr tap; + tap.attach(new (std::nothrow) LvtTap()); if (!tap) return E_OUTOFMEMORY; - HRESULT hr = tap->QueryInterface(riid, ppv); - tap->Release(); - return hr; + return tap->QueryInterface(riid, ppv); } HRESULT STDMETHODCALLTYPE LockServer(BOOL) override { return S_OK; } }; @@ -684,11 +693,10 @@ extern "C" { HRESULT STDAPICALLTYPE DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID* ppv) { LogMsg("DllGetClassObject called"); if (rclsid != CLSID_LvtTap) return CLASS_E_CLASSNOTAVAILABLE; - auto* factory = new (std::nothrow) LvtTapFactory(); + wil::com_ptr factory; + factory.attach(new (std::nothrow) LvtTapFactory()); if (!factory) return E_OUTOFMEMORY; - HRESULT hr = factory->QueryInterface(riid, ppv); - factory->Release(); - return hr; + return factory->QueryInterface(riid, ppv); } HRESULT STDAPICALLTYPE DllCanUnloadNow() { return S_FALSE; } diff --git a/src/tap_winforms/winforms_tap_native.cpp b/src/tap_winforms/winforms_tap_native.cpp index d33ff81..9617153 100644 --- a/src/tap_winforms/winforms_tap_native.cpp +++ b/src/tap_winforms/winforms_tap_native.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -87,8 +88,8 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring return false; } - ICLRMetaHost* metaHost = nullptr; - HRESULT hr = pCLRCreateInstance(CLSID_CLRMetaHost, IID_ICLRMetaHost, (void**)&metaHost); + wil::com_ptr metaHost; + HRESULT hr = pCLRCreateInstance(CLSID_CLRMetaHost, IID_PPV_ARGS(metaHost.put())); if (FAILED(hr)) { LogMsg("CLRCreateInstance failed: 0x%08X", hr); FreeLibrary(hMscoree); @@ -96,27 +97,24 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring } // Enumerate loaded runtimes to find the one already running - IEnumUnknown* pEnum = nullptr; - hr = metaHost->EnumerateLoadedRuntimes(GetCurrentProcess(), &pEnum); + wil::com_ptr pEnum; + hr = metaHost->EnumerateLoadedRuntimes(GetCurrentProcess(), pEnum.put()); if (FAILED(hr)) { LogMsg("EnumerateLoadedRuntimes failed: 0x%08X", hr); - metaHost->Release(); return false; } - ICLRRuntimeInfo* runtimeInfo = nullptr; - IUnknown* pUnk = nullptr; + wil::com_ptr runtimeInfo; + wil::com_ptr pUnk; ULONG fetched = 0; - while (pEnum->Next(1, &pUnk, &fetched) == S_OK) { - hr = pUnk->QueryInterface(IID_ICLRRuntimeInfo, (void**)&runtimeInfo); - pUnk->Release(); + while (pEnum->Next(1, pUnk.put(), &fetched) == S_OK) { + hr = pUnk->QueryInterface(IID_PPV_ARGS(runtimeInfo.put())); if (SUCCEEDED(hr)) break; + pUnk.reset(); } - pEnum->Release(); if (!runtimeInfo) { LogMsg("No loaded CLR runtime found"); - metaHost->Release(); return false; } @@ -125,10 +123,8 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring runtimeInfo->GetVersionString(version, &versionLen); LogMsg("Found CLR runtime: %ls", version); - ICLRRuntimeHost* runtimeHost = nullptr; - hr = runtimeInfo->GetInterface(CLSID_CLRRuntimeHost, IID_ICLRRuntimeHost, (void**)&runtimeHost); - runtimeInfo->Release(); - metaHost->Release(); + wil::com_ptr runtimeHost; + hr = runtimeInfo->GetInterface(CLSID_CLRRuntimeHost, IID_PPV_ARGS(runtimeHost.put())); if (FAILED(hr) || !runtimeHost) { LogMsg("GetInterface for ICLRRuntimeHost failed: 0x%08X", hr); @@ -145,8 +141,6 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring pipeName.c_str(), &retVal); - runtimeHost->Release(); - LogMsg("ExecuteInDefaultAppDomain returned 0x%08X, retVal=%lu", hr, retVal); return SUCCEEDED(hr); } diff --git a/src/tap_wpf/wpf_tap_native.cpp b/src/tap_wpf/wpf_tap_native.cpp index 3f5d3b2..ee7008d 100644 --- a/src/tap_wpf/wpf_tap_native.cpp +++ b/src/tap_wpf/wpf_tap_native.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -87,8 +88,8 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring return false; } - ICLRMetaHost* metaHost = nullptr; - HRESULT hr = pCLRCreateInstance(CLSID_CLRMetaHost, IID_ICLRMetaHost, (void**)&metaHost); + wil::com_ptr metaHost; + HRESULT hr = pCLRCreateInstance(CLSID_CLRMetaHost, IID_PPV_ARGS(metaHost.put())); if (FAILED(hr)) { LogMsg("CLRCreateInstance failed: 0x%08X", hr); FreeLibrary(hMscoree); @@ -96,27 +97,24 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring } // Enumerate loaded runtimes to find the one already running - IEnumUnknown* pEnum = nullptr; - hr = metaHost->EnumerateLoadedRuntimes(GetCurrentProcess(), &pEnum); + wil::com_ptr pEnum; + hr = metaHost->EnumerateLoadedRuntimes(GetCurrentProcess(), pEnum.put()); if (FAILED(hr)) { LogMsg("EnumerateLoadedRuntimes failed: 0x%08X", hr); - metaHost->Release(); return false; } - ICLRRuntimeInfo* runtimeInfo = nullptr; - IUnknown* pUnk = nullptr; + wil::com_ptr runtimeInfo; + wil::com_ptr pUnk; ULONG fetched = 0; - while (pEnum->Next(1, &pUnk, &fetched) == S_OK) { - hr = pUnk->QueryInterface(IID_ICLRRuntimeInfo, (void**)&runtimeInfo); - pUnk->Release(); + while (pEnum->Next(1, pUnk.put(), &fetched) == S_OK) { + hr = pUnk->QueryInterface(IID_PPV_ARGS(runtimeInfo.put())); if (SUCCEEDED(hr)) break; + pUnk.reset(); } - pEnum->Release(); if (!runtimeInfo) { LogMsg("No loaded CLR runtime found"); - metaHost->Release(); return false; } @@ -125,10 +123,8 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring runtimeInfo->GetVersionString(version, &versionLen); LogMsg("Found CLR runtime: %ls", version); - ICLRRuntimeHost* runtimeHost = nullptr; - hr = runtimeInfo->GetInterface(CLSID_CLRRuntimeHost, IID_ICLRRuntimeHost, (void**)&runtimeHost); - runtimeInfo->Release(); - metaHost->Release(); + wil::com_ptr runtimeHost; + hr = runtimeInfo->GetInterface(CLSID_CLRRuntimeHost, IID_PPV_ARGS(runtimeHost.put())); if (FAILED(hr) || !runtimeHost) { LogMsg("GetInterface for ICLRRuntimeHost failed: 0x%08X", hr); @@ -145,8 +141,6 @@ static bool TryNetFramework(const std::wstring& assemblyPath, const std::wstring pipeName.c_str(), &retVal); - runtimeHost->Release(); - LogMsg("ExecuteInDefaultAppDomain returned 0x%08X, retVal=%lu", hr, retVal); return SUCCEEDED(hr); }