1764314 - Handle AbortErrors when calling commands (eg "browsingContext._getBaseURL")

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P1)

Description

[Julian Descottes [:jdescottes]]

While investigating Bug 1763692, I intermittently had issues with handleCommand throwing with an AbortError, because the JSWindowActor pair we relied on was destroyed while waiting for the query.

https://searchfox.org/mozilla-central/rev/22a9cf9388cefa244ea2558efa748ce17a38d607/remote/webdriver-bidi/modules/root/browsingContext.jsm#249

This is something we also faced with Marionette and handled by adding a proxy to "retry" queries in Bug 1671347

For the initial MessageHandler implementation, I did not want to have a retry mechanism built in by default, because I felt like it could hide issues.

But we should start discussing how we want to handle that. Do we want a low-level (query) retry mechanism, same as in marionette. Do we want to handle it at command level?

Comment 1

[Julian Descottes [:jdescottes]]

We could add a default behavior (retry or not) and then allow each call to a command to override this.

Comment 2

[Julian Descottes [:jdescottes]]

Since Bug 1767226 started spiking, let's try to fix this.

Comment 3

[Julian Descottes [:jdescottes]]

Only commands sent to a windowglobal MessageHandler are impacted here.

And for now, this is only about browsingContext._getBaseUrl.

All other commands are either fully handled in root modules, or are about events which get propagated using SessionData and therefore are not subject to AbortError.

However the very next command we plan to implement, script.evaluate, will be sent explicitly to a windowglobal MessageHandler, so it's probably worth at least having this use case in mind when fixing this bug.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1764314 - [bidi] Cleanup private methods for events in windowglobal/log module

Two private methods which were still listed as internal commands.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1764314 - [remote] Allow MessageHandler commands to be retried upon AbortError

Depends on D147713

This adds a "retryOnAbort" property on MessageHandler Commands.
When set to true, this will allow the FrameTransport to retry a command up to 10 times in case of an AbortError.
Difference with Marionette, this doesn't attempt to detect BrowsingContexts in bfcache, because retrying commands
does not make sense in bfcache (this is not a temporary state, retrying in a loop will most likely not help).

A browser mochitest is added to cover various retry scenarios.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1764314 - [bidi] Allow browsingContext.navigate to retry the _getBaseURL command

Depends on D147715

No test added, hopefully this should address intermittents such as Bug 1767226
_getBaseURL is so far the only command explicitly sent to a windowglobal MessageHandler,
and therefore is the only one we should consider for the retry behavior.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad52948eb92f
[bidi] Cleanup private methods for events in windowglobal/log module r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/ac0842a7d090
[remote] Allow MessageHandler commands to be retried upon AbortError r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/72e63add1c92
[bidi] Allow browsingContext.navigate to retry the _getBaseURL command r=webdriver-reviewers,whimboo

Comment 8

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ad52948eb92f
https://hg.mozilla.org/mozilla-central/rev/ac0842a7d090
https://hg.mozilla.org/mozilla-central/rev/72e63add1c92