1726460 - Add automated testing for silent restarts

Status: RESOLVED FIXED

(Toolkit :: Application Update, task, P3)

Description

[Robin Steuber (she/her) [:bytesized]]

Attachment: wip.patch

This bug will track adding automated testing for Silent Restarts (Bug 1726239).

I am currently planning to land the Silent Restart feature without automated testing, because I don't believe that any of our testing frameworks can properly accommodate its testing needs. AFAIK, most of them are unable to tolerate Firefox restarting in the middle of a test. The exception is marionette, which makes it the obvious choice for this. Unfortunately, as things stand, 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.

At that point, I decided that the scale of changes that it would be necessary to make to marionette were outside the scope of this project. So I'm filing this bug to hopefully get tests added at a later time. I'm attaching the test that I wrote before the scope of the necessary marionette changes became clear.

Comment 1

[:Gijs (out for now; he/him)]

Could we add test-only code into the silent restart code that opens a window after waiting for 2 seconds or something along those lines? We can detect the test-only case by using Cu.isInAutomation

Alternatively, did you consider fixing marionette so it detects the silent restart case and doesn't wait for the parent window to open ( ie drop reliance on toplevel-window-ready - https://searchfox.org/mozilla-central/rev/9dceacf3d761eb91237108ec438d64099a56f442/remote/components/Marionette.jsm#149 and other places that gets mentioned in that file) -- but this may be more involved, you'd want to talk to someone, maybe jgraham or whimboo, about it...

Comment 2

[Robin Steuber (she/her) [:bytesized]]

(In reply to :Gijs (he/him) from comment #1)

Could we add test-only code into the silent restart code that opens a window after waiting for 2 seconds or something along those lines? We can detect the test-only case by using Cu.isInAutomation

We probably could, but at that point I'm not sure what we are testing for. If a window gets opened at startup (even 2 seconds later), the entire work of Bug 1726239 has kind of been defeated, so I'm not really sure what meaningful tests of that patch could be conducted at that point.

I'm not sure that using Cu.isInAutomation will actually work here. I believe that this limitation is why both checks are needed here. It might be reasonable to read the -marionette command line flag though. I believe that will be available at this point.

This could potentially enable some basic tests for Bug 1720742 so that we could check that we restart when windows were closed with an update ready (this is assuming that marionette is ok with having all the windows close - I haven't tried this yet). But I think that there are also other ways of testing this, like intercepting the quit() call.

Alternatively, did you consider fixing marionette so it detects the silent restart case and doesn't wait for the parent window to open ( ie drop reliance on toplevel-window-ready - https://searchfox.org/mozilla-central/rev/9dceacf3d761eb91237108ec438d64099a56f442/remote/components/Marionette.jsm#149 and other places that gets mentioned in that file) -- but this may be more involved, you'd want to talk to someone, maybe jgraham or whimboo, about it...

I actually spent a lot of time trying to make this work. I summarized those efforts in this bug's description. To make a long story short, I ended up determining that expecting a window at startup seems to be baked deeply enough into the design of the marionette server that it would delay Bug 1720742's timeline to block on changing marionette to accommodate this situation.

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #2)

(In reply to :Gijs (he/him) from comment #1)

Could we add test-only code into the silent restart code that opens a window after waiting for 2 seconds or something along those lines? We can detect the test-only case by using Cu.isInAutomation

We probably could, but at that point I'm not sure what we are testing for. If a window gets opened at startup (even 2 seconds later), the entire work of Bug 1726239 has kind of been defeated, so I'm not really sure what meaningful tests of that patch could be conducted at that point.

Well, you could open a dummy URL like about:robots or w/e, and verify that (a) the browser came back up and didn't just quit (which is what the "enter/exit" paired up guards are designed to prevent, AIUI), and that we didn't open a window with the default homepage instead, and you can check that the wasSilentlyRestarted produces the correct result after the restart. Checking all those things is materially better than not checking anything, if you ask me - we could easily end up breaking the (IMHO kinda fragile mechanism) enter/exit area stuff and quit entirely, or end up restarting and opening a blank window (which would mean it was no longer silent) if someone changes the macOS startup routine to move initializing the initial window sooner (a bit like the skeleton window on Windows).

I'm not sure that using Cu.isInAutomation will actually work here. I believe that this limitation is why both checks are needed here. It might be reasonable to read the -marionette command line flag though. I believe that will be available at this point.

Blegh, this is very annoying; we should fix bug 1371576 and bug 1272255 sometime... but that's not your yak. :-(

This could potentially enable some basic tests for Bug 1720742 so that we could check that we restart when windows were closed with an update ready (this is assuming that marionette is ok with having all the windows close - I haven't tried this yet). But I think that there are also other ways of testing this, like intercepting the quit() call.

Alternatively, did you consider fixing marionette so it detects the silent restart case and doesn't wait for the parent window to open ( ie drop reliance on toplevel-window-ready - https://searchfox.org/mozilla-central/rev/9dceacf3d761eb91237108ec438d64099a56f442/remote/components/Marionette.jsm#149 and other places that gets mentioned in that file) -- but this may be more involved, you'd want to talk to someone, maybe jgraham or whimboo, about it...

I actually spent a lot of time trying to make this work. I summarized those efforts in this bug's description. To make a long story short, I ended up determining that expecting a window at startup seems to be baked deeply enough into the design of the marionette server that it would delay Bug 1720742's timeline to block on changing marionette to accommodate this situation.

Fair enough; I read comment 0 and noticed you mentioned the marionette specific observers but not the toplevel window one, which I happened to know about because of bug 1382162. It'd probably be good for marionette if it was more robust to this kind of thing, but I agree that's not your yak either...

Comment 4

[Robin Steuber (she/her) [:bytesized]]

Nick and I were talking about this, and it seemed like he might have a pretty decent idea about how to write a test here that would hit the points that we want to hit.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #4)

Nick and I were talking about this, and it seemed like he might have a pretty decent idea about how to write a test here that would hit the points that we want to hit.

Here's what I am thinking. It's my understanding that the sticking point is that after we launch silently, Marionette won't have started up. I propose that we:

1. do the silent launch
2. possibly verify that we can't connect to Marionette
3. launch again and leverage the remoting protocol to open a new window
4. connect to Marionette
5. verify that we're in the expected state after a silent launch (i.e, wasSilentlyRestarted == true) and that the window/tab state is correct.

Locally, I can do this in the shell using:

nalexander@roboto ~> env MOZ_MARIONETTE=1 MOZ_APP_SILENT_RESTART=1 "/Users/nalexander/Applications/Firefox Nightly.app/Contents/MacOS/firefox" &
nalexander@roboto ~> _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
nalexander@roboto ~>
nalexander@roboto ~> telnet localhost 2828
Trying ::1...
telnet: connect to address ::1: Connection refused
Trying 127.0.0.1...
telnet: connect to address 127.0.0.1: Connection refused
telnet: Unable to connect to remote host
nalexander@roboto ~ [1]> "/Users/nalexander/Applications/Firefox Nightly.app/Contents/MacOS/firefox" --new-window "https://example.com"
nalexander@roboto ~> telnet localhost 2828
Trying ::1...
telnet: connect to address ::1: Connection refused
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
50:{"applicationType":"gecko","marionetteProtocol":3}^]
telnet> Connection closed.

bytesized: does that make sense? Can you have a crack at convincing a Marionette test to do this? I have written one of these before and can assist if needed; I could imagine handling a running process that Marionette can't connect to will require some hoops to be hurdled.

Comment 6

[Robin Steuber (she/her) [:bytesized]]

I'm busy with the patch that I'm currently working on, and I don't want to get distracted. But I'm leaving the needinfo on me and I'll come back and look at this, probably next week.

Comment 7

[Robin Steuber (she/her) [:bytesized]]

(In reply to Nick Alexander :nalexander [he/him] from comment #5)

1. do the silent launch
2. possibly verify that we can't connect to Marionette
3. launch again and leverage the remoting protocol to open a new window
4. connect to Marionette
5. verify that we're in the expected state after a silent launch (i.e, wasSilentlyRestarted == true) and that the window/tab state is correct.

There is one bit that I'm unsure of, and that is around step 2. Marionette has an existing restart function that does the work necessary to allow Firefox to restart during a test. IIRC, that work includes repeatedly attempting to reconnect to the Marionette server in order to effectively poll for Firefox restarting. I could potentially make a new argument to pass in to make it do something else, but I'm not sure what exactly. Are we just going to wait an exceedingly long time for it to time out and then just say "Well, it's probably open now"?

Comment 8

[Robin Steuber (she/her) [:bytesized]]

Oh, maybe we can poll via the remote interface (the telnet bit that you are demonstrating)? Is that what you were getting at and I was just missing it?

Comment 9

[Robin Steuber (she/her) [:bytesized]]

Nick and I talked about this further and tried some things out. Ultimately, the biggest problem seems to be that Marionette runs Firefox with -no-remote.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: WIP: Bug 1726460 - WIP on testing silent restart with Marionette.

Comment 11

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

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.

The toplevel-window-ready observer is only used to check for YSOD (yellow screen of dead) issues during startup. Maybe that is not necessary anymore given that DTD/property files do not exist anymore? I'm not sure how stable / failure-proof the new l10n code is.

Comment 12

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

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

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.

The toplevel-window-ready observer is only used to check for YSOD (yellow screen of dead) issues during startup. Maybe that is not necessary anymore given that DTD/property files do not exist anymore? I'm not sure how stable / failure-proof the new l10n code is.

I just noticed that there is bug 1726465. Lets move all discussion around this topic over to the other bug.

Comment 13

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

With bug 1726465 now fixed I've also added some basic Marionette unit tests to verify the windowless mode including restarts. These tests can be found here:

https://searchfox.org/mozilla-central/rev/88792eff309001778cb2431f2a0ed92f8f3c258a/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py#305-333
https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_windowless.py

Kirk, is there still something else to be addressed or is that coverage fine for now?

Comment 14

[Robin Steuber (she/her) [:bytesized]]

I think that covers what was needed here. Thanks!