From eb655653f26b9ee142f97da63fc3efcee9e3da33 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:59:14 +0000 Subject: [PATCH] Move Windows dialogs and credential I/O off the foreground thread (#64250) ## Summary Move Windows file-open, file-save, and native message dialogs off the foreground executor so time spent waiting for a user does not appear as a long foreground task poll. Move Windows Credential Manager reads, writes, and deletes to the background executor as well. The caller-facing receiver/task APIs remain unchanged. No hang-journal suppression or new public GPUI task-spawn API is introduced. ## Implementation - Run synchronous native dialogs on dedicated threads with COM STA initialization and cleanup on the same thread. - Keep the native owner relationship when a window exists; support ownerless file/save dialogs by passing a null owner HWND. - Serialize dialogs per owner using one mutex, also used to wait for active native work before destroying the owner HWND. Closing the GPUI window sets a shared cancellation flag and hides the native window; queued requests see that flag and cannot launch. No pending counter, lifetime lease, cancellation registry, or idle-notification channels are needed. - Use a worker-thread timer to request native cancellation when the caller drops its receiver or the owner closes. - Preserve message prompts without a Cancel option by vetoing user cancellation unless an internal cancellation was requested. - Distinguish actual file-dialog failures from user cancellation. Sleep/wake and frame-visibility telemetry are intentionally deferred. ## Validation - `corgi fmt -p gpui_windows -- --check` and scoped `git diff --check` passed. - Minimal scratch Windows-target harnesses type-checked the dialog module and native API wiring against real Windows 0.62 bindings. These are not full integration builds. - Six extracted native-independent state/policy tests passed, covering owner shutdown, queued-request cancellation state, per-request cancellation isolation, per-owner serialization, ownerless cancellation, and 12 message-prompt cancellation-policy combinations. These do not execute native dialogs or the complete coordinator/worker handoff. - Before the shutdown simplification, Cameron built and launched Zed successfully on Windows using a shorter `CARGO_TARGET_DIR`, and reported running the manual prompt checks. The captured output contains no foreground/action hang or saved task-trace messages, but exact prompt-by-prompt coverage and timing have not been established. - The committed Windows test suite has not yet been run natively. ## Open review items This PR remains a draft. The dialog helper has been simplified to approximately 245 production lines, retaining native ownership and using the existing per-owner mutex for shutdown instead of separate lifetime bookkeeping. Independent static review found no concrete lifetime regression. Native cancellation still uses the existing worker-thread timer/window-enumeration mechanism; Windows runtime validation of the revised shutdown path is outstanding. Before merging, verify ownerless prompts, focus/Alt+Tab behavior, overlapping requests for the same owner, cancellation while queued/shown, owner closure, and nested overwrite confirmations on Windows. Absence of hang logs alone does not establish those behaviors. The required review-confirmation marker in README.md must be removed manually by the human author after review. Release Notes: - N/A - [GPUI] Move window dialogs and credential I/O off foreground thread --- crates/gpui_windows/src/dialog.rs | 340 ++++++++++++++++++++++++ crates/gpui_windows/src/gpui_windows.rs | 1 + crates/gpui_windows/src/platform.rs | 54 ++-- crates/gpui_windows/src/window.rs | 48 ++-- 4 files changed, 401 insertions(+), 42 deletions(-) create mode 100644 crates/gpui_windows/src/dialog.rs diff --git a/crates/gpui_windows/src/dialog.rs b/crates/gpui_windows/src/dialog.rs new file mode 100644 index 0000000000..16866cfc35 --- /dev/null +++ b/crates/gpui_windows/src/dialog.rs @@ -0,0 +1,340 @@ +use std::{ + cell::RefCell, + rc::Rc, + sync::{ + Arc, + atomic::{AtomicBool, Ordering}, + }, +}; + +use anyhow::{Context as _, Result, anyhow}; +use futures::{ + channel::oneshot, + future::{Either, select}, + lock::Mutex, +}; +use gpui::ForegroundExecutor; +use gpui_util::ResultExt; +use windows::{ + Win32::{ + Foundation::*, + System::{Com::*, Threading::GetCurrentThreadId}, + UI::{ + Controls::{TASKDIALOG_NOTIFICATIONS, TDN_BUTTON_CLICKED}, + WindowsAndMessaging::*, + }, + }, + core::{BOOL, HRESULT, w}, +}; + +use crate::SafeHwnd; + +pub(crate) struct DialogOwner { + hwnd: HWND, + closed: Arc, + serialization: Mutex<()>, +} + +impl DialogOwner { + pub(crate) fn new(hwnd: HWND) -> Rc { + Rc::new(Self { + hwnd, + closed: Arc::new(AtomicBool::new(false)), + serialization: Mutex::new(()), + }) + } + + pub(crate) fn close(&self) { + self.closed.store(true, Ordering::Release); + } + + pub(crate) async fn when_idle(&self) { + // After close(), queued requests cannot start a native dialog. Only the + // active dialog can still use the HWND, and it holds this lock until exit. + let _guard = self.serialization.lock().await; + } +} + +#[derive(Clone, Default)] +struct Cancellation { + requested: Arc, + owner_closed: Option>, +} + +impl Cancellation { + fn new(owner: Option<&DialogOwner>) -> Self { + Self { + owner_closed: owner.map(|owner| owner.closed.clone()), + ..Self::default() + } + } + + fn is_requested(&self) -> bool { + self.requested.load(Ordering::Acquire) + || self + .owner_closed + .as_ref() + .is_some_and(|closed| closed.load(Ordering::Acquire)) + } +} + +/// Native modal loops may synchronously message their owner, so the foreground +/// thread must keep pumping messages rather than waiting for the dialog thread. +/// Holding the owner's lock through native completion also allows window +/// destruction to wait for the dialog without blocking the foreground thread. +pub(crate) fn show_dialog( + owner: Option>, + executor: &ForegroundExecutor, + dialog: impl FnOnce(HWND) -> Result + Send + 'static, +) -> oneshot::Receiver> { + let (mut sender, receiver) = oneshot::channel(); + let cancellation = Cancellation::new(owner.as_deref()); + if cancellation.is_requested() { + sender.send(Err(anyhow!("dialog owner has closed"))).ok(); + return receiver; + } + let hwnd = owner.as_ref().map(|owner| SafeHwnd::from(owner.hwnd)); + executor + .spawn(async move { + // Let Windows manage enabling and activating the owner. Overlapping + // modal loops could otherwise re-enable it while another dialog is open. + let serialization = if let Some(owner) = owner.as_ref() { + match select(owner.serialization.lock(), sender.cancellation()).await { + Either::Left((guard, _)) => Some(guard), + Either::Right(((), _)) => return, + } + } else { + None + }; + if sender.is_canceled() || cancellation.is_requested() { + return; + } + let (result_sender, mut result_receiver) = oneshot::channel(); + let thread = std::thread::Builder::new() + .name("windows-native-dialog".into()) + .spawn({ + let cancellation = cancellation.clone(); + move || { + let result = (|| { + let _apartment = Apartment::new()?; + let _timer = CancellationTimer::new(cancellation.clone())?; + if cancellation.is_requested() { + return Err(anyhow!("dialog cancelled")); + } + let result = dialog(hwnd.map(|hwnd| hwnd.as_raw()).unwrap_or_default()); + if cancellation.is_requested() { + return Err(anyhow!("dialog cancelled")); + } + result + })(); + result_sender.send(result).ok(); + } + }); + if let Err(error) = thread { + drop(serialization); + sender.send(Err(error.into())).ok(); + return; + } + let result = match select(&mut result_receiver, sender.cancellation()).await { + Either::Left((result, _)) => result, + Either::Right(((), _)) => { + cancellation.requested.store(true, Ordering::Release); + result_receiver.await + } + }; + // Neither launch another dialog nor destroy the owner until the + // native loop exits, even if the caller no longer wants its result. + drop(serialization); + sender + .send(result.unwrap_or_else(|error| Err(error.into()))) + .ok(); + }) + .detach(); + receiver +} + +struct Apartment; + +impl Apartment { + fn new() -> Result { + unsafe { CoInitializeEx(None, COINIT_APARTMENTTHREADED).ok()? }; + Ok(Self) + } +} + +impl Drop for Apartment { + fn drop(&mut self) { + unsafe { CoUninitialize() }; + } +} + +thread_local! { + static CANCELLATION: RefCell> = const { RefCell::new(None) }; +} + +struct CancellationTimer(usize); + +impl CancellationTimer { + fn new(cancellation: Cancellation) -> Result { + let timer = unsafe { SetTimer(None, 0, 100, Some(cancel_dialog)) }; + if timer == 0 { + return Err(windows::core::Error::from_thread()) + .context("starting native dialog cancellation timer"); + } + CANCELLATION.with(|slot| slot.replace(Some(cancellation))); + Ok(Self(timer)) + } +} + +impl Drop for CancellationTimer { + fn drop(&mut self) { + CANCELLATION.with(|slot| slot.take()); + unsafe { KillTimer(None, self.0).log_err() }; + } +} + +unsafe extern "system" fn cancel_dialog(_: HWND, _: u32, _: usize, _: u32) { + if cancellation_requested() { + unsafe { + EnumThreadWindows(GetCurrentThreadId(), Some(close_dialog), LPARAM(0)) + .ok() + .log_err(); + }; + } +} + +fn cancellation_requested() -> bool { + CANCELLATION.with(|slot| { + slot.borrow() + .as_ref() + .is_some_and(Cancellation::is_requested) + }) +} + +pub(crate) unsafe extern "system" fn task_dialog_callback( + _: HWND, + notification: TASKDIALOG_NOTIFICATIONS, + button: WPARAM, + _: LPARAM, + has_cancel_button: isize, +) -> HRESULT { + // WM_CLOSE requires TDF_ALLOW_DIALOG_CANCELLATION, which also enables Esc/X. + // Only allow those user gestures when the prompt offered a Cancel button. + if notification == TDN_BUTTON_CLICKED + && button.0 == IDCANCEL.0 as usize + && has_cancel_button == 0 + && !cancellation_requested() + { + S_FALSE + } else { + S_OK + } +} + +unsafe extern "system" fn close_dialog(hwnd: HWND, _: LPARAM) -> BOOL { + // Shell dialogs can open further modal dialogs. Close only dialog windows + // on this dedicated thread, not hidden COM infrastructure windows. + let mut class_name = [0u16; 32]; + let length = unsafe { GetClassNameW(hwnd, &mut class_name) } as usize; + let dialog_class = w!("#32770"); + if class_name.get(..length) == Some(unsafe { dialog_class.as_wide() }) { + unsafe { PostMessageW(Some(hwnd), WM_CLOSE, WPARAM(0), LPARAM(0)).log_err() }; + } + TRUE +} + +#[cfg(test)] +mod tests { + use super::*; + use futures::FutureExt as _; + + #[test] + fn task_dialog_cancellation_requires_cancel_button_or_internal_request() { + let cancellation = Cancellation::default(); + CANCELLATION.with(|slot| slot.replace(Some(cancellation.clone()))); + for internally_cancelled in [false, true] { + cancellation + .requested + .store(internally_cancelled, Ordering::Release); + for has_cancel_button in [false, true] { + for button in [IDCANCEL.0, IDOK.0, -1] { + let result = unsafe { + task_dialog_callback( + HWND::default(), + TDN_BUTTON_CLICKED, + WPARAM(button as usize), + LPARAM(0), + has_cancel_button as isize, + ) + }; + let should_veto = + button == IDCANCEL.0 && !has_cancel_button && !internally_cancelled; + assert_eq!(result, if should_veto { S_FALSE } else { S_OK }); + } + } + } + CANCELLATION.with(|slot| slot.take()); + } + + #[test] + fn closing_owner_cancels_every_outstanding_dialog() { + let owner = DialogOwner::new(HWND::default()); + let first = Cancellation::new(Some(&owner)); + let second = Cancellation::new(Some(&owner)); + assert!(!first.is_requested()); + assert!(!second.is_requested()); + + owner.close(); + owner.close(); + + assert!(first.is_requested()); + assert!(second.is_requested()); + assert!(Cancellation::new(Some(&owner)).is_requested()); + } + + #[test] + fn closed_owner_waits_for_active_dialog_but_queued_requests_stay_cancelled() { + let owner = DialogOwner::new(HWND::default()); + let active = owner.serialization.try_lock().expect("first dialog"); + let queued = Cancellation::new(Some(&owner)); + + owner.close(); + assert!(owner.when_idle().now_or_never().is_none()); + drop(active); + assert!(owner.when_idle().now_or_never().is_some()); + + let _guard = owner.serialization.try_lock().expect("queued request"); + assert!(queued.is_requested()); + } + + #[test] + fn cancelling_one_request_does_not_cancel_its_owner_or_other_requests() { + let owner = DialogOwner::new(HWND::default()); + let first = Cancellation::new(Some(&owner)); + let second = Cancellation::new(Some(&owner)); + first.requested.store(true, Ordering::Release); + assert!(first.is_requested()); + assert!(!second.is_requested()); + assert!(!owner.closed.load(Ordering::Acquire)); + } + + #[test] + fn dialogs_serialize_per_owner() { + let owner = DialogOwner::new(HWND::default()); + let other_owner = DialogOwner::new(HWND::default()); + let guard = owner.serialization.try_lock().expect("first dialog"); + + assert!(owner.serialization.try_lock().is_none()); + assert!(other_owner.serialization.try_lock().is_some()); + drop(guard); + assert!(owner.serialization.try_lock().is_some()); + } + + #[test] + fn ownerless_request_can_be_cancelled_without_a_window() { + let cancellation = Cancellation::new(None); + assert!(!cancellation.is_requested()); + cancellation.requested.store(true, Ordering::Release); + assert!(cancellation.is_requested()); + } +} diff --git a/crates/gpui_windows/src/gpui_windows.rs b/crates/gpui_windows/src/gpui_windows.rs index 692d0dbbfd..e7a45a034d 100644 --- a/crates/gpui_windows/src/gpui_windows.rs +++ b/crates/gpui_windows/src/gpui_windows.rs @@ -2,6 +2,7 @@ mod clipboard; mod destination_list; +mod dialog; mod direct_manipulation; mod direct_write; mod directx_atlas; diff --git a/crates/gpui_windows/src/platform.rs b/crates/gpui_windows/src/platform.rs index ea43485970..d2677f6f57 100644 --- a/crates/gpui_windows/src/platform.rs +++ b/crates/gpui_windows/src/platform.rs @@ -674,15 +674,13 @@ impl Platform for WindowsPlatform { &self, options: PathPromptOptions, ) -> Receiver>>> { - let (tx, rx) = oneshot::channel(); - let window = self.find_current_active_window(); - self.foreground_executor() - .spawn(async move { - let _ = tx.send(file_open_dialog(options, window)); - }) - .detach(); - - rx + let owner = self + .find_current_active_window() + .and_then(|hwnd| self.window_from_hwnd(hwnd)) + .map(|window| window.dialog_owner.clone()); + crate::dialog::show_dialog(owner, &self.foreground_executor, move |window| { + file_open_dialog(options, Some(window)) + }) } fn prompt_for_new_path( @@ -692,15 +690,13 @@ impl Platform for WindowsPlatform { ) -> Receiver>> { let directory = directory.to_owned(); let suggested_name = suggested_name.map(|s| s.to_owned()); - let (tx, rx) = oneshot::channel(); - let window = self.find_current_active_window(); - self.foreground_executor() - .spawn(async move { - let _ = tx.send(file_save_dialog(directory, suggested_name, window)); - }) - .detach(); - - rx + let owner = self + .find_current_active_window() + .and_then(|hwnd| self.window_from_hwnd(hwnd)) + .map(|window| window.dialog_owner.clone()); + crate::dialog::show_dialog(owner, &self.foreground_executor, move |window| { + file_save_dialog(directory, suggested_name, Some(window)) + }) } fn can_select_mixed_files_and_dirs(&self) -> bool { @@ -918,7 +914,7 @@ impl Platform for WindowsPlatform { .encode_utf16() .chain(Some(0)) .collect_vec(); - self.foreground_executor().spawn(async move { + self.background_executor().spawn(async move { let credentials = CREDENTIALW { LastWritten: unsafe { GetSystemTimeAsFileTime() }, Flags: CRED_FLAGS(0), @@ -947,7 +943,7 @@ impl Platform for WindowsPlatform { .encode_utf16() .chain(Some(0)) .collect_vec(); - self.foreground_executor().spawn(async move { + self.background_executor().spawn(async move { let mut credentials: *mut CREDENTIALW = std::ptr::null_mut(); let result = unsafe { CredReadW( @@ -989,7 +985,7 @@ impl Platform for WindowsPlatform { .encode_utf16() .chain(Some(0)) .collect_vec(); - self.foreground_executor().spawn(async move { + self.background_executor().spawn(async move { unsafe { CredDeleteW( PCWSTR::from_raw(target_name.as_ptr()), @@ -1398,9 +1394,11 @@ fn file_open_dialog( folder_dialog.SetOkButtonLabel(&HSTRING::from(prompt))?; } - if folder_dialog.Show(window).is_err() { - // User cancelled - return Ok(None); + if let Err(error) = folder_dialog.Show(window) { + if error.code() == HRESULT::from_win32(ERROR_CANCELLED.0) { + return Ok(None); + } + return Err(error.into()); } } @@ -1458,9 +1456,11 @@ fn file_save_dialog( pszName: windows::core::w!("All files"), pszSpec: windows::core::w!("*.*"), }])?; - if dialog.Show(window).is_err() { - // User cancelled - return Ok(None); + if let Err(error) = dialog.Show(window) { + if error.code() == HRESULT::from_win32(ERROR_CANCELLED.0) { + return Ok(None); + } + return Err(error.into()); } } let shell_item = unsafe { dialog.GetResult()? }; diff --git a/crates/gpui_windows/src/window.rs b/crates/gpui_windows/src/window.rs index 9988e44f6e..d6b02d78b4 100644 --- a/crates/gpui_windows/src/window.rs +++ b/crates/gpui_windows/src/window.rs @@ -93,6 +93,7 @@ pub struct WindowsWindowState { pub(crate) struct WindowsWindowInner { hwnd: HWND, + pub(crate) dialog_owner: Rc, drop_target_helper: IDropTargetHelper, pub(crate) state: WindowsWindowState, system_settings: WindowsSystemSettings, @@ -281,6 +282,7 @@ impl WindowsWindowInner { Ok(Rc::new(Self { hwnd, + dialog_owner: crate::dialog::DialogOwner::new(hwnd), drop_target_helper: context.drop_target_helper.clone(), state, handle: context.handle, @@ -605,6 +607,8 @@ impl rwh::HasDisplayHandle for WindowsWindow { impl Drop for WindowsWindow { fn drop(&mut self) { + self.0.dialog_owner.close(); + unsafe { ShowWindowAsync(self.0.hwnd, SW_HIDE).ok().log_err() }; // `DestroyWindow` below sends `WM_SHOWWINDOW`; without a callback the // resulting visibility report has nothing to notify. self.0.state.callbacks.visibility_change.take(); @@ -613,6 +617,7 @@ impl Drop for WindowsWindow { self.0 .executor .spawn(async move { + this.dialog_owner.when_idle().await; let handle = this.hwnd; unsafe { RevokeDragDrop(handle).log_err(); @@ -717,18 +722,19 @@ impl PlatformWindow for WindowsWindow { detail: Option<&str>, answers: &[PromptButton], ) -> Option> { - let (done_tx, done_rx) = oneshot::channel(); + let (mut done_tx, done_rx) = oneshot::channel(); let msg = msg.to_string(); let detail_string = detail.map(|detail| detail.to_string()); - let handle = self.0.hwnd; let answers = answers.to_vec(); - self.0 - .executor - .spawn(async move { + let dialog = crate::dialog::show_dialog( + Some(self.0.dialog_owner.clone()), + &self.0.executor, + move |handle| { unsafe { let mut config = TASKDIALOGCONFIG::default(); config.cbSize = std::mem::size_of::() as _; config.hwndParent = handle; + config.dwFlags = TDF_ALLOW_DIALOG_CANCELLATION; let title; let main_icon; match level { @@ -776,17 +782,26 @@ impl PlatformWindow for WindowsWindow { config.cButtons = buttons.len() as _; config.pButtons = buttons.as_ptr(); - config.pfCallback = None; + config.pfCallback = Some(crate::dialog::task_dialog_callback); + config.lpCallbackData = button_id_map.contains(&IDCANCEL.0) as isize; let mut res = std::mem::zeroed(); - let _ = TaskDialogIndirect(&config, Some(&mut res), None, None) - .context("unable to create task dialog") - .log_err(); - - if let Some(clicked) = - button_id_map.iter().position(|&button_id| button_id == res) - { - let _ = done_tx.send(clicked); - } + TaskDialogIndirect(&config, Some(&mut res), None, None) + .context("unable to create task dialog")?; + Ok(button_id_map.iter().position(|&button_id| button_id == res)) + } + }, + ); + self.0 + .executor + .spawn(async move { + if let futures::future::Either::Left((result, _)) = + futures::future::select(dialog, done_tx.cancellation()).await + && let Some(Some(clicked)) = result + .context("native dialog thread stopped") + .and_then(|result| result) + .log_err() + { + done_tx.send(clicked).ok(); } }) .detach(); @@ -1447,6 +1462,9 @@ unsafe extern "system" fn window_procedure( } let inner = unsafe { &*ptr }; let result = if let Some(inner) = inner.upgrade() { + if msg == WM_NCDESTROY { + inner.dialog_owner.close(); + } inner.handle_msg(hwnd, msg, wparam, lparam) } else { unsafe { DefWindowProcW(hwnd, msg, wparam, lparam) }