1922206 - Opening Firefox settings actively checks for updates like the about dialog does

Status: REOPENED

(Toolkit :: Application Update, defect)

Description

[:Gijs (he/him)]

AFAICT the Firefox Settings bit that lists updates actively checks for updates as soon as you open the settings. I checked by setting a breakpoint in the aboutDialog-appUpdater.js code that calls .check() on the updater, and verified that it gets run as soon as you open settings.

I don't know if we want to change this. It's not normally a problem as such, but when update rates are throttled, AIUI the about dialog can/will bypass this, as will (apparently) opening settings, which I don't think is expected. When evaluating what was going on in bug 1919721 comment 50 this certainly came as a bit of a surprise to me.

We should think about what the desired behaviour should look like here. Can we make the update check behave the same way as a "normal" background update check (which would get throttled) instead of the same way as the about dialog, perhaps?

Comment 1

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

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

AFAICT the Firefox Settings bit that lists updates actively checks for updates as soon as you open the settings.

This is expected behavior. I'm open to changing this to require, say, a button press. But I'm also not sure that I'm seeing a convincing reason to.

We should think about what the desired behaviour should look like here. Can we make the update check behave the same way as a "normal" background update check (which would get throttled) instead of the same way as the about dialog, perhaps?

I definitely don't want to do this. If background update doesn't happen (yet) due to throttling, fine. But we should not explicitly tell the user "there are no updates available" when there is an update available, regardless of throttling.

Comment 2

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

That interface does technically have a "Check for Updates" button that is currently (almost?) never shown. I think it would be pretty low effort to start in that state though, if we do decide we want that.

Comment 3

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

Since this is expected behavior, I'm going to go ahead and close this. If someone provides a good reason to make this change, we can reopen.

Comment 4

[:Gijs (he/him)]

(In reply to Robin Steuber (she/her) [:bytesized] from comment #1)

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

AFAICT the Firefox Settings bit that lists updates actively checks for updates as soon as you open the settings.

This is expected behavior.

No, it's the implemented behaviour, and you know what is implemented so you expect it. It's not expected by users that opening settings at all, for any reason, even if e.g. following links to other panes or just generally being interested in any other setting besides updates, would always cause an update to happen.

I'm open to changing this to require, say, a button press. But I'm also not sure that I'm seeing a convincing reason to.

It means that users who open settings will update despite throttling and despite never expressing any intention they wanted to update. That alone seems a good enough reason to me?

I'm not suggesting the interface should lie about the availability of updates (the strawman from comment 1). I'm saying that, as with bug 1919721 comment 50, if I'm having to explain to users "why did [you] update to this broken thing" despite throttling, having to include "surprise, if you ever opened settings that would have caused you to update", is super counterintuitive. It doesn't seem all that likely that, out of 100 users who open settings, 100 of them check the "am I up-to-date" bit or even care about that. (more explicitly: the data supports the opposite conclusion)

Comment 5

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

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

It means that users who open settings will update despite throttling and despite never expressing any intention they wanted to update. That alone seems a good enough reason to me?

This is only true if the user has the "Allow Nightly to: Automatically Install Updates" setting selected. And that's what we are doing. Automatically installing updates.

I'm not suggesting the interface should lie about the availability of updates (the strawman from comment 1). I'm saying that, as with bug 1919721 comment 50, if I'm having to explain to users "why did [you] update to this broken thing" despite throttling

It sounds to me like the problem here is that we are shipping a broken thing and expecting throttling to keep it from actually reaching users. That seems inappropriate.

@Gijs - Am I misunderstanding here? It seems like we are doing what the settings say we will do and your argument for not doing this is "what if we are shipping something broken but throttling it" to which it seems like the obvious answer is: "we should stop shipping it rather than relying on throttling to keep anyone from installing it"

Comment 6

[:Gijs (he/him)]

Attachment: Screenshot

(In reply to Robin Steuber (she/her) [:bytesized] from comment #5)

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

It means that users who open settings will update despite throttling and despite never expressing any intention they wanted to update. That alone seems a good enough reason to me?

This is only true if the user has the "Allow Nightly to: Automatically Install Updates" setting selected.

That's the default, and few people change those defaults, so that seems a moot point.

And that's what we are doing. Automatically installing updates.

But if updates were throttled and users didn't open settings (or the about dialog, or...), no updates would be installed until unthrottled, right?

I'm not suggesting the interface should lie about the availability of updates (the strawman from comment 1). I'm saying that, as with bug 1919721 comment 50, if I'm having to explain to users "why did [you] update to this broken thing" despite throttling

It sounds to me like the problem here is that we are shipping a broken thing and expecting throttling to keep it from actually reaching users. That seems inappropriate.

What else do you suggest we do once we've shipped the broken thing? Throttling update rates to 0 is routinely what we do when we discover breakage on nightly or beta. It's what routinely gets communicated by relman, sheriffs etc. as "updates are disabled".

More generally this bypasses throttling in all situations, also the "regular" release rollout throttling to 25% etc.

Doing so without an express user intent still seems broken to me.

@Gijs - Am I misunderstanding here? It seems like we are doing what the settings say we will do

I don't understand this claim. Nowhere does it say either before or after opening settings that doing so will check for updates. The user clicked a "settings" button, not a "check for updates" button. (The intent is also fuzzy for the "about" dialog but at least much less fuzzy.)

The UI for updates (see screenshot) likely won't even be visible, but also doesn't say anything about checking unless that is what it is doing at that time, or by implication by saying "we're up to date".

we should stop shipping it

I don't know of us ever having done this nor do I know of a mechanism to do so. AFAIK the throttling mechanism and then replacing with a new release that is less broken is the only one available. How else would you "stop shipping" it?

Comment 7

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

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

But if updates were throttled and users didn't open settings (or the about dialog, or...), no updates would be installed until unthrottled, right?

Not really. It's usually a random chance.

What else do you suggest we do once we've shipped the broken thing? Throttling update rates to 0 is routinely what we do when we discover breakage on nightly or beta. It's what routinely gets communicated by relman, sheriffs etc. as "updates are disabled".

Well then we should definitely do something about that. If things are broken and we want to disable updates, we should remove/deactivate the Balrog rule entirely. If this is indeed a problem, I don't feel like switching this to a button that just goes ahead and downloads the "disabled", update that we know to be broken is really an appropriate fix.

Nowhere does it say either before or after opening settings that doing so will check for updates. The user clicked a "settings" button, not a "check for updates" button. (The intent is also fuzzy for the "about" dialog but at least much less fuzzy.)

No, it says that we will check for updates "automatically". And that's what we are doing.

The UI for updates (see screenshot) likely won't even be visible, but also doesn't say anything about checking unless that is what it is doing at that time, or by implication by saying "we're up to date".

Or it can say that the download/staging is in-progress. Or it can say "Restart to Update". I'm not really sure what point you are making.

we should stop shipping it

I don't know of us ever having done this nor do I know of a mechanism to do so. AFAIK the throttling mechanism and then replacing with a new release that is less broken is the only one available. How else would you "stop shipping" it?

Remove or otherwise deactivate the whole rule in Balrog.

Comment 8

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

It seems to me that the purpose of this bug is less about the timing of when we should check for updates or bypassing a coin flip than it is about us downloading something intended to be disabled? Is that correct?

Comment 9

[:Gijs (he/him)]

(In reply to Robin Steuber (she/her) [:bytesized] from comment #7)

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

But if updates were throttled and users didn't open settings (or the about dialog, or...), no updates would be installed until unthrottled, right?

Not really. It's usually a random chance.

I don't understand what your reply here means. My understanding is that if updates are throttled, the background ("automatic") update checks will not cause updates to be installed. So no updates would be installed. The only exception is if the user "actively" looks to update their build, e.g. by going to the about dialog. I don't see how "random chance" comes into this. Can you elaborate?

What else do you suggest we do once we've shipped the broken thing? Throttling update rates to 0 is routinely what we do when we discover breakage on nightly or beta. It's what routinely gets communicated by relman, sheriffs etc. as "updates are disabled".

Well then we should definitely do something about that. If things are broken and we want to disable updates, we should remove/deactivate the Balrog rule entirely.

If I understand correctly, sheriffs/relman have the choice between throttling to 0 or disabling all updates on the channel. I don't know if that corresponds to what you're describing here - if it does, that's a very heavy hammer that we're probably disinclined to use on the release channel (and even beta/devedition), as it would misinform users on outdated builds that there are no updates at all. Passing to Ryan to offer some context from their PoV.

If this is indeed a problem, I don't feel like switching this to a button that just goes ahead and downloads the "disabled", update that we know to be broken is really an appropriate fix.

That's fair. On the flip side, it would be better than the status quo, where at least 20 times as many people do this without realizing (judging by how many people click the manual "check for updates" button in there vs. how many people open the general pane in settings or any of the others, on a daily basis)

(confusingly, the number of people clicking that button, btw, is pretty similar to the number of clients toggling updates to be manual rather than automatic, which is a surprising equivalence that does not make much sense, given one is a one-time action and the other is something you'd presumably have to do somewhat regularly if you were interested in updates at all. But that mystery is for another bug...)

Nowhere does it say either before or after opening settings that doing so will check for updates. The user clicked a "settings" button, not a "check for updates" button. (The intent is also fuzzy for the "about" dialog but at least much less fuzzy.)

No, it says that we will check for updates "automatically". And that's what we are doing.

The check passes the FOREGROUND_CHECK flag which isn't the same as the automatic, background updates, AIUI. Am I wrong that these checks are not equal, and do not behave the same way in this situation?

The UI for updates (see screenshot) likely won't even be visible, but also doesn't say anything about checking unless that is what it is doing at that time, or by implication by saying "we're up to date".

Or it can say that the download/staging is in-progress. Or it can say "Restart to Update". I'm not really sure what point you are making.

I'm making the point that this UI does not explain that the action of opening the settings caused an update check, as such. So I do not think it is reasonable to consider that behaviour "expected" as you advocated in the earlier comments.

(In reply to Robin Steuber (she/her) [:bytesized] from comment #8)

It seems to me that the purpose of this bug is less about the timing of when we should check for updates or bypassing a coin flip than it is about us downloading something intended to be disabled? Is that correct?

The thing that caused me to investigate the code here and file this ticket was about downloading something that we disabled, yes. And we found a second instance that surprised the people on slack looking at this, filed as bug 1922228. But I actually think the underlying issue is larger, as it would seem that we have upwards of 250k people every day opening settings (unclear how that splits across channels, though we could go find out) and bypassing the throttling mechanisms. That seems like an issue irrespective of the "we disabled updates because of brokenness" scenario, and just a correctness issue we should fix, also to make our gradual rollouts of new releases more accurate. The omc/asrouter route is similarly concerning, also because it is as yet unclear how many clients are hitting that.

Fundamentally I think it is unexpected that merely opening settings does update checks automatically and bypasses throttling. It is likely to cause other grief - for instance, if we created onboarding UI that had links/buttons pointing to specific parts of the settings (e.g. to make particular choices about features being turned on/off) then that would have the side effect of increasing traffic to the update server (and potentially updates that bypass regular release throttling) for people going through that onboarding. That would potentially interfere with any experiments we were running, almost certainly without us realizing that that was happening.

Comment 10

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

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

(In reply to Robin Steuber (she/her) [:bytesized] from comment #7)

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

But if updates were throttled and users didn't open settings (or the about dialog, or...), no updates would be installed until unthrottled, right?

Not really. It's usually a random chance.

I don't understand what your reply here means. My understanding is that if updates are throttled, the background ("automatic") update checks will not cause updates to be installed. So no updates would be installed. The only exception is if the user "actively" looks to update their build, e.g. by going to the about dialog. I don't see how "random chance" comes into this. Can you elaborate?

Most of the time, throttled updates are throttled at >0%. So there is generally a random chance that users will be served the throttled update. The exact mechanism whereby a throttled update is selected or not selected is an implementation detail that I don't think users have expectations of.

If I understand correctly, sheriffs/relman have the choice between throttling to 0 or disabling all updates on the channel. I don't know if that corresponds to what you're describing here - if it does, that's a very heavy hammer that we're probably disinclined to use on the release channel (and even beta/devedition), as it would misinform users on outdated builds that there are no updates at all. Passing to Ryan to offer some context from their PoV.

Sorry, I guess I did not speak precisely enough here. The rule with a main mapping to a broken build and a fallback to a good build should be replaced with one that only has a main mapping to a good build rather than just setting the backgroundRate for the broken build down to 0. We could add functionality to Balrog to do this in an easier fashion, but I believe rules can be manually tweaked pretty much arbitrarily now.

The check passes the FOREGROUND_CHECK flag which isn't the same as the automatic, background updates, AIUI. Am I wrong that these checks are not equal, and do not behave the same way in this situation?

Strictly speaking, it is a slight difference in behavior, yes.

I'm making the point that this UI does not explain that the action of opening the settings caused an update check, as such. So I do not think it is reasonable to consider that behaviour "expected" as you advocated in the earlier comments.

The check is happening automatically. Updates are set to happen automatically. It seems reasonable to me.

The thing that caused me to investigate the code here and file this ticket was about downloading something that we disabled, yes. And we found a second instance that surprised the people on slack looking at this, filed as bug 1922228.

That doesn't really make sense to me. I looked into Bug 1922228 and, as I commented, that will not start an update download.

But I actually think the underlying issue is larger, as it would seem that we have upwards of 250k people every day opening settings (unclear how that splits across channels, though we could go find out) and bypassing the throttling mechanisms. That seems like an issue irrespective of the "we disabled updates because of brokenness" scenario, and just a correctness issue we should fix also to make our gradual rollouts of new releases more accurate.

Technically, yes. Though when something is so sensitive that we need to target it so precisely, we generally use a different mechanism than throttling the rollout.