1484909 - Intermittent /webdriver/tests/delete_session/delete.py | test_dismissed_beforeunload_prompt - SessionNotCreatedException: session not created (500): Tried to run command without establishing a connection

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

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

+++ This bug was initially created as a clone of Bug #1473681 +++

PR: https://github.com/web-platform-tests/wpt/pull/11809
Details from upstream follow.

Jim Evans <[e-mail]> wrote:
>  Correcting expected exception type for WebDriver delete session test
>  
>  The SessionNotCreatedException should be thrown if an attempt to create a
>  session fails. If a user tries to access a session that existed, but has
>  already been deleted, the correct exception is InvalidSessionIdException.

During the sync the following failed:

(In reply to Web Platform Test Sync Bot from comment #3)
> Ran 1 tests and 2 subtests
> OK     : 1
> PASS   : 1
> FAIL   : 1
> 
> Existing tests that now have a worse result (e.g. they used to PASS and now
> FAIL):
> /webdriver/tests/delete_session/delete.py
>     test_dismissed_beforeunload_prompt: FAIL

The problem here most likely lays in geckodriver reporting the wrong error. But also in Marionette we can clean-up some code.

Comment 1

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

Attachment: [marionette] Fix handling of InvalidSessionException

The Marionette client raises a custom MarionetteException
instead of passing through the InvalidSessionIdException as
returned by the Marionette server.

Comment 2

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

Attachment: [geckodriver] Return "invalid session id" error when there is no active session

If a command is used before creating a new session, an
"invalid session id" error has to be returned by the
driver.

Comment 3

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

Attachment: [wdclient] Fix handling of "invalid session id" error

Without a session being created first the "send_command" method
inappropriately checks for the "session not created" error, which
only gets raised by the "new session" command. For all the other
commands the "invalid session id" error has to be handled.

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c216ab68ce8609e053540463c0c970e1646b3a3

Comment 5

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

Attachment: [marionette] Fix handling of InvalidSessionException

The Marionette client raises a custom MarionetteException
instead of passing through the InvalidSessionIdException as
returned by the Marionette server.

Comment 6

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

There were some failures for Mn jobs because I missed to update the expected exceptions in test_quit_restart.py and test_crash.py. Here a new try build for Mn only:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49e947cb4aeacc60bcd5e93278dc384a10c8dbea

Comment 7

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

Attachment: [marionette] Fix handling of InvalidSessionException

The Marionette client raises a custom MarionetteException
instead of passing through the InvalidSessionIdException as
returned by the Marionette server.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9002671 [details] [diff] [review]
[wdclient] Fix handling of "invalid session id" error

Review of attachment 9002671 [details] [diff] [review]:
-----------------------------------------------------------------

Giving this a tentative r+.  I expect the tests will pass, but we
want Session.end() to be callable also when the remote is dead.

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py
@@ -423,5 @@
>  
>      def end(self):
> -        """Tries to close the active session."""
> -        if self.session_id is None:
> -            return

I’m afraid what side effects might happen if we remove this
if-condition.  For example, if the remote is not listening I would
guess that send_command would fail with some other error than
InvalidSessionIdException.

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9002693 [details] [diff] [review]
[marionette] Fix handling of InvalidSessionException

Review of attachment 9002693 [details] [diff] [review]:
-----------------------------------------------------------------

I’m sure this is fine.

Comment 10

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

Comment on attachment 9002671 [details] [diff] [review]
[wdclient] Fix handling of "invalid session id" error

Review of attachment 9002671 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/tools/webdriver/webdriver/client.py
@@ -423,5 @@
>  
>      def end(self):
> -        """Tries to close the active session."""
> -        if self.session_id is None:
> -            return

Oh! I missed to revert this change. I had it similarly for Marionette client but then also reverted it there due to the reasons you mention here.

I will revert this change.

Comment 11

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

Attachment: [wdclient] Fix handling of "invalid session id" error

Without a session being created first the "send_command" method
inappropriately checks for the "session not created" error, which
only gets raised by the "new session" command. For all the other
commands the "invalid session id" error has to be handled.

Comment 12

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

Comment on attachment 9002708 [details] [diff] [review]
[wdclient] Fix handling of "invalid session id" error

Review of attachment 9002708 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, the flag got deleted. Mind re-reviewing?

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9002708 [details] [diff] [review]
[wdclient] Fix handling of "invalid session id" error

Review of attachment 9002708 [details] [diff] [review]:
-----------------------------------------------------------------

In this case I would be fine if you just landed the change with the
amendment on inbound.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/993d8d079fb3
[marionette] Fix handling of InvalidSessionException.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e28ac045028
[geckodriver] Return "invalid session id" error when there is no active session.
https://hg.mozilla.org/integration/mozilla-inbound/rev/91baa8e095b5
[wdclient] Fix handling of "invalid session id" error.

Comment 15

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12586 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/993d8d079fb3
https://hg.mozilla.org/mozilla-central/rev/8e28ac045028
https://hg.mozilla.org/mozilla-central/rev/91baa8e095b5

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged