Closed Bug 1764314 Opened 10 months ago Closed 8 months ago

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

Categories

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

defect
Points:
8

Tracking

(firefox103 fixed)

RESOLVED FIXED
103 Branch
Tracking Status
firefox103 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webdriver:m4])

Attachments

(3 files)

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?

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

Severity: -- → S3
Points: --- → 8
Priority: -- → P3
Whiteboard: [webdriver:triage] → [bidi-m3-mvp]

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

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [bidi-m3-mvp] → [webdriver:m4]

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.

Two private methods which were still listed as internal commands.

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.

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.

Pushed by jdescottes@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
You need to log in before you can comment on or make changes to this bug.