From 0eeb8b8f41aa7ba88d2652513a53d077151b97d5 Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Sat, 16 Mar 2024 08:58:57 +0100 Subject: [PATCH] compositor: Handle synchronous messages while shutting down During the shutdown process, various threads (such as the font cache thread) may be finishing up their work. If those threads make synchronous requests to the compositor, answer them -- even if the results will be unused. This is at least enough processing for them to finish their work and exit cleanly. This addresses crashes that are sometimes seen at exit, particuarly when the font cache thread tries to register a font during shutdown. In addition, this change also removes an unused compositor message. --- components/compositing/compositor.rs | 142 ++++++++++++++++++--------- components/shared/compositing/lib.rs | 5 - 2 files changed, 96 insertions(+), 51 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 0df828bb79f2..3e08d33f6957 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -524,38 +524,38 @@ impl IOCompositor { } fn handle_browser_message(&mut self, msg: CompositorMsg) -> bool { - if matches!(msg, CompositorMsg::NewWebRenderFrameReady(..)) { - self.pending_frames -= 1; - } - - match (msg, self.shutdown_state) { - (_, ShutdownState::FinishedShuttingDown) => { + match self.shutdown_state { + ShutdownState::NotShuttingDown => {}, + ShutdownState::ShuttingDown => { + return self.handle_browser_message_while_shutting_down(msg) + }, + ShutdownState::FinishedShuttingDown => { error!("compositor shouldn't be handling messages after shutting down"); return false; }, + } - (CompositorMsg::ShutdownComplete, _) => { + match msg { + CompositorMsg::ShutdownComplete => { + warn!("Received `ShutdownComplete` while not shutting down."); self.finish_shutting_down(); return false; }, - ( - CompositorMsg::ChangeRunningAnimationsState(pipeline_id, animation_state), - ShutdownState::NotShuttingDown, - ) => { + CompositorMsg::ChangeRunningAnimationsState(pipeline_id, animation_state) => { self.change_running_animations_state(pipeline_id, animation_state); }, - (CompositorMsg::SetFrameTree(frame_tree), ShutdownState::NotShuttingDown) => { + CompositorMsg::SetFrameTree(frame_tree) => { self.set_frame_tree(&frame_tree); self.send_scroll_positions_to_layout_for_pipeline(&frame_tree.pipeline.id); }, - (CompositorMsg::TouchEventProcessed(result), ShutdownState::NotShuttingDown) => { + CompositorMsg::TouchEventProcessed(result) => { self.touch_handler.on_event_processed(result); }, - (CompositorMsg::CreatePng(page_rect, reply), ShutdownState::NotShuttingDown) => { + CompositorMsg::CreatePng(page_rect, reply) => { let res = self.composite_specific_target(CompositeTarget::SharedMemory, page_rect); if let Err(ref e) = res { info!("Error retrieving PNG: {:?}", e); @@ -566,7 +566,7 @@ impl IOCompositor { } }, - (CompositorMsg::IsReadyToSaveImageReply(is_ready), ShutdownState::NotShuttingDown) => { + CompositorMsg::IsReadyToSaveImageReply(is_ready) => { assert_eq!( self.ready_to_save_state, ReadyState::WaitingForConstellationReply @@ -579,24 +579,20 @@ impl IOCompositor { self.composite_if_necessary(CompositingReason::Headless); }, - ( - CompositorMsg::PipelineVisibilityChanged(pipeline_id, visible), - ShutdownState::NotShuttingDown, - ) => { + CompositorMsg::PipelineVisibilityChanged(pipeline_id, visible) => { self.pipeline_details(pipeline_id).visible = visible; self.process_animations(true); }, - (CompositorMsg::PipelineExited(pipeline_id, sender), _) => { + CompositorMsg::PipelineExited(pipeline_id, sender) => { debug!("Compositor got pipeline exited: {:?}", pipeline_id); self.remove_pipeline_root_layer(pipeline_id); let _ = sender.send(()); }, - ( - CompositorMsg::NewWebRenderFrameReady(recomposite_needed), - ShutdownState::NotShuttingDown, - ) => { + CompositorMsg::NewWebRenderFrameReady(recomposite_needed) => { + self.pending_frames -= 1; + if recomposite_needed { if let Some(result) = self.hit_test_at_point(self.cursor_pos) { self.update_cursor(result); @@ -608,13 +604,7 @@ impl IOCompositor { } }, - (CompositorMsg::Dispatch(func), ShutdownState::NotShuttingDown) => { - // The functions sent here right now are really dumb, so they can't panic. - // But if we start running more complex code here, we should really catch panic here. - func(); - }, - - (CompositorMsg::LoadComplete(_), ShutdownState::NotShuttingDown) => { + CompositorMsg::LoadComplete(_) => { // If we're painting in headless mode, schedule a recomposite. if matches!(self.composite_target, CompositeTarget::PngFile(_)) || self.exit_after_load @@ -623,10 +613,7 @@ impl IOCompositor { } }, - ( - CompositorMsg::WebDriverMouseButtonEvent(mouse_event_type, mouse_button, x, y), - ShutdownState::NotShuttingDown, - ) => { + CompositorMsg::WebDriverMouseButtonEvent(mouse_event_type, mouse_button, x, y) => { let dppx = self.device_pixels_per_page_pixel(); let point = dppx.transform_point(Point2D::new(x, y)); self.on_mouse_window_event_class(match mouse_event_type { @@ -636,29 +623,29 @@ impl IOCompositor { }); }, - (CompositorMsg::WebDriverMouseMoveEvent(x, y), ShutdownState::NotShuttingDown) => { + CompositorMsg::WebDriverMouseMoveEvent(x, y) => { let dppx = self.device_pixels_per_page_pixel(); let point = dppx.transform_point(Point2D::new(x, y)); self.on_mouse_window_move_event_class(DevicePoint::new(point.x, point.y)); }, - (CompositorMsg::PendingPaintMetric(pipeline_id, epoch), _) => { + CompositorMsg::PendingPaintMetric(pipeline_id, epoch) => { self.pending_paint_metrics.insert(pipeline_id, epoch); }, - (CompositorMsg::GetClientWindow(req), ShutdownState::NotShuttingDown) => { + CompositorMsg::GetClientWindow(req) => { if let Err(e) = req.send(self.embedder_coordinates.window) { warn!("Sending response to get client window failed ({:?}).", e); } }, - (CompositorMsg::GetScreenSize(req), ShutdownState::NotShuttingDown) => { + CompositorMsg::GetScreenSize(req) => { if let Err(e) = req.send(self.embedder_coordinates.screen) { warn!("Sending response to get screen size failed ({:?}).", e); } }, - (CompositorMsg::GetScreenAvailSize(req), ShutdownState::NotShuttingDown) => { + CompositorMsg::GetScreenAvailSize(req) => { if let Err(e) = req.send(self.embedder_coordinates.screen_avail) { warn!( "Sending response to get screen avail size failed ({:?}).", @@ -667,14 +654,9 @@ impl IOCompositor { } }, - (CompositorMsg::Forwarded(msg), ShutdownState::NotShuttingDown) => { + CompositorMsg::Forwarded(msg) => { self.handle_webrender_message(msg); }, - - // When we are shutting_down, we need to avoid performing operations - // such as Paint that may crash because we have begun tearing down - // the rest of our resources. - (_, ShutdownState::ShuttingDown) => {}, } true @@ -917,6 +899,74 @@ impl IOCompositor { } } + /// Handle messages sent to the compositor during the shutdown process. In general, + /// the things the compositor can do in this state are limited. It's very important to + /// answer any synchronous messages though as other threads might be waiting on the + /// results to finish their own shut down process. We try to do as little as possible + /// during this time. + /// + /// When that involves generating WebRender ids, our approach here is to simply + /// generate them, but assume they will never be used, since once shutting down the + /// compositor no longer does any WebRender frame generation. + fn handle_browser_message_while_shutting_down(&mut self, msg: CompositorMsg) -> bool { + match msg { + CompositorMsg::ShutdownComplete => { + self.finish_shutting_down(); + return false; + }, + CompositorMsg::PipelineExited(pipeline_id, sender) => { + debug!("Compositor got pipeline exited: {:?}", pipeline_id); + self.remove_pipeline_root_layer(pipeline_id); + let _ = sender.send(()); + }, + CompositorMsg::Forwarded(ForwardedToCompositorMsg::Font( + FontToCompositorMsg::AddFontInstance(_, _, sender), + )) => { + let _ = sender.send(self.webrender_api.generate_font_instance_key()); + }, + CompositorMsg::Forwarded(ForwardedToCompositorMsg::Font( + FontToCompositorMsg::AddFont(_, sender), + )) => { + let _ = sender.send(self.webrender_api.generate_font_key()); + }, + CompositorMsg::Forwarded(ForwardedToCompositorMsg::Canvas( + CanvasToCompositorMsg::GenerateKey(sender), + )) => { + let _ = sender.send(self.webrender_api.generate_image_key()); + }, + CompositorMsg::GetClientWindow(sender) => { + if let Err(e) = sender.send(self.embedder_coordinates.window) { + warn!("Sending response to get client window failed ({:?}).", e); + } + }, + CompositorMsg::GetScreenSize(sender) => { + if let Err(e) = sender.send(self.embedder_coordinates.screen) { + warn!("Sending response to get screen size failed ({:?}).", e); + } + }, + CompositorMsg::GetScreenAvailSize(sender) => { + if let Err(e) = sender.send(self.embedder_coordinates.screen_avail) { + warn!( + "Sending response to get screen avail size failed ({:?}).", + e + ); + } + }, + CompositorMsg::NewWebRenderFrameReady(_) => { + // Subtract from the number of pending frames, but do not do any compositing. + self.pending_frames -= 1; + }, + CompositorMsg::PendingPaintMetric(pipeline_id, epoch) => { + self.pending_paint_metrics.insert(pipeline_id, epoch); + }, + + _ => { + debug!("Ignoring message ({:?} while shutting down", msg); + }, + } + true + } + /// Queue a new frame in the transaction and increase the pending frames count. fn generate_frame(&mut self, transaction: &mut Transaction, reason: RenderReasons) { self.pending_frames += 1; diff --git a/components/shared/compositing/lib.rs b/components/shared/compositing/lib.rs index a64f1a83eb30..a06261dd86f1 100644 --- a/components/shared/compositing/lib.rs +++ b/components/shared/compositing/lib.rs @@ -92,10 +92,6 @@ pub enum CompositorMsg { // sends a reply on the IpcSender, the constellation knows it's safe to // tear down the other threads associated with this pipeline. PipelineExited(PipelineId, IpcSender<()>), - /// Runs a closure in the compositor thread. - /// It's used to dispatch functions from webrender to the main thread's event loop. - /// Required to allow WGL GLContext sharing in Windows. - Dispatch(Box), /// Indicates to the compositor that it needs to record the time when the frame with /// the given ID (epoch) is painted and report it to the layout of the given /// pipeline ID. @@ -164,7 +160,6 @@ impl Debug for CompositorMsg { CompositorMsg::PipelineVisibilityChanged(..) => write!(f, "PipelineVisibilityChanged"), CompositorMsg::PipelineExited(..) => write!(f, "PipelineExited"), CompositorMsg::NewWebRenderFrameReady(..) => write!(f, "NewWebRenderFrameReady"), - CompositorMsg::Dispatch(..) => write!(f, "Dispatch"), CompositorMsg::PendingPaintMetric(..) => write!(f, "PendingPaintMetric"), CompositorMsg::LoadComplete(..) => write!(f, "LoadComplete"), CompositorMsg::WebDriverMouseButtonEvent(..) => write!(f, "WebDriverMouseButtonEvent"),