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

RESOLVED FIXED

Status

defect
--
blocker
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

({regression})

Version 3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 fixed, firefox55 fixed)

Details

Attachments

(2 attachments)

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.
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.
Summary: Total bustage of firefox-ui-update tests for Firefox 54.0b1 because Marionette is not getting started after a restart of Firefox → Total bustage of firefox-ui fallback update tests for Firefox 54.0b1 because Marionette is not getting started after a restart of Firefox
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Posted file 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.
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.
Component: Firefox UI Tests → Marionette
Keywords: regression
QA Contact: hskupin
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.
Assignee: hskupin → nobody
Blocks: 1344748
Status: ASSIGNED → NEW
Flags: needinfo?(ato)
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.
The regression has exactly been introduced by the following changeset which renamed the preferences:

https://hg.mozilla.org/releases/mozilla-aurora/rev/5917cc585718
Looks like I have an easy fix for this problem.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8860315 [details]
Bug 1358402 - Keep 'marionette.defaultPrefs.enabled' around as fallback.

https://reviewboard.mozilla.org/r/132350/#review135244
Attachment #8860315 - Flags: review?(ato) → review+

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/dd530a59750a
Keep 'marionette.defaultPrefs.enabled' around as fallback. r=ato a=tomcat
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?
Flags: needinfo?(mhowell)
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(mhowell)
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.
Flags: needinfo?(mhowell)
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.
Depends on: 1358987
I have moved the remaining items to bug 1358987.
Flags: needinfo?(mhowell)
You need to log in before you can comment on or make changes to this bug.