Bug 1448900 - Avoid killing exited process. r?whimboo draft
authorAndreas Tolfsen <ato@sny.no>
Wed, 28 Mar 2018 17:17:29 +0100
changeset 774647 4c5f79f63f6ec3a4657879ab1f54968342f38b50
parent 774444 6aa3b57955fed5e137d0306478e1a4b424a6d392
child 774648 7072c3a8d936c23c3c4b6b4355220e6dac43db99
push id104467
push userbmo:ato@sny.no
push dateThu, 29 Mar 2018 09:41:36 +0000
reviewerswhimboo
bugs1448900, 1443853
milestone61.0a1
Bug 1448900 - Avoid killing exited process. r?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,