1358402 - Total bustage of firefox-ui fallback update tests for Firefox 54.0b1 because Marionette is not getting started after a restart of Firefox

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

The following Treeherder job list says everything:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=cf76e00dcd6f&group_state=expanded&filter-tier=3&filter-searchStr=Fxup-beta-cdntest(&selectedJob=93232076

When I checked one of our mozmill-ci machines I have seen that Marionette is not getting started with the restart of Firefox after making the downloaded update invalid. 

I have no idea yet what's going on. Especially because older 53.0b releases are also affected which were working fine before. There are also no Marionette specific failures visible in the Gecko log.

Comment 1

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

So this seems to be only related to fallback updates. The direct updates work just fine across all platforms.

When I checked this locally I have made some changes to the update testcase, and noticed that this problem always happens when we modify the file `update.status` before a restart! If we don't do it, it's all fine.

I will try to find a minimal testcase for this.

Comment 2

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

Attachment: minimized testcase

So I found a minimal testcase to reproduce this problem with the beta releases of Firefox. As you can see no download of an update patch is necessary. Just modifying the `update.status` file before a restart causes Marionette not to be started anymore.

Comment 3

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

So it is clearly a regression in Marionette and has been caused by one of the merged patches from mozilla-central to mozilla-beta:

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=1d36370d5eac&tochange=d57aa49c3948

Suspected bugs are bug 1337743 and bug 1350887.

I'm going through the list of changesets now to nail down the range.

Comment 4

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

Actually this regression has been caused by bug 1344748, and is still present for latest mozilla-central when executed with Firefox builds now on Beta.

Steps to reproduce:

1. Download any of the builds from [1] or build yourself from changeset cf76e00dcd6f142acf5b49f8beeb0ac95b2afa31 on mozilla-beta

2. Run the minimized testcase against this build.

[1] here: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=cf76e00dcd6f142acf5b49f8beeb0ac95b2afa31&filter-searchStr=build&filter-tier=1&filter-tier=2&filter-tier=3

Andreas, I hope that you can take this and get fixed ASAP? Thanks.

Comment 5

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

I assume that even with the follow-up patch on bug 1350887 we seem to have busted everything before 54.0. So we clearly need a solution which will work for 3 releases back.

Comment 6

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

The regression has exactly been introduced by the following changeset which renamed the preferences:

https://hg.mozilla.org/releases/mozilla-aurora/rev/5917cc585718

Comment 7

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

Looks like I have an easy fix for this problem.

Comment 8

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

Attachment: Bug 1358402 - Keep 'marionette.defaultPrefs.enabled' around as fallback.

With bug 1344748 landed the default preferences and their handling has been
changed. Builds starting with Firefox 54.0 can handle that, but previous
releases don't enable Marionette at all after a restart under special
conditions (invalidating 'update.status' file before the restart).

To prevent the bustage we have to keep the preference
'marionette.defaultPrefs.enabled' around until the next ESR release
is out.

Review commit: https://reviewboard.mozilla.org/r/132350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132350/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8860315 [details]
Bug 1358402 - Keep 'marionette.defaultPrefs.enabled' around as fallback.

https://reviewboard.mozilla.org/r/132350/#review135244

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/dd530a59750a
Keep 'marionette.defaultPrefs.enabled' around as fallback. r=ato a=tomcat

Comment 11

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

Matt, I still do not understand why this behavior only happens when we modify/create the `update.status` file before a restart of Firefox. But I could assume that if the file is present, the application updater code is checking this file on the next startup, and due to a failed state starts Firefox in a way that some components are not getting correctly initialized?

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd530a59750adcaa0d48fa4f69b0cdb52715852a
https://hg.mozilla.org/releases/mozilla-beta/rev/ede33bdbabd89c8137c39dfb7b803ce2ca5a5c4f

landed directly on central on request from whimboo

Comment 13

[Molly Howell (she/her) (mostly inactive)]

We check the status file and kick off the fallback inside an observer for "sessionstore-windows-restored" and/or "xul-window-visible" [0]. I don't know the startup procedure thoroughly, but those sound like they would be late enough that whatever we would be worried about breaking is already initialized by then.

[0] https://dxr.mozilla.org/mozilla-central/rev/c8198aa6e7677e90cc7f1e2df0a14a5cc2719055/toolkit/mozapps/update/nsUpdateService.js#1759

Comment 14

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

So since my changes to bug 1315611, Marionette also uses "sessionstore-windows-restored" to initialize itself. So starting its server component will be some milliseconds later. But nothing in the _postUpdateProcessing() code shows something which might prevent Marionette from starting up.

Matt, is there somewhere an extra hidden restart which we do in such a failed case (invalid patch as listed in update.status)? Given that I can get it to work by adding back this old preference means, that for such a restart we loose the -marionette argument. It would somewhat be an indication for me because without modifying the file the restart works.

Comment 15

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

Btw in bug 1355888 we will make use of an environment variable to keep the status of Marionette across Firefox instances. So it might fix such a situation at least for builds of Firefox 55.0.

Comment 16

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

I have moved the remaining items to bug 1358987.