Review

Author: timon-schelling

desktop/src/app.rs

@@ -316,7 +316,7 @@ impl App {
 			}
 			DesktopFrontendMessage::PersistenceLoadDocuments => {
 				// Open all documents in persisted tab order, then select the current one
-				for (id, document) in self.persistent_data.all_documents() {
+				for (id, document) in self.persistent_data.documents() {
 					responses.push(DesktopWrapperMessage::LoadDocument {
 						id,
 						document,

desktop/src/cef.rs

@@ -27,7 +27,6 @@ use crate::wrapper::{WgpuContext, deserialize_editor_message};
 
 mod consts;
 mod context;
-mod dirs;
 mod input;
 mod internal;
 mod ipc;

desktop/src/cef/context/builder.rs

@@ -4,15 +4,15 @@ use cef::{
 	App, BrowserSettings, CefString, Client, DictionaryValue, ImplCommandLine, ImplRequestContext, LogSeverity, RequestContextSettings, SchemeHandlerFactory, Settings, WindowInfo, api_hash,
 	browser_host_create_browser_sync, execute_process,
 };
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
 use super::CefContext;
 use super::singlethreaded::SingleThreadedCefContext;
 use crate::cef::CefEventHandler;
 use crate::cef::consts::{RESOURCE_DOMAIN, RESOURCE_SCHEME};
-use crate::cef::dirs::{create_instance_dir, delete_instance_dirs};
 use crate::cef::input::InputState;
 use crate::cef::internal::{BrowserProcessAppImpl, BrowserProcessClientImpl, RenderProcessAppImpl, SchemeHandlerFactoryImpl};
+use crate::dirs::TempDir;
 
 pub(crate) struct CefContextBuilder<H: CefEventHandler> {
 	pub(crate) args: Args,
@@ -97,8 +97,7 @@ impl<H: CefEventHandler> CefContextBuilder<H> {
 
 	#[cfg(target_os = "macos")]
 	pub(crate) fn initialize(self, event_handler: H, disable_gpu_acceleration: bool) -> Result<impl CefContext, InitError> {
-		delete_instance_dirs();
-		let instance_dir = create_instance_dir();
+		let instance_dir = TempDir::new().expect("Failed to create temporary directory for CEF instance");
 
 		let exe = std::env::current_exe().expect("cannot get current exe path");
 		let app_root = exe.parent().and_then(|p| p.parent()).expect("bad path structure").parent().expect("bad path structure");
@@ -108,7 +107,7 @@ impl<H: CefEventHandler> CefContextBuilder<H> {
 			multi_threaded_message_loop: 0,
 			external_message_pump: 1,
 			no_sandbox: 1, // GPU helper crashes when running with sandbox
-			..Self::common_settings(&instance_dir)
+			..Self::common_settings(instance_dir.as_ref())
 		};
 
 		self.initialize_inner(&event_handler, settings)?;
@@ -118,14 +117,13 @@ impl<H: CefEventHandler> CefContextBuilder<H> {
 
 	#[cfg(not(target_os = "macos"))]
 	pub(crate) fn initialize(self, event_handler: H, disable_gpu_acceleration: bool) -> Result<impl CefContext, InitError> {
-		delete_instance_dirs();
-		let instance_dir = create_instance_dir();
+		let instance_dir = TempDir::new().expect("Failed to create temporary directory for CEF instance");
 
 		let settings = Settings {
 			multi_threaded_message_loop: 1,
 			#[cfg(target_os = "linux")]
 			no_sandbox: 1,
-			..Self::common_settings(&instance_dir)
+			..Self::common_settings(instance_dir.as_ref())
 		};
 
 		self.initialize_inner(&event_handler, settings)?;
@@ -157,7 +155,7 @@ impl<H: CefEventHandler> CefContextBuilder<H> {
 	}
 }
 
-fn create_browser<H: CefEventHandler>(event_handler: H, instance_dir: PathBuf, disable_gpu_acceleration: bool) -> Result<SingleThreadedCefContext, InitError> {
+fn create_browser<H: CefEventHandler>(event_handler: H, instance_dir: TempDir, disable_gpu_acceleration: bool) -> Result<SingleThreadedCefContext, InitError> {
 	let mut client = Client::new(BrowserProcessClientImpl::new(&event_handler));
 
 	#[cfg(feature = "accelerated_paint")]
@@ -211,7 +209,7 @@ fn create_browser<H: CefEventHandler>(event_handler: H, instance_dir: PathBuf, d
 			event_handler: Box::new(event_handler),
 			browser,
 			input_state: InputState::default(),
-			instance_dir,
+			_instance_dir: instance_dir,
 		})
 	} else {
 		tracing::error!("Failed to create browser");

desktop/src/cef/context/multithreaded.rs

@@ -54,7 +54,12 @@ impl CefContext for MultiThreadedCefContextProxy {
 impl Drop for MultiThreadedCefContextProxy {
 	fn drop(&mut self) {
 		// Force dropping underlying context on the UI thread
-		run_on_ui_thread(move || drop(CONTEXT.take()));
+		let (sync_drop_tx, sync_drop_rx) = std::sync::mpsc::channel();
+		run_on_ui_thread(move || {
+			drop(CONTEXT.take());
+			sync_drop_tx.send(()).unwrap();
+		});
+		sync_drop_rx.recv().unwrap();
 	}
 }
 

desktop/src/cef/context/singlethreaded.rs

@@ -4,14 +4,15 @@ use winit::event::WindowEvent;
 use crate::cef::input::InputState;
 use crate::cef::ipc::{MessageType, SendMessage};
 use crate::cef::{CefEventHandler, input};
+use crate::dirs::TempDir;
 
 use super::CefContext;
 
 pub(super) struct SingleThreadedCefContext {
 	pub(super) event_handler: Box<dyn CefEventHandler>,
 	pub(super) browser: Browser,
 	pub(super) input_state: InputState,
-	pub(super) instance_dir: std::path::PathBuf,
+	pub(super) _instance_dir: TempDir,
 }
 
 impl CefContext for SingleThreadedCefContext {
@@ -46,19 +47,6 @@ impl Drop for SingleThreadedCefContext {
 		// CEF wants us to close the browser before shutting down, otherwise it may run longer that necessary.
 		self.browser.host().unwrap().close_browser(1);
 		cef::shutdown();
-
-		// Sometimes some CEF processes still linger at this point and hold file handles to the cache directory.
-		// To mitigate this, we try to remove the directory multiple times with some delay.
-		// TODO: find a better solution if possible.
-		for _ in 0..30 {
-			match std::fs::remove_dir_all(&self.instance_dir) {
-				Ok(_) => break,
-				Err(e) => {
-					tracing::warn!("Failed to remove CEF cache directory, retrying...: {e}");
-					std::thread::sleep(std::time::Duration::from_millis(100));
-				}
-			}
-		}
 	}
 }
 
@@ -68,7 +56,6 @@ impl SendMessage for SingleThreadedCefContext {
 			tracing::error!("Main frame is not available, cannot send message");
 			return;
 		};
-
 		frame.send_message(message_type, message);
 	}
 }

desktop/src/cef/dirs.rs

@@ -1,11 +1,12 @@
-use std::fs::create_dir_all;
-use std::path::PathBuf;
+use std::fs;
+use std::io;
+use std::path::{Component, Path, PathBuf};
 
 use crate::consts::{APP_DIRECTORY_NAME, APP_DOCUMENTS_DIRECTORY_NAME};
 
 pub(crate) fn ensure_dir_exists(path: &PathBuf) {
 	if !path.exists() {
-		create_dir_all(path).unwrap_or_else(|_| panic!("Failed to create directory at {path:?}"));
+		fs::create_dir_all(path).unwrap_or_else(|_| panic!("Failed to create directory at {path:?}"));
 	}
 }
 
@@ -20,3 +21,37 @@ pub(crate) fn app_autosave_documents_dir() -> PathBuf {
 	ensure_dir_exists(&path);
 	path
 }
+
+/// Temporary directory that is automatically deleted when dropped.
+pub struct TempDir {
+	path: PathBuf,
+}
+
+impl TempDir {
+	pub fn new() -> io::Result<Self> {
+		Self::new_with_parent(dirs::cache_dir().unwrap_or_else(std::env::temp_dir))
+	}
+
+	pub fn new_with_parent(parent: impl AsRef<Path>) -> io::Result<Self> {
+		let random_suffix: String = (0..32).map(|_| format!("{:x}", rand::random::<u8>() % 16)).collect();
+		let name = format!("{}_{}_{}", APP_DIRECTORY_NAME, std::process::id(), random_suffix);
+		let path = parent.as_ref().join(name);
+		fs::create_dir_all(&path)?;
+		Ok(Self { path })
+	}
+}
+
+impl Drop for TempDir {
+	fn drop(&mut self) {
+		let result = fs::remove_dir_all(&self.path);
+		if let Err(e) = result {
+			tracing::error!("Failed to remove temporary directory at {:?}: {}", self.path, e);
+		}
+	}
+}
+
+impl AsRef<Path> for TempDir {
+	fn as_ref(&self) -> &Path {
+		&self.path
+	}
+}

desktop/src/dirs.rs

@@ -29,9 +29,6 @@ impl PersistentData {
 			tracing::error!("Failed to write document {id:?} to disk: {e}");
 		}
 
-		if let Some(order) = &self.document_order {
-			self.force_order(&order.clone());
-		}
 		self.flush();
 	}
 
@@ -55,7 +52,7 @@ impl PersistentData {
 		}
 	}
 
-	pub(crate) fn all_documents(&self) -> Vec<(DocumentId, Document)> {
+	pub(crate) fn documents(&self) -> Vec<(DocumentId, Document)> {
 		self.documents.iter().filter_map(|doc| Some((doc.id, self.read_document(&doc.id)?))).collect()
 	}
 
@@ -65,7 +62,16 @@ impl PersistentData {
 	}
 
 	pub(crate) fn force_document_order(&mut self, order: Vec<DocumentId>) {
-		self.force_order(&order);
+		let mut ordered_prefix_length = 0;
+		for id in &order {
+			if let Some(offset) = self.documents[ordered_prefix_length..].iter().position(|doc| doc.id == *id) {
+				let found_index = ordered_prefix_length + offset;
+				if found_index != ordered_prefix_length {
+					self.documents[ordered_prefix_length..=found_index].rotate_right(1);
+				}
+				ordered_prefix_length += 1;
+			}
+		}
 		self.document_order = Some(order);
 		self.flush();
 	}
@@ -82,20 +88,6 @@ impl PersistentData {
 		})
 	}
 
-	// Reorders the documents array to match a desired ordering, keeping unmentioned documents at the end
-	fn force_order(&mut self, desired_order: &[DocumentId]) {
-		let mut ordered_prefix_length = 0;
-		for id in desired_order {
-			if let Some(offset) = self.documents[ordered_prefix_length..].iter().position(|doc| doc.id == *id) {
-				let found_index = ordered_prefix_length + offset;
-				if found_index != ordered_prefix_length {
-					self.documents[ordered_prefix_length..=found_index].rotate_right(1);
-				}
-				ordered_prefix_length += 1;
-			}
-		}
-	}
-
 	fn flush(&self) {
 		let data = match ron::ser::to_string_pretty(self, Default::default()) {
 			Ok(d) => d,
@@ -132,7 +124,6 @@ impl PersistentData {
 		*self = loaded;
 
 		self.garbage_collect_document_files();
-		Self::delete_old_cef_browser_directory();
 	}
 
 	// Remove orphaned document content files that have no corresponding entry in the persisted state
@@ -159,14 +150,6 @@ impl PersistentData {
 		}
 	}
 
-	// TODO: Eventually remove this cleanup code for the old "browser" CEF directory (renamed to "cef")
-	fn delete_old_cef_browser_directory() {
-		let old_browser_dir = crate::dirs::app_data_dir().join("browser");
-		if old_browser_dir.is_dir() {
-			let _ = std::fs::remove_dir_all(&old_browser_dir);
-		}
-	}
-
 	fn state_file_path() -> std::path::PathBuf {
 		let mut path = crate::dirs::app_data_dir();
 		path.push(crate::consts::APP_STATE_FILE_NAME);