Bug 1448900 - Avoid killing exited process. r=jgraham,whimboo
authorAndreas Tolfsen <ato@sny.no>
Wed, 28 Mar 2018 17:17:29 +0100
changeset 410594 3d25026d07ef61dcb7951a9afd4c3cd6d4f8af22
parent 410593 84ba9406ef28f591ac98702ee3bcf0a656bb3cf2
child 410595 56f8adbe41b7850d48fce10066b640fa70df0527
push id33733
push useraciure@mozilla.com
push dateThu, 29 Mar 2018 22:05:29 +0000
treeherdermozilla-central@7ca58ce09779 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjgraham, whimboo
bugs1448900, 1443853
milestone61.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 1448900 - Avoid killing exited process. r=jgraham,whimboo std::process::Child::kill() will return Err if the process has already exited. The assumption in bug 1443853 was that calling ::kill() would consistently return the std::process::ExitStatus was the process already dead. This patches the regression from bug 1443853 by employing Child::try_wait() in a loop. When the process gives some exit status, this is return directly without relying on Child::kill() as before. If the process has not exited and the timeout has elapsed, we kill the process and return its return value. If the process has not exited but the timeout duration has not elapsed, we wait 100 ms as before. MozReview-Commit-ID: 4VENbrKtcEh
testing/geckodriver/src/marionette.rs
testing/mozbase/rust/mozrunner/src/runner.rs
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -595,18 +595,19 @@ impl WebDriverHandler<GeckoExtensionRout
             if let Some(conn) = connection.as_mut() {
                 conn.close();
             }
         }
 
         if let Some(ref mut runner) = self.browser {
             // 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");
+            match runner.wait(time::Duration::from_secs(70)) {
+                Ok(x) => debug!("Browser process stopped with exit status {}", x),
+                Err(e) => error!("Failed to stop browser process: {}", e),
             }
         }
 
         self.connection = Mutex::new(None);
         self.browser = None;
     }
 }
 
--- a/testing/mozbase/rust/mozrunner/src/runner.rs
+++ b/testing/mozbase/rust/mozrunner/src/runner.rs
@@ -132,31 +132,39 @@ pub struct FirefoxProcess {
 }
 
 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;
+        let start = time::Instant::now();
+        loop {
+            match self.try_wait() {
+                // child has already exited, reap its exit code
+                Ok(Some(status)) => return Ok(status),
+
+                // child still running and timeout elapsed, kill it
+                Ok(None) if start.elapsed() >= timeout => return self.kill(),
+
+                // child still running, let's give it more time
+                Ok(None) => thread::sleep(time::Duration::from_millis(100)),
+
+                Err(e) => return Err(e),
             }
-            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> {
+        debug!("Killing process {}", self.process.id());
         self.process.kill()?;
         self.process.wait()
     }
 }
 
 #[derive(Debug)]
 pub struct FirefoxRunner {
     binary: PathBuf,