Investigating test hangs

cmake/common_build_flags.cmake

@@ -56,8 +56,5 @@ else()
 
         # Some tests have a LOT of template instantiations
         /bigobj
-
-        # NOTE: Temporary workaround while https://github.com/microsoft/wil/issues/102 is being investigated
-        /d2FH4-
         )
 endif()

include/wil/com.h

@@ -3394,6 +3394,9 @@ class com_timeout_t
         }
     }
 
+    //! !IMPORTANT! This value is updated concurrently on a separate thread *after* cancellation is requested. Therefore, it is
+    //! not guaranteed that this value is up to date when read immediately after a cancelled call returns.
+    // TODO: This should **at the very least** be updated to use atomic reads and writes
     bool timed_out() const
     {
         return m_timedOut;

include/wil/com_apartment_variable.h

@@ -149,6 +149,7 @@ namespace details
     template <apartment_variable_leak_action leak_action = apartment_variable_leak_action::fail_fast, typename test_hook = apartment_variable_platform>
     struct apartment_variable_base
     {
+    protected:
         inline static winrt::slim_mutex s_lock;
 
         struct apartment_variable_storage
@@ -169,6 +170,50 @@ namespace details
         // Apartment id -> variables storage.
         inline static wil::object_without_destructor_on_shutdown<std::unordered_map<unsigned long long, apartment_variable_storage>> s_apartmentStorage;
 
+        // NOTE: Requires 's_lock' to be held
+        static apartment_variable_storage* get_current_apartment_variable_storage()
+        {
+            auto storage = s_apartmentStorage.get().find(test_hook::GetApartmentId());
+            if (storage != s_apartmentStorage.get().end())
+            {
+                return &storage->second;
+            }
+            return nullptr;
+        }
+
+        // NOTE: Requires 's_lock' to be held
+        apartment_variable_storage* ensure_current_apartment_variables()
+        {
+            auto variables = get_current_apartment_variable_storage();
+            if (variables)
+            {
+                return variables;
+            }
+
+            struct ApartmentObserver : public winrt::implements<ApartmentObserver, IApartmentShutdown>
+            {
+                void STDMETHODCALLTYPE OnUninitialize(unsigned long long apartmentId) noexcept override
+                {
+                    // This code runs at apartment rundown so be careful to avoid deadlocks by
+                    // extracting the variables under the lock then release them outside.
+                    auto variables = [apartmentId]() {
+                        auto lock = winrt::slim_lock_guard(s_lock);
+                        return s_apartmentStorage.get().extract(apartmentId);
+                    }();
+                    WI_ASSERT(variables.key() == apartmentId);
+                    // The system implicitly releases the shutdown observer
+                    // after invoking the callback and does not allow calling unregister
+                    // in the callback. So release the reference to the registration.
+                    variables.mapped().cookie.release();
+                }
+            };
+            auto shutdownRegistration = test_hook::RegisterForApartmentShutdown(winrt::make<ApartmentObserver>().get());
+            return &s_apartmentStorage.get()
+                        .insert({test_hook::GetApartmentId(), apartment_variable_storage(std::move(shutdownRegistration))})
+                        .first->second;
+        }
+
+    public:
         constexpr apartment_variable_base() = default;
         ~apartment_variable_base()
         {
@@ -226,47 +271,6 @@ namespace details
             return *any;
         }
 
-        static apartment_variable_storage* get_current_apartment_variable_storage()
-        {
-            auto storage = s_apartmentStorage.get().find(test_hook::GetApartmentId());
-            if (storage != s_apartmentStorage.get().end())
-            {
-                return &storage->second;
-            }
-            return nullptr;
-        }
-
-        apartment_variable_storage* ensure_current_apartment_variables()
-        {
-            auto variables = get_current_apartment_variable_storage();
-            if (variables)
-            {
-                return variables;
-            }
-
-            struct ApartmentObserver : public winrt::implements<ApartmentObserver, IApartmentShutdown>
-            {
-                void STDMETHODCALLTYPE OnUninitialize(unsigned long long apartmentId) noexcept override
-                {
-                    // This code runs at apartment rundown so be careful to avoid deadlocks by
-                    // extracting the variables under the lock then release them outside.
-                    auto variables = [apartmentId]() {
-                        auto lock = winrt::slim_lock_guard(s_lock);
-                        return s_apartmentStorage.get().extract(apartmentId);
-                    }();
-                    WI_ASSERT(variables.key() == apartmentId);
-                    // The system implicitly releases the shutdown observer
-                    // after invoking the callback and does not allow calling unregister
-                    // in the callback. So release the reference to the registration.
-                    variables.mapped().cookie.release();
-                }
-            };
-            auto shutdownRegistration = test_hook::RegisterForApartmentShutdown(winrt::make<ApartmentObserver>().get());
-            return &s_apartmentStorage.get()
-                        .insert({test_hook::GetApartmentId(), apartment_variable_storage(std::move(shutdownRegistration))})
-                        .first->second;
-        }
-
         // get current value or custom-construct one on demand
         template <typename T>
         std::any& get_or_create(any_maker<T>&& creator)

include/wil/resource.h

@@ -2602,13 +2602,10 @@ namespace details
     {
         static void Destroy(_In_ PTP_TIMER threadpoolTimer) WI_NOEXCEPT
         {
+            static_assert(cancellationBehavior != PendingCallbackCancellationBehavior::NoWait); // Handled by partial specialization
             threadpool_t::SetThreadpoolTimer(threadpoolTimer, nullptr, 0, 0);
-#pragma warning(suppress : 4127) // conditional expression is constant
-            if (cancellationBehavior != PendingCallbackCancellationBehavior::NoWait)
-            {
-                threadpool_t::WaitForThreadpoolTimerCallbacks(
-                    threadpoolTimer, (cancellationBehavior == PendingCallbackCancellationBehavior::Cancel));
-            }
+            threadpool_t::WaitForThreadpoolTimerCallbacks(
+                threadpoolTimer, (cancellationBehavior == PendingCallbackCancellationBehavior::Cancel));
             threadpool_t::CloseThreadpoolTimer(threadpoolTimer);
         }
     };

scripts/azure-pipelines.yml

@@ -80,5 +80,8 @@ jobs:
   - script: |
       call scripts\call-vcvars.cmd $(arch)
       set ASAN_WIN_CONTINUE_ON_INTERCEPTION_FAILURE=1
-      call scripts\runtests.cmd ~[LocalOnly]
+      REM NOTE: You can add '-s' to display detailed information about passing assertions, which can be helpful for diagnosing
+      REM hanging tests, however the callstack logic has mostly rendered this unnecssary for most scenarios.
+      REM That said, we do pass '-d yes' so that it's easier to see which tests were run immediately before a hanging/crashing test
+      call scripts\runtests.cmd ~[LocalOnly] -d yes
     displayName: 'Test $(compiler) $(arch)$(build-type)'

scripts/enable-appverifier.cmd

@@ -0,0 +1,11 @@
+@echo off
+setlocal
+setlocal EnableDelayedExpansion
+
+REM NOTE: AppVerifier & ASan don't mix well, so leaving it out
+set EXES=witest.app witest.cpplatest witest.cppwinrt-notifiable-server-lock witest.noexcept witest witest.win7
+set LAYERS=Exceptions Handles Heaps Locks Memory SRWLock Threadpool TLS
+
+for %%e in (%EXES%) do (
+    appverif /enable %LAYERS% /for %%e.exe
+)

scripts/init.cmd

@@ -163,7 +163,7 @@ goto :init
     if "%VCPKG_ROOT_PATH%"=="" (
         :: First check for %VCPKG_ROOT% variable
         if defined VCPKG_ROOT (
-            set VCPKG_ROOT_PATH=%VCPKG_ROOT%
+            set "VCPKG_ROOT_PATH=%VCPKG_ROOT%"
         ) else (
             :: Next check the PATH for vcpkg.exe
             for %%i in (vcpkg.exe) do set VCPKG_ROOT_PATH=%%~dp$PATH:i

scripts/stress-test.cmd

@@ -0,0 +1,10 @@
+@echo off
+setlocal
+setlocal EnableDelayedExpansion
+
+set THIS_DIR=%~dp0\
+
+:repeat
+REM If you're running this test, you probably don't want it constantly clearing out your clipboard...
+call %THIS_DIR%\runtests.cmd ~UniqueCloseClipboardCall %*
+if %ERRORLEVEL%==0 goto :repeat

tests/CMakeLists.txt

@@ -4,9 +4,6 @@ include(${PROJECT_SOURCE_DIR}/cmake/common_build_flags.cmake)
 # All projects need to reference the WIL headers
 include_directories(${PROJECT_SOURCE_DIR}/include)
 
-# TODO: Might be worth trying to conditionally do this on SDK version, assuming there's a semi-easy way to detect that
-include_directories(BEFORE SYSTEM ./workarounds/wrl)
-
 # Because we don't always use msbuild, we need to run nuget manually
 find_program(NUGET nuget)
 if (NOT NUGET)
@@ -62,7 +59,6 @@ if (MSVC AND "${CMAKE_BUILD_TYPE}" STREQUAL "RelWithDebInfo")
 endif()
 
 set(COMMON_SOURCES
-    ${CMAKE_CURRENT_SOURCE_DIR}/main.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/CommonTests.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/ComTests.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/FileSystemTests.cpp
@@ -72,6 +68,7 @@ set(COMMON_SOURCES
     ${CMAKE_CURRENT_SOURCE_DIR}/ResultTests.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/Rpc.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/SafeCastTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/StlTests.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/TraceLoggingTests.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/TraceLoggingTests_PartB.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/WindowingTests.cpp
@@ -80,6 +77,43 @@ set(COMMON_SOURCES
     ${CMAKE_CURRENT_SOURCE_DIR}/../natvis/wil.natvis
     )
 
+# Source files that can be compiled downlevel to at least Win7
+set(DOWNLEVEL_SOURCES
+    ${CMAKE_CURRENT_SOURCE_DIR}/NetworkTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/RegistryTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/TokenHelpersTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/WatcherTests.cpp
+    )
+
+# Source files that require WinRT (Win8+ & can compile with App partition)
+set(WINRT_SOURCES
+    ${CMAKE_CURRENT_SOURCE_DIR}/UniqueWinRTEventTokenTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/WinRTTests.cpp
+    )
+
+# Source files that can compile on Win11+, but only on the Destkop partition
+set(WIN11_DESTKTOP_SOURCES
+    ${CMAKE_CURRENT_SOURCE_DIR}/WinVerifyTrustTest.cpp
+    )
+
+# Source files that require C++20
+set(CPP20_SOURCES
+    ${CMAKE_CURRENT_SOURCE_DIR}/CoroutineTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/CppWinRT20Tests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/CppWinRTAuthoringTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/CppWinRTTests.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/app.manifest
+    )
+
+# It appears as though Clang has some issues with exception handling inside of coroutines, which causes issues when
+# trying to run the ComApartmentVariableTests.cpp tests, so disable on Clang for now
+if (NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+    set(CPP20_SOURCES
+        ${CPP20_SOURCES}
+        ${CMAKE_CURRENT_SOURCE_DIR}/ComApartmentVariableTests.cpp
+        )
+endif()
+
 if (MSVC)
     add_link_options(/NATVIS:${CMAKE_SOURCE_DIR}/natvis/wil.natvis)
 endif()
@@ -91,7 +125,25 @@ find_package(Catch2 CONFIG REQUIRED)
 
 include_directories(${DETOURS_INCLUDE_DIRS})
 add_definitions(-DNOMINMAX)
-link_libraries(${DETOURS_LIBRARY} Catch2::Catch2WithMain ws2_32.lib ntdll.lib)
+
+# We build the 'app' tests just to ensure various things can build for the App partition. We don't actually run them in
+# an app container or anything. By building 'main.cpp' separately and linking to it, we allow the app tests to get the
+# same callstack printing logic as the rest of the tests.
+add_library(witest.main STATIC)
+target_sources(witest.main
+    PRIVATE
+        main.cpp
+    )
+target_link_libraries(witest.main
+    INTERFACE
+        ${DETOURS_LIBRARY}
+        Catch2::Catch2
+        ntdll.lib
+        dbghelp.lib
+        ws2_32.lib
+    )
+
+link_libraries(witest.main)
 
 add_subdirectory(app)
 add_subdirectory(cpplatest)

tests/ComApartmentVariableTests.cpp

@@ -5,6 +5,7 @@
 #include <functional>
 
 #include "common.h"
+#include "cppwinrt_threadpool_guard.h"
 
 #if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP | WINAPI_PARTITION_SYSTEM)
 
@@ -38,7 +39,11 @@ void co_wait(const wil::unique_event& e)
 
 void RunApartmentVariableTest(void (*test)())
 {
-    test();
+    {
+        cppwinrt_threadpool_guard guard;
+        test();
+    }
+
     // Apartment variable rundown is async, wait for the last COM apartment
     // to rundown before proceeding to the next test.
     WaitForAllComApartmentsToRundown();
@@ -130,6 +135,10 @@ void TestApartmentVariableAllMethods()
     auto coUninit = platform::CoInitializeEx(COINIT_MULTITHREADED);
 
     std::ignore = g_v1.get_or_create(fn);
+    auto clearOnExit = wil::scope_exit([&] {
+        // Other tests rely on the C++/WinRT module lock count being zero at the start. Ensure we clean up after ourselves
+        g_v1.clear();
+    });
 
     wil::apartment_variable<int, wil::apartment_variable_leak_action::fail_fast, platform> v1;