1768890 - "Refresh Firefox" no-ops silently when there is a pending update.

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Mike Hoye [:mhoye]]

I've been able to reproduce this in Nightly twice. I'm on current Windows 11, current Nightly.

STRs are, waiting until a Nightly update is pending, opening "about:support" and choosing Refresh Nightly and agreeing to it.

The browser snaps back fast, like it's a normal update - the "refresh" part of the exercise, new profile etc, doesn't happen - and all my windows/tabs/etc are right where they were.

When an update is not pending, I get the expected Refresh Nightly experience and see a new folder created in "Old Firefox Data" on my desktop; one does not appear for the failed updates.

Comment 1

[:Gijs (he/him)]

Fx Refresh uses environment variables as well as commandline switches to propagate its intent to refresh when restarting. Is it expected those get lost when restarting with a pending update? And/or, can we change that? And how much risk is there from trying to simultaneously apply the update and execute the refresh (which means creating a new profile, starting with that profile rather than the old one, and migrating a bunch of stuff across into it on first startup - but notably not prefs (or at least, only a handful of prefs to maintain sync and telemetry state)). If we can only do one, should we prio updates or refresh in this case? If this is all a hornet's nest, do we need to somehow hide / make refresh inaccessible while there is a pending update?

Comment 2

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

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

Fx Refresh uses environment variables as well as commandline switches to propagate its intent to refresh when restarting. Is it expected those get lost when restarting with a pending update? And/or, can we change that? And how much risk is there from trying to simultaneously apply the update and execute the refresh (which means creating a new profile, starting with that profile rather than the old one, and migrating a bunch of stuff across into it on first startup - but notably not prefs (or at least, only a handful of prefs to maintain sync and telemetry state)). If we can only do one, should we prio updates or refresh in this case? If this is all a hornet's nest, do we need to somehow hide / make refresh inaccessible while there is a pending update?

I'm interested in what :bytesized thinks, but I would like to see the update cycle preserve command line arguments across the refresh loop. If that's broken, we should fix it. If that's not broken, we have an affordance in the form of ShouldNotProcessUpdates that is intended to query the runtime invocation and conditionally avoid processing updates; we could teach that query to recognize refreshes, I expect, to address this use case.

Comment 3

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

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

Fx Refresh uses environment variables

I believe that environment variables should be preserved when updating (outside of a couple that, I believe, are used specifically by the update process).

as well as commandline switches

I'm less familiar with the command line argument situation, but they appear to be piped through correctly. We collect them here, pass them to the updater, and the updater passes them back to the callback app (Firefox).

I don't know what arguments are relevant here. Maybe CheckArg(..., CheckArgFlag::RemoveArg) is being called on them before they can be collected into gRestartArgv?

And how much risk is there from trying to simultaneously apply the update and execute the refresh (which means creating a new profile, starting with that profile rather than the old one, and migrating a bunch of stuff across into it on first startup - but notably not prefs (or at least, only a handful of prefs to maintain sync and telemetry state)).

I don't think that there is much risk here. The important bits of update state aren't really kept in prefs, they are kept in the update directory. It seems like the only real problem is making sure that the migration still happens on first startup, even after an update. I'm guessing that that is what's going wrong here?

I'd certainly be willing to look further into what's going wrong here. If you would like me to do that, could you please tell me the relevant environment variables and command line switches?

Comment 4

[:Gijs (he/him)]

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

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

as well as commandline switches

I'm less familiar with the command line argument situation, but they appear to be piped through correctly. We collect them here, pass them to the updater, and the updater passes them back to the callback app (Firefox).

The collection looks like it happens on startup. That's sort of interesting because now I wonder (and this would be a separate problem!), if the initial startup of Firefox is the result of a refresh (and gets passed commandline args to that effect) and then we find an update, if the update-related restart then preserves those args and runs a second refresh... That would be Not Good.

I don't know what arguments are relevant here. Maybe CheckArg(..., CheckArgFlag::RemoveArg) is being called on them before they can be collected into gRestartArgv?

And how much risk is there from trying to simultaneously apply the update and execute the refresh (which means creating a new profile, starting with that profile rather than the old one, and migrating a bunch of stuff across into it on first startup - but notably not prefs (or at least, only a handful of prefs to maintain sync and telemetry state)).

I don't think that there is much risk here. The important bits of update state aren't really kept in prefs, they are kept in the update directory. It seems like the only real problem is making sure that the migration still happens on first startup, even after an update. I'm guessing that that is what's going wrong here?

I'd certainly be willing to look further into what's going wrong here. If you would like me to do that, could you please tell me the relevant environment variables and command line switches?

https://searchfox.org/mozilla-central/search?q=MOZ_RESET_PROFILE_RESTART&redirect=false

is the env var that gets used if refresh is triggered from within the application, and then we restart with something like https://searchfox.org/mozilla-central/rev/b72e9d7d63bf499d1d8168291b93d4ec7fde236e/toolkit/modules/ResetProfile.jsm#100-102 .

(unfortunately there are a few duplicate implementations of this procedure)

It's also possible to invoke the same logic using the -reset-profile commandline flag, cf. https://searchfox.org/mozilla-central/rev/b72e9d7d63bf499d1d8168291b93d4ec7fde236e/toolkit/xre/nsAppRunner.cpp#3054-3059 . The installer and uninstaller use this to trip Fx refresh from the installer (which I suppose is very unlikely to have an update waiting...).

I haven't touched profile refresh code in years, so I don't recall some of the specifics here at the moment - but broadly speaking I'm confused because what you're saying (env vars should definitely work, commandline args might not) and what I'm reading (in-app refresh pathways only use the env var anyway) suggests what Mike did should work, but it clearly doesn't right now... If you have time to dig into why that would be very helpful.

Comment 5

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

It looks like MOZ_RESET_PROFILE_RESTART is removed from the environment by SelectProfile(), which is called before ProcessUpdates() in XREMain::XRE_mainStartup(). We don't create the new profile to migrate the old one into until after ProcessUpdates() is called (and causes Firefox to exit).

It seems likely that the appropriate fix is to not remove MOZ_RESET_PROFILE_RESTART from the environment until after the ProcessUpdates() call. I think that that will probably fix the problem.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1768890 - ensure profile refresh works if an update is processed at the same time, r?bytesized!,nalexander!

Comment 7

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:nrishel, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9359e82d0c6f
ensure profile refresh works if an update is processed at the same time, r=bytesized

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/9359e82d0c6f

Comment 10

[Monica Chiorean]

Reproduced the issue on Win10 using build from 102.0a1(20220511214930).
Verified on Win10/Ubuntu20.4/Mac 10.13 that issue is fixed on Beta 103.0b7 / Nightly 104.0a1 and that the Old Firefox Data folder is created on desktop.