1922228 - Remove needsUpdate asrouter targeting prop (was: asrouter shouldn't do a forced (foreground) update check)

Status: NEW

(Firefox :: Messaging System, defect, P2)

Description

[Julien Cristau [:jcristau]]

https://searchfox.org/mozilla-central/rev/3966e5534ddf922b186af4777051d579fd052bad/browser/components/asrouter/modules/ASRouterTargeting.sys.mjs#280 calls checkForUpdates with FOREGROUND_CHECK, which seems wrong since that is meant to be used when the user actively checks for update (i.e. opens the about dialog, essentially).

Comment 1

[Punam Dahiya [:pdahiya]]

needsUpdate got added in Fx65 and hasn't been in use for a while. FOREGROUND_CHECK in checkforUpdates got added as part of refactor with
https://phabricator.services.mozilla.com/D159295

Comment 2

[Julien Cristau [:jcristau]]

D159295 (bug 1727820) just changed checkForUpdates(updateServiceListener, /*force*/ true) to checkForUpdates(lazy.UpdateCheckSvc.FOREGROUND_CHECK). The code was already doing a foreground check since its addition in bug 1499886 AFAICT.

Comment 3

[:Gijs (he/him)]

Based on this conversation and bug 1775138 I am nervous that we're evaluating this periodically from the background task code, and thus doing update checks with FOREGROUND_CHECK from that code. Nick has graciously agreed to do some initial poking to see if this understanding is correct, as that would (in the words of several of us on that thread) be both bad and unexpected.

Comment 4

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

:nalexander and I were chatting about this this today and I offered to update this bug since I could pretty immediately provide the answer here. Obviously this does do a foreground update check, yes. But it only does an update check.

For some background, there are basically two ways to control UpdateService:

1. Call into checkForBackgroundUpdates() (or notify(), which is basically the same thing), here. This basically kicks off the entire update process. In other words, UpdateService pushes the update process forwards.
2. "Manually" push the update process forwards (or have AppUpdater do it for you).

The UpdateCheckSvc.checkForUpdates call in ASRouter is part of method (2). If you wanted the update process to actually continue after this update check, you'd have to do what AppUpdater does and call selectUpdate (here) and then downloadUpdate (here).

Since ASRouter doesn't make those calls, it isn't going to download any updates.

Comment 5

[:Gijs (he/him)]

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

Since ASRouter doesn't make those calls, it isn't going to download any updates.

Thanks for the clarification!

What happens with the result from the ASRouter check, the next time that the background/notify calls (1) runs? Does the result get used/reused, or do we re-fetch the information? (Does the answer change if the result from the ASRouter check was "there is an update" and the result from the background check is "no update" ?)

Comment 6

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

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

What happens with the result from the ASRouter check, the next time that the background/notify calls (1) runs? Does the result get used/reused, or do we re-fetch the information?

The update checking service only returns the result to the caller. It does not cache it anywhere.

(Does the answer change if the result from the ASRouter check was "there is an update" and the result from the background check is "no update" ?)

No.

Comment 7

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

I'm a bit surprised to see someone assigned to this since I'm confused as to what we want to change here and why. Is it to have ASRouter do a background check instead of a foreground check? That seems like a bit of a poor idea since it makes it somewhat non-deterministic as to whether the messaging system thinks there is an update available or not. The update server, Balrog, typically chooses a random number to decide if the main update mapping or the fallback update mapping is used. If the browser is already running the version offered as the fallback update, whether or not the messaging system says there is an update available is dependent on the result of that random value. That seems undesirable to me.

Am I understanding change that is planned correctly? If so, are we really okay with having this value be non-deterministic?

Comment 8

[Punam Dahiya [:pdahiya]]

@bytesized this bug can be repurposed to remove needsUpdate targeting which isn't in use for last 6 years, this will help reduce cost of maintaining this code

https://searchfox.org/mozilla-central/rev/488d81581a9142d532bf814efa60564ff11599ca/browser/components/asrouter/modules/ASRouterTargeting.sys.mjs#719

In addition we should audit as part of this or a separate bug what all targetings a) shouldn't be eagerly evaluated b) Filter those out from targeting snapshot exposed to Background Updater.

Comment 9

[:Gijs (he/him)]

(In reply to Punam Dahiya [:pdahiya] from comment #8)

In addition we should audit as part of this or a separate bug what all targetings a) shouldn't be eagerly evaluated b) Filter those out from targeting snapshot exposed to Background Updater.

A separate bug would probably make sense. Can you link it here once filed? Thank you!

Comment 10

[Punam Dahiya [:pdahiya]]

Filed targeting audit as a separate bug 1924253 thanks!

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit BugBot documentation.