From 413f499da8c218cd6b6544f2516db4c3ae9b2eea Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 27 Nov 2019 20:32:52 +0100 Subject: [PATCH 1/3] Avoid allocation in the signal handler to fix a deadlock Fixes https://github.com/servo/servo/issues/24881 CC https://github.com/rust-lang/backtrace-rs/pull/265 --- ports/glutin/backtrace.rs | 98 +++++++++++++++++++++++++++++++++++++++ ports/glutin/main2.rs | 17 +++---- 2 files changed, 107 insertions(+), 8 deletions(-) create mode 100644 ports/glutin/backtrace.rs diff --git a/ports/glutin/backtrace.rs b/ports/glutin/backtrace.rs new file mode 100644 index 00000000000..0c089336557 --- /dev/null +++ b/ports/glutin/backtrace.rs @@ -0,0 +1,98 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +//! Similar to `println!("{:?}", Backtrace::new())`, but doesn’t allocate. +//! +//! Seems to fix some deadlocks: https://github.com/servo/servo/issues/24881 +//! +//! FIXME: if/when a future version of the `backtrace` crate has +//! https://github.com/rust-lang/backtrace-rs/pull/265, use that instead. + +use std::fmt::{self, Write}; +use backtrace::{BytesOrWideString, PrintFmt}; + +#[inline(never)] +pub(crate) fn print() { + println!("{:?}", Print { + print_fn_address: print as usize, + }) +} + +struct Print { + print_fn_address: usize, +} + +impl fmt::Debug for Print { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + let mut print_fn_frame = 0; + let mut frame_count = 0; + backtrace::trace(|frame| { + let found = frame.symbol_address() as usize == self.print_fn_address; + if found { + print_fn_frame = frame_count; + } + frame_count += 1; + !found + }); + + let mode = PrintFmt::Short; + let mut p = print_path; + let mut f = backtrace::BacktraceFmt::new(fmt, mode, &mut p); + f.add_context()?; + let mut result = Ok(()); + let mut frame_count = 0; + backtrace::trace(|frame| { + let skip = frame_count < print_fn_frame; + frame_count += 1; + if skip { + return true + } + + let mut frame_fmt = f.frame(); + let mut any_symbol = false; + backtrace::resolve_frame(frame, |symbol| { + any_symbol = true; + if let Err(e) = frame_fmt.symbol(frame, symbol) { + result = Err(e) + } + }); + if !any_symbol { + if let Err(e) = frame_fmt.print_raw(frame.ip(), None, None, None) { + result = Err(e) + } + } + result.is_ok() + }); + result?; + f.finish() + } +} + +fn print_path(fmt: &mut fmt::Formatter, path: BytesOrWideString) -> fmt::Result { + match path { + BytesOrWideString::Bytes(mut bytes) => { + loop { + match std::str::from_utf8(bytes) { + Ok(s) => { + fmt.write_str(s)?; + break; + } + Err(err) => { + fmt.write_char(std::char::REPLACEMENT_CHARACTER)?; + match err.error_len() { + Some(len) => bytes = &bytes[err.valid_up_to() + len..], + None => break, + } + } + } + } + } + BytesOrWideString::Wide(wide) => { + for c in std::char::decode_utf16(wide.iter().cloned()) { + fmt.write_char(c.unwrap_or(std::char::REPLACEMENT_CHARACTER))? + } + } + } + Ok(()) +} diff --git a/ports/glutin/main2.rs b/ports/glutin/main2.rs index b8d784db762..e1a848902f8 100644 --- a/ports/glutin/main2.rs +++ b/ports/glutin/main2.rs @@ -11,6 +11,7 @@ extern crate log; extern crate sig; mod app; +mod backtrace; mod browser; mod context; mod embedder; @@ -23,7 +24,6 @@ mod skia_symbols; mod window_trait; use app::App; -use backtrace::Backtrace; use getopts::Options; use servo::config::opts::{self, ArgumentParsingResult}; use servo::config::servo_version; @@ -31,6 +31,7 @@ use servo::servo_config::pref; use std::env; use std::panic; use std::process; +use std::sync::atomic; use std::thread; pub mod platform { @@ -49,17 +50,17 @@ fn install_crash_handler() {} #[cfg(any(target_os = "macos", target_os = "linux"))] fn install_crash_handler() { - use backtrace::Backtrace; use libc::_exit; use sig::ffi::Sig; use std::thread; extern "C" fn handler(sig: i32) { - let name = thread::current() - .name() - .map(|n| format!(" for thread \"{}\"", n)) - .unwrap_or("".to_owned()); - println!("Stack trace{}\n{:?}", name, Backtrace::new()); + print!("Stack trace"); + if let Some(name) = thread::current().name() { + print!(" for thread \"{}\"", name); + } + println!(); + backtrace::print(); unsafe { _exit(sig); } @@ -139,7 +140,7 @@ pub fn main() { println!("{} (thread {})", msg, name); } if env::var("RUST_BACKTRACE").is_ok() { - println!("{:?}", Backtrace::new()); + backtrace::print(); } error!("{}", msg); From 99804eb5a6a6c2f4b1384f735cc43f2cc8b61b4f Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 27 Nov 2019 20:35:41 +0100 Subject: [PATCH 2/3] Only print a backtrace the first time the signal handler is called. This avoids infinite recursion if the printing causes another signal. --- ports/glutin/main2.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/ports/glutin/main2.rs b/ports/glutin/main2.rs index e1a848902f8..630ccaf8945 100644 --- a/ports/glutin/main2.rs +++ b/ports/glutin/main2.rs @@ -31,7 +31,6 @@ use servo::servo_config::pref; use std::env; use std::panic; use std::process; -use std::sync::atomic; use std::thread; pub mod platform { @@ -55,12 +54,16 @@ fn install_crash_handler() { use std::thread; extern "C" fn handler(sig: i32) { - print!("Stack trace"); - if let Some(name) = thread::current().name() { - print!(" for thread \"{}\"", name); + use std::sync::atomic; + static BEEN_HERE_BEFORE: atomic::AtomicBool = atomic::AtomicBool::new(false); + if !BEEN_HERE_BEFORE.swap(true, atomic::Ordering::SeqCst) { + print!("Stack trace"); + if let Some(name) = thread::current().name() { + print!(" for thread \"{}\"", name); + } + println!(); + backtrace::print(); } - println!(); - backtrace::print(); unsafe { _exit(sig); } From 5bcd2f564297aacc04189fd6001c0ab0b03ffad2 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 27 Nov 2019 21:10:18 +0100 Subject: [PATCH 3/3] Avoid locking in the signal handler --- ports/glutin/backtrace.rs | 81 +++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/ports/glutin/backtrace.rs b/ports/glutin/backtrace.rs index 0c089336557..6e5a8390042 100644 --- a/ports/glutin/backtrace.rs +++ b/ports/glutin/backtrace.rs @@ -25,47 +25,52 @@ struct Print { impl fmt::Debug for Print { fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { - let mut print_fn_frame = 0; - let mut frame_count = 0; - backtrace::trace(|frame| { - let found = frame.symbol_address() as usize == self.print_fn_address; - if found { - print_fn_frame = frame_count; - } - frame_count += 1; - !found - }); - - let mode = PrintFmt::Short; - let mut p = print_path; - let mut f = backtrace::BacktraceFmt::new(fmt, mode, &mut p); - f.add_context()?; - let mut result = Ok(()); - let mut frame_count = 0; - backtrace::trace(|frame| { - let skip = frame_count < print_fn_frame; - frame_count += 1; - if skip { - return true - } - - let mut frame_fmt = f.frame(); - let mut any_symbol = false; - backtrace::resolve_frame(frame, |symbol| { - any_symbol = true; - if let Err(e) = frame_fmt.symbol(frame, symbol) { - result = Err(e) + // Safety: we’re in a signal handler that is about to call `libc::_exit`. + // Potential data races from using `*_unsynchronized` functions are perhaps + // less bad than potential deadlocks? + unsafe { + let mut print_fn_frame = 0; + let mut frame_count = 0; + backtrace::trace_unsynchronized(|frame| { + let found = frame.symbol_address() as usize == self.print_fn_address; + if found { + print_fn_frame = frame_count; } + frame_count += 1; + !found }); - if !any_symbol { - if let Err(e) = frame_fmt.print_raw(frame.ip(), None, None, None) { - result = Err(e) + + let mode = PrintFmt::Short; + let mut p = print_path; + let mut f = backtrace::BacktraceFmt::new(fmt, mode, &mut p); + f.add_context()?; + let mut result = Ok(()); + let mut frame_count = 0; + backtrace::trace_unsynchronized(|frame| { + let skip = frame_count < print_fn_frame; + frame_count += 1; + if skip { + return true } - } - result.is_ok() - }); - result?; - f.finish() + + let mut frame_fmt = f.frame(); + let mut any_symbol = false; + backtrace::resolve_frame_unsynchronized(frame, |symbol| { + any_symbol = true; + if let Err(e) = frame_fmt.symbol(frame, symbol) { + result = Err(e) + } + }); + if !any_symbol { + if let Err(e) = frame_fmt.print_raw(frame.ip(), None, None, None) { + result = Err(e) + } + } + result.is_ok() + }); + result?; + f.finish() + } } }