Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify closing the browser. #119

Closed
jgraham opened this issue Jul 14, 2021 · 9 comments
Closed

Specify closing the browser. #119

jgraham opened this issue Jul 14, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@jgraham
Copy link
Member

jgraham commented Jul 14, 2021

There should be a way to close the browser (and thereby terminate the session). c.f. https://chromedevtools.github.io/devtools-protocol/tot/Browser/#method-close

@whimboo whimboo added the enhancement New feature or request label Jul 15, 2021
@sadym-chromium
Copy link
Contributor

If we decide not to allow reconnections, closing or dropping connection would mean the browser should be closed.

Options described here: #25

@jgraham
Copy link
Member Author

jgraham commented Nov 26, 2021

I think we definitely want to support reconnection to an existing session; that was a request that came from clients and is pretty much already supported in the spec. Also, ending the session shouldn't necessarily close the browser; it makes more sense to build "end session" and "close browser" as separate primitives. Automation tools may have more cases where they close the browser and thereby implicitly end all sessions, but it still makes sense to seperate concerns.

@whimboo whimboo added the needs-discussion Issues to be discussed by the working group label Sep 7, 2022
@OrKoN
Copy link
Contributor

OrKoN commented Sep 7, 2022

Puppeteer is using this method and we will need it eventually to offer the same API. Currently, we use Browser.close for remote connections only. If the browser process is running locally, we kill the process.

Edit: we kill if the process is local AND the user profile folder was created by Puppeteer. If the user provides the custom user profile folder, we fallback to Browser.close

@whimboo
Copy link
Contributor

whimboo commented Sep 7, 2022

Note that force killing a process will NOT shutdown the browser cleanly. A lot of shutdown steps that the browser usually runs will be skipped and data-loss might be involved. So having a clean method to close the browser from within the application itself should be much preferred.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Closing the browser.

The full IRC log of that discussion <AutomatedTester> Topic: Closing the browser
<AutomatedTester> github: https://github.com//issues/119
<gsnedders> q+
<JimEvans> q+
<AutomatedTester> jgraham (IRC): The question on this topic is "should we allow people to close the borwser?"
<AutomatedTester> ... forcible quits might not clean up things as people expect
<AutomatedTester> ... since we aren't stopping multiple sessions then 1 session could kill other sessions
<sadym> q+
<AutomatedTester> ... the second question is "Should closing the last browsing context shutdown the browser"?
<AutomatedTester> q?
<AutomatedTester> ack next
<AutomatedTester> Sam Sneddon [:gsnedders]: in MacOS applications are singletons so safari automation is running in the same safari as users
<jgraham> q+
<AutomatedTester> ... but quiting the entire browser kills all sessions and I think that is unacceptable
<AutomatedTester> ack next
<AutomatedTester> Jim Evans: I am want this primative
<AutomatedTester> ... in the last 2 days I have been working on a bidi client
<AutomatedTester> ... I am able to use Firefox to get things working
<AutomatedTester> ... but I don't have a mechanism to kill the browser that could leave things in an incomplete state
<AutomatedTester> q?
<AutomatedTester> ack next
<AutomatedTester> sadym (IRC): my question is why shouldnt we just close the windows that automation has control over
<AutomatedTester> Sam Sneddon [:gsnedders]: when the session is closed we close the windows under automation control?
<AutomatedTester> sadym (IRC): if there are more than 1 session?
<AutomatedTester> Sam Sneddon [:gsnedders]: we only allow 1 session
<AutomatedTester> ... but I will defer to patrickangle
<AutomatedTester> patrickangle: we have things we need to untangle to support multiple sessions
<AutomatedTester> sadym (IRC): for quit we only need close the automation that should be fine?
<AutomatedTester> Sam Sneddon [:gsnedders]: it depends on how we spec the sematics
<AutomatedTester> jgraham (IRC): I agree what people want for desktop is that the browser process has shut down
<gsnedders> s/sematics/semantics/
<AutomatedTester> ... and it makes sense that this is not always implementable
<AutomatedTester> ... in bidi one could observe all the windows
<AutomatedTester> ... but I would expect apple would split what you can and can't observe
<gsnedders> s/apple/safari/
<AutomatedTester> ... for most use cases it would be sufficient that we just say that all automation windows are closed and shutdown
<JimEvans> q+
<AutomatedTester> ... I would expect the spec that we spec out that we close it all the automation windows
<jgraham> ack jgraham
<AutomatedTester> Sam Sneddon [:gsnedders]: it's not just safari that is a singleton app... all browsers are
<AutomatedTester> ... so all browsers can have this problem and you can get around it would be weird
<gsnedders> s/all browsers are/all browsers are on macOS/
<AutomatedTester> ack next
<gsnedders> s/all browsers are on macOS/all applications are on macOS/
<gsnedders> RRSAgent, make the minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2022/09/15-webdriver-minutes.html gsnedders
<AutomatedTester> Jim Evans: I think your assessment that the close on the browser closing all the automation windows is fine
<AutomatedTester> ... but from a test author point of view
<AutomatedTester> ... I can ensure restore state when the test is finished that the machine is back to the same state
<jgraham> q+
<AutomatedTester> ... if this isn't the case then it will cause issue reports if we leave anything behind
<AutomatedTester> ... and those issues will just be forwarded to the spec
<AutomatedTester> ack next
<AutomatedTester> jgraham (IRC): i think the sematics is we drop state of anything related to webdriver
<AutomatedTester> ... i think I agree that people will find it will be weird if we don't kill things
<AutomatedTester> ... but there are times where we can't fully kill things e.g. FirefoxOS/ChromeOS/KiaOS
<gsnedders> q+ to mention alternative suggestion
<AutomatedTester> ... I think we can add something to help clients "clean up the webdriver state'
<AutomatedTester> q?
<AutomatedTester> ack next
<Zakim> gsnedders, you wanted to mention alternative suggestion
<AutomatedTester> Sam Sneddon [:gsnedders]: my proposal is we have a flag that opts into closing all browsers that is fallible in an implementation specific way
<AutomatedTester> ... [explains how this could look using Safari based on state of different things]
<AutomatedTester> jgraham (IRC): yea. that's fine, but it also doesn't need a flag
<JimEvans> q+ for a quick poll of implementors before we end
<AutomatedTester> ... we need to think about mobile and we might not be able to get PID etc and wait for that
<AutomatedTester> ... or return that info
<AutomatedTester> q?
<AutomatedTester> ack next
<Zakim> JimEvans, you wanted to discuss a quick poll of implementors before we end
<AutomatedTester> Jim Evans: the bidi spec has a definition of a bidi only session
<AutomatedTester> ... Firefox has an implementation
<AutomatedTester> ... can we expect bidi only sessions in all browsers?
<AutomatedTester> Sam Sneddon [:gsnedders]: we can't comment on what is going into a future safari release
<AutomatedTester> sadym (IRC): technically you can do that now in chrome
<sadym> https://github.com/GoogleChromeLabs/chromium-bidi
<AutomatedTester> Brandon Walderman: edge will follow chromium
<AutomatedTester> q?
<gsnedders> q+
<jgraham> ack gsnedders
<jgraham> q+
<AutomatedTester> Sam Sneddon [:gsnedders]: going back to the comment on option that could be a capability to request a completely isolated instance would make sense
<AutomatedTester> ... for most that's a easy capability to satisfy
<AutomatedTester> ack next
<sadym> to @jimevans: WebDriver BiDi in Chromium could be used via ChromeDriver (classic+BiDi) or via Mapper (BiDi only)
<sadym> https://github.com/GoogleChromeLabs/chromium-bidi
<AutomatedTester> jgraham (IRC): I am trying to think how a client would do things? clients would have to understnad what it is speaking to
<gsnedders> q+ to also mention non-browser cases
<AutomatedTester> ... we need them to expect to handle all of this and not just call end and assume something else has handled it
<AutomatedTester> ... in the first case we dont have a return value or similar
<AutomatedTester> ... there are differences between OSes and people need to be aware
<AutomatedTester> ... I think we should just say that it cleans up the automation state
<AutomatedTester> ... we can figure out the wording
<sadym> q+
<AutomatedTester> ...we should say something about that we don't guarantee events of things being shutdown will be sent over
<AutomatedTester> q?
<AutomatedTester> ack next
<Zakim> gsnedders, you wanted to also mention non-browser cases
<AutomatedTester> Sam Sneddon [:gsnedders]: I wanted to point out that we can't guarantee that it will be a full browser, it could be a web view in an app
<AutomatedTester> ... it would be good to make sure that we can connect to an embedded webview
<AutomatedTester> ack next
<AutomatedTester> sadym (IRC): how do we test it with wpt? I am guessing we wont?
<AutomatedTester> q?
<jgraham> RRSAgent: make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2022/09/15-webdriver-minutes.html jgraham
<jgraham> Zakim, bye
<Zakim> leaving. As of this point the attendees have been AutomatedTester, karlcow, zcorpan, jgraham, sadym, BrandonWalderman, JimEvans, patrickangle
<AutomatedTester> github: end topic

@whimboo
Copy link
Contributor

whimboo commented Apr 12, 2023

Since the last discussion on Sep 16th last year we haven't made progress for this particular API. Now that we want to see more WebDriver BiDi only clients (also porting their code from CDP to BIDi) there is a higher need for such a command.

Maybe we should allow the browser process to be killed if the client actually started the process and hasn't attached to it? We might not be able to verify that on the remote end side but it could be the responsibility of the client to verify that? At least for the first step? If complains come up we could always improve the API.

@jgraham
Copy link
Member Author

jgraham commented Apr 12, 2023

I think there's pretty much a conclusion in the discussion above, which is that if the command succeeds it will end all automation sessions and close all windows that were under automation, but it won't necessarily actually terminate the top-level browser process. If you want to do that you need to use os-level features (like signalling the top level process), which of course means that you're subject to whatever normal permissions checks the OS has.

All that's really required here is someone to write up the command.

@jgraham
Copy link
Member Author

jgraham commented Apr 12, 2023

FWIW I think it should also be possible for the command to fail and end up just closing the actual session (e.g. that might be reasonable if you allow multiple automation sessions in a single browser instance and want to allow only the first session to actually have the ability to shut down the browser).

@whimboo
Copy link
Contributor

whimboo commented May 3, 2023

The required work was done on #286.

@whimboo whimboo closed this as completed May 3, 2023
@whimboo whimboo removed the needs-discussion Issues to be discussed by the working group label May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants