Closed Bug 1443853 Opened 6 years ago Closed 6 years ago

Move geckodriver browser process shutdown code to mozrunner

Categories

(Testing :: Mozbase Rust, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

geckodriver has some specialised code for safely waiting for the
browser process to shut down in [1].  Notably this waits for the
exception handler/watchdog in Firefox to interrupt the process if
the shutdown hangs.

It would make sense to uplift this to mozrunner as it is likely
useful for other programs wrapping Firefox.  It will also make
MarionetteHandler::delete_session() easier to read.

  [1] https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/testing/geckodriver/src/marionette.rs#599-632
Blocks: 1443520
Assignee: nobody → ato
Status: NEW → ASSIGNED
Blocks: 1443922
Comment on attachment 8956982 [details]
Bug 1443853 - Move browser process shutdown monitor to mozrunner.

https://reviewboard.mozilla.org/r/225944/#review231938

::: testing/geckodriver/src/marionette.rs:601
(Diff revision 1)
>              }
>          }
>  
> -        // If the browser is still open then kill the process
>          if let Some(ref mut runner) = self.browser {
> -            if runner.running() {
> +            // TODO: https://bugzil.la/1443922

Maybe explain the TODO a bit so we are not forced to always have to lookup the bug details.

::: testing/mozbase/rust/mozrunner/src/runner.rs:143
(Diff revision 1)
>  
> +    fn wait_for(&mut self, timeout: time::Duration) -> io::Result<process::ExitStatus> {
> +        match self.process.try_wait() {
> +            Ok(Some(status)) => Ok(status),
> +            Ok(None) => {
> +                debug!("Waiting {}s for browser to shut down", timeout.as_secs());

Just as a side-note... maybe we should do the same for the connection attempts? There is also a long list of entries. File a new bug for it if you think it is worth.

::: testing/mozbase/rust/mozrunner/src/runner.rs:157
(Diff revision 1)
> +                }
> +
> +                if self.running() {
> +                    warn!("Forcing browser process to shut down");
> +                }
> +                self.kill()

You only want to do this in the if condition.
Comment on attachment 8956976 [details]
Bug 1443853 - Remove unnecessary paranthesis around function argument.

https://reviewboard.mozilla.org/r/225932/#review231962
Attachment #8956976 - Flags: review?(james) → review+
Comment on attachment 8956977 [details]
Bug 1443853 - Drop unused std::ascii::AsciiExt trait.

https://reviewboard.mozilla.org/r/225934/#review231964

BTW "Quences" isn't a word: https://en.oxforddictionaries.com/search?utf8=%E2%9C%93&filter=dictionary&query=Quences You  probably mean "Quenches"?
Attachment #8956977 - Flags: review?(james) → review+
Comment on attachment 8956978 [details]
Bug 1443853 - Rename RunnerProcess::is_running() to ::running().

https://reviewboard.mozilla.org/r/225936/#review231966
Attachment #8956978 - Flags: review?(james) → review+
Comment on attachment 8956979 [details]
Bug 1443853 - Rename RunnerProcess::stop() to ::kill().

https://reviewboard.mozilla.org/r/225938/#review231986

Arguably this is wrong because it doesn't quite do the same thing as ::kill(). In particular it actually waits for the process to finish rather just signalling it. But I don't mind too much.
Attachment #8956979 - Flags: review?(james) → review+
Comment on attachment 8956980 [details]
Bug 1443853 - Avoid std::io::{Result,Error} renaming.

https://reviewboard.mozilla.org/r/225940/#review231988
Attachment #8956980 - Flags: review?(james) → review+
Comment on attachment 8956981 [details]
Bug 1443853 - Rename RunnerProcess::status() to ::try_wait().

https://reviewboard.mozilla.org/r/225942/#review231990

::: testing/geckodriver/src/marionette.rs:1362
(Diff revision 1)
> -                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()),

At some point we should use `std::os::unix` to work out what the signal is in this case.
Attachment #8956981 - Flags: review?(james) → review+
Comment on attachment 8956981 [details]
Bug 1443853 - Rename RunnerProcess::status() to ::try_wait().

https://reviewboard.mozilla.org/r/225942/#review231990

> At some point we should use `std::os::unix` to work out what the signal is in this case.

I have written a crate, sysexit, which can do that.  We can consider integrating it here.
Comment on attachment 8956982 [details]
Bug 1443853 - Move browser process shutdown monitor to mozrunner.

https://reviewboard.mozilla.org/r/225944/#review231938

> Just as a side-note... maybe we should do the same for the connection attempts? There is also a long list of entries. File a new bug for it if you think it is worth.

Yeah, I thought about that too.  I’ve filed
https://bugzil.la/1444068.

> You only want to do this in the if condition.

Calling ::kill() here is functionally equivalent to an if-condition
call ::wait(), because ::kill() guarantees to return the same value
when called a second time, and by this point the process should have
exited.
Comment on attachment 8956982 [details]
Bug 1443853 - Move browser process shutdown monitor to mozrunner.

https://reviewboard.mozilla.org/r/225944/#review232100

::: testing/geckodriver/src/marionette.rs:603
(Diff revision 2)
>  
> -        // 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() {
> +            // TODO(https://bugzil.la/1443922):
> +            // Use toolkit.asyncshutdown.crash_timout pref
> +            if runner.wait_for(time::Duration::from_secs(70)).is_err() {

I don't think `wait_for` is a good name because it doesn't tell me what's going on here; it sounds like a general condition mechanism (i.e. "just wait for 70s"). I see that the etomology is from `wait` in the stdlib, but I find this confusing.

I would I think be happy with just `wait` but accepting a timeout or something more explicit.

::: testing/mozbase/rust/mozrunner/src/runner.rs:140
(Diff revision 2)
>      fn try_wait(&mut self) -> io::Result<Option<process::ExitStatus>> {
>          self.process.try_wait()
>      }
>  
> +    fn wait_for(&mut self, timeout: time::Duration) -> io::Result<process::ExitStatus> {
> +        match self.process.try_wait() {

Why don't we just write this with the loop on the outside i.e.

```
let now = time::Instant::now();
while self.running() {
    if now.elapsed() > timeout {
        break;
    }
    thread::sleep(time::Duration::from_millis(100));
}
if self.running() {
    self.kill()
} else {
    self.try_wait()
}
```
Attachment #8956982 - Flags: review?(james) → review-
Comment on attachment 8956982 [details]
Bug 1443853 - Move browser process shutdown monitor to mozrunner.

https://reviewboard.mozilla.org/r/225944/#review232100

> Why don't we just write this with the loop on the outside i.e.
> 
> ```
> let now = time::Instant::now();
> while self.running() {
>     if now.elapsed() > timeout {
>         break;
>     }
>     thread::sleep(time::Duration::from_millis(100));
> }
> if self.running() {
>     self.kill()
> } else {
>     self.try_wait()
> }
> ```

This looks conceptually simpler, but has two problems: it makes it
harder to atomically decide whether to log before waiting, and would
force us to match on ::try_wait().

There are three variants we need to take care of from ::try_wait():
process has exited and returned with some exit status, process is
still running, and an error occurred.  The return type of this
function is io::Result<process::ExitStatus>, which means it isn’t
compatible with ::try_wait()’s io::Result<Option<process::ExitStatus>>.

This is fixable by always calling self.kill() since this returns
the return value of Child::wait(), and guarantees a consistent
return value when called again.

So if we are happy with dropping the logging I like this succinct
approach better.  I’ve uploaded an amended version for you to look at.
Comment on attachment 8956982 [details]
Bug 1443853 - Move browser process shutdown monitor to mozrunner.

https://reviewboard.mozilla.org/r/225944/#review232594
Attachment #8956982 - Flags: review?(james) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6a8d0f3c88d
Remove unnecessary paranthesis around function argument. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/985f63927da9
Drop unused std::ascii::AsciiExt trait. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/4988c81161f2
Rename RunnerProcess::is_running() to ::running(). r=jgraham
https://hg.mozilla.org/integration/autoland/rev/1730b54d8ebf
Rename RunnerProcess::stop() to ::kill(). r=jgraham
https://hg.mozilla.org/integration/autoland/rev/83bb33caf3d3
Avoid std::io::{Result,Error} renaming. r=jgraham
https://hg.mozilla.org/integration/autoland/rev/2da49990b247
Rename RunnerProcess::status() to ::try_wait(). r=jgraham
https://hg.mozilla.org/integration/autoland/rev/a5d22f442915
Move browser process shutdown monitor to mozrunner. r=jgraham
No longer blocks: 1443520
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: