Open Bug 1430064 Opened 2 years ago Updated 21 days ago

Firefox does not exit when geckodriver is terminated

Categories

(Testing :: geckodriver, defect, P3)

Version 3
defect

Tracking

(Not tracked)

People

(Reporter: whimboo, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

In case a Selenium test doesn't explicitly call "driver.quit()" at the end of the test, geckodriver will be terminated by Selenium. Sadly this results in a situation that Firefox continues to run, even without geckodriver running. It also means that the profile geckodriver own continues to be around.

As Andreas mentioned on IRC this could be because of issues with signal handling in Rust.
Selenium advice has always been to call `quit()` at the end of a test. If people are doing this then they are getting themselves into weird edge cases that go against all advice.

Dropping this to P5 and if a contributor wants to fix it then great
Priority: -- → P5
I would be interested in helping to get this resolved.  There are certain cases where disaster prevents an application from properly calling "quit()", like a failure in Selenium Grid.  In such a failure, you might be left with tens of browsers to cleanup.

There are two crates I've earmarked that can handle interrupt or term signals: chan-signal and ctrlc.  Ctrlc provides handling for interrupt and term, with the latter being a configurable option in Cargo.toml.  With ctrlc, all signals must be handled with the same handler, and the process is rather opaque.

Chan-signal provides finer-grained control of signal handling, with the option for separate signal handlers for separate signals, but currently only supports Unix.  It looks like Windows support is in the scope, but there's been no forward development towards it.  It's also based on the chan crate, which provides a more transparent mechanism for handling the signals.

Chan-signal seems like the better option moving forward, but the lack of current Windows support is a miss.  I would be interested in getting feedback on the decision before I move forward with a test patch.
I think Andreas can give some helpful feedback here given that he AFAIR had a quick look at signal handling lately. Maybe there is even another bug open.
Flags: needinfo?(ato)
We ran into another case where signal handling is necessary when
discussing the patch for preserving minidump files when Firefox
crashes.  jgraham has summarised the discussion in
https://bugzilla.mozilla.org/show_bug.cgi?id=1433495#c4.  It is
also my impression that chan-signal is the right path forward, even
if it currently does not give us Windows.

Greg: Can you actually confirm that handling SIGINT and stopping
Firefox solves this problem?  whimboo recently recently changed
geckodriver to more gracefully deal with stopping Firefox.
Flags: needinfo?(ato) → needinfo?(gsfraley)
Summary: Geckodriver doesn't close Firefox if getting terminated → Firefox does not exit when geckodriver is terminated
Sure! I'll get a first-pass patch going to see what impact it has on the browser being left over.  I'll also reach out to the chan-signal dev to see how close Windows support is on the horizon.
Flags: needinfo?(gsfraley)
Even the simplest examples of chan-signal don't seem to work when used in conjunction with hyper, like the following modification of main.rs in GeckoDriver:

```
let listening = webdriver::server::start(addr, handler, &extension_routes()[..])
    .map_err(|err| (ExitCode::Unavailable, err.to_string()))?;
info!("Listening on {}", listening.socket);

let signal_channel = chan_signal::notify(&[Signal::INT, Signal::TERM]);
let signal = signal_channel.recv().unwrap();
println!("Received signal {:?}", signal);
```

Sending a Ctrl+C or TERM doesn't result in the received-signal-message being printed out.  Spinning up a Hyper 0.10 server on a test project results in the same behavior.

I'll check into the ctrlc crate next, but the chan-signal crate has me scratching my in regards to why it won't work.
So one idea I just had is to handle the signal, get Firefox to shut down and then just call `std::process::exit()` to exit without worrying about terminating the hyper event loop.
OK, so I just pushed my bits to mozreview and would appreciate feedback (I marked r=ato, but it's more like f?ato). I didn't extensively test this, but it seems to work at least  a little. Also it's basically just what was in my git stash, so it's not a super-clean patch at this point.
Oh and, Greg, apologies for hijacking this bug; I just happened  to have been looking at a related issue a few weeks ago and so had a nearly working patch. If you can find a way to make this work on Windows, that would be amazing.
No prob!  I've been reading the chan-signal code/Microsoft docs to get an idea of how Windows support would play out.  I don't know what timeframe it would take for me to come up with a solution, but I'll pay attention to this bug as a test point.
Windows support can be added pretty easily to chan-signal.  I'm going to work with the author to get the functionality merged back into his version.  I'd recommend sticking with the crate on crates.io, but if you want to test windows functionality in a pinch, my fork lives at https://github.com/gsfraley/chan-signal.git.
Comment on attachment 8958770 [details]
Bug 1430064 - Use chan_signal to handle signals sent to geckodriver,

https://reviewboard.mozilla.org/r/227680/#review234104

This is a pretty clever patch!  I just have some minor comments and
things you need to fix up.

::: commit-message-d6957:5
(Diff revision 1)
> +Bug 1430064 - Use chan_signal to handle signals sent to geckodriver, r=ato
> +
> +This requires a refactor of the WebDriver main loop so that we are able to send
> +a message from the geckodriver process in. Hyper doesn't allow us to
> +exit it's handler loop cleanly so we take the approach of doing the

its

::: testing/geckodriver/Cargo.toml:13
(Diff revision 1)
> +chan = "0.1.12"
> +chan-signal = "0.3.1"

Let’s depend on chan "0.1" and chan-signal "0.3" so that it is
easier to share the vendored crates with other Rust code in the
tree.

::: testing/geckodriver/Cargo.toml:13
(Diff revision 1)
> +chan = "0.1.12"
> +chan-signal = "0.3.1"

You also need to run "./mach vendor rust" because these are not
currently vendored.

::: testing/geckodriver/src/main.rs:1
(Diff revision 1)
>  extern crate chrono;
> +extern crate chan;
> +extern crate chan_signal;

Surely chan* should come before chrono?

::: testing/geckodriver/src/main.rs:213
(Diff revision 1)
> +    let server =  webdriver::server::WebDriverServer::new();
> +
> +    let builder = thread::Builder::new().name("signal handler".to_string());
> +    let chan = server.send_chan();
> +    builder.spawn(move || {
> +        signal_notify(signal, chan)

Missing semicolon.  I don’t think this is supposed to return anything?

::: testing/geckodriver/src/main.rs:232
(Diff revision 1)
> +{
> +
> +    // Blocks until this process is sent an INT or TERM signal.
> +    signal.recv().unwrap();
> +
> +    info!("Got a signal, quitting");

s/info/debug/

::: testing/webdriver/src/server.rs:64
(Diff revision 1)
>      }
>  
>      fn run(&mut self, msg_chan: Receiver<DispatchMessage<U>>) {
>          loop {
> -            match msg_chan.recv() {
> +            let msg = msg_chan.recv();
> +            info!("Got msg");

Drop this.

::: testing/webdriver/src/server.rs:92
(Diff revision 1)
>                      if resp_chan.send(resp).is_err() {
>                          error!("Sending response to the main thread failed");
>                      };
>                  }
> -                Ok(DispatchMessage::Quit) => break,
> +                Ok(DispatchMessage::Quit) => {
> +                    info!("Got quit");

Drop or lower to debug.

::: testing/webdriver/src/server.rs:259
(Diff revision 1)
> +        let (msg_send, msg_recv) = channel();
> +        WebDriverServer {
> +            send_chan: msg_send,
> +            recv_chan: msg_recv
> +        }

If you name the return values from channel() send_chan and recv_chan,
you can avoid the field assignments when constructing WebDriverServer:

> let (send_chan, recv_chan) = channel();
> WebDriverServer {
>   send_chan,
>   recv_chan,
> }
Attachment #8958770 - Flags: review?(ato) → review-
Comment on attachment 8960180 [details]
Bug 1430064 - Vendor chan_signal crate,

https://reviewboard.mozilla.org/r/228948/#review234652
Attachment #8960180 - Flags: review?(ato) → review+
Comment on attachment 8958770 [details]
Bug 1430064 - Use chan_signal to handle signals sent to geckodriver,

https://reviewboard.mozilla.org/r/227680/#review234654
Attachment #8958770 - Flags: review?(ato) → review+
Flags: needinfo?(ato)
Blocks: 1491288
Flags: needinfo?(ato)
Flags: needinfo?(ato)
Flags: needinfo?(ato)
Priority: P5 → P2
Resetting ni? flag because it was unintentionally removed.
Flags: needinfo?(ato)
I’m going to pick this up and rebase jgraham’s patch.  We now use
a hyper version which _does_ support shutting down.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
Blocks: 1495062
No longer blocks: 1491288

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #20)

I’m going to pick this up and rebase jgraham’s patch. We now use
a hyper version which does support shutting down.

Looks like you missed to follow-up here. Will you be able to do it soon? Otherwise we should move this bug to the next 0.25 release (bug 1520585).

Flags: needinfo?(ato)

No if you want to do it, please go ahead!

Assignee: ato → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ato)
Priority: P2 → P3
Blocks: 1520585
No longer blocks: 1495062
No longer blocks: 1520585
No longer blocks: 1573798
You need to log in before you can comment on or make changes to this bug.