I would like for marionette to support connecting to Firefox when it is Silently Restarted (see Bug 1726239).

I'll quote Bug 1726460 to explain the issue:

... marionette cannot connect to Firefox in its Silent Restart state because Firefox's marionette server doesn't start until a window opens.

I tried adding these observer notifications to the Silent Restart command line handler. Unfortunately, even then, the connection does not work. I started getting errors coming from Firefox's marionette server's newSession function. I tried passing some parameters through to that function to prevent it from attempting to access windows, but wasn't enough either.

Note that Marionette primarily relies on the marionette-startup-requested observer notification. And this gets fired by Firefox itself when session restore has finished loading all the windows. So it's not just Marionette that would need an update to handle a window-less startup of Firefox. Not sure what the best place would be to actually send the marionette-startup-requested notification. Right now it's run as late as possible to not negatively impact the startup time of other components.

Florian, where would we have to move the above mentioned code for both Marionette and the Remote Agent so that the observer notifications get send out correctly and we can support a silent Firefox restart? I wouldn't use the command line handler for that, as done in a prototype by Kirk given that we have to send these out only once.

Beside that change we indeed would have to make further adjustments to the Marionette startup code, because right now we rely on an already existing browser window. Changing that might not be that easy especially for not affecting any existent consumer of geckodriver/Marionette. For example the existent Firefox UI tests for session restore would be busted, and would have to wait until all the windows have been restored.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

Florian, where would we have to move the above mentioned code for both Marionette and the Remote Agent so that the observer notifications get send out correctly and we can support a silent Firefox restart? I wouldn't use the command line handler for that, as done in a prototype by Kirk given that we have to send these out only once.

Just to add my main worry here is to add any kind of startup performance regression when moving the marionette-startup-requested notification. And when we move it we might also wanna do it for remote-startup-requested at the same time (which backs WebDriver BiDi).

Which test harness do we actually use for measuring startup performance? Is it just Talos or also some other harness?

Maybe we could try to just ignore the marionette-startup-requested notification in Marionette.jsm, and move the toplevel-window-ready notification handling over to WebDriver:NewSession. There we could define the behavior to wait for a top-level window or not based on a vendor prefixed capability like moz:WindowLess to not wait for any browser window, and maybe work with the hidden window on MacOS? Note that all the WebDriver commands require a valid browsing context.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #1)

Florian, where would we have to move the above mentioned code for both Marionette and the Remote Agent so that the observer notifications get send out correctly and we can support a silent Firefox restart? I wouldn't use the command line handler for that, as done in a prototype by Kirk given that we have to send these out only once.

Sorry for not replying earlier in the bug (we had a brief Matrix chat a while ago). The short answer is "I don't know, I would look at a profile of a windowless startup of Firefox to figure it out". I was hoping to find time to capture that profile and look at it to give you a better answer... But that hasn't happened.

To look at the startup impact, I would profile startup with the planned changes. There are some Talos tests looking at startup, but I'm not sure they match real life scenario (they focus on warm startup, which is pretty artificial). We also have some telemetry data about startup time.

Since Firefox 84 the GFX sanity window is force disabled in
Marionette by a preference (bug 1572687). As such the related
code can be removed.

Depends on D134275

Depends on D134276

I've attached some patches that actually make the feature work for me locally. There are still some dependencies that need to be fixed first, and a proper refactoring of the Marionette JS code is required to better handle registered events and notifications.

I'll continue to work on this bug over the next couple days.

Depends on D134276

Depends on D134386

Geoff, please note that we soon might initialize Marionette way earlier in the startup process. As such the marionette-startup-requested notification needs to be also send out earlier in Thunderbird.

Agi, with this patch we are going to initialize Marionette way earlier during startup of the application. This allows us to accept socket connections from clients even before the first window has been opened. I'm not sure where in the GeckoView code the marionette-startup-requested notification should be moved to. Maybe you have a hint for me? Thanks!

With the patch that causes Marionette to start way earlier (not awaiting the toplevel-browser-window notification I can see a permanent failure in the test test_preloader_telemetry.py:

FAIL js/xpconnect/tests/marionette/test_preloader_telemetry.py TestScriptPreloader.test_preloader_requests_histogram - AssertionError: 64 not greater than 152.5
Traceback (most recent call last):
  File "/Users/henrik/code/gecko/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 202, in run
    testMethod()
  File "/Users/henrik/code/gecko/js/xpconnect/tests/marionette/test_preloader_telemetry.py", line 43, in test_preloader_requests_histogram
    self.assertSimilar(misses, hits)
  File "/Users/henrik/code/gecko/js/xpconnect/tests/marionette/test_preloader_telemetry.py", line 52, in assertSimilar
    self.assertGreater(lhs, rhs / 2)

This is caused by the ratio between misses and hits will change:

Currently:

1640159269852 Marionette DEBUG 1 <- [1,5,null,{"value":{"0":282,"1":9,"2":178,"3":0}}]

With my patch:

1640160243185 Marionette DEBUG 1 <- [1,5,null,{"value":{"0":305,"1":11,"2":64,"3":0}}]

As you can see the number of misses of the script cache is only a third now. So I wonder what's actually measured here?

I wouldn't expect a change here given that starting a new Marionette session for this test will still be done once the first browser window has appeared and finished loading.

Doug or Kris, maybe one of you could explain what that means and if that is an improvement (and the test needs to be updated) or a regression as caused by my change? Thanks!

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #10)

Geoff, please note that we soon might initialize Marionette way earlier in the startup process. As such the marionette-startup-requested notification needs to be also send out earlier in Thunderbird.

Thanks for the heads-up. I've tried running mochitest with all of the patches from this stack applied and it still worked, so hopefully that is still the case when you come to land as the Thunderbird team will have very limited availability over Christmas/New Year.

Great to hear that! The landing will probably not happen before mid January. There are still quite some issues to fix first.

Geoff, did you also make the change to MailGlue.js similar to what I did to BrowserGlue.jsm?
https://searchfox.org/comm-central/source/mail/components/MailGlue.jsm#576-591

Hmm. That does seem weird. To be clear, all this does is cause marionette to be initialized earlier? I don't quite have an explanation that makes sense for this. If you have worries that your patch might be affecting real Firefox startup performance, you should add logging into ScriptPreloader::GetCachedStencil and diff the logs of hits/misses to see what's changing.

But anyway, maybe Kris has a different opinion, and he'd certainly be more qualified than me to answer, but I'm not particularly worried about what we're seeing here. Fewer misses and more hits generally shouldn't be bad - the only slightly worrying thing is that the total number of requests went down a little bit. So I would be fine with updating the test, but bonus points for seeing a log of GetCachedStencil requests before and after your patch.

(In reply to Doug Thayer [:dthayer] (he/him) from comment #16)

But anyway, maybe Kris has a different opinion, and he'd certainly be more qualified than me to answer, but I'm not particularly worried about what we're seeing here. Fewer misses and more hits generally shouldn't be bad - the only slightly worrying thing is that the total number of requests went down a little bit. So I would be fine with updating the test, but bonus points for seeing a log of GetCachedStencil requests before and after your patch.

As first step I simply moved the marionette-startup-requested notification within BrowserGlue.jsm to different places.

• end of _beforeUIStartup: {"value":{"0":304,"1":11,"2":58,"3":0}}
• end of _onFirstWindowLoaded: {"value":{"0":286,"1":9,"2":105,"3":0}}
• end of _scheduleStartupIdleTasks() (after browser-startup-idle-tasks-finished): {"value":{"0":282,"1":9,"2":178,"3":0}}

This list is sorted by time called. As later during the start-up process we initialize Marionette as more misses we actually collect.

To prevent the shutdown hangs and crashes I'm going to move the marionette-startup-requested notification from the beginning of _beforeUIStartup to its end. This might even be the right choice so that we can still correctly handle the safe mode dialog during startup.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #11)

Agi, with this patch we are going to initialize Marionette way earlier during startup of the application. This allows us to accept socket connections from clients even before the first window has been opened. I'm not sure where in the GeckoView code the marionette-startup-requested notification should be moved to. Maybe you have a hint for me? Thanks!

I think https://searchfox.org/mozilla-central/rev/7271a078fa0c1b858a52614ea60ac82fdd8b3d23/mobile/android/components/geckoview/GeckoViewStartup.jsm#204 would be a good place.

Could starting marionette cause performance problems? That code path is very performance sensitive as it blocks starting everything else in the browser.

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #19)

I think https://searchfox.org/mozilla-central/rev/7271a078fa0c1b858a52614ea60ac82fdd8b3d23/mobile/android/components/geckoview/GeckoViewStartup.jsm#204 would be a good place.

This would actually be too early. In Marionette we add the marionette-startup-requested observer within command-line-startup, which itself gets registered in profile-after-change. The code you referenced above is within the profile-after-change handling. It means that we would never receive such a notification because it is sent out before Marionette actually has checked itself if it is enabled or not.

So do you have a suggestion where it can be enabled later? In Firefox we might do it at the end of _beforeUIStartup (BrowserGlue.jsm).

Could starting marionette cause performance problems? That code path is very performance sensitive as it blocks starting everything else in the browser.

Marionette only loads modules if it is enabled via --marionette or the environment variable. If not nothing gets run.

Comment on attachment 9256327 [details]
WIP: Bug 1726465 - [marionette-client] marionette.restart() has to preserve requested capabilities.

Revision D134386 was moved to bug 1750711. Setting attachment 9256327 [details] to obsolete.

I've updated the patches given that Marionette and Firefox-UI tests are close to passing all. Wdspec seem to be completely fine:
https://treeherder.mozilla.org/jobs?repo=try&revision=339d5aa820f541fdc7e75f12501174eadade94be

There is only the problem with js/xpconnect/tests/marionette/test_preloader_telemetry.py.

(In reply to Doug Thayer [:dthayer] (he/him) from comment #16)

But anyway, maybe Kris has a different opinion, and he'd certainly be more qualified than me to answer, but I'm not particularly worried about what we're seeing here. Fewer misses and more hits generally shouldn't be bad - the only slightly worrying thing is that the total number of requests went down a little bit. So I would be fine with updating the test, but bonus points for seeing a log of GetCachedStencil requests before and after your patch.

I'll try to get these logs and hope that we can figure out what's causing the changes.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #22)

I've updated the patches given that Marionette and Firefox-UI tests are close to passing all. Wdspec seem to be completely fine:
https://treeherder.mozilla.org/jobs?repo=try&revision=339d5aa820f541fdc7e75f12501174eadade94be

There is only the problem with js/xpconnect/tests/marionette/test_preloader_telemetry.py.

As it turned out there are actually two underlying problems.

First the helper method in the test module that is waiting for a given observer notification is using the wrong Marionette command:

https://searchfox.org/mozilla-central/rev/d6850ecec549f9a1b10dd8ec8823db3791cf81fd/js/xpconnect/tests/marionette/test_preloader_telemetry.py#65-73

By using execute_script the script will run but also immediately return. To await the notification execute_async_script needs to be called. Making that change resolves the issue and the test passes.

Second when starting Marionette earlier the code that retrieves the histogram for SCRIPT_PRELOADER_REQUESTS is also run earlier which is leading to these different numbers. Note that not all the modules have been loaded at that time. Awaiting the observer notification for browser-startup-idle-tasks-finished which marks that all the initial tasks have been done let the test pass. This addition is necessary because Marionette will no longer be initialized after this notification.

Depends on D140367

Depends on D134387

Depends on D140369

Depends on D140370

(In reply to [PTO] Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #19)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #11)

Agi, with this patch we are going to initialize Marionette way earlier during startup of the application. This allows us to accept socket connections from clients even before the first window has been opened. I'm not sure where in the GeckoView code the marionette-startup-requested notification should be moved to. Maybe you have a hint for me? Thanks!

I think https://searchfox.org/mozilla-central/rev/7271a078fa0c1b858a52614ea60ac82fdd8b3d23/mobile/android/components/geckoview/GeckoViewStartup.jsm#204 would be a good place.

I actually tried that now with the current set of patches but when moving the code Marionette doesn't start at all anymore. Maybe you have an idea what's wrong. If not I'll have to dig into next week.

Geoff, similar to the changes to Firefox we would like to see that Marionette and the Remote Agent start earlier during startup. Right now we initialize both components here:

https://searchfox.org/comm-central/rev/e9d7ef8bfd4e0b63b1896b45578a811665d2859e/mail/components/MailGlue.jsm#602-605

Would it be ok with you to also move that code block to the end of _beforeUIStartup()? I assume that's what you have to change anyway when the code merges to mozilla-central. Thanks!

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #29)

I think https://searchfox.org/mozilla-central/rev/7271a078fa0c1b858a52614ea60ac82fdd8b3d23/mobile/android/components/geckoview/GeckoViewStartup.jsm#204 would be a good place.

I actually tried that now with the current set of patches but when moving the code Marionette doesn't start at all anymore. Maybe you have an idea what's wrong. If not I'll have to dig into next week.

Does that code get executed? do you see the profile-after-change event? there might be something really broken if not.

Comment on attachment 9266542 [details]
WIP: Bug 1726465 - Improvements for test_preloader_telemetry.py. r?dthayer

Revision D140367 was moved to bug 1758909. Setting attachment 9266542 [details] to obsolete.

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #31)

I actually tried that now with the current set of patches but when moving the code Marionette doesn't start at all anymore. Maybe you have an idea what's wrong. If not I'll have to dig into next week.

Does that code get executed? do you see the profile-after-change event? there might be something really broken if not.

The problem here is that Marionette gets enabled by a command line argument. To handle the argument we have to register an observer for command-line-startup from within profile-after-change. So we clearly enable Marionette after profile-after-change. As such we would need an entry point for Marionette and Remote Agent which is after command-line-startup. Another option would be to just remove both observer notifications and hook into the application directly when we process command-line-startup. Would that be ok with you?

It looks like on desktop you're activating marionette in final-ui-startup? you could do the same on GeckoView (adding an observer to final-ui-startup).

So I talked to Mossop, and Agi and we all are ok with getting the marionette-startup-requested observer removed and instead let Marionette listening for final-ui-startup itself. This notification seems to work for each and every application based on Gecko.

I'll leave the remote-startup-requested observer for now, and remove it in the same as I do with Marionette but in a follow-up bug.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #30)

Geoff, similar to the changes to Firefox we would like to see that Marionette and the Remote Agent start earlier during startup. Right now we initialize both components here:

https://searchfox.org/comm-central/rev/e9d7ef8bfd4e0b63b1896b45578a811665d2859e/mail/components/MailGlue.jsm#602-605

Would it be ok with you to also move that code block to the end of _beforeUIStartup()? I assume that's what you have to change anyway when the code merges to mozilla-central. Thanks!

That seems fine to me. Sorry about the slow reply, I somehow never noticed this NI.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #35)

I'll leave the remote-startup-requested observer for now, and remove it in the same as I do with Marionette but in a follow-up bug.

Actually this doesn't work. Marionette's New Session command requires the Remote Agent and the WebDriver BiDi protocol to be already active. As such we have to work on bug 1759169 first.

Depends on D134276

Comment on attachment 9266543 [details]
Bug 1726465 - [remote] Add DeferredPromise helper.

Revision D140368 was moved to bug 1759169. Setting attachment 9266543 [details] to obsolete.

Bug 1766314 isn't needed here given that bug 1767387 will fix the underlying issue.

With waiting long enough during the startup we do not hit the crash as handled on bug 1747209 so we shouldn't block this feature on it.

Depends on D134387

Backed out for causing several browser-chrome failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel&revision=74016350f30206bd057e8a9b843ff26f3914e3e7&selectedTaskRun=SiVaa-1KQ5GLrOPXR9Nplg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=377393612&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=377393602&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/e25c2349494cabfb9e0a2f06ca30268c020c3e53

The affected tests are testing the creation of screenshots in headless mode. Therefore they start a new Firefox process, which actually have Marionette enabled. Given that the first Firefox instance has Marionette running as well the Marionette port is taken. As result the Marionette code of the second Firefox process shutdown the application.

It was not failing before because Marionette started later and the forced quit happened after the screenshot had already been taken and saved to the file.

Now the question is why Marionette is enabled in the second Firefox process. I cannot see that the --marionette argument is getting passed in:
https://searchfox.org/mozilla-central/source/browser/components/shell/test/head.js#11-36

Per logging output the following arguments are passed in: -profile,/var/folders/sd/hgkqcwv90sz_q1pqjww78nfm0000gn/T/headless_test_screenshot_profile,-no-remote,-url,http://mochi.test:8888/browser/browser/components/shell/test/headless.html,-screenshot,/var/folders/sd/hgkqcwv90sz_q1pqjww78nfm0000gn/T/headless_test_screenshot.png.

As such it can only be the environment variable MOZ_MARIONETTE which is inherited by the new Firefox processes.

As such it can only be the environment variable MOZ_MARIONETTE which is inherited by the new Firefox processes.

This is a common failure mode: see, for example, https://bugzilla.mozilla.org/show_bug.cgi?id=1675332.

While the above can easily be fixed the failures in browser/base/content/test/performance/browser_startup_flicker.js are more concerning. These happen because we enable Marionette earlier and as such send out the marionette-listening earlier as well. This is causing the browser window to apply the remote cue immediately when it gets opened. Sadly this update is slow by the robot icon loaded lazily. This is causing image differences with the reference image which gets captured when the startup scripts have all been run.

With my patch we can no longer wait for the latter before enabling Marionette. As such we cannot easily defer the marionette-listening notification. I can see 3 options right now (hardest first):

1. Change Mochitest's runner to not use Marionette. Right now it's only purpose is to install the special powers and mochitest extensions. After that it's not used anymore during the whole lifetime of the browser. Right now I don't see an easy way how to get this changed at all beside restarting the browser with Marionette disabled, after the extension installation. But this means a lot of extra restarts for each browser mochitest manifest.

2. We fix bug 1696425 so that the remote cue is only activated when a WebDriver session is active. This would fix the test because it runs in in-windowless mode and requires a window to be present. While working on this we most likely trap into performance issues, which make it harder to change the logic. We would have to do that anyway but it might take a bit of time and it wasn't something scheduled for right now.

3. We could ignore bug 1696425for now because it doesn't affect the application's chrome cue. Firing the marionette-listening observer in WebDriver:NewSession when the first application window has been opened and the startup scripts been finished might get it working. But then we also have to fire it in WebDriver:DeleteSession to turn the cue off.

I would lean towards option 3 and would try out if it really works.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #48)

3. We could ignore bug 1696425for now because it doesn't affect the application's chrome cue. Firing the marionette-listening observer in WebDriver:NewSession when the first application window has been opened and the startup scripts been finished might get it working. But then we also have to fire it in WebDriver:DeleteSession to turn the cue off.

Now when thinking more about this solution the question comes up if we actually want to disable the remote cue when no WebDriver session is active or if it should be enabled all the time. Security wise the latter is definitely the better option and would us allow to inform the user about an active remote protocol across the whole lifetime of the browser process and not only when a potential attacker connected to the user's Firefox instance. This would mean that we most likely should drop options 2 and 3.

I've discussed with Florian on Element and there is actually a better way. We could prevent the browser from updating the remote control cue when the browser is running under automation (Cu.isInAutomation) and when a special preference is set only for these tests that measure the startup performance. Guarding that behavior behind these two flags should be safe enough. Lets see what browser peers / owners think about it.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #50)

I've discussed with Florian on Element and there is actually a better way. We could prevent the browser from updating the remote control cue when the browser is running under automation (Cu.isInAutomation) and when a special preference is set only for these tests that measure the startup performance. Guarding that behavior behind these two flags should be safe enough. Lets see what browser peers / owners think about it.

Speaking as an XPConnect peer and the de facto SpecialPowers owner, I support this approach.

Startup performance tests are browser-chrome tests which use Marionette
to install required extensions. Because Marionette will be initialized
earlier during the startup of Firefox this can cause a partially updated
remote control cue for the first opened browser window.

As such stop updating the remote control cue by guarding this behavior
behind two flags - when the browser is in automation and a special
preference set.

Depends on D145933

Backed out for causing failures on browser_preferences_usage.js.

Backout link : https://hg.mozilla.org/integration/autoland/rev/cd2eb9c687d7ad1f094ee607e9dcac75b2bd725e

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=4b57310622a00af76914d1898527d9b024ed6451&selectedTaskRun=Ou6CEXNBQdefo8cdctg-vg.0

Link to failure log: https://treeherder.mozilla.org/logviewer?job_id=377522540&repo=autoland&lineNumber=5904

The hopefully last remaining failure here was related to the module loader. In this patch I moved the registration of the JSWindowActors for Marionette commands and events to before the first application window is opened. This should not really be necessary and can be reverted.

Here a try build:
https://treeherder.mozilla.org/jobs?repo=try&revision=4d58f2c7860081cbc8177feefe42d5e1f76aa4e4

https://hg.mozilla.org/mozilla-central/rev/270db5a817d8
https://hg.mozilla.org/mozilla-central/rev/cf03994d3cf0
https://hg.mozilla.org/mozilla-central/rev/a0c1d7517502
https://hg.mozilla.org/mozilla-central/rev/4f801ff80d1d
https://hg.mozilla.org/mozilla-central/rev/0be37b042b50
https://hg.mozilla.org/mozilla-central/rev/36be55efd2be
https://hg.mozilla.org/mozilla-central/rev/693adc00b79a
https://hg.mozilla.org/mozilla-central/rev/91e613541d7c