Bug 1443853 - Move browser process shutdown monitor to mozrunner. r=jgraham
authorAndreas Tolfsen <ato@sny.no>
Wed, 07 Mar 2018 21:57:53 +0000
changeset 462564 a5d22f442915eaeca18002aa555aa22936f6c855
parent 462563 2da49990b24780b797b7371ed9d54acc7641d20f
child 462565 6d0d32f6570d875ae9191c07be0355e9083114e8
child 462571 c5e091682eb164417dea678551c968cb0f6da792
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjgraham
bugs1443853
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1443853 - Move browser process shutdown monitor to mozrunner. r=jgraham This moves the shutdown monitor for the Firefox process from geckodriver to mozrunner, which is a more suitable home for it. We will likely need specialised versions of this in the future with products such as GeckoView and Fennec. In addition to the move it also cleans up the polling loop by employing std::time::SystemTime which lets us match on the elapsed time since its construction. This seems nicer than having to perform division operations on integers, which in Rust are inherently unsafe (there is no guard against SIGFPE). This change should be functionally equivalent to the existing code. MozReview-Commit-ID: 1asnFbixhcY
testing/geckodriver/src/marionette.rs
testing/mozbase/rust/mozrunner/src/runner.rs
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -12,18 +12,18 @@ use std::error::Error;
 use std::fs::File;
 use std::io::Error as IoError;
 use std::io::ErrorKind;
 use std::io::prelude::*;
 use std::path::PathBuf;
 use std::io::Result as IoResult;
 use std::net::{TcpListener, TcpStream};
 use std::sync::Mutex;
-use std::thread::sleep;
-use std::time::Duration;
+use std::thread;
+use std::time;
 use uuid::Uuid;
 use webdriver::capabilities::CapabilitiesMatching;
 use webdriver::command::{WebDriverCommand, WebDriverMessage, Parameters,
                          WebDriverExtensionCommand};
 use webdriver::command::WebDriverCommand::{
     NewSession, DeleteSession, Status, Get, GetCurrentUrl,
     GoBack, GoForward, Refresh, GetTitle, GetPageSource, GetWindowHandle,
     GetWindowHandles, CloseWindow, SetWindowRect, GetWindowRect,
@@ -51,20 +51,16 @@ use webdriver::server::{WebDriverHandler
 use webdriver::httpapi::{WebDriverExtensionRoute};
 
 use capabilities::{FirefoxCapabilities, FirefoxOptions};
 use logging;
 use prefs;
 
 const DEFAULT_HOST: &'static str = "localhost";
 
-// Firefox' integrated background monitor which observes long running threads during
-// shutdown kills those after 65s. Wait some additional seconds for a safe shutdown
-const TIMEOUT_BROWSER_SHUTDOWN: u64 = 70 * 1000;
-
 pub fn extension_routes() -> Vec<(Method, &'static str, GeckoExtensionRoute)> {
     return vec![(Method::Get, "/session/{sessionId}/moz/context", GeckoExtensionRoute::GetContext),
              (Method::Post, "/session/{sessionId}/moz/context", GeckoExtensionRoute::SetContext),
              (Method::Post,
               "/session/{sessionId}/moz/xbl/{elementId}/anonymous_children",
               GeckoExtensionRoute::XblAnonymousChildren),
              (Method::Post,
               "/session/{sessionId}/moz/xbl/{elementId}/anonymous_by_attribute",
@@ -566,73 +562,51 @@ impl WebDriverHandler<GeckoExtensionRout
             Ok(ref mut connection) => {
                 match connection.as_mut() {
                     Some(conn) => {
                         conn.send_command(resolved_capabilities, &msg)
                             .map_err(|mut err| {
                                 // Shutdown the browser if no session can
                                 // be established due to errors.
                                 if let NewSession(_) = msg.command {
-                                    err.delete_session=true;
+                                    err.delete_session = true;
                                 }
                                 err})
                     },
                     None => panic!("Connection missing")
                 }
             },
             Err(_) => {
                 Err(WebDriverError::new(
                     ErrorStatus::UnknownError,
                     "Failed to aquire Marionette connection"))
             }
         }
     }
 
     fn delete_session(&mut self, session: &Option<Session>) {
-        // If there is still an active session send a delete session command
-        // and wait for the browser to quit
         if let Some(ref s) = *session {
             let delete_session = WebDriverMessage {
                 session_id: Some(s.id.clone()),
-                command: WebDriverCommand::DeleteSession
+                command: WebDriverCommand::DeleteSession,
             };
             let _ = self.handle_command(session, delete_session);
-
-            if let Some(ref mut runner) = self.browser {
-                let timeout = TIMEOUT_BROWSER_SHUTDOWN;
-                let poll_interval = 100;
-                let poll_attempts = timeout / poll_interval;
-                let mut poll_attempt = 0;
-
-                while runner.running() {
-                    if poll_attempt <= poll_attempts {
-                        debug!("Waiting for the browser process to shutdown");
-                        poll_attempt += 1;
-                        sleep(Duration::from_millis(poll_interval));
-                    } else {
-                        warn!("Browser process did not shutdown");
-                        break;
-                    }
-                }
-            }
         }
 
         if let Ok(ref mut connection) = self.connection.lock() {
             if let Some(conn) = connection.as_mut() {
                 conn.close();
             }
         }
 
-        // If the browser is still open then kill the process
         if let Some(ref mut runner) = self.browser {
-            if runner.running() {
-                info!("Forcing a shutdown of the browser process");
-                if runner.kill().is_err() {
-                    error!("Failed to kill browser process");
-                };
+            // TODO(https://bugzil.la/1443922):
+            // Use toolkit.asyncshutdown.crash_timout pref
+            if runner.wait(time::Duration::from_secs(70)).is_err() {
+                error!("Failed to stop browser process");
             }
         }
 
         self.connection = Mutex::new(None);
         self.browser = None;
     }
 }
 
@@ -1376,17 +1350,17 @@ impl MarionetteConnection {
                 Ok(stream) => {
                     self.stream = Some(stream);
                     break;
                 }
                 Err(e) => {
                     trace!("  connection attempt {}/{}", poll_attempt, poll_attempts);
                     if poll_attempt <= poll_attempts {
                         poll_attempt += 1;
-                        sleep(Duration::from_millis(poll_interval));
+                        thread::sleep(time::Duration::from_millis(poll_interval));
                     } else {
                         return Err(WebDriverError::new(
                             ErrorStatus::UnknownError,
                             e.description().to_owned(),
                         ));
                     }
                 }
             }
--- a/testing/mozbase/rust/mozrunner/src/runner.rs
+++ b/testing/mozbase/rust/mozrunner/src/runner.rs
@@ -4,18 +4,20 @@ use std::collections::HashMap;
 use std::convert::From;
 use std::env;
 use std::error::Error;
 use std::ffi::{OsStr, OsString};
 use std::fmt;
 use std::io;
 use std::io::ErrorKind;
 use std::path::{Path, PathBuf};
+use std::process;
 use std::process::{Child, Command, Stdio};
-use std::process;
+use std::thread;
+use std::time;
 
 pub trait Runner {
     type Process;
 
     fn arg<'a, S>(&'a mut self, arg: S) -> &'a mut Self
     where
         S: AsRef<OsStr>;
 
@@ -54,16 +56,27 @@ pub trait RunnerProcess {
     /// is reaped.  This function is guaranteed to repeatedly return a successful exit status so
     /// long as the child has already exited.
     ///
     /// If the process has exited, then `Ok(Some(status))` is returned.  If the exit status is not
     /// available at this time then `Ok(None)` is returned.  If an error occurs, then that error is
     /// returned.
     fn try_wait(&mut self) -> io::Result<Option<process::ExitStatus>>;
 
+    /// Waits for the process to exit completely, killing it if it does not stop within `timeout`,
+    /// and returns the status that it exited with.
+    ///
+    /// Firefox' integrated background monitor observes long running threads during shutdown and
+    /// kills these after 63 seconds.  If the process fails to exit within the duration of
+    /// `timeout`, it is forcefully killed.
+    ///
+    /// This function will continue to have the same return value after it has been called at least
+    /// once.
+    fn wait(&mut self, timeout: time::Duration) -> io::Result<process::ExitStatus>;
+
     /// Determine if the process is still running.
     fn running(&mut self) -> bool;
 
     /// Forces the process to exit and returns the exit status.  This is
     /// equivalent to sending a SIGKILL on Unix platforms.
     fn kill(&mut self) -> io::Result<process::ExitStatus>;
 }
 
@@ -118,16 +131,27 @@ pub struct FirefoxProcess {
     profile: Profile,
 }
 
 impl RunnerProcess for FirefoxProcess {
     fn try_wait(&mut self) -> io::Result<Option<process::ExitStatus>> {
         self.process.try_wait()
     }
 
+    fn wait(&mut self, timeout: time::Duration) -> io::Result<process::ExitStatus> {
+        let now = time::Instant::now();
+        while self.running() {
+            if now.elapsed() >= timeout {
+                break;
+            }
+            thread::sleep(time::Duration::from_millis(100));
+        }
+        self.kill()
+    }
+
     fn running(&mut self) -> bool {
         self.try_wait().unwrap().is_none()
     }
 
     fn kill(&mut self) -> io::Result<process::ExitStatus> {
         self.process.kill()?;
         self.process.wait()
     }