From 22de401622f4f9f7df711b10a0ade60eab8aee8b Mon Sep 17 00:00:00 2001 From: Florian Wagner Date: Sun, 11 Mar 2018 07:47:21 -0400 Subject: [PATCH 1/8] servo: Merge #20185 - Make KeyframesPercentage::parse faster (from Beta-Alf:master); r=emilio Minor refactoring of the parsing of keyframes. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #20179 (github issue number if applicable). - [ ] There are tests for these changes OR - [X] These changes do not require tests because it is a minor refactor so tests are already in place. The original Issue explicitly says so. Source-Repo: https://github.com/servo/servo Source-Revision: 29e10d4f88a7564f4d6a4496d3d1be5949962f8a --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : cf1ac3eb68cdf37d44f5c1e0a9e478f2a23bc3d8 --- .../style/stylesheets/keyframes_rule.rs | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/servo/components/style/stylesheets/keyframes_rule.rs b/servo/components/style/stylesheets/keyframes_rule.rs index 266d953cd996..302ac36e46b4 100644 --- a/servo/components/style/stylesheets/keyframes_rule.rs +++ b/servo/components/style/stylesheets/keyframes_rule.rs @@ -5,7 +5,7 @@ //! Keyframes: https://drafts.csswg.org/css-animations/#keyframes use cssparser::{AtRuleParser, Parser, QualifiedRuleParser, RuleListParser, ParserInput, CowRcStr}; -use cssparser::{DeclarationListParser, DeclarationParser, parse_one_rule, SourceLocation}; +use cssparser::{DeclarationListParser, DeclarationParser, parse_one_rule, SourceLocation, Token}; use error_reporting::{NullReporter, ContextualParseError, ParseErrorReporter}; use parser::{ParserContext, ParserErrorContext}; use properties::{DeclarationSource, Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId}; @@ -126,20 +126,21 @@ impl KeyframePercentage { } fn parse<'i, 't>(input: &mut Parser<'i, 't>) -> Result> { - let percentage = if input.try(|input| input.expect_ident_matching("from")).is_ok() { - KeyframePercentage::new(0.) - } else if input.try(|input| input.expect_ident_matching("to")).is_ok() { - KeyframePercentage::new(1.) - } else { - let percentage = input.expect_percentage()?; - if percentage >= 0. && percentage <= 1. { - KeyframePercentage::new(percentage) - } else { - return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError)); - } - }; - - Ok(percentage) + let token = input.next()?.clone(); + match token { + Token::Ident(ref identifier) if identifier.as_ref().eq_ignore_ascii_case("from") => { + Ok(KeyframePercentage::new(0.)) + }, + Token::Ident(ref identifier) if identifier.as_ref().eq_ignore_ascii_case("to") => { + Ok(KeyframePercentage::new(1.)) + }, + Token::Percentage { unit_value: percentage, .. } if percentage >= 0. && percentage <= 1. => { + Ok(KeyframePercentage::new(percentage)) + }, + _ => { + Err(input.new_unexpected_token_error(token)) + }, + } } } From bf3fd4b45fa36a77a4f9a45b32557b19a47708e0 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:26:27 +0000 Subject: [PATCH 2/8] Bug 1443853 - Remove unnecessary paranthesis around function argument. r=jgraham Silences one compiler warning. MozReview-Commit-ID: FSKV9Ia2iXt --HG-- extra : rebase_source : 04c247b5e0f763a971b0e8886bb82d6b6942af59 --- testing/geckodriver/src/marionette.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/geckodriver/src/marionette.rs b/testing/geckodriver/src/marionette.rs index 13cd45d51d55..1aad05876925 100644 --- a/testing/geckodriver/src/marionette.rs +++ b/testing/geckodriver/src/marionette.rs @@ -985,10 +985,10 @@ impl MarionetteSession { let expiry = try!( Nullable::from_json(x.find("expiry").unwrap_or(&Json::Null), |x| { - Ok(Date::new((try_opt!( + Ok(Date::new(try_opt!( x.as_u64(), ErrorStatus::UnknownError, - "Cookie expiry must be a positive integer")))) + "Cookie expiry must be a positive integer"))) })); let secure = try_opt!( x.find("secure").map_or(Some(false), |x| x.as_boolean()), From 8e428fe6bbb56cb4d2f9e81a22586ebc34a3a394 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:27:18 +0000 Subject: [PATCH 3/8] Bug 1443853 - Drop unused std::ascii::AsciiExt trait. r=jgraham Silences another compiler warning. MozReview-Commit-ID: 6Bcw7Ej9uIC --HG-- extra : rebase_source : 4f6d8522434fed7e673b71d028dce53753f4a3e5 --- testing/mozbase/rust/mozrunner/src/runner.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/mozbase/rust/mozrunner/src/runner.rs b/testing/mozbase/rust/mozrunner/src/runner.rs index c65b26723e39..e47a930b1c64 100644 --- a/testing/mozbase/rust/mozrunner/src/runner.rs +++ b/testing/mozbase/rust/mozrunner/src/runner.rs @@ -1,6 +1,5 @@ use mozprofile::prefreader::PrefReaderError; use mozprofile::profile::Profile; -use std::ascii::AsciiExt; use std::collections::HashMap; use std::convert::From; use std::env; From ecbbc100b2f91b054bc93191213a6b878c1607a7 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:23:57 +0000 Subject: [PATCH 4/8] Bug 1443853 - Rename RunnerProcess::is_running() to ::running(). r=jgraham The ideom for getters in Rust is to not prefix them with "is_". Setters should, however, have the "set_" prefix. MozReview-Commit-ID: 9kXHBYGK7aL --HG-- extra : rebase_source : 6c2591771646c8b7c5b0e6b1af5427455938b4cf --- testing/geckodriver/src/marionette.rs | 4 ++-- testing/mozbase/rust/mozrunner/src/runner.rs | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/testing/geckodriver/src/marionette.rs b/testing/geckodriver/src/marionette.rs index 1aad05876925..ef9059ed5ad4 100644 --- a/testing/geckodriver/src/marionette.rs +++ b/testing/geckodriver/src/marionette.rs @@ -602,7 +602,7 @@ impl WebDriverHandler for MarionetteHandler { let poll_attempts = timeout / poll_interval; let mut poll_attempt = 0; - while runner.is_running() { + while runner.running() { if poll_attempt <= poll_attempts { debug!("Waiting for the browser process to shutdown"); poll_attempt += 1; @@ -623,7 +623,7 @@ impl WebDriverHandler for MarionetteHandler { // If the browser is still open then kill the process if let Some(ref mut runner) = self.browser { - if runner.is_running() { + if runner.running() { info!("Forcing a shutdown of the browser process"); if runner.stop().is_err() { error!("Failed to kill browser process"); diff --git a/testing/mozbase/rust/mozrunner/src/runner.rs b/testing/mozbase/rust/mozrunner/src/runner.rs index e47a930b1c64..2fd400558ea2 100644 --- a/testing/mozbase/rust/mozrunner/src/runner.rs +++ b/testing/mozbase/rust/mozrunner/src/runner.rs @@ -48,7 +48,9 @@ pub trait Runner { pub trait RunnerProcess { fn status(&mut self) -> IoResult>; fn stop(&mut self) -> IoResult; - fn is_running(&mut self) -> bool; + + /// Determine if the process is still running. + fn running(&mut self) -> bool; } #[derive(Debug)] @@ -99,7 +101,7 @@ impl From for RunnerError { #[derive(Debug)] pub struct FirefoxProcess { process: Child, - profile: Profile + profile: Profile, } impl RunnerProcess for FirefoxProcess { @@ -107,7 +109,7 @@ impl RunnerProcess for FirefoxProcess { self.process.try_wait() } - fn is_running(&mut self) -> bool { + fn running(&mut self) -> bool { self.status().unwrap().is_none() } From a1615d76acaece5dbea790ca433879e7fa280ae8 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:29:23 +0000 Subject: [PATCH 5/8] Bug 1443853 - Rename RunnerProcess::stop() to ::kill(). r=jgraham This renames RunnerProcess::stop() to ::kill() for symmetry with the standard library's std::process::Child. MozReview-Commit-ID: 20vSni9bA0X --HG-- extra : rebase_source : 112b29249563154b50d9a72c141034e5cdf7f19b --- testing/geckodriver/src/marionette.rs | 2 +- testing/mozbase/rust/mozrunner/src/runner.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/testing/geckodriver/src/marionette.rs b/testing/geckodriver/src/marionette.rs index ef9059ed5ad4..0e55f2b0b3cc 100644 --- a/testing/geckodriver/src/marionette.rs +++ b/testing/geckodriver/src/marionette.rs @@ -625,7 +625,7 @@ impl WebDriverHandler for MarionetteHandler { if let Some(ref mut runner) = self.browser { if runner.running() { info!("Forcing a shutdown of the browser process"); - if runner.stop().is_err() { + if runner.kill().is_err() { error!("Failed to kill browser process"); }; } diff --git a/testing/mozbase/rust/mozrunner/src/runner.rs b/testing/mozbase/rust/mozrunner/src/runner.rs index 2fd400558ea2..24fe4c038509 100644 --- a/testing/mozbase/rust/mozrunner/src/runner.rs +++ b/testing/mozbase/rust/mozrunner/src/runner.rs @@ -47,10 +47,13 @@ pub trait Runner { pub trait RunnerProcess { fn status(&mut self) -> IoResult>; - fn stop(&mut self) -> IoResult; /// 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) -> IoResult; } #[derive(Debug)] @@ -113,7 +116,7 @@ impl RunnerProcess for FirefoxProcess { self.status().unwrap().is_none() } - fn stop(&mut self) -> IoResult { + fn kill(&mut self) -> IoResult { self.process.kill()?; self.process.wait() } From f57be4355e4feb625a9491f4a8999e1cad107245 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:31:31 +0000 Subject: [PATCH 6/8] Bug 1443853 - Avoid std::io::{Result,Error} renaming. r=jgraham We can pick up std::io::Result and std::io::Error directly from the std::io namespace without having to rename them. MozReview-Commit-ID: 9Xz92HvcFpO --HG-- extra : rebase_source : 89a006c40e11d9e7fc5706d3a6612f916e00f919 --- testing/mozbase/rust/mozrunner/src/runner.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/testing/mozbase/rust/mozrunner/src/runner.rs b/testing/mozbase/rust/mozrunner/src/runner.rs index 24fe4c038509..17d19ddd1b9c 100644 --- a/testing/mozbase/rust/mozrunner/src/runner.rs +++ b/testing/mozbase/rust/mozrunner/src/runner.rs @@ -6,7 +6,8 @@ use std::env; use std::error::Error; use std::ffi::{OsStr, OsString}; use std::fmt; -use std::io::{Error as IoError, ErrorKind, Result as IoResult}; +use std::io; +use std::io::ErrorKind; use std::path::{Path, PathBuf}; use std::process::{Child, Command, Stdio}; use std::process; @@ -46,19 +47,19 @@ pub trait Runner { } pub trait RunnerProcess { - fn status(&mut self) -> IoResult>; + fn status(&mut self) -> io::Result>; /// 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) -> IoResult; + fn kill(&mut self) -> io::Result; } #[derive(Debug)] pub enum RunnerError { - Io(IoError), + Io(io::Error), PrefReader(PrefReaderError), } @@ -89,8 +90,8 @@ impl Error for RunnerError { } } -impl From for RunnerError { - fn from(value: IoError) -> RunnerError { +impl From for RunnerError { + fn from(value: io::Error) -> RunnerError { RunnerError::Io(value) } } @@ -108,7 +109,7 @@ pub struct FirefoxProcess { } impl RunnerProcess for FirefoxProcess { - fn status(&mut self) -> IoResult> { + fn status(&mut self) -> io::Result> { self.process.try_wait() } @@ -116,7 +117,7 @@ impl RunnerProcess for FirefoxProcess { self.status().unwrap().is_none() } - fn kill(&mut self) -> IoResult { + fn kill(&mut self) -> io::Result { self.process.kill()?; self.process.wait() } From 1509c2ba22f49efaf219e0287ca12cea2cafc3cb Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:43:49 +0000 Subject: [PATCH 7/8] Bug 1443853 - Rename RunnerProcess::status() to ::try_wait(). r=jgraham This renames RunnerProcess::status() to ::try_wait() for symmetry with std::process::Child::try_wait() in the standard library. The patch also makes an attempt at cleaning up its usage in geckodriver, however this can be further improved. MozReview-Commit-ID: 14ihT7MpM7l --HG-- extra : rebase_source : 4e96c79c6ebbb256c4a08cb4dd86c99aacaa13ac --- testing/geckodriver/src/marionette.rs | 43 +++++++++++--------- testing/mozbase/rust/mozrunner/src/runner.rs | 16 ++++++-- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/testing/geckodriver/src/marionette.rs b/testing/geckodriver/src/marionette.rs index 0e55f2b0b3cc..4892ef481289 100644 --- a/testing/geckodriver/src/marionette.rs +++ b/testing/geckodriver/src/marionette.rs @@ -1341,37 +1341,42 @@ impl MarionetteConnection { MarionetteConnection { port: port, stream: None, - session: MarionetteSession::new(session_id) + session: MarionetteSession::new(session_id), } } pub fn connect(&mut self, browser: &mut Option) -> WebDriverResult<()> { - let timeout = 60 * 1000; // ms - let poll_interval = 100; // ms + let timeout = 60 * 1000; // ms + let poll_interval = 100; // ms let poll_attempts = timeout / poll_interval; let mut poll_attempt = 0; loop { - // If the process is gone, immediately abort the connection attempts + // immediately abort connection attempts if process disappears if let &mut Some(ref mut runner) = browser { - let status = runner.status(); - if status.is_err() || status.as_ref().map(|x| *x).unwrap_or(None) != None { + let exit_status = match runner.try_wait() { + Ok(Some(status)) => Some( + status + .code() + .map(|c| c.to_string()) + .unwrap_or("signal".into()), + ), + Ok(None) => None, + Err(_) => Some("{unknown}".into()), + }; + if let Some(s) = exit_status { return Err(WebDriverError::new( ErrorStatus::UnknownError, - format!("Process unexpectedly closed with status: {}", status - .ok() - .and_then(|x| x) - .and_then(|x| x.code()) - .map(|x| x.to_string()) - .unwrap_or("{unknown}".into())))); + format!("Process unexpectedly closed with status {}", s), + )); } } match TcpStream::connect(&(DEFAULT_HOST, self.port)) { Ok(stream) => { self.stream = Some(stream); - break - }, + break; + } Err(e) => { trace!(" connection attempt {}/{}", poll_attempt, poll_attempts); if poll_attempt <= poll_attempts { @@ -1379,16 +1384,16 @@ impl MarionetteConnection { sleep(Duration::from_millis(poll_interval)); } else { return Err(WebDriverError::new( - ErrorStatus::UnknownError, e.description().to_owned())); + ErrorStatus::UnknownError, + e.description().to_owned(), + )); } } } - }; + } debug!("Connected to Marionette on {}:{}", DEFAULT_HOST, self.port); - - try!(self.handshake()); - Ok(()) + self.handshake() } fn handshake(&mut self) -> WebDriverResult<()> { diff --git a/testing/mozbase/rust/mozrunner/src/runner.rs b/testing/mozbase/rust/mozrunner/src/runner.rs index 17d19ddd1b9c..26738b7ac9e6 100644 --- a/testing/mozbase/rust/mozrunner/src/runner.rs +++ b/testing/mozbase/rust/mozrunner/src/runner.rs @@ -47,7 +47,17 @@ pub trait Runner { } pub trait RunnerProcess { - fn status(&mut self) -> io::Result>; + /// Attempts to collect the exit status of the process if it has already exited. + /// + /// This function will not block the calling thread and will only advisorily check to see if + /// the child process has exited or not. If the process has exited then on Unix the process ID + /// 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>; /// Determine if the process is still running. fn running(&mut self) -> bool; @@ -109,12 +119,12 @@ pub struct FirefoxProcess { } impl RunnerProcess for FirefoxProcess { - fn status(&mut self) -> io::Result> { + fn try_wait(&mut self) -> io::Result> { self.process.try_wait() } fn running(&mut self) -> bool { - self.status().unwrap().is_none() + self.try_wait().unwrap().is_none() } fn kill(&mut self) -> io::Result { From 98cdaaee7258066a77389b1e14109834bd348824 Mon Sep 17 00:00:00 2001 From: Andreas Tolfsen Date: Wed, 7 Mar 2018 21:57:53 +0000 Subject: [PATCH 8/8] 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 --HG-- extra : rebase_source : f21f734862bfbbc1ed665dc9c9f611c5968d662f --- testing/geckodriver/src/marionette.rs | 44 ++++---------------- testing/mozbase/rust/mozrunner/src/runner.rs | 26 +++++++++++- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/testing/geckodriver/src/marionette.rs b/testing/geckodriver/src/marionette.rs index 4892ef481289..b0536c2c0e15 100644 --- a/testing/geckodriver/src/marionette.rs +++ b/testing/geckodriver/src/marionette.rs @@ -17,8 +17,8 @@ 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, @@ -56,10 +56,6 @@ 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), @@ -571,7 +567,7 @@ impl WebDriverHandler for MarionetteHandler { // 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}) }, @@ -587,32 +583,12 @@ impl WebDriverHandler for MarionetteHandler { } fn delete_session(&mut self, session: &Option) { - // 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() { @@ -621,13 +597,11 @@ impl WebDriverHandler for MarionetteHandler { } } - // 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"); } } @@ -1381,7 +1355,7 @@ impl MarionetteConnection { 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, diff --git a/testing/mozbase/rust/mozrunner/src/runner.rs b/testing/mozbase/rust/mozrunner/src/runner.rs index 26738b7ac9e6..2282c48f7e53 100644 --- a/testing/mozbase/rust/mozrunner/src/runner.rs +++ b/testing/mozbase/rust/mozrunner/src/runner.rs @@ -9,8 +9,10 @@ use std::fmt; use std::io; use std::io::ErrorKind; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Stdio}; use std::process; +use std::process::{Child, Command, Stdio}; +use std::thread; +use std::time; pub trait Runner { type Process; @@ -59,6 +61,17 @@ pub trait RunnerProcess { /// returned. fn try_wait(&mut self) -> io::Result>; + /// 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; + /// Determine if the process is still running. fn running(&mut self) -> bool; @@ -123,6 +136,17 @@ impl RunnerProcess for FirefoxProcess { self.process.try_wait() } + fn wait(&mut self, timeout: time::Duration) -> io::Result { + 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() }