diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 538d63b9405..37b7bd2a6ba 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -20,7 +20,8 @@ use compositing_traits::display_list::{ }; use compositing_traits::rendering_context::RenderingContext; use compositing_traits::{ - CompositionPipeline, CompositorMsg, ImageUpdate, SendableFrameTree, WebViewTrait, + CompositionPipeline, CompositorMsg, ImageUpdate, PipelineExitSource, SendableFrameTree, + WebViewTrait, }; use constellation_traits::{EmbedderToConstellationMessage, PaintMetricEvent}; use crossbeam_channel::{Receiver, Sender}; @@ -226,6 +227,10 @@ pub(crate) struct PipelineDetails { /// The paint metric status of the first contentful paint. pub first_contentful_paint_metric: PaintMetricState, + + /// Which parts of Servo have reported that this `Pipeline` has exited. Only when all + /// have done so will it be discarded. + pub exited: PipelineExitSource, } impl PipelineDetails { @@ -255,6 +260,7 @@ impl PipelineDetails { scroll_tree: ScrollTree::default(), first_paint_metric: PaintMetricState::Waiting, first_contentful_paint_metric: PaintMetricState::Waiting, + exited: PipelineExitSource::empty(), } } @@ -604,15 +610,14 @@ impl IOCompositor { } }, - CompositorMsg::PipelineExited(webview_id, pipeline_id, sender) => { + CompositorMsg::PipelineExited(webview_id, pipeline_id, pipeline_exit_source) => { debug!( "Compositor got pipeline exited: {:?} {:?}", webview_id, pipeline_id ); if let Some(webview_renderer) = self.webview_renderers.get_mut(webview_id) { - webview_renderer.remove_pipeline(pipeline_id); + webview_renderer.pipeline_exited(pipeline_id, pipeline_exit_source); } - let _ = sender.send(()); }, CompositorMsg::NewWebRenderFrameReady(_document_id, recomposite_needed) => { @@ -987,15 +992,14 @@ impl IOCompositor { /// compositor no longer does any WebRender frame generation. fn handle_browser_message_while_shutting_down(&mut self, msg: CompositorMsg) { match msg { - CompositorMsg::PipelineExited(webview_id, pipeline_id, sender) => { + CompositorMsg::PipelineExited(webview_id, pipeline_id, pipeline_exit_source) => { debug!( "Compositor got pipeline exited: {:?} {:?}", webview_id, pipeline_id ); if let Some(webview_renderer) = self.webview_renderers.get_mut(webview_id) { - webview_renderer.remove_pipeline(pipeline_id); + webview_renderer.pipeline_exited(pipeline_id, pipeline_exit_source); } - let _ = sender.send(()); }, CompositorMsg::GenerateImageKey(sender) => { let _ = sender.send(self.global.borrow().webrender_api.generate_image_key()); diff --git a/components/compositing/webview_renderer.rs b/components/compositing/webview_renderer.rs index 448117ffc34..64045712b0b 100644 --- a/components/compositing/webview_renderer.rs +++ b/components/compositing/webview_renderer.rs @@ -3,7 +3,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use std::cell::{Cell, RefCell}; -use std::collections::hash_map::Keys; +use std::collections::hash_map::{Entry, Keys}; use std::collections::{HashMap, VecDeque}; use std::rc::Rc; @@ -12,7 +12,7 @@ use compositing_traits::display_list::ScrollType; use compositing_traits::viewport_description::{ DEFAULT_ZOOM, MAX_ZOOM, MIN_ZOOM, ViewportDescription, }; -use compositing_traits::{SendableFrameTree, WebViewTrait}; +use compositing_traits::{PipelineExitSource, SendableFrameTree, WebViewTrait}; use constellation_traits::{EmbedderToConstellationMessage, WindowSizeType}; use embedder_traits::{ AnimationState, CompositorHitTestResult, InputEvent, MouseButton, MouseButtonAction, @@ -175,12 +175,26 @@ impl WebViewRenderer { }) } - pub(crate) fn remove_pipeline(&mut self, pipeline_id: PipelineId) { + pub(crate) fn pipeline_exited(&mut self, pipeline_id: PipelineId, source: PipelineExitSource) { + let pipeline = self.pipelines.entry(pipeline_id); + let Entry::Occupied(mut pipeline) = pipeline else { + return; + }; + + pipeline.get_mut().exited.insert(source); + + // Do not remove pipeline details until both the Constellation and Script have + // finished processing the pipeline shutdown. This prevents any followup messges + // from re-adding the pipeline details and creating a zombie. + if !pipeline.get().exited.is_all() { + return; + } + + pipeline.remove_entry(); self.global .borrow_mut() .pipeline_to_webview_map .remove(&pipeline_id); - self.pipelines.remove(&pipeline_id); } pub(crate) fn set_frame_tree(&mut self, frame_tree: &SendableFrameTree) { diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 8013c04d689..0513492eee3 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -109,7 +109,8 @@ use canvas_traits::ConstellationCanvasMsg; use canvas_traits::canvas::{CanvasId, CanvasMsg}; use canvas_traits::webgl::WebGLThreads; use compositing_traits::{ - CompositorMsg, CompositorProxy, SendableFrameTree, WebrenderExternalImageRegistry, + CompositorMsg, CompositorProxy, PipelineExitSource, SendableFrameTree, + WebrenderExternalImageRegistry, }; use constellation_traits::{ AuxiliaryWebViewCreationRequest, AuxiliaryWebViewCreationResponse, BroadcastMsg, DocumentState, @@ -2840,7 +2841,18 @@ where fn handle_pipeline_exited(&mut self, pipeline_id: PipelineId) { debug!("{}: Exited", pipeline_id); - self.pipelines.remove(&pipeline_id); + let Some(pipeline) = self.pipelines.remove(&pipeline_id) else { + return; + }; + + // Now that the Script and Constellation parts of Servo no longer have a reference to + // this pipeline, tell the compositor that it has shut down. This is delayed until the + // last moment. + self.compositor_proxy.send(CompositorMsg::PipelineExited( + pipeline.webview_id, + pipeline.id, + PipelineExitSource::Constellation, + )); } #[cfg_attr( @@ -5910,10 +5922,8 @@ where } // Inform script, compositor that this pipeline has exited. - match exit_mode { - ExitPipelineMode::Normal => pipeline.exit(dbc), - ExitPipelineMode::Force => pipeline.force_exit(dbc), - } + pipeline.send_exit_message_to_script(dbc); + debug!("{}: Closed", pipeline_id); } @@ -5952,7 +5962,7 @@ where // Note that we deliberately do not do any of the tidying up // associated with closing a pipeline. The constellation should cope! warn!("{}: Randomly closing pipeline", pipeline_id); - pipeline.force_exit(DiscardBrowsingContext::No); + pipeline.send_exit_message_to_script(DiscardBrowsingContext::No); } } } diff --git a/components/constellation/pipeline.rs b/components/constellation/pipeline.rs index a0bf106ea06..53a7560ed1f 100644 --- a/components/constellation/pipeline.rs +++ b/components/constellation/pipeline.rs @@ -385,41 +385,20 @@ impl Pipeline { pipeline } - /// A normal exit of the pipeline, which waits for the compositor, - /// and delegates layout shutdown to the script thread. - pub fn exit(&self, discard_bc: DiscardBrowsingContext) { - debug!("pipeline {:?} exiting", self.id); - - // The compositor wants to know when pipelines shut down too. - // It may still have messages to process from these other threads - // before they can be safely shut down. - // It's OK for the constellation to block on the compositor, - // since the compositor never blocks on the constellation. - if let Ok((sender, receiver)) = ipc::channel() { - self.compositor_proxy.send(CompositorMsg::PipelineExited( - self.webview_id, - self.id, - sender, - )); - if let Err(e) = receiver.recv() { - warn!("Sending exit message failed ({:?}).", e); - } - } + /// Let the `ScriptThread` for this [`Pipeline`] know that it has exited. If the `ScriptThread` hasn't + /// panicked and is still alive, it will send a `PipelineExited` message back to the `Constellation` + /// when it finishes cleaning up. + pub fn send_exit_message_to_script(&self, discard_bc: DiscardBrowsingContext) { + debug!("{:?} Sending exit message to script", self.id); // Script thread handles shutting down layout, and layout handles shutting down the painter. // For now, if the script thread has failed, we give up on clean shutdown. - let msg = ScriptThreadMessage::ExitPipeline(self.id, discard_bc); - if let Err(e) = self.event_loop.send(msg) { - warn!("Sending script exit message failed ({}).", e); - } - } - - /// A forced exit of the shutdown, which does not wait for the compositor, - /// or for the script thread to shut down layout. - pub fn force_exit(&self, discard_bc: DiscardBrowsingContext) { - let msg = ScriptThreadMessage::ExitPipeline(self.id, discard_bc); - if let Err(e) = self.event_loop.send(msg) { - warn!("Sending script exit message failed ({}).", e); + if let Err(error) = self.event_loop.send(ScriptThreadMessage::ExitPipeline( + self.webview_id, + self.id, + discard_bc, + )) { + warn!("Sending script exit message failed ({error})."); } } diff --git a/components/script/messaging.rs b/components/script/messaging.rs index 41465796505..a3d0e0046f1 100644 --- a/components/script/messaging.rs +++ b/components/script/messaging.rs @@ -58,7 +58,7 @@ impl MixedMessage { ScriptThreadMessage::ThemeChange(id, ..) => Some(*id), ScriptThreadMessage::ResizeInactive(id, ..) => Some(*id), ScriptThreadMessage::UnloadDocument(id) => Some(*id), - ScriptThreadMessage::ExitPipeline(id, ..) => Some(*id), + ScriptThreadMessage::ExitPipeline(_webview_id, id, ..) => Some(*id), ScriptThreadMessage::ExitScriptThread => None, ScriptThreadMessage::SendInputEvent(id, ..) => Some(*id), ScriptThreadMessage::Viewport(id, ..) => Some(*id), diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 6418506ef96..37130701967 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -36,7 +36,7 @@ use base::cross_process_instant::CrossProcessInstant; use base::id::{BrowsingContextId, HistoryStateId, PipelineId, PipelineNamespace, WebViewId}; use canvas_traits::webgl::WebGLPipeline; use chrono::{DateTime, Local}; -use compositing_traits::CrossProcessCompositorApi; +use compositing_traits::{CompositorMsg, CrossProcessCompositorApi, PipelineExitSource}; use constellation_traits::{ JsEvalResult, LoadData, LoadOrigin, NavigationHistoryBehavior, ScriptToConstellationChan, ScriptToConstellationMessage, StructuredSerializedData, WindowSizeType, @@ -1605,10 +1605,12 @@ impl ScriptThread { return false; }, MixedMessage::FromConstellation(ScriptThreadMessage::ExitPipeline( + webview_id, pipeline_id, discard_browsing_context, )) => { self.handle_exit_pipeline_msg( + webview_id, pipeline_id, discard_browsing_context, can_gc, @@ -1970,9 +1972,16 @@ impl ScriptThread { self.handle_css_error_reporting(pipeline_id, filename, line, column, msg) }, ScriptThreadMessage::Reload(pipeline_id) => self.handle_reload(pipeline_id, can_gc), - ScriptThreadMessage::ExitPipeline(pipeline_id, discard_browsing_context) => { - self.handle_exit_pipeline_msg(pipeline_id, discard_browsing_context, can_gc) - }, + ScriptThreadMessage::ExitPipeline( + webview_id, + pipeline_id, + discard_browsing_context, + ) => self.handle_exit_pipeline_msg( + webview_id, + pipeline_id, + discard_browsing_context, + can_gc, + ), ScriptThreadMessage::PaintMetric( pipeline_id, metric_type, @@ -2975,6 +2984,7 @@ impl ScriptThread { /// Handles a request to exit a pipeline and shut down layout. fn handle_exit_pipeline_msg( &self, + webview_id: WebViewId, id: PipelineId, discard_bc: DiscardBrowsingContext, can_gc: CanGc, @@ -3032,6 +3042,15 @@ impl ScriptThread { .send((id, ScriptToConstellationMessage::PipelineExited)) .ok(); + self.compositor_api + .sender() + .send(CompositorMsg::PipelineExited( + webview_id, + id, + PipelineExitSource::Script, + )) + .ok(); + debug!("{id}: Finished pipeline exit"); } @@ -3039,24 +3058,29 @@ impl ScriptThread { fn handle_exit_script_thread_msg(&self, can_gc: CanGc) { debug!("Exiting script thread."); - let mut pipeline_ids = Vec::new(); - pipeline_ids.extend( + let mut webview_and_pipeline_ids = Vec::new(); + webview_and_pipeline_ids.extend( self.incomplete_loads .borrow() .iter() .next() - .map(|load| load.pipeline_id), + .map(|load| (load.webview_id, load.pipeline_id)), ); - pipeline_ids.extend( + webview_and_pipeline_ids.extend( self.documents .borrow() .iter() .next() - .map(|(pipeline_id, _)| pipeline_id), + .map(|(pipeline_id, document)| (document.webview_id(), pipeline_id)), ); - for pipeline_id in pipeline_ids { - self.handle_exit_pipeline_msg(pipeline_id, DiscardBrowsingContext::Yes, can_gc); + for (webview_id, pipeline_id) in webview_and_pipeline_ids { + self.handle_exit_pipeline_msg( + webview_id, + pipeline_id, + DiscardBrowsingContext::Yes, + can_gc, + ); } self.background_hang_monitor.unregister(); diff --git a/components/shared/compositing/lib.rs b/components/shared/compositing/lib.rs index 6685ecbdbf9..d6bf3ce2c70 100644 --- a/components/shared/compositing/lib.rs +++ b/components/shared/compositing/lib.rs @@ -28,6 +28,7 @@ pub mod viewport_description; use std::collections::HashMap; use std::sync::{Arc, Mutex}; +use bitflags::bitflags; use display_list::CompositorDisplayListInfo; use embedder_traits::{CompositorHitTestResult, ScreenGeometry}; use euclid::default::Size2D as UntypedSize2D; @@ -96,12 +97,11 @@ pub enum CompositorMsg { /// the frame is ready. It contains a bool to indicate if it needs to composite and the /// `DocumentId` of the new frame. NewWebRenderFrameReady(DocumentId, bool), - /// A pipeline was shut down. - // This message acts as a synchronization point between the constellation, - // when it shuts down a pipeline, to the compositor; when the compositor - // sends a reply on the IpcSender, the constellation knows it's safe to - // tear down the other threads associated with this pipeline. - PipelineExited(WebViewId, PipelineId, IpcSender<()>), + /// Script or the Constellation is notifying the renderer that a Pipeline has finished + /// shutting down. The renderer will not discard the Pipeline until both report that + /// they have fully shut it down, to avoid recreating it due to any subsequent + /// messages. + PipelineExited(WebViewId, PipelineId, PipelineExitSource), /// The load of a page has completed LoadComplete(WebViewId), /// WebDriver mouse button event @@ -562,3 +562,15 @@ pub trait WebViewTrait { fn screen_geometry(&self) -> Option; fn set_animating(&self, new_value: bool); } + +/// What entity is reporting that a `Pipeline` has exited. Only when all have +/// done this will the renderer discard its details. +#[derive(Clone, Copy, Default, Deserialize, PartialEq, Serialize)] +pub struct PipelineExitSource(u8); + +bitflags! { + impl PipelineExitSource: u8 { + const Script = 1 << 0; + const Constellation = 1 << 1; + } +} diff --git a/components/shared/script/lib.rs b/components/shared/script/lib.rs index 069ca2f6a30..3086d10bc88 100644 --- a/components/shared/script/lib.rs +++ b/components/shared/script/lib.rs @@ -140,7 +140,7 @@ pub enum ScriptThreadMessage { /// Notifies the script that the document associated with this pipeline should 'unload'. UnloadDocument(PipelineId), /// Notifies the script that a pipeline should be closed. - ExitPipeline(PipelineId, DiscardBrowsingContext), + ExitPipeline(WebViewId, PipelineId, DiscardBrowsingContext), /// Notifies the script that the whole thread should be closed. ExitScriptThread, /// Sends a DOM event.