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

Standardize errors thrown when a command needs to be aborted (eg discarded context) #540

Open
juliandescottes opened this issue Sep 13, 2023 · 3 comments
Labels
needs-discussion Issues to be discussed by the working group

Comments

@juliandescottes
Copy link
Contributor

juliandescottes commented Sep 13, 2023

With the Firefox implementation, trying to send commands during a navigation will often result in an internal exception being thrown (AbortError). This is typically the case when the underlying context is destroyed by the navigation before it could handle whatever the command needed.

I think Chrome has similar errors when the browsing context becomes unavailable while the command is processed.

Could we agree on a standard error when a command needs to be aborted?
It can be useful for polling patterns for instance (used at least by puppeteer).

@juliandescottes juliandescottes changed the title Standardize errors thrown when a command fails because the context is discarded halfway through Standardize errors thrown when a command needs to be aborted (eg discarded context) Sep 13, 2023
@whimboo whimboo added the needs-discussion Issues to be discussed by the working group label Sep 19, 2023
@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Standardize errors thrown when a command needs to be aborted (GitHub issue).

The full IRC log of that discussion <jgraham_> Topic: Standardize errors thrown when a command needs to be aborted (GitHub issue)
<orkon> jgraham_ (IRC): lets discuss on the PR, it sounds like PR could be landed.
<jgraham_> github: https://github.com//issues/540
<orkon> whimboo: there are problems with Puppeteer: how should we handle commands that are interrupted? For example, script evaluation can be interruped by a navigation. For those cases we don't have anything in the spec and it would be great to have a specific error
<orkon> whimboo: so that the client can decide on the abort error what to do with the command
<jgraham_> q?
<orkon> whimboo: right not it is not possible for the client to have a correct behaviour
<orkon> q?
<jgraham_> q+
<orkon> jgraham_ (IRC): I agree we need this. From the spec point of view it is a bit tricky. I guess what we need a generic handler in the spec that if unload handler is invoked, then each running command is abort. Except maybe navigation commands because it should not be aborted. To begin with we could put a vague wording.
<gsnedders> q+
<gsnedders> ack
<jgraham_> ack next
<orkon> jgraham_ (IRC): we should have a DocumentNavigated error to know that the command was aborted
<jgraham_> ack gsnedders
<orkon> Sam Sneddon [:gsnedders]: we cannot use the unload handler because the unload handler can be cancelled in some circumtances.
<orkon> jgraham_ (IRC): I was thinking in the spec terms to be after there is no possibility to cancel the unload
<orkon> jgraham_ (IRC): in gecko we get a notification that a context is being destroyed
<orkon> Sam Sneddon [:gsnedders]: I think the complexity will be on how to spec this

@whimboo
Copy link
Contributor

whimboo commented Sep 13, 2024

Browsers may behave differently internally, so it could be challenging to create a specification for this. Perhaps commands should be implemented with the expectation that a browsing context could be replaced at any time. This may not be too difficult for most commands, except for script.evaluate and script.callFunction.

For example, here's a problem we encountered in Firefox when running the browsingContext.setViewport command while a new tab is being created. Currently, we send the browsingContext.contextCreated event too early, while the initial about:blank page is still loading (see also #766). As a result, the browsing context is replaced, causing our internal code—designed to wait for the page to render with the correct dimensions—to fail. Right now, we don’t ignore this failure but instead fail the entire command, which might not be necessary. Since the new page’s rendered output should immediately match the specified dimensions, we could simply retry the check.

@css-meeting-bot
Copy link
Member

The Browser Testing and Tools Working Group just discussed Browsing Context Replacements and Resilient Command Execution.

The full IRC log of that discussion <AutomatedTester> topic: Browsing Context Replacements and Resilient Command Execution
<AutomatedTester> github: https://github.com//issues/540
<AutomatedTester> whimboo: this was raise d awhile ago
<AutomatedTester> .... when we have browsing context and we have changed it to navigables
<AutomatedTester> ... when we architected this we worked with browsing contexts
<AutomatedTester> ... which was fine when there was creation since we just sent an about event
<AutomatedTester> ... we can do some basic things like get the event
<sadym> q+
<AutomatedTester> ... but we don't know what to do with actions when the page has been replaced partway through the execution
<AutomatedTester> ... we can repeat the command if the context gets replaced and error if things are missing that we are expecting.
<AutomatedTester> ... if we agree that we want to rerun the commands which is what is in firefox we can close this issue and raise another
<AutomatedTester> ... we noticed this with the latest change to navigables
<AutomatedTester> q?
<AutomatedTester> jgraham: I don't think it's obvious what we should do if executing the script and to context goes away. e.g. waiting for a promise
<AutomatedTester> ... I don't think it's defined. It could be "wait for ever"...
<AutomatedTester> ack sadym
<AutomatedTester> sadym: it highlights another issue where we we have navigables and contexts
<AutomatedTester> ... previously we said we didn't want to redo that work as it was a major breaking change
<AutomatedTester> jgraham: I don't think we have a resolution... we want to perhaps have a look at doing this for all commands. I think we need to relook at it
<AutomatedTester> RRSAgent: make minutes
<RRSAgent> I have made the request to generate https://www.w3.org/2024/09/27-webdriver-minutes.html AutomatedTester
<simons> q?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Issues to be discussed by the working group
Projects
None yet
Development

No branches or pull requests

3 participants