Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

In the following example we still expect to see a `httpProxyPort` entry added to the capabilities. Given that this is not done, an exception is raised which quits the test command but leaves Firefox open.

Whatever failure happens we always have to Firefox at the end otherwise newly created processes will fail to connect to the default Marionette port.

	Andreas Tolfsen ❲:ato❳
	Comment 1 • 7 years ago

In what example?

httpProxyPort (and other *Port’s) was removed from the
specification some time ago, so it should probably removed.

Also for anything related to parsing the proxy
configuration object, a bug should likely be filed against
https://github.com/mozilla/webdriver-rust for as long as that’s
the canonical repository.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 7 years ago

I know that this will be removed, but I don't know which other wrong values could trigger that. I haven't tested it yet.

Given that geckodriver takes care about the firefox process it should be its task to quit the application and not leave it alone.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

The same can be reproduced if you are calling an unknown method/command in a Selenium test like the following for the Python bindings 3.4.3 which do not have support for installing an addon:

    def test(self):
        self.driver = webdriver.Firefox(capabilities=capabilities)
        self.driver.install_addon("test.xpi")

Maybe that's a bug in the bindings not cleaning up via geckodriver correctly? At least it produces the same visual result.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 7 years ago

Oh, I think this might have been my fault given that I forget to setup a cleanup method which calls `driver.quit()`.

If the test has to take care in those cases to close the browser, please mark this bug as invalid.

	David Burns :automatedtester
	Comment 5 • 7 years ago

The test needs to handle the start up and the closure of the browser. This can be in the test or in setup/teardown methods.

if we are mid New Session we should make sure the browser is cleaned up and return the correct error accordingly.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago

So not sure if all is fine here or not... lets see this example:

Via https://github.com/mozilla/webdriver-rust/issues/115 I remove the *ProxyPort capabilites from the proxy object. When I compile this into current geckodriver and run a test as the following attachment shows the browser is not getting closed. It doesn't matter if I call `driver.quit()` in tearDown or register it via `addCleanup`. 

The problem here is that we currently spawn Firefox and fail in Marionette by extracting the proxy port. As such no session is getting created, and `driver.quit()` doesn't work due to no session identifier.

> WebDriverException: Message: InvalidArgumentError: Expected [object Undefined] undefined to be an integer

This problem will persist as long as we do capability checks in Marionette, and fail creating the session. Usually it should only happen when there is a mismatch between the used geckodriver version and the release of Firefox.

David, is that a problem in the binding so that it force kill the process if a normal quit doesn't work? How should Selenium or maybe better geckodriver ensure  to clean up correctly?

I think we should reopen the bug and implant a force close for the `quit` command in case no session but a valid process exists. Or we force quit Firefox in case no session can be created, which might even be better.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

Ok, so it would happen at any time when an exception is raised while creating the session. This clearly needs to be fixed.

	David Burns :automatedtester
	Comment 8 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #6)
> 
> 
> David, is that a problem in the binding so that it force kill the process if
> a normal quit doesn't work? How should Selenium or maybe better geckodriver
> ensure  to clean up correctly?

Selenium sends `DELETE /session` to quit the session. It is the responsibility of the driver, in this case GeckoDriver, to make sure that the browser is torn down correctly (correctly means that we don't really care how just that its not there anymore)

	David Burns :automatedtester
	Comment 9 • 7 years ago

Not everything can be done in GeckoDriver unfortunately, so in this case we need to start a browser. GeckoDriver needs to make sure that if Marionette says it can't start a session for any reason that the browser process is killed and not left in a weird state.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 7 years ago

Ok, I will give it a shot.

	James Graham [:jgraham]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8898259 [details]
Bug 1387403 - Force quitting the browser if no session can be established.

https://reviewboard.mozilla.org/r/169632/#review174862

::: testing/geckodriver/src/marionette.rs:557
(Diff revision 1)
> +                        match conn.send_command(resolved_capabilities, &msg) {
> +                            Ok(resp) => Ok(resp),
> +                            Err(mut err) => {
> +                                // Force killing the browser if no session can
> +                                // be established due to failures.
> +                                match msg.command {

```
conn.send_command(resolved_capabilities, &msg)
    .as_mut()
    .map_error(|err| {
        if let NewSession(_) = msg.command {
            err.delete_session=true;
    }; err)
```

or something, perhaps.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8898259 [details]
Bug 1387403 - Force quitting the browser if no session can be established.

https://reviewboard.mozilla.org/r/169632/#review174862

> ```
> conn.send_command(resolved_capabilities, &msg)
>     .as_mut()
>     .map_error(|err| {
>         if let NewSession(_) = msg.command {
>             err.delete_session=true;
>     }; err)
> ```
> 
> or something, perhaps.

I've made smaller changes to your suggested code to let it compile. Otherwise looks way better. Thanks.

	James Graham [:jgraham]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8898259 [details]
Bug 1387403 - Force quitting the browser if no session can be established.

https://reviewboard.mozilla.org/r/169632/#review174878

Thanks!

	Pulsebot
	Comment 17 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/832139e8b9c6
Force quitting the browser if no session can be established. r=jgraham

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/832139e8b9c6