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
--- 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()
}