1443520 - Marionette:Quit is sent twice during session deletion

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P3)

Description

[Andreas Tolfsen ❲:ato❳]

When geckodriver is asked to explicitly delete a session through
WebDriverCommand::DeleteSession it sends a Marionette:Quit
(or rather, the legacy "quit") command to Marionette.  When we
receive WebDriverResponse::DeleteSession, the webdriver crate calls
MarionetteHandler::delete_session().

If for whatever reason the session in geckodriver has not been
deleted yet by this point, the Marionette:Quit message is sent again.
Because the session was deleted in Marionette the response will be an
"invalid session id" error.

I can’t see any disasterous side-effect from this as geckodriver
throws away any errors from this second attempt, but it is probably
not necessary to send the Marionette:Quit command twice.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: log.txt

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not a dupe of bug 1403510.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'll fix it as part of my work on bug 1714330.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1443520 - [geckodriver] Don't send Marionette:Quit twice during session deletion.

Depends on D116716

Comment 6

[James Graham [:jgraham]]

Attachment: Bug 1443520 - Move session deletion on teardown into webdriver lib,

Currently we have a delete_session function in
webdriver::server::Dispatcher which is called either when the session
has been cleanly deleted with a delete session method, or when we want
the session to be deleted. But all that method actually does is call
into WebDriverHander::delete_session, which then has to decide whether
to initiate a clean shutdown and then do any impl-specific shutdown
steps (close the connection in the case of geckodriver).

The whole setup is easier to understand if we make
webdriver::server::Dispatcher resposible for ensuring that the session
has been deleted by the time we notify the handler. We rename
delete_session teardown_session to reflect the fact that the
session may already have been deleted explicitly by the user; if this
is the case no DeleteSession message is sent, otherwise we send one
implicitly.

On the handler side the &Session reference in delete_session is
replaced with a simple boolean indicating whether shutdown was
clean (a session was started, and it responded to the DeleteSession
message without error). In geckodriver we use this to decide whether
to wait for the browser to close before signalling it.

This fixes several issues; a regression in which we were waiting for
the browser to shut down when we shouldn't, and an issue where we were
sending too many delete session messages in some cases.

Comment 7

[James Graham [:jgraham]]

Attachment: Bug 1443520 - Move session deletion on teardown into webdriver lib,

Currently we have a delete_session function in
webdriver::server::Dispatcher which is called either when the session
has been cleanly deleted with a delete session method, or when we want
the session to be deleted. But all that method actually does is call
into WebDriverHander::delete_session, which then has to decide whether
to initiate a clean shutdown and then do any impl-specific shutdown
steps (close the connection in the case of geckodriver).

The whole setup is easier to understand if we make
webdriver::server::Dispatcher resposible for ensuring that the session
has been deleted by the time we notify the handler. We rename
delete_session teardown_session to reflect the fact that the
session may already have been deleted explicitly by the user; if this
is the case no DeleteSession message is sent, otherwise we send one
implicitly.

On the handler side the &Session reference in delete_session is
replaced with a simple boolean indicating whether shutdown was
clean (a session was started, and it responded to the DeleteSession
message without error). In geckodriver we use this to decide whether
to wait for the browser to close before signalling it.

This fixes several issues; a regression in which we were waiting for
the browser to shut down when we shouldn't, and an issue where we were
sending too many delete session messages in some cases.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9655609efa4
Move session deletion on teardown into webdriver lib, r=webdriver-reviewers,whimboo

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/d9655609efa4