1427471 - Building with --disable-updater should set app.update.enabled to false

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P2)

Description

[Mike Hommey [:glandium]]

While some code does check AppConstants.MOZ_UPDATER before app.update.enabled, not everything does, and for the principle of least surprise, it would be better that --disable-updater implies app.update.enabled is false. (although, it would still be weird that the user flipping the pref would enable /some/ things related to app updates)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1427471 - Set app.update.enabled to false when building with --disable-updater.

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

Comment 2

[Robert Helmer [:rhelmer]]

I think this is OK as far as system add-on updates go, but I'd like to make sure there isn't some other point to having app.update.enabled set to true when Firefox is built with --disable-updater - maybe Matt knows?

Comment 3

[Molly Howell (she/her) (no longer active)]

Inside app update, I don't know of (and can't find) anywhere that we check app.update.enabled without first going through a check of MOZ_UPDATER, usually at the UI level. I do see that happening in the add-ons manager, where MOZ_UPDATER of course isn't relevant, so if it's okay in there, I'd say we're good to do this patch.

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 8939192 [details]
Bug 1427471 - Set app.update.enabled to false when building with --disable-updater.

https://reviewboard.mozilla.org/r/209580/#review217954

Other than app update, it looks like: system add-on updates, the old hotfix system (being removed soon and replaced by SAO updates) and blocklist all honor this setting.

I could imagine there are some folks (distros maybe?) building with updater disabled that still would expect to get blocklist updates... the fix to that is probably to provide a pref to more specifically enable/disable blocklist, as we are doing with system add-ons in bug 1428459.

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #4)
> Comment on attachment 8939192 [details]
> Bug 1427471 - Set app.update.enabled to false when building with
> --disable-updater.
> 
> https://reviewboard.mozilla.org/r/209580/#review217954
> 
> Other than app update, it looks like: system add-on updates, the old hotfix
> system (being removed soon and replaced by SAO updates) and blocklist all
> honor this setting.
> 
> I could imagine there are some folks (distros maybe?) building with updater
> disabled that still would expect to get blocklist updates... the fix to that
> is probably to provide a pref to more specifically enable/disable blocklist,
> as we are doing with system add-ons in bug 1428459.

You'd know this use case better than me - do you think there is a reason to block on moving SAO updates and blocklist to not depend on the app.update.enabled pref before landing this?

Comment 6

[Mike Hommey [:glandium]]

So here's the funny thing. At least in my case, I don't pass --enable-update-channel=release, so I get "default". I expect many distros are in the same boat. Guess what? SAO updates for "default" are empty. Blocklist updates aren't though. But it looks like they already rely on a separate pref (services.blocklist.update_enabled).

Comment 7

[Robert Helmer [:rhelmer]]

(In reply to Mike Hommey [:glandium] from comment #6)
> So here's the funny thing. At least in my case, I don't pass
> --enable-update-channel=release, so I get "default". I expect many distros
> are in the same boat. Guess what? SAO updates for "default" are empty.
> Blocklist updates aren't though. But it looks like they already rely on a
> separate pref (services.blocklist.update_enabled).

Hm, what about Fennec? We haven't enabled SAO updates there yet (but will soon in bug 1260213), but the old hotfix system also relies on this pref.... do you know if we are building with --disable-updater there? I think it must have the app.update.enabled pref set, I can't see how hotfix would be working if this were not the case?

Comment 8

[Mike Hommey [:glandium]]

Fennec is not built with --disable-updater, and is built with --enable-update-channel with the right channel (release, beta or nightly)

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8939192 [details]
Bug 1427471 - Set app.update.enabled to false when building with --disable-updater.

https://reviewboard.mozilla.org/r/209582/#review217984

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54e562a94c3c
Set app.update.enabled to false when building with --disable-updater. r=rhelmer

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/54e562a94c3c

Comment 12

[Robin Steuber (they/them) [:bytesized]]

`app.update.enabled` is being removed in Bug 1420514. This mechanism will no longer be possible.