Bug 2067218 - Add a mutex to avoid duplicate helpers for a profile. r=aborondo,nrishel

temp: guard test

Differential Revision: https://phabricator.services.mozilla.com/D322655

toolkit/mozapps/pushnotificationhelper/src/lifecycle.rs

@@ -2,14 +2,18 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-//! The named Windows event that shuts a helper down.
+//! The two named Windows objects that govern a helper's lifetime.
 //!
 //! [`StopEvent`] is a manual-reset event named after a profile and an
 //! installation. A helper serves exactly one profile, so signalling the event
-//! asks that profile's helper, and only it, to exit.
+//! asks that profile's helper, and only it, to exit. Manual reset means a
+//! signal landing before the helper reaches its wait is still seen.
 //!
-//! Manual reset means a signal landing before the helper reaches its wait is
-//! still seen rather than missed.
+//! [`ProfileGuard`] is a mutex named after the same profile. Only the first
+//! helper to serve a profile takes it.
+//!
+//! Note: currently, the helpers are not revived on system restarts. Firefox needs to
+//! readd these helpers on first launch for the system login session.
 
 use std::ffi::OsStr;
 use std::hash::Hasher;
@@ -17,9 +21,12 @@ use std::os::windows::ffi::OsStrExt;
 use std::path::Path;
 
 use fnv::FnvHasher;
-use windows_sys::Win32::Foundation::{CloseHandle, GetLastError, HANDLE, WAIT_OBJECT_0};
+use windows_sys::Win32::Foundation::{
+    CloseHandle, GetLastError, ERROR_ALREADY_EXISTS, HANDLE, WAIT_OBJECT_0,
+};
 use windows_sys::Win32::System::Threading::{
-    CreateEventW, OpenEventW, SetEvent, WaitForSingleObject, EVENT_MODIFY_STATE, INFINITE,
+    CreateEventW, CreateMutexW, OpenEventW, SetEvent, WaitForSingleObject, EVENT_MODIFY_STATE,
+    INFINITE,
 };
 
 /// `Local\` scopes this to the caller's logon session. The prefix is case sensitive.
@@ -28,9 +35,12 @@ const PREFIX: &str = "Local\\MozillaNotificationHelper";
 
 struct OwnedHandle(HANDLE);
 
-// SAFETY: Win32 handles belong to the process rather than to a thread, so an
-// event handle can be waited on and signalled from any of them. OwnedHandle
-// owns its handle, so nothing else closes it out from under that thread.
+// SAFETY: Win32 handles belong to the process rather than to a thread, so any
+// of them can use and close one. OwnedHandle owns its handle, so nothing else
+// closes it out from under that thread.
+//
+// This moves the handle, not the kernel object's ownership: a mutex stays owned
+// by the thread that acquired it, wherever its handle ends up.
 unsafe impl Send for OwnedHandle {}
 
 impl OwnedHandle {
@@ -47,12 +57,42 @@ impl OwnedHandle {
 impl Drop for OwnedHandle {
     fn drop(&mut self) {
         // SAFETY: OwnedHandle::new rejects a null handle, so self.0 is one that
-        // CreateEventW or OpenEventW returned and that nothing has closed yet.
-        // drop runs once, so it is closed exactly once.
+        // CreateEventW, OpenEventW or CreateMutexW returned and that nothing
+        // has closed yet. drop runs once, so it is closed exactly once.
         unsafe { CloseHandle(self.0) };
     }
 }
 
+/// Ensures that there is only one helper for each profile.
+///
+/// The mutex is never released explicitly: a helper holds it until the process
+/// ends, and Windows drops an abandoned mutex when its last handle closes.
+pub struct ProfileGuard {
+    _handle: OwnedHandle,
+}
+
+impl ProfileGuard {
+    /// Returns `None` when a helper is already serving `profile`, including one
+    /// left running by an earlier Firefox session.
+    /// https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-createmutexw
+    pub fn acquire(profile: &Path) -> Result<Option<Self>, String> {
+        let name = wide(&object_name("profile", profile)?);
+
+        // SAFETY: A null security descriptor asks for the default one, and
+        // `name` is NUL terminated by `wide` and outlives the call.
+        let handle = unsafe { CreateMutexW(std::ptr::null(), 1, name.as_ptr()) };
+        let handle = OwnedHandle::new(handle)
+            .ok_or_else(|| format!("CreateMutexW failed: {}", last_error()))?;
+
+        if last_error() == ERROR_ALREADY_EXISTS {
+            // Dropping `handle` closes it, leaving the mutex to its owner.
+            return Ok(None);
+        }
+
+        Ok(Some(ProfileGuard { _handle: handle }))
+    }
+}
+
 /// The shutdown channel for one profile's helper.
 pub struct StopEvent {
     handle: OwnedHandle,
@@ -98,7 +138,8 @@ pub fn signal(profile: &Path) -> Result<(), String> {
     };
 
     // SAFETY: `handle` is still open, nothing having closed it since
-    // OpenEventW, and that call requested the EVENT_MODIFY_STATE
+    // OpenEventW, and that call requested the EVENT_MODIFY_STATE right that
+    // SetEvent requires.
     if unsafe { SetEvent(handle.get()) } == 0 {
         return Err(format!("SetEvent failed: {}", last_error()));
     }
@@ -168,6 +209,18 @@ mod tests {
         );
     }
 
+    /// A mutex and an event sharing a name would collide in the one namespace,
+    /// so the kind has to reach the hash.
+    #[test]
+    fn a_guard_never_collides_with_a_stop_event() {
+        let profile = Path::new(r"c:\profiles\kinds");
+
+        assert_ne!(
+            object_name("profile", profile).unwrap(),
+            object_name("stop", profile).unwrap()
+        );
+    }
+
     #[test]
     fn signal_wakes_a_waiting_helper() {
         let profile = Path::new(r"c:\profiles\signal-wakes");
@@ -203,4 +256,31 @@ mod tests {
         signal(mine).unwrap();
         waiter.join().unwrap().unwrap();
     }
+
+    #[test]
+    fn a_second_helper_is_refused_then_allowed_once_the_first_exits() {
+        let profile = Path::new(r"c:\profiles\guard-test");
+
+        let first = ProfileGuard::acquire(profile).unwrap();
+        assert!(first.is_some(), "the first helper takes the guard");
+        assert!(
+            ProfileGuard::acquire(profile).unwrap().is_none(),
+            "a second helper is refused while the first holds it"
+        );
+
+        drop(first);
+
+        assert!(
+            ProfileGuard::acquire(profile).unwrap().is_some(),
+            "the guard is released when its owner exits"
+        );
+    }
+
+    #[test]
+    fn separate_profiles_do_not_block_each_other() {
+        let one = ProfileGuard::acquire(Path::new(r"c:\profiles\one")).unwrap();
+        let two = ProfileGuard::acquire(Path::new(r"c:\profiles\two")).unwrap();
+
+        assert!(one.is_some() && two.is_some());
+    }
 }

toolkit/mozapps/pushnotificationhelper/src/main.rs

@@ -55,9 +55,19 @@ fn check_profile(path: &Path) -> Result<(), String> {
 /// Starts the notification work for `profile`, parks until another process asks
 /// it to stop, then tears the work down.
 fn run(profile: &Path) -> ExitCode {
-    // TODO: nothing stops a second helper starting for a profile that already
-    // has one, so every Firefox restart while enabled stacks another. The
-    // per-profile guard that fixes this lands separately.
+    // Ensure that only one helper exists per profile. Held until the process exits.
+    let _guard = match lifecycle::ProfileGuard::acquire(profile) {
+        Ok(Some(guard)) => guard,
+        Ok(None) => {
+            // A helper is already serving this profile. Leave it alone.
+            return ExitCode::SUCCESS;
+        }
+        Err(message) => {
+            eprintln!("{PROGRAM}: {message}");
+            return ExitCode::FAILURE;
+        }
+    };
+
     let stop = match lifecycle::StopEvent::open(profile) {
         Ok(stop) => stop,
         Err(message) => {
@@ -172,4 +182,15 @@ mod tests {
     fn stop_takes_no_value() {
         assert!(Args::try_parse_from([PROGRAM, "--stop=yes", "--profile", r"c:\p"]).is_err());
     }
+
+    /// Firefox starts a helper on every launch, so finding one already running
+    /// is the ordinary case rather than a failure.
+    #[test]
+    fn run_declines_quietly_when_a_helper_already_has_the_profile() {
+        let profile = Path::new(r"c:\profiles\run-declines");
+
+        let _held = lifecycle::ProfileGuard::acquire(profile).unwrap().unwrap();
+
+        assert_eq!(run(profile), ExitCode::SUCCESS);
+    }
 }