Andreas Tolfsen ❲:ato❳ Reporter
	Description • 7 years ago

The Delete Session command should always return success.  If there is no active session, it should not try to close it.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 1 • 7 years ago

Given that lots of people are affected by that I will take it. We should ship a fix with the 0.20 release.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 7 years ago

So that bug is actually interesting and currently I don't know yet if that is in geckodriver, Selenium, or both. To figure that out I might want to use raw HTTP packages to send the commands directly to geckodriver. 

By using `close()` on the last window from Selenium, we internally delete the session, which also kills the browser process. Also using `close()` again reports an error with the message: `Tried to run command without establishing a connection`. Exactly that doesn't happen when I use the `quit()` method from Selenium right after each other. So there is something special with `quit` in Selenium.

Also I'm not sure what to do in geckodriver if a `DeleteSession` command is going to run an no connection has been established. Should we silently ignore the command, and don't send it while returning success? Or should it also fail with a no connection error? I feel we should simply ignore it.

All that should become more clear when I run the pure commands without Selenium.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago

So I can reproduce it with the following commands by not using Selenium:

> curl http://localhost:4444/session
> curl -X DELETE http://localhost:4444/session/d0b67bbe-5744-9641-a316-72fa9e5c0c9b/window
> curl -X DELETE http://localhost:4444/session/d0b67bbe-5744-9641-a316-72fa9e5c0c9b

In this case the first call to delete session works, but the second fails with:

> "error":"session not created","message":"Tried to run command without establishing a connection"

This is expected given that the Firefox process doesn't exist anymore, and as such we cannot send the command at all.

Given the WebDriver spec we should still try to send send the command, and return success in all cases. But given that we do not have a connection at all it means that special-casing this method would be necessary. So it would behave differently compared to all the other commands. Is that really what we want?

David, before I actually get started to work on a fix, could you please tell me what you think? Would this special-casing be fine?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 4 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #3)

> So I can reproduce it with the following commands by not using
> Selenium:
> 
> > curl http://localhost:4444/session
> > curl -X DELETE http://localhost:4444/session/d0b67bbe-5744-9641-a316-72fa9e5c0c9b/window
> > curl -X DELETE http://localhost:4444/session/d0b67bbe-5744-9641-a316-72fa9e5c0c9b
> 
> In this case the first call to delete session works, but the
> second fails with:
> 
> > "error":"session not created","message":"Tried to run command without establishing a connection"
> 
> This is expected given that the Firefox process doesn't exist
> anymore, and as such we cannot send the command at all.

The expected behaviour here is for the second HTTP request to close
the window and end the session implicitly, and for the third request
to return an error.

You are probably right that Selenium has a special-case when
invoking the Delete Session command from a user-facing API level
because I think they want repeated calls to driver.quit() to act as
in-ops.

> Given the WebDriver spec we should still try to send send the
> command, and return success in all cases. But given that we do not
> have a connection at all it means that special-casing this method
> would be necessary. So it would behave differently compared to all
> the other commands. Is that really what we want?

I think you may have misread the specification.  If the session
does not exist you will hit step 3 of the processing model, which
instructs the remote end to return an error if it can’t find an
active session by ID:

> 3. If the current session is null send an error with error code
> invalid session id, then jump to step 1 in this overall algorithm.

I believe the specification has recently changed in this area, which
is why we are returning the incorrect ‘session not created’
error when we should be returning ‘invalid session id’.

	David Burns :automatedtester
	Comment 5 • 7 years ago

:ato has answered this sufficiently.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 6 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #4)
> You are probably right that Selenium has a special-case when
> invoking the Delete Session command from a user-facing API level
> because I think they want repeated calls to driver.quit() to act as
> in-ops.

What do you mean with in-ops? As you filed this bug we have only two situations:

1) There is an active sessions and this leads to return success

2) There is no active session, and running a command including `deleteSession` causes a failure

So what should this part be:

> If there is no active session, it should not try to close it.

We do not have such a situation, given that we always shutdown the browser when deleting a session, or closing the last window.

> I think you may have misread the specification.  If the session
> does not exist you will hit step 3 of the processing model, which
> instructs the remote end to return an error if it can’t find an
> active session by ID:
> 
> > 3. If the current session is null send an error with error code
> > invalid session id, then jump to step 1 in this overall algorithm.
> 
> I believe the specification has recently changed in this area, which
> is why we are returning the incorrect ‘session not created’
> error when we should be returning ‘invalid session id’.

I did not have a look at the processing model! I only checked the `Delete Session` section. So this gives indeed a bit more details.

So we don't hit any of those steps because after closing the last window the process gets quit, and the connection goes down. So calling `Delete Session` afterward would fail in:

> After such a connection has been established, a remote end MUST run the following steps

I don't see which kind of failure should be returned if we don't reach that state.