1767387 - Sharing a WebDriver session with Marionette doesn't wait for browserStartupFinished

Status: RESOLVED FIXED

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

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Currently we have the situation on Android that WebDriver BiDi and CDP tests of the webdriver test suite are failing if tests make use of a direct connection and not geckodriver. If the tests use geckodriver all the tests are passing.

Here are the reasons for:

1. On Android the sessionstore-windows-restored observer notification is not used and as such the /session path handler is never registered.

2. WebDriver BiDi connections as initiated by WebDriver HTTP by passing the webSocketURL capability will not take the above code path. Instead they call WebDriverBiDi:createSession directly and setup the appropriate /session/%id% path handler.

To solve this two fixes are needed:

1. Instead of sessionstore-windows-restored we probably should use browser-idle-startup-tasks-finished for browsers and mail-idle-startup-tasks-finished for Thunderbird. We would still have to differentiate between these two and maybe others in the future. That opens the question if maybe we should keep the marionette-startup-requested and remote-startup-requested notifications whereby the latter would have to be re-introduced. It might be cleaner and applications can define where to get these added. Maybe we should rename these to a more appropriate name like marionette-final-startup-requested and remote-final-startup-requested?

2. Extending the check in createSession to not only cover the this.agent.running state but also awaits for browserStartupFinished.

Julian, would do you think about 1)? Would you join the idea to keep the remote/marionette specific notifications in the application code? I don't like the removal and re-adding of this code but maybe it's still better as having to handle different observers per application.

Comment 1

[Julian Descottes [:jdescottes]]

Thanks for the investigation. Basically we have to choose between relying on existing events or re-introducing specific events for remote.
Ideally I feel like we should have a platform event, that works on mobile/desktop/thunderbird etc... and which notifies us that startup is fully finished and that the browser can now be used.

Given the current situation, I don't have a strong preference. But if browser-idle-startup-tasks-finished is a good match here (minus the fact that it's called differently for Thunderbird), I lean towards using it instead of (re)introducing another event. I'm worried that with a different event, folks might move "browser-idle-startup-tasks-finished" but leave out our event because it's not clear that they are closely related. And we are not waiting for anything else than browser-idle-startup-tasks-finished, so we would really just create a duplicated event. If we had to wait for other things, if we had some custom logic... that would be another discussion.

Maybe in a followup we can start asking platform to align and emit the same event across all applications.

As I said no strong preference, so if you prefer to re-introduce custom events, I'm also fine with that.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Good points Julian! I agree and lets move forward by using these platform events instead, which means continuing what we already decided on. I'll make a patch to get this bug fixed as soon as possible.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This also blocks the windowless support in Marionette.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1767387 - [remote] Don't use sessionstore-windows-restored to determine whenapplication is fully started.

This observer notification is only available on desktop and for
applications that actually use the feature. This is not the case
for GeckoView and as such it cannot be used.

Instead the notifications "browser-idle-startup-tasks-finished"
and "mail-idle-startup-tasks-finished" can be used. These also
align to our formerly used custom "remote-startup-requested"
notification, which now no longer exists.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1767387 - [remote-bidi] Also await browser startup finished when creating a newsession from Marionette.

Previously the check has been added when starting the protocol.
But given that httpd.js gets started already in final-ui-startup
a new BiDi session request from Marionette can add required
path handlers, which results in accepting incoming commands
even with the browser not fully started up yet.

Depends on D145310

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1767387 - [remote-cdp] Register handlers for httpd.js before waiting for the initial application window.

Similar to WebDriver BiDi the JSON handlers for the CDP protocol
should be immediately registered, so that only the announcement
that CDP is enabled gets delayed.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1767387 - [wdspec] Remove remaining obsolete release_or_beta disable flags.

Depends on D145311

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c72c57c55390
[remote-cdp] Register handlers for httpd.js before waiting for the initial application window. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/fd4bd21f54fe
[remote] Don't use sessionstore-windows-restored to determine whenapplication is fully started. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/056c67d81fe6
[remote-bidi] Also await browser startup finished when creating a newsession from Marionette. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/621def30243d
[wdspec] Remove remaining obsolete release_or_beta disable flags. r=webdriver-reviewers,jdescottes

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c72c57c55390
https://hg.mozilla.org/mozilla-central/rev/fd4bd21f54fe
https://hg.mozilla.org/mozilla-central/rev/056c67d81fe6
https://hg.mozilla.org/mozilla-central/rev/621def30243d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9274782 [details]
Bug 1767387 - [remote] Don't use sessionstore-windows-restored to determine whenapplication is fully started.

Covered by the approval request in https://bugzilla.mozilla.org/show_bug.cgi?id=1753997#c14. Note for the future that we strongly prefer that each bug get its own approval requests, however. Approved for 101.0b3.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cc9ca5685410
https://hg.mozilla.org/releases/mozilla-beta/rev/3fe775ad16e9
https://hg.mozilla.org/releases/mozilla-beta/rev/9404abc25d38
https://hg.mozilla.org/releases/mozilla-beta/rev/639476d33fd4