Bug 1443853 - Move browser process shutdown monitor to mozrunner. r?jgraham draft
authorAndreas Tolfsen <ato@sny.no>
Wed, 07 Mar 2018 21:57:53 +0000
changeset 765981 78fff5aa8b635161b2e339c75b26f1d896282010
parent 765980 a57f837800f6de114b429524e02a102f22fa2c29
push id102198
push userbmo:ato@sny.no
push dateSun, 11 Mar 2018 16:03:30 +0000
reviewersjgraham
bugs1443853
milestone60.0a1
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()
     }