Application is not getting closed for exceptions raised during session creation

RESOLVED FIXED in Firefox 57

Status

--
major
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

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.
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.
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(dburns)
Summary: Application is not getting closed for exceptions in capability parsing → Application is not getting closed for exceptions in capability parsing and handling commands
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.
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(dburns)
Resolution: --- → INVALID
Created attachment 8897325 [details]
python binding testcase

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.
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Ok, so it would happen at any time when an exception is raised while creating the session. This clearly needs to be fixed.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: Application is not getting closed for exceptions in capability parsing and handling commands → Application is not getting closed for exceptions raised during session creation
(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)
Flags: needinfo?(dburns)
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.
Ok, I will give it a shot.
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Flags: needinfo?(ato)
Comment hidden (mozreview-request)

Comment 12

2 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.
Attachment #8898259 - Flags: review?(james)
(Assignee)

Comment 13

2 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.
Comment hidden (mozreview-request)

Comment 15

2 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!
Attachment #8898259 - Flags: review?(james) → review+
Comment hidden (mozreview-request)

Comment 17

2 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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/832139e8b9c6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.