compositor: Handle synchronous messages while shutting down #31733
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -524,38 +524,38 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| } | ||
|
|
||
| fn handle_browser_message(&mut self, msg: CompositorMsg) -> bool { | ||
| 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; | ||
| }, | ||
| } | ||
|
|
||
| match msg { | ||
| CompositorMsg::ShutdownComplete => { | ||
| warn!("Received `ShutdownComplete` while not shutting down."); | ||
| self.finish_shutting_down(); | ||
| return false; | ||
| }, | ||
|
|
||
| CompositorMsg::ChangeRunningAnimationsState(pipeline_id, animation_state) => { | ||
| self.change_running_animations_state(pipeline_id, animation_state); | ||
| }, | ||
|
|
||
| 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) => { | ||
| self.touch_handler.on_event_processed(result); | ||
| }, | ||
|
|
||
| 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<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| } | ||
| }, | ||
|
|
||
| CompositorMsg::IsReadyToSaveImageReply(is_ready) => { | ||
| assert_eq!( | ||
| self.ready_to_save_state, | ||
| ReadyState::WaitingForConstellationReply | ||
|
|
@@ -579,24 +579,20 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| self.composite_if_necessary(CompositingReason::Headless); | ||
| }, | ||
|
|
||
| CompositorMsg::PipelineVisibilityChanged(pipeline_id, visible) => { | ||
| self.pipeline_details(pipeline_id).visible = visible; | ||
| self.process_animations(true); | ||
| }, | ||
|
|
||
| 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) => { | ||
| 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<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| } | ||
| }, | ||
|
|
||
| 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<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| } | ||
| }, | ||
|
|
||
| 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<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| }); | ||
| }, | ||
|
|
||
| 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) => { | ||
| self.pending_paint_metrics.insert(pipeline_id, epoch); | ||
| }, | ||
|
|
||
| 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) => { | ||
| if let Err(e) = req.send(self.embedder_coordinates.screen) { | ||
| warn!("Sending response to get screen size failed ({:?}).", e); | ||
| } | ||
| }, | ||
|
|
||
| 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<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| } | ||
| }, | ||
|
|
||
| CompositorMsg::Forwarded(msg) => { | ||
| self.handle_webrender_message(msg); | ||
| }, | ||
|
There was a problem hiding this comment. If this comment is still valid, I think it makes sense to copy it to the new location where messages are ignored while shutting down. There was a problem hiding this comment. I think this comment might be invalid now. It refers to |
||
| } | ||
|
|
||
| true | ||
|
|
@@ -917,6 +899,74 @@ impl<Window: WindowMethods + ?Sized> IOCompositor<Window> { | |
| } | ||
| } | ||
|
|
||
| /// 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; | ||
|
There was a problem hiding this comment. Note: the current code does this even after shutdown has completed, so there is a change of logic, but the change seems to make more sense. There was a problem hiding this comment. This is a really good point that I had overlooked. I agree that it doesn't make sense to keep subtracting one from the |
||
| }, | ||
| 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; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shutdown flow appears to be:
EmbedderEvent::Quithandled bymaybe_start_shutting_down. This makes sure the state is set toShutdownState::ShuttingDown, and sends the exit message to the constellation.CompositorMsg::ShutdownCompletewhen it is done shutting down, after a blocking exit call to the font cache thread.So receiving this message in the
ShutdownState::NotShuttingDownstate appears impossible, so I think should panic here, or at least error. It appears to me we don't get many issues based on errors, but we do get a lot of issues when servo crashes. So panicking appears to result in more action being taken to address the underlying problems, hence I prefer it. But I know you prefer to log an error so let's at least do that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this situation is highly unexpected, but in the end it's a non-fatal error as we are essentially about to quit anyway. This was handled with a warning before this change as well. Regarding crashing versus recovering from errors, I think that we cannot trust the messages that come from other threads / processes, as they may be experiencing runtime errors or be controlled by an attacker. Instead of immediately crashing, I think there is some benefit to attempting to properly handle when this happens. We should obviously be on top of any warnings printed during the execution of Servo, as they are only printed in very unexpected situations.
In my experience working on other browsers though, they are very complex pieces of software. If you turn on debug assertions, it's quite likely that you can hit one simply by browsing the web or running the tests. If the browser crashed every time that happened, it would be pretty unusable.