compositor: Wait for both Script and the Constellation when shutting down Pipelines (#37505)

Previously, the Constellation would immediately ask the Compositor to
shut down a pipeline, even before the ScriptThread finished shutting it
down. This meant that the Compositor might remove a Pipeline and then
re-add it if the ScriptThread sent a Pipeline-related message (such as a
new display list) in the meantime.

This change makes it so that the Compositor waits for both the
Constellation and the ScriptThread to finish shutting down a Pipeline
before removing its data. In addition, the Constellation no longer
synchronously waits on the Compositor when shutting down Pipelines. This
was important when the Compositor would talk to the ScriptThread
directly, but isn't necessary any longer.

Testing: This is very hard to test, because it depends on the creation
and destruction of many iframes and the particular timing of of all
the messaging between Servo bits. That said, this was tested manually
by observing the completion of Speedometer 2.1.
Fixes: #37458.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Oriol Brufau <obrufau@igalia.com>

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());

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) {

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 {
+        pipeline.send_exit_message_to_script(dbc);
-            ExitPipelineMode::Normal => pipeline.exit(dbc),
+
-            ExitPipelineMode::Force => pipeline.force_exit(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);
                     }
                 }
             }

components/constellation/pipeline.rs

@@ -385,41 +385,20 @@ impl Pipeline {
         pipeline
     }
 
-    /// A normal exit of the pipeline, which waits for the compositor,
+    /// Let the `ScriptThread` for this [`Pipeline`] know that it has exited. If the `ScriptThread` hasn't
-    /// and delegates layout shutdown to the script thread.
+    /// panicked and is still alive, it will send a `PipelineExited` message back to the `Constellation`
-    pub fn exit(&self, discard_bc: DiscardBrowsingContext) {
+    /// when it finishes cleaning up.
-        debug!("pipeline {:?} exiting", self.id);
+    pub fn send_exit_message_to_script(&self, discard_bc: DiscardBrowsingContext) {
-
+        debug!("{:?} Sending exit message to script", 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);
-            }
-        }
 
         // 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(error) = self.event_loop.send(ScriptThreadMessage::ExitPipeline(
-        if let Err(e) = self.event_loop.send(msg) {
+            self.webview_id,
-            warn!("Sending script exit message failed ({}).", e);
+            self.id,
-        }
+            discard_bc,
-    }
+        )) {
-
+            warn!("Sending script exit message failed ({error}).");
-    /// 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);
         }
     }
 

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),

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) => {
+            ScriptThreadMessage::ExitPipeline(
-                self.handle_exit_pipeline_msg(pipeline_id, discard_browsing_context, can_gc)
+                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();
+        let mut webview_and_pipeline_ids = Vec::new();
-        pipeline_ids.extend(
+        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 {
+        for (webview_id, pipeline_id) in webview_and_pipeline_ids {
-            self.handle_exit_pipeline_msg(pipeline_id, DiscardBrowsingContext::Yes, can_gc);
+            self.handle_exit_pipeline_msg(
+                webview_id,
+                pipeline_id,
+                DiscardBrowsingContext::Yes,
+                can_gc,
+            );
         }
 
         self.background_hang_monitor.unregister();

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.
+    /// Script or the Constellation is notifying the renderer that a Pipeline has finished
-    // This message acts as a synchronization point between the constellation,
+    /// shutting down. The renderer will not discard the Pipeline until both report that
-    // when it shuts down a pipeline, to the compositor; when the compositor
+    /// they have fully shut it down, to avoid recreating it due to any subsequent
-    // sends a reply on the IpcSender, the constellation knows it's safe to
+    /// messages.
-    // tear down the other threads associated with this pipeline.
+    PipelineExited(WebViewId, PipelineId, PipelineExitSource),
-    PipelineExited(WebViewId, PipelineId, IpcSender<()>),
     /// 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<ScreenGeometry>;
     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;
+    }
+}

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.