Fix vague startup crash messages with WinPTY

Fixes #2344.

2 files changed, 80 insertions, 60 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0ead9370..3bd9f84d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Width of Unicode 11/12 emojis
 - Minimize on windows causing layout issues
 - Performance bottleneck when clearing colored rows
+- Vague startup crash messages on Windows with WinPTY backend
 
 ## 0.4.0
 
diff --git a/winpty/src/windows.rs b/winpty/src/windows.rs
index 27780d4f..20cb3dc3 100644
--- a/winpty/src/windows.rs
+++ b/winpty/src/windows.rs
@@ -1,4 +1,3 @@
-use std::error::Error;
 use std::fmt::{self, Display, Formatter};
 use std::os::windows::io::RawHandle;
 use std::path::PathBuf;
@@ -9,8 +8,8 @@ use winpty_sys::*;
 
 use widestring::WideCString;
 
-pub enum ErrorCodes {
-    Success,
+#[derive(Copy, Clone, Debug)]
+pub enum ErrorCode {
     OutOfMemory,
     SpawnCreateProcessFailed,
     LostConnection,
@@ -19,6 +18,7 @@ pub enum ErrorCodes {
     AgentDied,
     AgentTimeout,
     AgentCreationFailed,
+    UnknownError(u32),
 }
 
 pub enum MouseMode {
@@ -43,32 +43,43 @@ bitflags!(
 );
 
 #[derive(Debug)]
-pub struct Err {
-    code: u32,
+pub struct Error {
+    code: ErrorCode,
     message: String,
 }
 
 // Check to see whether winpty gave us an error, and perform the necessary memory freeing
-fn check_err(e: *mut winpty_error_t) -> Option<Err> {
+fn check_err(e: *mut winpty_error_t) -> Result<(), Error> {
     unsafe {
         let code = winpty_error_code(e);
         let raw = winpty_error_msg(e);
         let message = String::from_utf16_lossy(std::slice::from_raw_parts(raw, wcslen(raw)));
         winpty_error_free(e);
-        match code {
-            0 => None,
-            _ => Some(Err { code, message }),
-        }
+
+        let code = match code {
+            WINPTY_ERROR_SUCCESS => return Ok(()),
+            WINPTY_ERROR_OUT_OF_MEMORY => ErrorCode::OutOfMemory,
+            WINPTY_ERROR_SPAWN_CREATE_PROCESS_FAILED => ErrorCode::SpawnCreateProcessFailed,
+            WINPTY_ERROR_LOST_CONNECTION => ErrorCode::LostConnection,
+            WINPTY_ERROR_AGENT_EXE_MISSING => ErrorCode::AgentExeMissing,
+            WINPTY_ERROR_UNSPECIFIED => ErrorCode::Unspecified,
+            WINPTY_ERROR_AGENT_DIED => ErrorCode::AgentDied,
+            WINPTY_ERROR_AGENT_TIMEOUT => ErrorCode::AgentTimeout,
+            WINPTY_ERROR_AGENT_CREATION_FAILED => ErrorCode::AgentCreationFailed,
+            code => ErrorCode::UnknownError(code),
+        };
+
+        Err(Error { code, message })
     }
 }
 
-impl Display for Err {
+impl Display for Error {
     fn fmt(&self, f: &mut Formatter) -> Result<(), fmt::Error> {
-        write!(f, "Code: {}, Message: {}", self.code, self.message)
+        write!(f, "Code: {:?}, Message: {}", self.code, self.message)
     }
 }
 
-impl Error for Err {
+impl std::error::Error for Error {
     fn description(&self) -> &str {
         &self.message
     }
@@ -79,10 +90,12 @@ impl Error for Err {
 pub struct Config(*mut winpty_config_t);
 
 impl Config {
-    pub fn new(flags: ConfigFlags) -> Result<Self, Err> {
+    pub fn new(flags: ConfigFlags) -> Result<Self, Error> {
         let mut err = null_mut() as *mut winpty_error_t;
         let config = unsafe { winpty_config_new(flags.bits(), &mut err) };
-        check_err(err).map_or(Ok(Self(config)), Result::Err)
+        check_err(err)?;
+
+        Ok(Self(config))
     }
 
     /// Set the initial size of the console window
@@ -95,12 +108,12 @@ impl Config {
     /// Set the mouse mode
     pub fn set_mouse_mode(&mut self, mode: &MouseMode) {
         let m = match mode {
-            MouseMode::None => 0,
-            MouseMode::Auto => 1,
-            MouseMode::Force => 2,
+            MouseMode::None => WINPTY_MOUSE_MODE_NONE,
+            MouseMode::Auto => WINPTY_MOUSE_MODE_AUTO,
+            MouseMode::Force => WINPTY_MOUSE_MODE_FORCE,
         };
         unsafe {
-            winpty_config_set_mouse_mode(self.0, m);
+            winpty_config_set_mouse_mode(self.0, m as i32);
         }
     }
 
@@ -127,14 +140,21 @@ impl Drop for Config {
 /// A struct representing the winpty agent process
 pub struct Winpty(*mut winpty_t);
 
+pub struct ChildHandles {
+    pub process: HANDLE,
+    pub thread: HANDLE,
+}
+
 impl Winpty {
     /// Starts the agent. This process will connect to the agent
     /// over a control pipe, and the agent will open data pipes
     /// (e.g. CONIN and CONOUT).
-    pub fn open(cfg: &Config) -> Result<Self, Err> {
+    pub fn open(cfg: &Config) -> Result<Self, Error> {
         let mut err = null_mut() as *mut winpty_error_t;
         let winpty = unsafe { winpty_open(cfg.0, &mut err) };
-        check_err(err).map_or(Ok(Self(winpty)), Result::Err)
+        check_err(err)?;
+
+        Ok(Self(winpty))
     }
 
     /// Returns the handle to the winpty agent process
@@ -173,7 +193,7 @@ impl Winpty {
     /// Change the size of the Windows console window.
     ///
     /// cols & rows MUST be greater than 0
-    pub fn set_size(&mut self, cols: u16, rows: u16) -> Result<(), Err> {
+    pub fn set_size(&mut self, cols: u16, rows: u16) -> Result<(), Error> {
         assert!(cols > 0 && rows > 0);
         let mut err = null_mut() as *mut winpty_error_t;
 
@@ -181,14 +201,14 @@ impl Winpty {
             winpty_set_size(self.0, i32::from(cols), i32::from(rows), &mut err);
         }
 
-        check_err(err).map_or(Ok(()), Result::Err)
+        check_err(err)
     }
 
     /// Get the list of processes running in the winpty agent. Returns <= count processes
     ///
     /// `count` must be greater than 0. Larger values cause a larger allocation.
     // TODO: This should return Vec<Handle> instead of Vec<i32>
-    pub fn console_process_list(&mut self, count: usize) -> Result<Vec<i32>, Err> {
+    pub fn console_process_list(&mut self, count: usize) -> Result<Vec<i32>, Error> {
         assert!(count > 0);
 
         let mut err = null_mut() as *mut winpty_error_t;
@@ -204,7 +224,9 @@ impl Winpty {
             process_list.set_len(len);
         }
 
-        check_err(err).map_or(Ok(process_list), Result::Err)
+        check_err(err)?;
+
+        Ok(process_list)
     }
 
     /// Spawns the new process.
@@ -213,26 +235,29 @@ impl Winpty {
     /// before the output data pipe(s) is/are connected, then collected output is
     /// buffered until the pipes are connected, rather than being discarded.
     /// (https://blogs.msdn.microsoft.com/oldnewthing/20110107-00/?p=11803)
-    // TODO: Support getting the process and thread handle of the spawned process (Not the agent)
-    // TODO: Support returning the error from CreateProcess
-    pub fn spawn(&mut self, cfg: &SpawnConfig) -> Result<(), Err> {
+    pub fn spawn(&mut self, cfg: &SpawnConfig) -> Result<ChildHandles, Error> {
+        let mut handles =
+            ChildHandles { process: std::ptr::null_mut(), thread: std::ptr::null_mut() };
+
+        let mut create_process_error: DWORD = 0;
         let mut err = null_mut() as *mut winpty_error_t;
 
         unsafe {
-            let ok = winpty_spawn(
+            winpty_spawn(
                 self.0,
                 cfg.0 as *const winpty_spawn_config_s,
-                null_mut(), // Process handle
-                null_mut(), // Thread handle
-                null_mut(), // Create process error
+                &mut handles.process as *mut _,
+                &mut handles.thread as *mut _,
+                &mut create_process_error as *mut _,
                 &mut err,
             );
-            if ok == 0 {
-                return Ok(());
-            }
         }
 
-        check_err(err).map_or(Ok(()), Result::Err)
+        let mut result = check_err(err);
+        if let Err(Error { code: ErrorCode::SpawnCreateProcessFailed, message }) = &mut result {
+            *message = format!("{} (error code {})", message, create_process_error);
+        }
+        result.map(|_| handles)
     }
 }
 
@@ -260,36 +285,30 @@ impl SpawnConfig {
         cmdline: Option<&str>,
         cwd: Option<&str>,
         end: Option<&str>,
-    ) -> Result<Self, Err> {
+    ) -> Result<Self, Error> {
         let mut err = null_mut() as *mut winpty_error_t;
-        let (appname, cmdline, cwd, end) = (
-            appname.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()),
-            cmdline.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()),
-            cwd.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()),
-            end.map_or(null(), |s| WideCString::from_str(s).unwrap().into_raw()),
-        );
 
+        let to_wstring = |s| WideCString::from_str(s).unwrap();
+        let appname = appname.map(to_wstring);
+        let cmdline = cmdline.map(to_wstring);
+        let cwd = cwd.map(to_wstring);
+        let end = end.map(to_wstring);
+
+        let wstring_ptr = |opt: &Option<WideCString>| opt.as_ref().map_or(null(), |ws| ws.as_ptr());
         let spawn_config = unsafe {
-            winpty_spawn_config_new(spawnflags.bits(), appname, cmdline, cwd, end, &mut err)
+            winpty_spawn_config_new(
+                spawnflags.bits(),
+                wstring_ptr(&appname),
+                wstring_ptr(&cmdline),
+                wstring_ptr(&cwd),
+                wstring_ptr(&end),
+                &mut err,
+            )
         };
 
-        // Required to free the strings
-        unsafe {
-            if !appname.is_null() {
-                WideCString::from_raw(appname as *mut u16);
-            }
-            if !cmdline.is_null() {
-                WideCString::from_raw(cmdline as *mut u16);
-            }
-            if !cwd.is_null() {
-                WideCString::from_raw(cwd as *mut u16);
-            }
-            if !end.is_null() {
-                WideCString::from_raw(end as *mut u16);
-            }
-        }
+        check_err(err)?;
 
-        check_err(err).map_or(Ok(Self(spawn_config)), Result::Err)
+        Ok(Self(spawn_config))
     }
 }