Fixed exceptions in dispose not allowing PT Run to terminate (#5615)

Co-authored-by: Clint Rutkas <crutkas@microsoft.com>

installer/Version.props

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <PropertyGroup>
-    <Version>0.20.0</Version>
+    <Version>0.20.1</Version>
     <DefineConstants>Version=$(Version);</DefineConstants>
   </PropertyGroup>
 </Project>

src/bootstrapper/bootstrapper.cpp

@@ -90,7 +90,7 @@ int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int)
     {
         return 1;
     }
-    notifications::set_application_id(APPLICATION_ID);
+    notifications::override_application_id(APPLICATION_ID);
 
     fs::path iconPath{ L"C:\\" };
     if (auto extractedIcon = extractIcon())

src/common/common.cpp

@@ -774,19 +774,25 @@ bool find_app_name_in_path(const std::wstring& where, const std::vector<std::wst
     return false;
 }
 
-std::optional<std::string> exec_and_read_output(const std::wstring_view command, const DWORD timeout)
+std::optional<std::string> exec_and_read_output(const std::wstring_view command, DWORD timeout_ms)
 {
     SECURITY_ATTRIBUTES saAttr{ sizeof(saAttr) };
-    saAttr.bInheritHandle = true;
+    saAttr.bInheritHandle = false;
 
-    wil::unique_handle childStdoutRead;
-    wil::unique_handle childStdoutWrite;
-    if (!CreatePipe(&childStdoutRead, &childStdoutWrite, &saAttr, 0))
+    constexpr size_t bufferSize = 4096;
+    // We must use a named pipe for async I/O
+    char pipename[MAX_PATH + 1];
+    if (!GetTempFileNameA(R"(\\.\pipe\)", "tmp", 1, pipename))
     {
         return std::nullopt;
     }
 
-    if (!SetHandleInformation(childStdoutRead.get(), HANDLE_FLAG_INHERIT, 0))
+    wil::unique_handle readPipe{ CreateNamedPipeA(pipename, PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED, PIPE_TYPE_BYTE | PIPE_READMODE_BYTE, PIPE_UNLIMITED_INSTANCES, bufferSize, bufferSize, 0, &saAttr) };
+
+    saAttr.bInheritHandle = true;
+    wil::unique_handle writePipe{ CreateFileA(pipename, GENERIC_WRITE, 0, &saAttr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr) };
+
+    if (!readPipe || !writePipe)
     {
         return std::nullopt;
     }
@@ -794,8 +800,8 @@ std::optional<std::string> exec_and_read_output(const std::wstring_view command,
     PROCESS_INFORMATION piProcInfo{};
     STARTUPINFOW siStartInfo{ sizeof(siStartInfo) };
 
-    siStartInfo.hStdError = childStdoutWrite.get();
-    siStartInfo.hStdOutput = childStdoutWrite.get();
+    siStartInfo.hStdError = writePipe.get();
+    siStartInfo.hStdOutput = writePipe.get();
     siStartInfo.dwFlags |= STARTF_USESTDHANDLES | STARTF_USESHOWWINDOW;
     siStartInfo.wShowWindow = SW_HIDE;
 
@@ -813,24 +819,49 @@ std::optional<std::string> exec_and_read_output(const std::wstring_view command,
     {
         return std::nullopt;
     }
+    // Child process inherited the write end of the pipe, we can close it now
+    writePipe.reset();
 
-    WaitForSingleObject(piProcInfo.hProcess, timeout);
-
-    childStdoutWrite.reset();
-    CloseHandle(piProcInfo.hThread);
+    auto closeProcessHandles = wil::scope_exit([&] {
+        CloseHandle(piProcInfo.hThread);
+        CloseHandle(piProcInfo.hProcess);
+    });
 
     std::string childOutput;
+    bool processExited = false;
     for (;;)
     {
-        char buffer[4096];
+        char buffer[bufferSize];
         DWORD gotBytes = 0;
-        if (!ReadFile(childStdoutRead.get(), buffer, sizeof(buffer), &gotBytes, nullptr) || !gotBytes)
-        {
-            break;
-        }
-        childOutput += std::string_view{ buffer, gotBytes };
-    }
+        wil::unique_handle IOEvent{ CreateEventW(nullptr, true, false, nullptr) };
+        OVERLAPPED overlapped{ .hEvent = IOEvent.get() };
+        ReadFile(readPipe.get(), buffer, sizeof(buffer), nullptr, &overlapped);
 
-    CloseHandle(piProcInfo.hProcess);
+        const std::array<HANDLE, 2> handlesToWait = { overlapped.hEvent, piProcInfo.hProcess };
+        switch (WaitForMultipleObjects(1 + !processExited, handlesToWait.data(), false, timeout_ms))
+        {
+        case WAIT_OBJECT_0 + 1:
+            if (!processExited)
+            {
+                // When the process exits, we can reduce timeout and read the rest of the output w/o possibly big timeout
+                timeout_ms = 1000;
+                processExited = true;
+                closeProcessHandles.reset();
+            }
+            [[fallthrough]];
+        case WAIT_OBJECT_0:
+            if (GetOverlappedResultEx(readPipe.get(), &overlapped, &gotBytes, timeout_ms, true))
+            {
+                childOutput += std::string_view{ buffer, gotBytes };
+                break;
+            }
+            // Timeout
+            [[fallthrough]];
+        default:
+            goto exit;
+        }
+    }
+exit:
+    CancelIo(readPipe.get());
     return childOutput;
 }

src/common/common.h

@@ -99,7 +99,7 @@ std::wstring get_resource_string(UINT resource_id, HINSTANCE instance, const wch
 // is added to the .cpp file.
 #define GET_RESOURCE_STRING(resource_id) get_resource_string(resource_id, reinterpret_cast<HINSTANCE>(&__ImageBase), L#resource_id)
 
-std::optional<std::string> exec_and_read_output(const std::wstring_view command, const DWORD timeout = INFINITE);
+std::optional<std::string> exec_and_read_output(const std::wstring_view command, DWORD timeout_ms = 30000);
 
 // Helper class for various COM-related APIs, e.g working with security descriptors
 template<typename T>

src/common/notifications.cpp

@@ -30,6 +30,7 @@ using winrt::Windows::UI::Notifications::ToastNotificationManager;
 
 namespace fs = std::filesystem;
 
+// This namespace contains strings that SHOULD NOT be localized
 namespace
 {
     constexpr std::wstring_view TASK_NAME = L"PowerToysBackgroundNotificationsHandler";
@@ -37,7 +38,7 @@ namespace
     constexpr std::wstring_view PACKAGED_APPLICATION_ID = L"PowerToys";
     constexpr std::wstring_view APPIDS_REGISTRY = LR"(Software\Classes\AppUserModelId\)";
 
-    std::wstring APPLICATION_ID;
+    std::wstring APPLICATION_ID = L"Microsoft.PowerToysWin32";
 }
 
 namespace localized_strings
@@ -184,7 +185,7 @@ void notifications::unregister_application_id()
     RegDeleteKeyW(registryRoot.get(), APPLICATION_ID.data());
 }
 
-void notifications::set_application_id(const std::wstring_view appID)
+void notifications::override_application_id(const std::wstring_view appID)
 {
     APPLICATION_ID = appID;
     SetCurrentProcessExplicitAppUserModelID(APPLICATION_ID.c_str());

src/common/notifications.h

@@ -10,7 +10,7 @@ namespace notifications
 {
     constexpr inline const wchar_t TOAST_ACTIVATED_LAUNCH_ARG[] = L"-ToastActivated";
 
-    void set_application_id(const std::wstring_view appID);
+    void override_application_id(const std::wstring_view appID);
     void register_background_toast_handler();
     void run_desktop_app_activator_loop();
 

src/modules/colorPicker/ColorPicker/ColorPicker.rc

@@ -4,7 +4,7 @@
 
 STRINGTABLE
 BEGIN
-    IDS_LAUNCHER_NAME            L"Color Picker" 
+    IDS_LAUNCHER_NAME            L"ColorPicker" 
     IDS_LAUNCHER_SETTINGS_DESC   L"This feature requires Windows 10 version 1903 or higher"
 END
 

src/modules/colorPicker/ColorPickerUI/Helpers/Logger.cs

@@ -64,7 +64,7 @@ namespace ColorPicker.Helpers
 
             var methodName = stackTrace.GetFrame(3)?.GetMethod();
             var className = methodName?.DeclaringType.Name;
-            return "[Method]: " + methodName.Name + " [Class]: " + className;
+            return "[Method]: " + methodName?.Name + " [Class]: " + className;
         }
     }
 }

src/modules/colorPicker/ColorPickerUI/Settings/UserSettings.cs

@@ -78,7 +78,13 @@ namespace ColorPicker.Settings
                         }
                         catch (Exception ex)
                         {
+                            if (retryCount > MaxNumberOfRetry)
+                            {
+                                retry = false;
+                            }
+
                             Logger.LogError("Failed to read changed settings", ex);
+                            Thread.Sleep(500);
                         }
                     }
                 };

src/modules/launcher/PowerLauncher/App.xaml.cs

@@ -60,8 +60,15 @@ namespace PowerLauncher
         {
             RunnerHelper.WaitForPowerToysRunner(_powerToysPid, () =>
             {
-                Dispose();
-                Environment.Exit(0);
+                try
+                {
+                    Dispose();
+                }
+                finally
+                {
+
+                    Environment.Exit(0);
+                }
             });
 
             var bootTime = new System.Diagnostics.Stopwatch();

src/modules/launcher/PowerLauncher/MainWindow.xaml.cs

@@ -1,20 +1,16 @@
-using System;
-using System.ComponentModel;
-using System.Windows;
-using System.Windows.Input;
-using System.Windows.Media.Animation;
-using PowerLauncher.Helper;
-using Wox.Infrastructure.UserSettings;
-using PowerLauncher.ViewModel;
-
-using Screen = System.Windows.Forms.Screen;
-using KeyEventArgs = System.Windows.Input.KeyEventArgs;
-using System.Windows.Controls;
-using System.Threading.Tasks;
-using System.Diagnostics;
-using System.Timers;
-using Microsoft.PowerLauncher.Telemetry;
+using Microsoft.PowerLauncher.Telemetry;
 using Microsoft.PowerToys.Telemetry;
+using PowerLauncher.Helper;
+using PowerLauncher.ViewModel;
+using System;
+using System.ComponentModel;
+using System.Timers;
+using System.Windows;
+using System.Windows.Controls;
+using System.Windows.Input;
+using Wox.Infrastructure.UserSettings;
+using KeyEventArgs = System.Windows.Input.KeyEventArgs;
+using Screen = System.Windows.Forms.Screen;
 
 
 namespace PowerLauncher
@@ -47,10 +43,13 @@ namespace PowerLauncher
 
         private void CheckForFirstDelete(object sender, ElapsedEventArgs e)
         {
-            _firstDeleteTimer.Stop();
-            if (_deletePressed)
+            if (_firstDeleteTimer != null)
             {
-                PowerToysTelemetry.Log.WriteEvent(new LauncherFirstDeleteEvent());
+                _firstDeleteTimer.Stop();
+                if (_deletePressed)
+                {
+                    PowerToysTelemetry.Log.WriteEvent(new LauncherFirstDeleteEvent());
+                }
             }
 
         }
@@ -350,7 +349,10 @@ namespace PowerLauncher
             if (Visibility == Visibility.Visible)
             {
                 _deletePressed = false;
-                _firstDeleteTimer.Start();
+                if (_firstDeleteTimer != null)
+                {
+                    _firstDeleteTimer.Start();
+                }
                 // (this.FindResource("IntroStoryboard") as Storyboard).Begin();
 
                 SearchBox.QueryTextBox.Focus();
@@ -376,7 +378,10 @@ namespace PowerLauncher
             }
             else
             {
-                _firstDeleteTimer.Stop();
+                if (_firstDeleteTimer != null)
+                {
+                    _firstDeleteTimer.Stop();
+                }
             }
         }
 
@@ -402,11 +407,15 @@ namespace PowerLauncher
             {
                 if (disposing)
                 {
-                    _firstDeleteTimer.Dispose();
+                    if (_firstDeleteTimer != null)
+                    {
+                        _firstDeleteTimer.Dispose();
+                    }
                 }
 
                 // TODO: free unmanaged resources (unmanaged objects) and override finalizer
                 // TODO: set large fields to null
+                _firstDeleteTimer = null;
                 disposedValue = true;
             }
         }

src/modules/launcher/PowerLauncher/ViewModel/MainViewModel.cs

@@ -815,8 +815,8 @@ namespace PowerLauncher.ViewModel
                     {
                         _hotkeyManager.UnregisterHotkey(_hotkeyHandle);
                     }
-                    _hotkeyManager.Dispose();
-                    _updateSource.Dispose();
+                    _hotkeyManager?.Dispose();
+                    _updateSource?.Dispose();
                     _disposed = true;
                 }
             }

src/runner/main.cpp

@@ -48,7 +48,6 @@ namespace localized_strings
 namespace
 {
     const wchar_t PT_URI_PROTOCOL_SCHEME[] = L"powertoys://";
-    const wchar_t APPLICATION_ID[] = L"Microsoft.PowerToysWin32";
 }
 
 void chdir_current_executable()
@@ -114,7 +113,6 @@ int runner(bool isProcessElevated)
             } }.detach();
         }
 
-        notifications::set_application_id(APPLICATION_ID);
         notifications::register_background_toast_handler();
 
         chdir_current_executable();
@@ -306,7 +304,6 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine
     {
         return 0;
     }
-
     int n_cmd_args = 0;
     LPWSTR* cmd_arg_list = CommandLineToArgvW(GetCommandLineW(), &n_cmd_args);
     switch (should_run_in_special_mode(n_cmd_args, cmd_arg_list))