Closed
Bug 1387403
Opened 7 years ago
Closed 7 years ago
Application is not getting closed for exceptions raised during session creation
Categories
(Testing :: geckodriver, defect)
Testing
geckodriver
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
Details
Attachments
(2 files)
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.
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.
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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.
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
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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(dburns)
Resolution: --- → INVALID
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.
Flags: needinfo?(dburns)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ato)
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.
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
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)
Flags: needinfo?(dburns)
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.
Assignee | ||
Comment 10•7 years ago
|
||
Ok, I will give it a shot.
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Flags: needinfo?(ato)
Comment hidden (mozreview-request) |
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.
Attachment #8898259 -
Flags: review?(james)
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.
Comment hidden (mozreview-request) |
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!
Attachment #8898259 -
Flags: review?(james) → review+
Comment hidden (mozreview-request) |
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
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/832139e8b9c6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 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.
Description
•