From c3c6c95a9b1fd81d740aa8a300174fbbead8f38c Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Wed, 6 Sep 2023 17:52:37 +0800 Subject: [PATCH] =?UTF-8?q?constellation:=20crash=20to=20a=20new=20?= =?UTF-8?q?=E2=80=9Csad=20tab=E2=80=9D=20error=20page=20(#30290)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * constellation: crash to a new “sad tab” page * check in resources/crash.html * use a separate enum variant instead of keying on reason * fmt + tidy * rename Resource::Crash to Resource::CrashHTML * clean up crash page and add details (reason + backtrace) * avoid repeating crash errors in script::script_thread warn log * make new LoadData init more idiomatic * clarify comments and new fields * fix doc comment style --- components/constellation/constellation.rs | 39 ++++++++------ components/embedder_traits/resources.rs | 2 + components/net/fetch/methods.rs | 7 +++ components/net_traits/lib.rs | 4 +- components/net_traits/request.rs | 12 +++++ components/script/dom/servoparser/mod.rs | 64 +++++++++++++---------- components/script/fetch.rs | 1 + components/script/script_thread.rs | 4 +- components/script_traits/lib.rs | 4 ++ ports/gstplugin/resources.rs | 1 + ports/libsimpleservo/api/src/lib.rs | 1 + ports/servoshell/resources.rs | 1 + resources/crash.html | 8 +++ 13 files changed, 102 insertions(+), 46 deletions(-) create mode 100644 resources/crash.html diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 2cfe3ac2422..50a18ade6ba 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -2782,7 +2782,7 @@ where self.embedder_proxy.send(( Some(top_level_browsing_context_id), - EmbedderMsg::Panic(reason, backtrace), + EmbedderMsg::Panic(reason.clone(), backtrace.clone()), )); let browsing_context = match self.browsing_contexts.get(&browsing_context_id) { @@ -2797,7 +2797,6 @@ where Some(p) => p, None => return warn!("failed pipeline is missing"), }; - let pipeline_url = pipeline.url.clone(); let opener = pipeline.opener; self.close_browsing_context_children( @@ -2806,23 +2805,27 @@ where ExitPipelineMode::Force, ); - let failure_url = ServoUrl::parse("about:failure").expect("infallible"); - - if pipeline_url == failure_url { - return error!("about:failure failed"); + let old_pipeline_id = pipeline_id; + let old_load_data = match self.pipelines.get(&pipeline_id) { + Some(pipeline) => pipeline.load_data.clone(), + None => return warn!("failed pipeline is missing"), + }; + if old_load_data.crash.is_some() { + return error!("crash page crashed"); } - warn!("creating replacement pipeline for about:failure"); + warn!("creating replacement pipeline for crash page"); let new_pipeline_id = PipelineId::new(); - let load_data = LoadData::new( - LoadOrigin::Constellation, - failure_url, - None, - Referrer::NoReferrer, - None, - None, - ); + let new_load_data = LoadData { + crash: Some( + backtrace + .map(|b| format!("{}\n{}", reason, b)) + .unwrap_or(reason), + ), + ..old_load_data.clone() + }; + let sandbox = IFrameSandboxState::IFrameSandboxed; let is_private = false; self.new_pipeline( @@ -2832,7 +2835,7 @@ where None, opener, window_size, - load_data, + new_load_data, sandbox, is_private, is_visible, @@ -2841,7 +2844,9 @@ where top_level_browsing_context_id, browsing_context_id, new_pipeline_id, - replace: None, + // Pipeline already closed by close_browsing_context_children, so we can pass Yes here + // to avoid closing again in handle_activate_document_msg (though it would be harmless) + replace: Some(NeedsToReload::Yes(old_pipeline_id, old_load_data)), new_browsing_context_info: None, window_size, }); diff --git a/components/embedder_traits/resources.rs b/components/embedder_traits/resources.rs index d35bece3995..6cee8029076 100644 --- a/components/embedder_traits/resources.rs +++ b/components/embedder_traits/resources.rs @@ -61,6 +61,7 @@ pub enum Resource { RippyPNG, MediaControlsCSS, MediaControlsJS, + CrashHTML, } pub trait ResourceReaderMethods { @@ -96,6 +97,7 @@ fn resources_for_tests() -> Box { Resource::RippyPNG => "rippy.png", Resource::MediaControlsCSS => "media-controls.css", Resource::MediaControlsJS => "media-controls.js", + Resource::CrashHTML => "crash.html", }; let mut path = env::current_exe().unwrap(); path = path.canonicalize().unwrap(); diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index f11a203b3bc..cb32e7a62d2 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -195,6 +195,13 @@ pub async fn main_fetch( // Step 1. let mut response = None; + // Servo internal: return a crash error when a crash error page is needed + if let Some(ref details) = request.crash { + response = Some(Response::network_error(NetworkError::Crash( + details.clone(), + ))); + } + // Step 2. if request.local_urls_only { if !matches!( diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs index 1df0e5e0374..c3816e1494b 100644 --- a/components/net_traits/lib.rs +++ b/components/net_traits/lib.rs @@ -758,8 +758,10 @@ pub enum NetworkError { /// Could be any of the internal errors, like unsupported scheme, connection errors, etc. Internal(String), LoadCancelled, - /// SSL validation error that has to be handled in the HTML parser + /// SSL validation error, to be converted to Resource::BadCertHTML in the HTML parser. SslValidation(String, Vec), + /// Crash error, to be converted to Resource::Crash in the HTML parser. + Crash(String), } impl NetworkError { diff --git a/components/net_traits/request.rs b/components/net_traits/request.rs index 1fb33e37bbd..3a76d3d5185 100644 --- a/components/net_traits/request.rs +++ b/components/net_traits/request.rs @@ -254,6 +254,8 @@ pub struct RequestBuilder { pub initiator: Initiator, pub https_state: HttpsState, pub response_tainting: ResponseTainting, + /// Servo internal: if crash details are present, trigger a crash error page with these details. + pub crash: Option, } impl RequestBuilder { @@ -284,6 +286,7 @@ impl RequestBuilder { csp_list: None, https_state: HttpsState::None, response_tainting: ResponseTainting::Basic, + crash: None, } } @@ -382,6 +385,11 @@ impl RequestBuilder { self } + pub fn crash(mut self, crash: Option) -> Self { + self.crash = crash; + self + } + pub fn build(self) -> Request { let mut request = Request::new( self.url.clone(), @@ -415,6 +423,7 @@ impl RequestBuilder { request.parser_metadata = self.parser_metadata; request.csp_list = self.csp_list; request.response_tainting = self.response_tainting; + request.crash = self.crash; request } } @@ -488,6 +497,8 @@ pub struct Request { #[ignore_malloc_size_of = "Defined in rust-content-security-policy"] pub csp_list: Option, pub https_state: HttpsState, + /// Servo internal: if crash details are present, trigger a crash error page with these details. + pub crash: Option, } impl Request { @@ -528,6 +539,7 @@ impl Request { response_tainting: ResponseTainting::Basic, csp_list: None, https_state: https_state, + crash: None, } } diff --git a/components/script/dom/servoparser/mod.rs b/components/script/dom/servoparser/mod.rs index d8436b930bd..62d963e2e74 100644 --- a/components/script/dom/servoparser/mod.rs +++ b/components/script/dom/servoparser/mod.rs @@ -774,28 +774,29 @@ impl FetchResponseListener for ParserContext { fn process_request_eof(&mut self) {} fn process_response(&mut self, meta_result: Result) { - let mut ssl_error = None; - let mut network_error = None; - let metadata = match meta_result { - Ok(meta) => Some(match meta { - FetchMetadata::Unfiltered(m) => m, - FetchMetadata::Filtered { unsafe_, .. } => unsafe_, - }), - Err(NetworkError::SslValidation(reason, cert_bytes)) => { - ssl_error = Some((reason, cert_bytes)); - let mut meta = Metadata::default(self.url.clone()); - let mime: Option = "text/html".parse().ok(); - meta.set_content_type(mime.as_ref()); - Some(meta) - }, - Err(NetworkError::Internal(reason)) => { - network_error = Some(reason); - let mut meta = Metadata::default(self.url.clone()); - let mime: Option = "text/html".parse().ok(); - meta.set_content_type(mime.as_ref()); - Some(meta) - }, - Err(_) => None, + let (metadata, error) = match meta_result { + Ok(meta) => ( + Some(match meta { + FetchMetadata::Unfiltered(m) => m, + FetchMetadata::Filtered { unsafe_, .. } => unsafe_, + }), + None, + ), + Err(error) => ( + // Check variant without moving + match &error { + NetworkError::SslValidation(..) | + NetworkError::Internal(..) | + NetworkError::Crash(..) => { + let mut meta = Metadata::default(self.url.clone()); + let mime: Option = "text/html".parse().ok(); + meta.set_content_type(mime.as_ref()); + Some(meta) + }, + _ => None, + }, + Some(error), + ), }; let content_type: Option = metadata .clone() @@ -876,8 +877,8 @@ impl FetchResponseListener for ParserContext { parser.parse_sync(); parser.tokenizer.borrow_mut().set_plaintext_state(); }, - (mime::TEXT, mime::HTML, _) => { - if let Some((reason, bytes)) = ssl_error { + (mime::TEXT, mime::HTML, _) => match error { + Some(NetworkError::SslValidation(reason, bytes)) => { self.is_synthesized_document = true; let page = resources::read_string(Resource::BadCertHTML); let page = page.replace("${reason}", &reason); @@ -887,14 +888,23 @@ impl FetchResponseListener for ParserContext { page.replace("${secret}", &net_traits::PRIVILEGED_SECRET.to_string()); parser.push_string_input_chunk(page); parser.parse_sync(); - } - if let Some(reason) = network_error { + }, + Some(NetworkError::Internal(reason)) => { self.is_synthesized_document = true; let page = resources::read_string(Resource::NetErrorHTML); let page = page.replace("${reason}", &reason); parser.push_string_input_chunk(page); parser.parse_sync(); - } + }, + Some(NetworkError::Crash(details)) => { + self.is_synthesized_document = true; + let page = resources::read_string(Resource::CrashHTML); + let page = page.replace("${details}", &details); + parser.push_string_input_chunk(page); + parser.parse_sync(); + }, + Some(_) => {}, + None => {}, }, (mime::TEXT, mime::XML, _) | (mime::APPLICATION, mime::XML, _) | diff --git a/components/script/fetch.rs b/components/script/fetch.rs index 5e88d46dffb..b3db51d2a1d 100644 --- a/components/script/fetch.rs +++ b/components/script/fetch.rs @@ -129,6 +129,7 @@ fn request_init_from_request(request: NetTraitsRequest) -> RequestBuilder { csp_list: None, https_state: request.https_state, response_tainting: request.response_tainting, + crash: None, } } diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 43ea9833a2f..6f8c60ad7e7 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -3843,7 +3843,8 @@ impl ScriptThread { .headers(load_data.headers) .body(load_data.data) .redirect_mode(RedirectMode::Manual) - .origin(incomplete.origin.immutable().clone()); + .origin(incomplete.origin.immutable().clone()) + .crash(load_data.crash); let context = ParserContext::new(id, load_data.url); self.incomplete_parser_contexts @@ -3869,6 +3870,7 @@ impl ScriptThread { ) { match fetch_metadata { Ok(_) => (), + Err(NetworkError::Crash(..)) => (), Err(ref e) => { warn!("Network error: {:?}", e); }, diff --git a/components/script_traits/lib.rs b/components/script_traits/lib.rs index a6291cf5745..c1b02c4335a 100644 --- a/components/script_traits/lib.rs +++ b/components/script_traits/lib.rs @@ -184,6 +184,9 @@ pub struct LoadData { pub srcdoc: String, /// The inherited context is Secure, None if not inherited pub inherited_secure_context: Option, + + /// Servo internal: if crash details are present, trigger a crash error page with these details. + pub crash: Option, } /// The result of evaluating a javascript scheme url. @@ -218,6 +221,7 @@ impl LoadData { referrer_policy: referrer_policy, srcdoc: "".to_string(), inherited_secure_context, + crash: None, } } } diff --git a/ports/gstplugin/resources.rs b/ports/gstplugin/resources.rs index a01ebbda4f3..bd02e1576b6 100644 --- a/ports/gstplugin/resources.rs +++ b/ports/gstplugin/resources.rs @@ -35,6 +35,7 @@ fn filename(file: Resource) -> &'static str { Resource::RippyPNG => "rippy.png", Resource::MediaControlsCSS => "media-controls.css", Resource::MediaControlsJS => "media-controls.js", + Resource::CrashHTML => "crash.html", } } diff --git a/ports/libsimpleservo/api/src/lib.rs b/ports/libsimpleservo/api/src/lib.rs index 73ed9f1a3b6..9df7845bbf5 100644 --- a/ports/libsimpleservo/api/src/lib.rs +++ b/ports/libsimpleservo/api/src/lib.rs @@ -920,6 +920,7 @@ impl ResourceReaderMethods for ResourceReaderInstance { Resource::MediaControlsJS => { &include_bytes!("../../../../resources/media-controls.js")[..] }, + Resource::CrashHTML => &include_bytes!("../../../../resources/crash.html")[..], }) } diff --git a/ports/servoshell/resources.rs b/ports/servoshell/resources.rs index 57050233bc4..dfd358e9f01 100644 --- a/ports/servoshell/resources.rs +++ b/ports/servoshell/resources.rs @@ -30,6 +30,7 @@ fn filename(file: Resource) -> &'static str { Resource::RippyPNG => "rippy.png", Resource::MediaControlsCSS => "media-controls.css", Resource::MediaControlsJS => "media-controls.js", + Resource::CrashHTML => "crash.html", } } diff --git a/resources/crash.html b/resources/crash.html new file mode 100644 index 00000000000..fc54ae890c8 --- /dev/null +++ b/resources/crash.html @@ -0,0 +1,8 @@ +

Servo crashed!

+ + + + + +

+${details}