Closed Bug 1484909 Opened 2 years ago Closed 2 years ago

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

Categories

(Testing :: geckodriver, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(3 files, 3 obsolete files)

+++ 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 <james.h.evans.jr@gmail.com> 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.
The Marionette client raises a custom MarionetteException
instead of passing through the InvalidSessionIdException as
returned by the Marionette server.
If a command is used before creating a new session, an
"invalid session id" error has to be returned by the
driver.
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.
The Marionette client raises a custom MarionetteException
instead of passing through the InvalidSessionIdException as
returned by the Marionette server.
Attachment #9002669 - Attachment is obsolete: true
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
The Marionette client raises a custom MarionetteException
instead of passing through the InvalidSessionIdException as
returned by the Marionette server.
Attachment #9002687 - Attachment is obsolete: true
Attachment #9002670 - Flags: review?(ato)
Attachment #9002671 - Flags: review?(ato)
Attachment #9002693 - Flags: review?(ato)
Attachment #9002670 - Flags: review?(ato) → review+
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.
Attachment #9002671 - Flags: review?(ato) → review+
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.
Attachment #9002693 - Flags: review?(ato) → review+
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.
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.
Attachment #9002671 - Attachment is obsolete: true
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?
Attachment #9002708 - Flags: review?(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.
Attachment #9002708 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
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.
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12586 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/993d8d079fb3
https://hg.mozilla.org/mozilla-central/rev/8e28ac045028
https://hg.mozilla.org/mozilla-central/rev/91baa8e095b5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.