fix(backtrace): Fix backtrace logging and stdout (#988)

* fix(backtrace): Fix backtrace logging and stdout
- Add manual implementation for panic that logs backtrace to a file.
- Remove all manual output to stdout.
- Fix new clippy warnings from Rust 1.65.

* Update docs

Co-authored-by: Henrik Friedrichsen <henrik@affekt.org>

.github/ISSUE_TEMPLATE/bug_report.md

@@ -30,9 +30,14 @@ If applicable, add screenshots to help explain your problem.
  - Installed from: [e.g. AUR, brew, cargo]
 
 **Backtrace/Debug log**
-Instructions on how to capture debug logs: https://github.com/hrkfdn/ncspot#usage
+Please attach a debug log and backtrace if ncspot has crashed.
 
-To debug crashes a backtrace is very helpful. Make sure you run a debug build of ncspot, e.g. by running the command mentioned in the link above.
+Instructions on how to capture debug logs: https://github.com/hrkfdn/ncspot#debugging
+
+For backtraces, make sure you run a debug build of ncspot, e.g. by running the
+command mentioned in the [compilation
+instructions](https://github.com/hrkfdn/ncspot#compiling).  You can find the
+latest backtrace at `~/.cache/ncspot/backtrace.log`.
 
 **Additional context**
 Add any other context about the problem here.

README.md

@@ -37,6 +37,7 @@ You **must** have an existing premium Spotify subscription to use `ncspot`.
     - [On Linux](#on-linux)
   - [Build](#build)
     - [Prerequisites](#prerequisites)
+    - [Debugging](#debugging)
     - [Compiling](#compiling)
       - [Building a Debian Package](#building-a-debian-package)
     - [Audio Backends](#audio-backends)
@@ -126,6 +127,16 @@ On Linux, you also need:
     sudo pacman -S dbus libpulse libxcb ncurses openssl
     ```
 
+### Debugging
+
+For debugging, you can pass a debug log filename:
+
+```sh
+RUST_BACKTRACE=full cargo run -- -d debug.log
+```
+
+If ncspot has crashed you can find the latest backtrace at `~/.cache/ncspot/backtrace.log`.
+
 ### Compiling
 
 Compile and install the latest release with `cargo-install`:
@@ -144,12 +155,6 @@ cargo build --release
 **You may need to manually set the audio backend on non-Linux OSes.** See
 [Audio Backends](#audio-backends).
 
-For debugging, you can pass a debug log filename and pipe `stderr` to a file:
-
-```sh
-RUST_BACKTRACE=full cargo run -- -d debug.log 2> stderr.log
-```
-
 #### Building a Debian Package
 
 You can also use `cargo-deb` to build a Debian package

src/config.rs

@@ -242,14 +242,24 @@ fn load(filename: &str) -> Result<ConfigValues, String> {
 }
 
 fn proj_dirs() -> AppDirs {
-    match *BASE_PATH.read().expect("can't readlock BASE_PATH") {
-        Some(ref basepath) => AppDirs {
+    try_proj_dirs().unwrap()
+}
+
+/// Returns the plaform app directories for ncspot if they could be determined,
+/// or an error otherwise.
+pub fn try_proj_dirs() -> Result<AppDirs, String> {
+    match *BASE_PATH
+        .read()
+        .map_err(|_| String::from("Poisoned RWLock"))?
+    {
+        Some(ref basepath) => Ok(AppDirs {
             cache_dir: basepath.join(".cache"),
             config_dir: basepath.join(".config"),
             data_dir: basepath.join(".local/share"),
             state_dir: basepath.join(".local/state"),
-        },
-        None => AppDirs::new(Some("ncspot"), true).expect("can't determine project paths"),
+        }),
+        None => AppDirs::new(Some("ncspot"), true)
+            .ok_or_else(|| String::from("Couldn't determine platform standard directories")),
     }
 }
 

src/main.rs

@@ -5,7 +5,9 @@ extern crate lazy_static;
 #[macro_use]
 extern crate serde;
 
-use std::fs;
+use std::backtrace;
+use std::fs::{self, File};
+use std::io::Write;
 use std::path::PathBuf;
 use std::str::FromStr;
 use std::sync::Arc;
@@ -91,6 +93,25 @@ fn credentials_prompt(error_message: Option<String>) -> Result<Credentials, Stri
     authentication::create_credentials()
 }
 
+#[inline]
+fn register_backtrace_panic_handler() {
+    // During most of the program, Cursive is responsible for drawing to the
+    // tty. Since stdout probably doesn't work as expected during a panic, the
+    // backtrace is written to a file at $USER_CACHE_DIR/ncspot/backtrace.log.
+    std::panic::set_hook(Box::new(|_panic_info| {
+        // A panic hook will prevent the default panic handler from being
+        // called. An unwrap in this part would cause a hard crash of ncspot.
+        // Don't unwrap/expect/panic in here!
+        if let Ok(backtrace_log) = config::try_proj_dirs() {
+            let mut path = backtrace_log.cache_dir;
+            path.push("backtrace.log");
+            if let Ok(mut file) = File::create(path) {
+                writeln!(file, "{}", backtrace::Backtrace::force_capture()).unwrap_or_default();
+            }
+        }
+    }));
+}
+
 type UserData = Arc<UserDataInner>;
 struct UserDataInner {
     pub cmd: CommandManager,
@@ -98,6 +119,8 @@ struct UserDataInner {
 
 #[tokio::main]
 async fn main() -> Result<(), String> {
+    register_backtrace_panic_handler();
+
     let backends = {
         let backends: Vec<&str> = audio_backend::BACKENDS.iter().map(|b| b.0).collect();
         format!("Audio backends: {}", backends.join(", "))
@@ -168,6 +191,7 @@ async fn main() -> Result<(), String> {
         credentials = credentials_prompt(Some(error_msg))?;
     }
 
+    // DON'T USE STDOUT AFTER THIS CALL!
     let backend = cursive::backends::try_default().map_err(|e| e.to_string())?;
     let buffered_backend = Box::new(cursive_buffered_backend::BufferedBackend::new(backend));
 
@@ -179,7 +203,6 @@ async fn main() -> Result<(), String> {
 
     let event_manager = EventManager::new(cursive.cb_sink().clone());
 
-    println!("Connecting to Spotify..");
     let spotify = spotify::Spotify::new(event_manager.clone(), credentials, cfg.clone());
 
     let library = Arc::new(Library::new(&event_manager, spotify.clone(), cfg.clone()));
@@ -317,7 +340,7 @@ async fn main() -> Result<(), String> {
     cursive.add_fullscreen_layer(layout.with_name("main"));
 
     #[cfg(unix)]
-    let mut signals = Signals::new(&[SIGTERM, SIGHUP]).expect("could not register signal handler");
+    let mut signals = Signals::new([SIGTERM, SIGHUP]).expect("could not register signal handler");
 
     // cursive event loop
     while cursive.is_running() {

src/serialization.rs

@@ -18,13 +18,13 @@ pub trait Serializer {
         // Nothing exists so just write the default and return it
         if !path.exists() {
             let value = default()?;
-            return self.write(&path, value);
+            return self.write(path, value);
         }
 
-        let result = self.load(&path);
+        let result = self.load(path);
         if default_on_parse_failure && result.is_err() {
             let value = default()?;
-            return self.write(&path, value);
+            return self.write(path, value);
         }
         result.map_err(|e| format!("Unable to parse {}: {}", path.to_string_lossy(), e))
     }

src/ui/layout.rs

@@ -204,7 +204,7 @@ impl View for Layout {
         let result = self.get_result();
 
         let cmdline_visible = self.cmdline.get_content().len() > 0;
-        let mut cmdline_height = if cmdline_visible { 1 } else { 0 };
+        let mut cmdline_height = usize::from(cmdline_visible);
         if result.as_ref().map(Option::is_some).unwrap_or(true) {
             cmdline_height += 1;
         }
@@ -318,7 +318,7 @@ impl View for Layout {
             let result = self.get_result();
 
             let cmdline_visible = self.cmdline.get_content().len() > 0;
-            let mut cmdline_height = if cmdline_visible { 1 } else { 0 };
+            let mut cmdline_height = usize::from(cmdline_visible);
             if result.as_ref().map(Option::is_some).unwrap_or(true) {
                 cmdline_height += 1;
             }