From 9153333f6260df520ddaba3f8c0344ec5031a4ca Mon Sep 17 00:00:00 2001 From: Alan Jeffrey Date: Fri, 22 Apr 2016 16:26:13 -0500 Subject: [PATCH] Communicate a backtrace to the constellation when panicking. --- components/compositing/constellation.rs | 84 +++++++++++++------------ components/msg/constellation_msg.rs | 2 +- components/servo/Cargo.lock | 31 +++++++++ components/util/Cargo.toml | 1 + components/util/lib.rs | 1 + components/util/panicking.rs | 32 ++-------- components/util/thread.rs | 18 +++++- ports/cef/Cargo.lock | 31 +++++++++ ports/geckolib/Cargo.lock | 36 +++++++++++ ports/gonk/Cargo.lock | 31 +++++++++ 10 files changed, 197 insertions(+), 70 deletions(-) diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index 026edcb9b9b..829eca097f8 100644 --- a/components/compositing/constellation.rs +++ b/components/compositing/constellation.rs @@ -54,7 +54,6 @@ use std::borrow::ToOwned; use std::collections::HashMap; use std::env; use std::io::Error as IOError; -use std::io::{self, Write}; use std::marker::PhantomData; use std::mem::replace; use std::process; @@ -183,6 +182,9 @@ pub struct Constellation { // Webrender interface, if enabled. webrender_api_sender: Option, + /// Have we seen any panics? Hopefully always false! + handled_panic: bool, + /// The random number generator and probability for closing pipelines. /// This is for testing the hardening of the constellation. random_pipeline_closure: Option<(StdRng, f32)>, @@ -369,6 +371,7 @@ impl Constellation child_processes: Vec::new(), document_states: HashMap::new(), webrender_api_sender: state.webrender_api_sender, + handled_panic: false, random_pipeline_closure: opts::get().random_pipeline_closure_probability.map(|prob| { let seed = opts::get().random_pipeline_closure_seed.unwrap_or_else(random); let rng = StdRng::from_seed(&[seed]); @@ -810,9 +813,9 @@ impl Constellation // Panic messages - Request::Panic((pipeline_id, panic_reason)) => { + Request::Panic((pipeline_id, panic_reason, backtrace)) => { debug!("handling panic message ({:?})", pipeline_id); - self.handle_panic(pipeline_id, panic_reason); + self.handle_panic(pipeline_id, panic_reason, backtrace); } } true @@ -842,55 +845,58 @@ impl Constellation fn handle_send_error(&mut self, pipeline_id: PipelineId, err: IOError) { // Treat send error the same as receiving a panic message debug!("Pipeline {:?} send error ({}).", pipeline_id, err); - self.handle_panic(Some(pipeline_id), format!("Send failed ({})", err)); + self.handle_panic(Some(pipeline_id), format!("Send failed ({})", err), String::from("")); } - fn handle_panic(&mut self, pipeline_id: Option, reason: String) { + fn handle_panic(&mut self, pipeline_id: Option, reason: String, backtrace: String) { + error!("Panic: {}", reason); + if !self.handled_panic || opts::get().full_backtraces { + // For the first panic, we print the full backtrace + error!("Backtrace:\n{}", backtrace); + } else { + error!("Backtrace skipped (run with -Z full-backtraces to see every backtrace)."); + } + if opts::get().hard_fail { // It's quite difficult to make Servo exit cleanly if some threads have failed. // Hard fail exists for test runners so we crash and that's good enough. - let mut stderr = io::stderr(); - stderr.write_all("Pipeline failed in hard-fail mode. Crashing!\n".as_bytes()) - .expect("Failed to write to stderr!"); + error!("Pipeline failed in hard-fail mode. Crashing!"); process::exit(1); } debug!("Panic handler for pipeline {:?}: {}.", pipeline_id, reason); if let Some(pipeline_id) = pipeline_id { - self.replace_pipeline_with_about_failure(pipeline_id); + + let parent_info = self.pipelines.get(&pipeline_id).and_then(|pipeline| pipeline.parent_info); + let window_size = self.pipelines.get(&pipeline_id).and_then(|pipeline| pipeline.size); + + // Notify the browser chrome that the pipeline has failed + self.trigger_mozbrowsererror(pipeline_id); + + self.close_pipeline(pipeline_id, ExitPipelineMode::Force); + + while let Some(pending_pipeline_id) = self.pending_frames.iter().find(|pending| { + pending.old_pipeline_id == Some(pipeline_id) + }).map(|frame| frame.new_pipeline_id) { + warn!("removing pending frame change for failed pipeline"); + self.close_pipeline(pending_pipeline_id, ExitPipelineMode::Force); + } + + warn!("creating replacement pipeline for about:failure"); + + let new_pipeline_id = PipelineId::new(); + self.new_pipeline(new_pipeline_id, + parent_info, + window_size, + None, + LoadData::new(Url::parse("about:failure").expect("infallible"), None, None)); + + self.push_pending_frame(new_pipeline_id, Some(pipeline_id)); + } - } - - fn replace_pipeline_with_about_failure(&mut self, pipeline_id: PipelineId) { - - let parent_info = self.pipelines.get(&pipeline_id).and_then(|pipeline| pipeline.parent_info); - let window_size = self.pipelines.get(&pipeline_id).and_then(|pipeline| pipeline.size); - - // Notify the browser chrome that the pipeline has failed - self.trigger_mozbrowsererror(pipeline_id); - - self.close_pipeline(pipeline_id, ExitPipelineMode::Force); - - while let Some(pending_pipeline_id) = self.pending_frames.iter().find(|pending| { - pending.old_pipeline_id == Some(pipeline_id) - }).map(|frame| frame.new_pipeline_id) { - warn!("removing pending frame change for failed pipeline"); - self.close_pipeline(pending_pipeline_id, ExitPipelineMode::Force); - } - - warn!("creating replacement pipeline for about:failure"); - - let new_pipeline_id = PipelineId::new(); - self.new_pipeline(new_pipeline_id, - parent_info, - window_size, - None, - LoadData::new(Url::parse("about:failure").expect("infallible"), None, None)); - - self.push_pending_frame(new_pipeline_id, Some(pipeline_id)); - + self.handled_panic = true; } fn handle_init_load(&mut self, url: Url) { diff --git a/components/msg/constellation_msg.rs b/components/msg/constellation_msg.rs index 8fa5b8bba9b..e1a5b9ef681 100644 --- a/components/msg/constellation_msg.rs +++ b/components/msg/constellation_msg.rs @@ -35,7 +35,7 @@ impl Clone for ConstellationChan { } } -pub type PanicMsg = (Option, String); +pub type PanicMsg = (Option, String, String); #[derive(Copy, Clone, Deserialize, Serialize, HeapSizeOf)] pub struct WindowSizeData { diff --git a/components/servo/Cargo.lock b/components/servo/Cargo.lock index 620ec07eb28..77ff834c4c7 100644 --- a/components/servo/Cargo.lock +++ b/components/servo/Cargo.lock @@ -116,6 +116,27 @@ dependencies = [ "x11 2.3.0 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "backtrace" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "backtrace-sys 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)", + "cfg-if 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "dbghelp-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "kernel32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", +] + +[[package]] +name = "backtrace-sys" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "bincode" version = "0.5.3" @@ -380,6 +401,15 @@ dependencies = [ "serde_macros 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "dbghelp-sys" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "debug_unreachable" version = "0.0.6" @@ -2253,6 +2283,7 @@ name = "util" version = "0.0.1" dependencies = [ "app_units 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", + "backtrace 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "deque 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.6.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/components/util/Cargo.toml b/components/util/Cargo.toml index ac7bc43b1dd..c2b830f8849 100644 --- a/components/util/Cargo.toml +++ b/components/util/Cargo.toml @@ -29,6 +29,7 @@ git = "https://github.com/servo/ipc-channel" [dependencies] app_units = {version = "0.2.3", features = ["plugins"]} +backtrace = "0.2.1" bitflags = "0.3" deque = "0.3.1" euclid = {version = "0.6.4", features = ["unstable", "plugins"]} diff --git a/components/util/lib.rs b/components/util/lib.rs index b4998b0580c..17f4a2226d5 100644 --- a/components/util/lib.rs +++ b/components/util/lib.rs @@ -18,6 +18,7 @@ #![deny(unsafe_code)] extern crate app_units; +extern crate backtrace; #[allow(unused_extern_crates)] #[macro_use] extern crate bitflags; diff --git a/components/util/panicking.rs b/components/util/panicking.rs index 95abf897e19..9e53ba83eed 100644 --- a/components/util/panicking.rs +++ b/components/util/panicking.rs @@ -2,11 +2,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use opts; use std::any::Any; use std::boxed::FnBox; use std::cell::RefCell; -use std::io::{Write, stderr}; use std::panic::{PanicInfo, take_hook, set_hook}; use std::sync::{Once, ONCE_INIT}; use std::thread; @@ -31,9 +29,6 @@ pub fn set_thread_local_hook(local: Box) { /// Should be called in main() after arguments have been parsed pub fn initiate_panic_hook() { - // store it locally, we can't trust that opts::get() will work whilst panicking - let full_backtraces = opts::get().full_backtraces; - // Set the panic handler only once. It is global. HOOK_SET.call_once(|| { // The original backtrace-printing hook. We still want to call this @@ -44,34 +39,15 @@ pub fn initiate_panic_hook() { let name = thread::current().name().unwrap_or("").to_string(); // Notify error handlers stored in LOCAL_INFO if any LOCAL_INFO.with(|i| { - if let Some(info) = i.borrow_mut().take() { + if let Some(local_info) = i.borrow_mut().take() { debug!("Thread `{}` failed, notifying error handlers", name); - (info.fail).call_box((payload, )); + (local_info.fail).call_box((payload, )); + } else { + hook(&info); } }); - - // Skip cascading SendError/RecvError backtraces if allowed - if !full_backtraces { - if let Some(s) = payload.downcast_ref::() { - if s.contains("SendError") { - let err = stderr(); - let _ = write!(err.lock(), "Thread \"{}\" panicked with an unwrap of \ - `SendError` (backtrace skipped)\n", name); - return; - } else if s.contains("RecvError") { - let err = stderr(); - let _ = write!(err.lock(), "Thread \"{}\" panicked with an unwrap of \ - `RecvError` (backtrace skipped)\n", name); - return; - } - } - } - - // Call the old hook to get the backtrace - hook(&info); }; set_hook(Box::new(new_hook)); }); - } diff --git a/components/util/thread.rs b/components/util/thread.rs index f6bb72e20a3..87d8954dcf9 100644 --- a/components/util/thread.rs +++ b/components/util/thread.rs @@ -2,6 +2,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +use backtrace::Backtrace; use ipc_channel::ipc::IpcSender; use panicking; use serde::Serialize; @@ -20,7 +21,7 @@ pub fn spawn_named_with_send_on_panic(name: String, state: thread_state::ThreadState, f: F, id: Id, - panic_chan: IpcSender<(Id, String)>) + panic_chan: IpcSender<(Id, String, String)>) where F: FnOnce() + Send + 'static, Id: Copy + Send + Serialize + 'static, { @@ -31,7 +32,20 @@ pub fn spawn_named_with_send_on_panic(name: String, let reason = payload.downcast_ref::().map(|s| String::from(&**s)) .or(payload.downcast_ref::<&'static str>().map(|s| String::from(*s))) .unwrap_or_else(|| String::from("")); - let _ = panic_chan.send((id, reason)); + // FIXME(ajeffrey): Really we should send the backtrace itself, + // not just a string representation. Unfortunately we can't, because + // Servo won't compile backtrace with the serialize-serde feature: + // + // .../quasi_macros-0.9.0/src/lib.rs:19:29: 19:32 error: mismatched types: + // expected `&mut syntex::Registry`, + // found `&mut rustc_plugin::Registry<'_>` + // (expected struct `syntex::Registry`, + // found struct `rustc_plugin::Registry`) [E0308] + // .../quasi_macros-0.9.0/src/lib.rs:19 quasi_codegen::register(reg); + // + // so for the moment we just send a debug string. + let backtrace = format!("{:?}", Backtrace::new()); + let _ = panic_chan.send((id, reason, backtrace)); })); f() }).expect("Thread spawn failed"); diff --git a/ports/cef/Cargo.lock b/ports/cef/Cargo.lock index d310b1f1a7b..1b54bd49f88 100644 --- a/ports/cef/Cargo.lock +++ b/ports/cef/Cargo.lock @@ -101,6 +101,27 @@ dependencies = [ "x11 2.3.0 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "backtrace" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "backtrace-sys 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)", + "cfg-if 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "dbghelp-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "kernel32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", +] + +[[package]] +name = "backtrace-sys" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "bincode" version = "0.5.3" @@ -350,6 +371,15 @@ dependencies = [ "serde_macros 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "dbghelp-sys" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "debug_unreachable" version = "0.0.6" @@ -2130,6 +2160,7 @@ name = "util" version = "0.0.1" dependencies = [ "app_units 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", + "backtrace 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "deque 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.6.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/ports/geckolib/Cargo.lock b/ports/geckolib/Cargo.lock index 2d0d34535e5..a075d211c3f 100644 --- a/ports/geckolib/Cargo.lock +++ b/ports/geckolib/Cargo.lock @@ -39,6 +39,27 @@ name = "aster" version = "0.15.0" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "backtrace" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "backtrace-sys 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)", + "cfg-if 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "dbghelp-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "kernel32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", +] + +[[package]] +name = "backtrace-sys" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "bincode" version = "0.5.3" @@ -65,6 +86,11 @@ name = "byteorder" version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" +[[package]] +name = "cfg-if" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "cssparser" version = "0.5.5" @@ -78,6 +104,15 @@ dependencies = [ "serde_macros 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "dbghelp-sys" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "debug_unreachable" version = "0.0.6" @@ -545,6 +580,7 @@ name = "util" version = "0.0.1" dependencies = [ "app_units 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", + "backtrace 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "deque 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.6.6 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/ports/gonk/Cargo.lock b/ports/gonk/Cargo.lock index c6b6564497d..237ec1bec8f 100644 --- a/ports/gonk/Cargo.lock +++ b/ports/gonk/Cargo.lock @@ -94,6 +94,27 @@ dependencies = [ "x11 2.3.0 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "backtrace" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "backtrace-sys 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)", + "cfg-if 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "dbghelp-sys 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "kernel32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", +] + +[[package]] +name = "backtrace-sys" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "libc 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "bincode" version = "0.5.3" @@ -343,6 +364,15 @@ dependencies = [ "serde_macros 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "dbghelp-sys" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "winapi 0.2.6 (registry+https://github.com/rust-lang/crates.io-index)", + "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "debug_unreachable" version = "0.0.6" @@ -2111,6 +2141,7 @@ name = "util" version = "0.0.1" dependencies = [ "app_units 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", + "backtrace 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "bitflags 0.3.3 (registry+https://github.com/rust-lang/crates.io-index)", "deque 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "euclid 0.6.6 (registry+https://github.com/rust-lang/crates.io-index)",