1713782 - "Delete Session" inappropriately closes the browser even with "--connect-existing" specified

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Even with the --connect-existing argument set for geckodriver, the Delete Session command closes the browser. Instead only the active WebDriver session has to be closed.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Maybe the patch on bug 1705770 helps here.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Bug 1705770 actually doesn't fix it. A running instance is still killed when sending the following HTTP Post:

curl -H 'Content-Type: application/json' -d '{}' -X DELETE http://localhost:4444/session/7932a123-8161-7741-a270-2afcb45ddf1

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Under such a condition the trace log shows the following output:

1622705090665	webdriver::server	DEBUG	-> DELETE /session/cf6aca71-4be6-d646-9696-eaf314f959fe {}
1622705090744	webdriver::server	DEBUG	Deleting session
1622705090781	geckodriver::marionette	ERROR	Failed to close browser connection: Socket is not connected (os error 57)
1622705090781	webdriver::server	DEBUG	<- 200 OK {"value":null}

Note that we never reach the Browser:close() method here.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

So the actual problem here is that we force DeleteSession to actually call Marionette:Quit here:
https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/testing/geckodriver/marionette/src/marionette.rs#22-23

Keeping it calling WebDriver:DeleteSession will make it work. But then we would no longer shutdown the browser in normal scenarios, what should be the default.

The solution might be to check which kind of browser is in use when trying to convert to the Marionette message, and call the appropriate method WebDriver:DeleteSession vs Marionette:Quit, but that would mean we have to pass-down the browser instance into MarionetteCommand and then into try_convert_to_marionette_message:

https://searchfox.org/mozilla-central/rev/9975889f5c0d5c59bd22121a454beba774cbae71/testing/geckodriver/src/marionette.rs#686-690

James, do you see a better way here?

Comment 5

[James Graham [:jgraham]]

Attachment: Bug 1713782 - Don't close the browser when deleting session with --connect-existing,

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1dc45c680665
Don't close the browser when deleting session with --connect-existing, r=webdriver-reviewers,whimboo

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1dc45c680665