fix(bhm): deliver exit signal reliably when component registration and signal submission coincide

> There's a race condition between the reception of
> `BackgroundHangMonitorControlMsg::Exit` and `MonitoredComponentMsg::
> Register`. When the worker receives `Exit`, it stops receiving
> messages, and any remaining messages (including the
> `MonitoredComponentMsg::Register` we sent) in the channel are dropped.
> Wrapping `exit_signal` with this RAII wrapper ensures the exit signal
> is delivered even in such cases.

This should (hopefully) eliminate the intermittent hang-ups in the test
case `test_hang_monitoring_exit_signal` for good.

2 files changed, 119 insertions, 30 deletions

diff --git a/components/background_hang_monitor/background_hang_monitor.rs b/components/background_hang_monitor/background_hang_monitor.rs
index fb51df811a4..7d06f61f633 100644
--- a/components/background_hang_monitor/background_hang_monitor.rs
+++ b/components/background_hang_monitor/background_hang_monitor.rs
@@ -44,6 +44,11 @@ impl HangMonitorRegister {
                 while monitor.run() {
                     // Monitoring until all senders have been dropped...
                 }
+
+                // Suppress `signal_to_exit`
+                for (_, mut component) in monitor.monitored_components.drain() {
+                    component.exit_signal.release();
+                }
             })
             .expect("Couldn't start BHM worker.");
         Box::new(HangMonitorRegister {
@@ -85,6 +90,15 @@ impl BackgroundHangMonitorRegister for HangMonitorRegister {
         #[cfg(any(target_os = "android", target_arch = "arm", target_arch = "aarch64"))]
         let sampler = crate::sampler::DummySampler::new();
 
+        // There's a race condition between the reception of
+        // `BackgroundHangMonitorControlMsg::Exit` and
+        // `MonitoredComponentMsg::Register`. When the worker receives `Exit`,
+        // it stops receiving messages, and any remaining messages (including
+        // the `MonitoredComponentMsg::Register` we sent) in the channel are
+        // dropped. Wrapping `exit_signal` with this RAII wrapper ensures the
+        // exit signal is delivered even in such cases.
+        let exit_signal = SignalToExitOnDrop(exit_signal);
+
         bhm_chan.send(MonitoredComponentMsg::Register(
             sampler,
             thread::current().name().map(str::to_owned),
@@ -110,7 +124,7 @@ enum MonitoredComponentMsg {
         Option<String>,
         Duration,
         Duration,
-        Option<Box<dyn BackgroundHangMonitorExitSignal>>,
+        SignalToExitOnDrop,
     ),
     /// Unregister component for monitoring.
     Unregister,
@@ -174,6 +188,33 @@ impl BackgroundHangMonitor for BackgroundHangMonitorChan {
     }
 }
 
+/// Wraps [`BackgroundHangMonitorExitSignal`] and calls `signal_to_exit` when
+/// dropped.
+struct SignalToExitOnDrop(Option<Box<dyn BackgroundHangMonitorExitSignal>>);
+
+impl SignalToExitOnDrop {
+    /// Call `BackgroundHangMonitorExitSignal::signal_to_exit` now.
+    fn signal_to_exit(&mut self) {
+        if let Some(signal) = self.0.take() {
+            signal.signal_to_exit();
+        }
+    }
+
+    /// Disassociate `BackgroundHangMonitorExitSignal` from itself, preventing
+    /// `BackgroundHangMonitorExitSignal::signal_to_exit` from being called in
+    /// the future.
+    fn release(&mut self) {
+        self.0 = None;
+    }
+}
+
+impl Drop for SignalToExitOnDrop {
+    #[inline]
+    fn drop(&mut self) {
+        self.signal_to_exit();
+    }
+}
+
 struct MonitoredComponent {
     sampler: Box<dyn Sampler>,
     last_activity: Instant,
@@ -183,7 +224,7 @@ struct MonitoredComponent {
     sent_transient_alert: bool,
     sent_permanent_alert: bool,
     is_waiting: bool,
-    exit_signal: Option<Box<dyn BackgroundHangMonitorExitSignal>>,
+    exit_signal: SignalToExitOnDrop,
 }
 
 struct Sample(MonitoredComponentId, Instant, NativeStack);
@@ -306,10 +347,8 @@ impl BackgroundHangMonitorWorker {
                         return true;
                     },
                     Ok(BackgroundHangMonitorControlMsg::Exit(sender)) => {
-                        for component in self.monitored_components.values() {
-                            if let Some(signal) = component.exit_signal.as_ref() {
-                                signal.signal_to_exit();
-                            }
+                        for component in self.monitored_components.values_mut() {
+                            component.exit_signal.signal_to_exit();
                         }
 
                         // Confirm exit with to the constellation.
@@ -379,10 +418,13 @@ impl BackgroundHangMonitorWorker {
                 );
             },
             (component_id, MonitoredComponentMsg::Unregister) => {
-                let _ = self
+                let (_, mut component) = self
                     .monitored_components
                     .remove_entry(&component_id)
                     .expect("Received Unregister for an unknown component");
+
+                // Prevent `signal_to_exit` from being called
+                component.exit_signal.release();
             },
             (component_id, MonitoredComponentMsg::NotifyActivity(annotation)) => {
                 let component = self
diff --git a/components/background_hang_monitor/tests/hang_monitor_tests.rs b/components/background_hang_monitor/tests/hang_monitor_tests.rs
index 23b32dd1bbb..b92344039bd 100644
--- a/components/background_hang_monitor/tests/hang_monitor_tests.rs
+++ b/components/background_hang_monitor/tests/hang_monitor_tests.rs
@@ -164,10 +164,45 @@ fn test_hang_monitoring_unregister() {
     assert!(background_hang_monitor_receiver.try_recv().is_err());
 }
 
+// Perform two certain steps in `test_hang_monitoring_exit_signal_inner` in
+// different orders to check for the race condition that
+// caused <https://github.com/servo/servo/issues/28270> and
+// <https://github.com/servo/servo/issues/27191>.
 #[test]
-// https://github.com/servo/servo/issues/28270
-#[cfg(not(any(target_os = "windows", target_os = "macos")))]
-fn test_hang_monitoring_exit_signal() {
+fn test_hang_monitoring_exit_signal1() {
+    test_hang_monitoring_exit_signal_inner(|e1, e2| {
+        e1();
+        thread::sleep(Duration::from_millis(100));
+        e2();
+    });
+}
+
+#[test]
+fn test_hang_monitoring_exit_signal2() {
+    test_hang_monitoring_exit_signal_inner(|e1, e2| {
+        e1();
+        e2();
+    });
+}
+
+#[test]
+fn test_hang_monitoring_exit_signal3() {
+    test_hang_monitoring_exit_signal_inner(|e1, e2| {
+        e2();
+        e1();
+    });
+}
+
+#[test]
+fn test_hang_monitoring_exit_signal4() {
+    test_hang_monitoring_exit_signal_inner(|e1, e2| {
+        e2();
+        thread::sleep(Duration::from_millis(100));
+        e1();
+    });
+}
+
+fn test_hang_monitoring_exit_signal_inner(op_order: fn(&mut dyn FnMut(), &mut dyn FnMut())) {
     let _lock = SERIAL.lock().unwrap();
 
     let (background_hang_monitor_ipc_sender, _background_hang_monitor_receiver) =
@@ -185,9 +220,9 @@ fn test_hang_monitoring_exit_signal() {
     }
 
     let closing = Arc::new(AtomicBool::new(false));
-    let signal = BHMExitSignal {
+    let mut signal = Some(Box::new(BHMExitSignal {
         closing: closing.clone(),
-    };
+    }));
 
     // Init a worker, without active monitoring.
     let background_hang_monitor_register = HangMonitorRegister::init(
@@ -195,26 +230,38 @@ fn test_hang_monitoring_exit_signal() {
         control_receiver,
         false,
     );
-    let _background_hang_monitor = background_hang_monitor_register.register_component(
-        MonitoredComponentId(TEST_PIPELINE_ID, MonitoredComponentType::Script),
-        Duration::from_millis(10),
-        Duration::from_millis(1000),
-        Some(Box::new(signal)),
-    );
 
+    let mut background_hang_monitor = None;
     let (exit_sender, exit_receiver) = ipc::channel().expect("Failed to create IPC channel!");
+    let mut exit_sender = Some(exit_sender);
+
+    // `op_order` determines the order in which these two closures are
+    // executed.
+    op_order(
+        &mut || {
+            // Register a component.
+            background_hang_monitor = Some(background_hang_monitor_register.register_component(
+                MonitoredComponentId(TEST_PIPELINE_ID, MonitoredComponentType::Script),
+                Duration::from_millis(10),
+                Duration::from_millis(1000),
+                Some(signal.take().unwrap()),
+            ));
+        },
+        &mut || {
+            // Send the exit message.
+            control_sender
+                .send(BackgroundHangMonitorControlMsg::Exit(
+                    exit_sender.take().unwrap(),
+                ))
+                .unwrap();
+        },
+    );
 
-    // Send the exit message.
-    if control_sender
-        .send(BackgroundHangMonitorControlMsg::Exit(exit_sender))
-        .is_ok()
-    {
-        // Assert we receive a confirmation back.
-        assert!(exit_receiver.recv().is_ok());
-
-        // Assert we get the exit signal.
-        while !closing.load(Ordering::SeqCst) {
-            thread::sleep(Duration::from_millis(10));
-        }
+    // Assert we receive a confirmation back.
+    assert!(exit_receiver.recv().is_ok());
+
+    // Assert we get the exit signal.
+    while !closing.load(Ordering::SeqCst) {
+        thread::sleep(Duration::from_millis(10));
     }
 }