1759169 - Start the Remote Agent earlier during the startup of Firefox

Status: RESOLVED FIXED

(Remote Protocol :: Agent, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Similar to what I'm working on for Marionette over on bug 1726465 we should also start the Remote Agent earlier during startup of Firefox. For Marionette we seem to go with final-ui-startup.

Right now I'm not sure if we can expect regressions for the Remote Agent especially for CDP and when clients try to connect before a browser window is open. Any change should be thoughtful tested.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: Bug 1759169 - [remote] Initialize the Remote Agent before the first top-level window has been opened.

Depends on D140368

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

I pushed a patch to try and as it looks like it's mainly the Puppeteer launcher tests which cause shutdown hangs:
https://treeherder.mozilla.org/jobs?repo=try&revision=90c73c5ae251dd55b60d9ef66f460d890f761f7c

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: Bug 1759169 - [remote] Add DeferredPromise helper.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Note that the crashes seem to have started to happen due to one of the commits in this range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3649f9875f3ded66f3f50b8ecfd21a0a7ea438f2&tochange=10178bda5cdd23c669578c163c350adf728a08a8

I might further bisect when I find the time. For now I would try to delay the CDP startup until the first window is ready.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Some more updates... when our CDP protocol gets started with the final-ui-startup observer and a CDP client connects immediately and triggers a shutdown of the browser Firefox ends-up hanging during shutdown with the following output:

FATAL ERROR: AsyncShutdown timeout in IOUtils: waiting for profileBeforeChange IO to complete Conditions: [{"name":"CrashMonitor: Writing notifications to file after receiving profile-before-change and awaiting all checkpoints written","state":{"profile-after-change":true,"final-ui-startup":true,"quit-application-granted":true,"sessionstore-windows-restored":true,"quit-application":true,"profile-change-net-teardown":true,"profile-change-teardown":true,"p 

https://treeherder.mozilla.org/jobs?repo=try&revision=bb1ec799634ffeaf1dab7999a3879b5b055f95bd&selectedTaskRun=TPsjrpYhQ-ip6cAtDdHEyA.0

This seems to be related to changes from bug 1749996. I wonder if it is expected that we hang and see a crash here when code tries to shutdown the browser earlier. Barrett could you please shed some light on this? If that should not happen (what I assume) I'm happy to file a bug. Thanks.

Comment 6

[Beth Rennie [:beth] (she/her)]

The culprit here is CrashMonitor.jsm is waiting for SessionStore to complete its final write but if we are shutting down before session store is properly initialized, I don't think it will ever write to disk and thus we crash. This is not intended and needs a fix. We've been working around this in tests by making them less artificial (i.e., avoiding very soon-after-startup shutdowns).

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: WIP: Bug 1759169 - [remote] Await "quit-application" notification in browser.close command.

Depends on D143380

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: Bug 1759169 - [puppeteer] Use separate tests for "Puppeteer.launch should filter out ignored default arguments".

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: Bug 1759169 - [puppeteer] Also kill Firefox when temporary profile is used.

Depends on D144027

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/183793870b8c
[puppeteer] Use separate tests for "Puppeteer.launch should filter out ignored default arguments". r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/2ae34a4a5e03
[puppeteer] Also kill Firefox when temporary profile is used. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/a29fcae74671
[remote] Add DeferredPromise helper. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/0a781928b33b
[remote] Initialize the Remote Agent before the first top-level window has been opened. r=webdriver-reviewers,geckoview-reviewers,jdescottes,agi,mossop

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/183793870b8c
https://hg.mozilla.org/mozilla-central/rev/2ae34a4a5e03
https://hg.mozilla.org/mozilla-central/rev/a29fcae74671
https://hg.mozilla.org/mozilla-central/rev/0a781928b33b