constellation: join on script-threads (#38424)

We currently send exit signals to script-threads, and we also join on
the BHM worker. This ensure the constellation shuts-down only after
script has dropped it's sender to the BHM worker. By joining on the
script-threads, we have a guarantee that they have exited(which is
stronger than having dropped their senders) by the time the
constellation exits.

Testing: Manually opened many tabs and closed the window, both in
single- and multi-process modes.
Fixes: Part of - https://github.com/servo/servo/issues/30849

---------

Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>

components/constellation/constellation.rs

@@ -461,6 +461,9 @@ pub struct Constellation<STF, SWF> {
 
     /// The async runtime.
     async_runtime: Box<dyn AsyncRuntime>,
+
+    /// When in single-process mode, join handles for script-threads.
+    script_join_handles: HashMap<WebViewId, JoinHandle<()>>,
 }
 
 /// State needed to construct a constellation.
@@ -711,6 +714,7 @@ where
                     user_content_manager: state.user_content_manager,
                     process_manager: ProcessManager::new(state.mem_profiler_chan),
                     async_runtime: state.async_runtime,
+                    script_join_handles: Default::default(),
                 };
 
                 constellation.run();
@@ -972,6 +976,10 @@ where
             self.background_monitor_control_senders.push(chan);
         }
 
+        if let Some(join_handle) = pipeline.join_handle {
+            self.script_join_handles.insert(webview_id, join_handle);
+        }
+
         if let Some(host) = host {
             debug!("{}: Adding new host entry {}", webview_id, host,);
             self.set_event_loop(
@@ -2523,12 +2531,20 @@ where
     fn handle_shutdown(&mut self) {
         debug!("Handling shutdown.");
 
+        // In single process mode, join on script-threads
+        // from webview which haven't been manually closed before.
+        for (_, join_handle) in self.script_join_handles.drain() {
+            if join_handle.join().is_err() {
+                error!("Failed to join on a script-thread.");
+            }
+        }
+
         // In single process mode, join on the background hang monitor worker thread.
         drop(self.background_monitor_register.take());
         if let Some(join_handle) = self.background_monitor_register_join_handle.take() {
-            join_handle
+            if join_handle.join().is_err() {
-                .join()
+                error!("Failed to join on the bhm background thread.");
-                .expect("Failed to join on the BHM background thread.");
+            }
         }
 
         // At this point, there are no active pipelines,
@@ -3053,6 +3069,11 @@ where
                 .remove(&browsing_context.bc_group_id);
         }
 
+        // Note: In single-process mode,
+        // if the webview is manually closed, we drop the join handle without joining on it.
+        // It is unlikely the thread will still run when the constellation shuts-down.
+        self.script_join_handles.remove(&webview_id);
+
         debug!("{webview_id}: Closed");
     }
 

components/constellation/pipeline.rs

@@ -209,6 +209,7 @@ pub struct NewPipeline {
     pub pipeline: Pipeline,
     pub bhm_control_chan: Option<IpcSender<BackgroundHangMonitorControlMsg>>,
     pub lifeline: Option<(IpcReceiver<()>, Process)>,
+    pub join_handle: Option<JoinHandle<()>>,
 }
 
 impl Pipeline {
@@ -218,7 +219,7 @@ impl Pipeline {
     ) -> Result<NewPipeline, Error> {
         // Note: we allow channel creation to panic, since recovering from this
         // probably requires a general low-memory strategy.
-        let (script_chan, (bhm_control_chan, lifeline)) = match state.event_loop {
+        let (script_chan, (bhm_control_chan, lifeline, join_handle)) = match state.event_loop {
             Some(script_chan) => {
                 let new_layout_info = NewLayoutInfo {
                     parent_info: state.parent_pipeline_id,
@@ -235,7 +236,7 @@ impl Pipeline {
                 {
                     warn!("Sending to script during pipeline creation failed ({})", e);
                 }
-                (script_chan, (None, None))
+                (script_chan, (None, None, None))
             },
             None => {
                 let (script_chan, script_port) = ipc::channel().expect("Pipeline script chan");
@@ -315,18 +316,18 @@ impl Pipeline {
                         ipc::channel().expect("Failed to create lifeline channel");
                     unprivileged_pipeline_content.lifeline_sender = Some(sender);
                     let process = unprivileged_pipeline_content.spawn_multiprocess()?;
-                    (Some(bhm_control_chan), Some((receiver, process)))
+                    (Some(bhm_control_chan), Some((receiver, process)), None)
                 } else {
                     // Should not be None in single-process mode.
                     let register = state
                         .background_monitor_register
                         .expect("Couldn't start content, no background monitor has been initiated");
-                    unprivileged_pipeline_content.start_all::<STF>(
+                    let join_handle = unprivileged_pipeline_content.start_all::<STF>(
                         false,
                         state.layout_factory,
                         register,
                     );
-                    (None, None)
+                    (None, None, Some(join_handle))
                 };
 
                 (EventLoop::new(script_chan), multiprocess_data)
@@ -347,6 +348,7 @@ impl Pipeline {
             pipeline,
             bhm_control_chan,
             lifeline,
+            join_handle,
         })
     }
 
@@ -501,7 +503,7 @@ impl UnprivilegedPipelineContent {
         wait_for_completion: bool,
         layout_factory: Arc<dyn LayoutFactory>,
         background_hang_monitor_register: Box<dyn BackgroundHangMonitorRegister>,
-    ) {
+    ) -> JoinHandle<()> {
         // Setup pipeline-namespace-installing for all threads in this process.
         // Idempotent in single-process mode.
         PipelineNamespace::set_installer_sender(self.namespace_request_sender);
@@ -511,7 +513,7 @@ impl UnprivilegedPipelineContent {
             self.rippy_data,
         ));
         let (content_process_shutdown_chan, content_process_shutdown_port) = unbounded();
-        STF::create(
+        let join_handle = STF::create(
             InitialScriptState {
                 id: self.id,
                 browsing_context_id: self.browsing_context_id,
@@ -551,6 +553,8 @@ impl UnprivilegedPipelineContent {
                 Err(_) => error!("Script-thread shut-down unexpectedly"),
             }
         }
+
+        join_handle
     }
 
     pub fn spawn_multiprocess(self) -> Result<Process, Error> {

components/script/script_thread.rs

@@ -25,7 +25,7 @@ use std::rc::Rc;
 use std::result::Result;
 use std::sync::Arc;
 use std::sync::atomic::{AtomicBool, Ordering};
-use std::thread;
+use std::thread::{self, JoinHandle};
 use std::time::{Duration, Instant, SystemTime};
 
 use background_hang_monitor_api::{
@@ -412,7 +412,7 @@ impl ScriptThreadFactory for ScriptThread {
         layout_factory: Arc<dyn LayoutFactory>,
         system_font_service: Arc<SystemFontServiceProxy>,
         load_data: LoadData,
-    ) {
+    ) -> JoinHandle<()> {
         thread::Builder::new()
             .name(format!("Script{:?}", state.id))
             .spawn(move || {
@@ -462,7 +462,7 @@ impl ScriptThreadFactory for ScriptThread {
                 // This must always be the very last operation performed before the thread completes
                 failsafe.neuter();
             })
-            .expect("Thread spawning failed");
+            .expect("Thread spawning failed")
     }
 }
 

components/servo/lib.rs

@@ -1265,7 +1265,7 @@ pub fn run_content_process(token: String) {
 
             content.register_system_memory_reporter();
 
-            content.start_all::<script::ScriptThread>(
+            let script_join_handle = content.start_all::<script::ScriptThread>(
                 true,
                 layout_factory,
                 background_hang_monitor_register,
@@ -1274,7 +1274,10 @@ pub fn run_content_process(token: String) {
             // Since wait_for_completion is true,
             // here we know that the script-thread
             // will exit(or already has),
-            // and so we can join on the BHM worker thread.
+            // and so we can join first on the script, and then on the BHM worker, threads.
+            script_join_handle
+                .join()
+                .expect("Failed to join on the script thread.");
             join_handle
                 .join()
                 .expect("Failed to join on the BHM background thread.");

components/shared/layout/lib.rs

@@ -15,6 +15,7 @@ use std::any::Any;
 use std::collections::HashMap;
 use std::sync::Arc;
 use std::sync::atomic::{AtomicIsize, AtomicU64, Ordering};
+use std::thread::JoinHandle;
 
 use app_units::Au;
 use atomic_refcell::AtomicRefCell;
@@ -305,7 +306,7 @@ pub trait ScriptThreadFactory {
         layout_factory: Arc<dyn LayoutFactory>,
         system_font_service: Arc<SystemFontServiceProxy>,
         load_data: LoadData,
-    );
+    ) -> JoinHandle<()>;
 }
 #[derive(Clone, Default)]
 pub struct OffsetParentResponse {