1273709 - system add-on updates should not require downloads unless newer than defaults

Status: RESOLVED DUPLICATE of bug 1295732

(Toolkit :: Add-ons Manager, defect)

Description

[Robert Helmer [:rhelmer]]

Currently, the whole set of system add-ons expected to be installed in Firefox must be present in the Balrog response, and they will be downloaded if the set is modified at all (for instance a new version of just one add-on).

This means that all system add-ons must be signed and staged on the server in order to push an update for one of them, which is a waste of time for folks preparing releases.

It also means that the add-ons manager is wasting time downloading add-ons that are unnecessary.

Comment 1

[Robert Helmer [:rhelmer]]

Hm, so there was a patch to avoid re-downloading in bug 1204158, *but* my read of this is that it's just not re-downloading files that have already been staged for update so that doesn't help us here:

https://hg.mozilla.org/mozilla-central/rev/7221fb39f8e5

We shouldn't be requiring that XPIs are signed+staged either though. I think it's reasonable to require that all addon IDs and version numbers are present in the Balrog response, but update URL should be optional.

Ben, would it be troublesome to have Balrog generate an <addon> in the update response without any URL attribute, in the case where we want to use the built-in system add-ons? Also - I checked and I believe the current behavior when we have a totally empty Balrog response is to do nothing:

https://dxr.mozilla.org/mozilla-central/rev/7221fb39f8e5/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2805-2809

This would allow us to control what system add-ons users have installed and activated, but not require us to sign+stage XPIs that are not actually useful.

I am concerned that we're not being diligent enough with version numbers, and that checking the addon ID and version in the Balrog response against what we have locally is not enough.

However, if the URL attribute is missing, then that's a very safe way to avoid requiring a download, while still requiring the addon to be activated.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Robert Helmer [:rhelmer] from comment #1)
> Hm, so there was a patch to avoid re-downloading in bug 1204158, *but* my
> read of this is that it's just not re-downloading files that have already
> been staged for update so that doesn't help us here:
> 
> https://hg.mozilla.org/mozilla-central/rev/7221fb39f8e5
> 
> We shouldn't be requiring that XPIs are signed+staged either though. I think
> it's reasonable to require that all addon IDs and version numbers are
> present in the Balrog response, but update URL should be optional.
> 
> Ben, would it be troublesome to have Balrog generate an <addon> in the
> update response without any URL attribute, in the case where we want to use
> the built-in system add-ons?

I think it would be much better in the grand schemue of things if we had the right processes and tools in place such that we could rely on version numbers.

However, if we need to this it would probably be okay from a Balrog standpoint. We should take the opportunity to fork the GMP blobs that we're currently using for SystemAddons (https://github.com/mozilla/balrog/blob/master/auslib/blobs/gmp.py, https://github.com/mozilla/balrog/blob/master/auslib/blobs/schemas/gmp.yml), as this isn't a feature we'd want for them, and I suspect it won't be the last divergence between the two.

We should also think about this in the bigger picture of the other changes we're thinking about making to the update request/response before we move ahead with anything.

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #1)
> I am concerned that we're not being diligent enough with version numbers,
> and that checking the addon ID and version in the Balrog response against
> what we have locally is not enough.

Actually Balrog responds with the size and expected checksum for the file as well, so we could check this and it should be a reliable indicator if we already have the file locally.

So we should check the Balrog response first against the "default" then against the "upgraded" add-ons, if the following match then the download can be skipped safely:

1) addon ID
2) version
3) size
4) checksum

This means that we still need to go through the pain and annoyance of signing+uploading XPI files for each system add-on we ship, but we expect that this process can be automated in the near future.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> (In reply to Robert Helmer [:rhelmer] from comment #1)
> > I am concerned that we're not being diligent enough with version numbers,
> > and that checking the addon ID and version in the Balrog response against
> > what we have locally is not enough.
> 
> Actually Balrog responds with the size and expected checksum for the file as
> well, so we could check this and it should be a reliable indicator if we
> already have the file locally.
> 
> So we should check the Balrog response first against the "default" then
> against the "upgraded" add-ons, if the following match then the download can
> be skipped safely:
> 
> 1) addon ID
> 2) version
> 3) size
> 4) checksum
> 
> This means that we still need to go through the pain and annoyance of
> signing+uploading XPI files for each system add-on we ship, but we expect
> that this process can be automated in the near future.

Are the XPI hashes and sizes published in Balrog going to be the ones that ship as part of Firefox? I'm not sure if we're adding existing hashes/sizes to Balrog, or actually signing/uploading a new XPI. This is probably a question for Mark.

Comment 5

[Mark Banner (:standard8)]

(In reply to Ben Hearsum (:bhearsum) from comment #4)
> (In reply to Robert Helmer [:rhelmer] from comment #3)
> > This means that we still need to go through the pain and annoyance of
> > signing+uploading XPI files for each system add-on we ship, but we expect
> > that this process can be automated in the near future.
> 
> Are the XPI hashes and sizes published in Balrog going to be the ones that
> ship as part of Firefox? I'm not sure if we're adding existing hashes/sizes
> to Balrog, or actually signing/uploading a new XPI. This is probably a
> question for Mark.

Rob's comment is slightly unclear.

Currently the xpis in Firefox are unsigned, the ones we put on ftp for Balrog are signed.

We do put hashes/sizes onto Balrog for all xpis, but that's after they've been signed (even if they were included in FF build).

So if we were signing the ones in the FF build, then we could potentially just use the same sizes/hashes.

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Mark Banner (:standard8) from comment #5)
> (In reply to Ben Hearsum (:bhearsum) from comment #4)
> > (In reply to Robert Helmer [:rhelmer] from comment #3)
> > > This means that we still need to go through the pain and annoyance of
> > > signing+uploading XPI files for each system add-on we ship, but we expect
> > > that this process can be automated in the near future.
> > 
> > Are the XPI hashes and sizes published in Balrog going to be the ones that
> > ship as part of Firefox? I'm not sure if we're adding existing hashes/sizes
> > to Balrog, or actually signing/uploading a new XPI. This is probably a
> > question for Mark.
> 
> Rob's comment is slightly unclear.
> 
> Currently the xpis in Firefox are unsigned, the ones we put on ftp for
> Balrog are signed.
> 
> We do put hashes/sizes onto Balrog for all xpis, but that's after they've
> been signed (even if they were included in FF build).
> 
> So if we were signing the ones in the FF build, then we could potentially
> just use the same sizes/hashes.

OK. But as things stand right now, publishing the signed hashes/sizes of the addons we ship with Firefox won't fix this bug AFAICT, because we ship the unsigned ones (and thus have no way to know whether the hashes/sizes in Balrog match)?

Comment 7

[Mark Banner (:standard8)]

(In reply to Ben Hearsum (:bhearsum) from comment #6)
> OK. But as things stand right now, publishing the signed hashes/sizes of the
> addons we ship with Firefox won't fix this bug AFAICT, because we ship the
> unsigned ones (and thus have no way to know whether the hashes/sizes in
> Balrog match)?

Correct.

Comment 8

[Robert Helmer [:rhelmer]]

Changing summary to reflect discussion.

Comment 9

[Robert Helmer [:rhelmer]]

Maybe we should take this approach:

1) require an ID in the Balrog response for all built-in ("default") system add-ons
2) require an ID+version+size+hash+URL for all updates

The client will reject any Balrog response that doesn't meet these criteria.

I think we can trust that the default location is OK in case #1 so don't need to worry about hash+size there, it's really the update location that we need to be concerned with. I don't think the version number is important in this case either, since we know what version of Firefox we are targeting, so it's just extra noise as far as Balrog configuration is concerned.

One outstanding question is - what do we do about short-lived updates that never ship with the browser (hotfixes for instance)? We don't want to have to keep specifying these forever.

I think the best way to deal with that might be to simply discard updates when we do an application update - bug 1273707 implies that we already do this, but I need to confirm. We *could* implicitly discard any updates not specified in the response. The only real benefit would be that we could remove updates more quickly, I think doing it on app update gives us more predictability.

Either way, I think this would be better than the current situation where we implicitly disable default add-ons as well as updates.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #9)
> I think the best way to deal with that might be to simply discard updates
> when we do an application update - bug 1273707 implies that we already do
> this, but I need to confirm.

Confirmed, and we have test coverage too.

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Robert Helmer [:rhelmer] from comment #9)
> Maybe we should take this approach:
> 
> 1) require an ID in the Balrog response for all built-in ("default") system
> add-ons
> 2) require an ID+version+size+hash+URL for all updates
> 
> The client will reject any Balrog response that doesn't meet these criteria.
> 
> I think we can trust that the default location is OK in case #1 so don't
> need to worry about hash+size there, it's really the update location that we
> need to be concerned with. I don't think the version number is important in
> this case either, since we know what version of Firefox we are targeting, so
> it's just extra noise as far as Balrog configuration is concerned.

I agree that this is better than requiring metadata for stuff that shipped with the browser. It makes me question why we should include addons at all though? If we've decided not to allow uninstalls at all (except through no-op XPIs) per https://bugzilla.mozilla.org/show_bug.cgi?id=1272672#c1, I'm not sure what the difference between this and only listing updates in the response is?

Comment 12

[Robert Helmer [:rhelmer]]

(In reply to Ben Hearsum (:bhearsum) from comment #11)
> (In reply to Robert Helmer [:rhelmer] from comment #9)
> > Maybe we should take this approach:
> > 
> > 1) require an ID in the Balrog response for all built-in ("default") system
> > add-ons
> > 2) require an ID+version+size+hash+URL for all updates
> > 
> > The client will reject any Balrog response that doesn't meet these criteria.
> > 
> > I think we can trust that the default location is OK in case #1 so don't
> > need to worry about hash+size there, it's really the update location that we
> > need to be concerned with. I don't think the version number is important in
> > this case either, since we know what version of Firefox we are targeting, so
> > it's just extra noise as far as Balrog configuration is concerned.
> 
> I agree that this is better than requiring metadata for stuff that shipped
> with the browser. It makes me question why we should include addons at all
> though? If we've decided not to allow uninstalls at all (except through
> no-op XPIs) per https://bugzilla.mozilla.org/show_bug.cgi?id=1272672#c1, I'm
> not sure what the difference between this and only listing updates in the
> response is?

Good point - if there is no way to disable built-in system add-ons then "Firefox 51.0 with built-in set" is equivalent to "Firefox 51.0 + e10srollout + pocket + ...", since we know what was shipped.

I think you're right that we only need to really focus on update sets in the Balrog response.

One issue right now is that we can't mix+match the built-in versus updated set - when an update happens, the client switches from using the built-in set location to using the updated set location. This is one of the reasons we require signing+hosting all the updates and specifying them all in the update response.

So to avoid downloading already-present built-in system add-ons, we'd either need to copy them from the default to the update location, or (to avoid the copy+wasted disk space) let the update location override the default location on per-ID basis.

The latter is the approach used to allow normal add-ons to override system add-ons.

Comment 13

[[e-mail] (:bhearsum)]

(In reply to Robert Helmer [:rhelmer] from comment #12)
> (In reply to Ben Hearsum (:bhearsum) from comment #11)
> > (In reply to Robert Helmer [:rhelmer] from comment #9)
> > > Maybe we should take this approach:
> > > 
> > > 1) require an ID in the Balrog response for all built-in ("default") system
> > > add-ons
> > > 2) require an ID+version+size+hash+URL for all updates
> > > 
> > > The client will reject any Balrog response that doesn't meet these criteria.
> > > 
> > > I think we can trust that the default location is OK in case #1 so don't
> > > need to worry about hash+size there, it's really the update location that we
> > > need to be concerned with. I don't think the version number is important in
> > > this case either, since we know what version of Firefox we are targeting, so
> > > it's just extra noise as far as Balrog configuration is concerned.
> > 
> > I agree that this is better than requiring metadata for stuff that shipped
> > with the browser. It makes me question why we should include addons at all
> > though? If we've decided not to allow uninstalls at all (except through
> > no-op XPIs) per https://bugzilla.mozilla.org/show_bug.cgi?id=1272672#c1, I'm
> > not sure what the difference between this and only listing updates in the
> > response is?
> 
> Good point - if there is no way to disable built-in system add-ons then
> "Firefox 51.0 with built-in set" is equivalent to "Firefox 51.0 +
> e10srollout + pocket + ...", since we know what was shipped.
> 
> I think you're right that we only need to really focus on update sets in the
> Balrog response.

Just to make we're both fully on the same page....it sounds like we're saying that:
- When we first ship a release with no out of band updates, Balrog should return nothing in the response.
- If we ship a new System Addon to that version, Balrog should _only_ return that new system addon in the response.
- If we ship an update to a built in system addon, Balrog should _only_ return that new version for that system addon in the response.
- If we ship an update to a built in system addon and a new System Addon, Balrog should return both of those in the response (but not any built in system addons that don't have updates).

> One issue right now is that we can't mix+match the built-in versus updated
> set - when an update happens, the client switches from using the built-in
> set location to using the updated set location. This is one of the reasons
> we require signing+hosting all the updates and specifying them all in the
> update response.
> 
> So to avoid downloading already-present built-in system add-ons, we'd either
> need to copy them from the default to the update location, or (to avoid the
> copy+wasted disk space) let the update location override the default
> location on per-ID basis.
> 
> The latter is the approach used to allow normal add-ons to override system
> add-ons.

And I read these last few paragraphs as "the client needs a few changes to make the above points possible"

Comment 14

[Robert Helmer [:rhelmer]]

(In reply to Ben Hearsum (:bhearsum) from comment #13)
> (In reply to Robert Helmer [:rhelmer] from comment #12)
> > (In reply to Ben Hearsum (:bhearsum) from comment #11)
> > > (In reply to Robert Helmer [:rhelmer] from comment #9)
> > > > Maybe we should take this approach:
> > > > 
> > > > 1) require an ID in the Balrog response for all built-in ("default") system
> > > > add-ons
> > > > 2) require an ID+version+size+hash+URL for all updates
> > > > 
> > > > The client will reject any Balrog response that doesn't meet these criteria.
> > > > 
> > > > I think we can trust that the default location is OK in case #1 so don't
> > > > need to worry about hash+size there, it's really the update location that we
> > > > need to be concerned with. I don't think the version number is important in
> > > > this case either, since we know what version of Firefox we are targeting, so
> > > > it's just extra noise as far as Balrog configuration is concerned.
> > > 
> > > I agree that this is better than requiring metadata for stuff that shipped
> > > with the browser. It makes me question why we should include addons at all
> > > though? If we've decided not to allow uninstalls at all (except through
> > > no-op XPIs) per https://bugzilla.mozilla.org/show_bug.cgi?id=1272672#c1, I'm
> > > not sure what the difference between this and only listing updates in the
> > > response is?
> > 
> > Good point - if there is no way to disable built-in system add-ons then
> > "Firefox 51.0 with built-in set" is equivalent to "Firefox 51.0 +
> > e10srollout + pocket + ...", since we know what was shipped.
> > 
> > I think you're right that we only need to really focus on update sets in the
> > Balrog response.
> 
> Just to make we're both fully on the same page....it sounds like we're
> saying that:
> - When we first ship a release with no out of band updates, Balrog should
> return nothing in the response.


Right, we already know what set shipped with Firefox. This implies fixing bug 1287191 and changing the client behavior of the "empty" response (`<addons></addons>`) that currently causes built-in system add-ons to be disabled.


> - If we ship a new System Addon to that version, Balrog should _only_ return
> that new system addon in the response.
> - If we ship an update to a built in system addon, Balrog should _only_
> return that new version for that system addon in the response.
> - If we ship an update to a built in system addon and a new System Addon,
> Balrog should return both of those in the response (but not any built in
> system addons that don't have updates).


I'm not sure about the above. I think we might want to continue serving all updates, and fall back to built-in for any unspecified updates. We should be keeping track of which updates are being served to which Firefox versions, IMHO.

Is this problematic?


> > One issue right now is that we can't mix+match the built-in versus updated
> > set - when an update happens, the client switches from using the built-in
> > set location to using the updated set location. This is one of the reasons
> > we require signing+hosting all the updates and specifying them all in the
> > update response.
> > 
> > So to avoid downloading already-present built-in system add-ons, we'd either
> > need to copy them from the default to the update location, or (to avoid the
> > copy+wasted disk space) let the update location override the default
> > location on per-ID basis.
> > 
> > The latter is the approach used to allow normal add-ons to override system
> > add-ons.
> 
> And I read these last few paragraphs as "the client needs a few changes to
> make the above points possible"

Right.

Comment 15

[[e-mail] (:bhearsum)]

(In reply to Robert Helmer [:rhelmer] from comment #14)
> (In reply to Ben Hearsum (:bhearsum) from comment #13)
> > (In reply to Robert Helmer [:rhelmer] from comment #12)
> > > (In reply to Ben Hearsum (:bhearsum) from comment #11)
> > > > (In reply to Robert Helmer [:rhelmer] from comment #9)
> > > > > Maybe we should take this approach:
> > > > > 
> > > > > 1) require an ID in the Balrog response for all built-in ("default") system
> > > > > add-ons
> > > > > 2) require an ID+version+size+hash+URL for all updates
> > > > > 
> > > > > The client will reject any Balrog response that doesn't meet these criteria.
> > > > > 
> > > > > I think we can trust that the default location is OK in case #1 so don't
> > > > > need to worry about hash+size there, it's really the update location that we
> > > > > need to be concerned with. I don't think the version number is important in
> > > > > this case either, since we know what version of Firefox we are targeting, so
> > > > > it's just extra noise as far as Balrog configuration is concerned.
> > > > 
> > > > I agree that this is better than requiring metadata for stuff that shipped
> > > > with the browser. It makes me question why we should include addons at all
> > > > though? If we've decided not to allow uninstalls at all (except through
> > > > no-op XPIs) per https://bugzilla.mozilla.org/show_bug.cgi?id=1272672#c1, I'm
> > > > not sure what the difference between this and only listing updates in the
> > > > response is?
> > > 
> > > Good point - if there is no way to disable built-in system add-ons then
> > > "Firefox 51.0 with built-in set" is equivalent to "Firefox 51.0 +
> > > e10srollout + pocket + ...", since we know what was shipped.
> > > 
> > > I think you're right that we only need to really focus on update sets in the
> > > Balrog response.
> > 
> > Just to make we're both fully on the same page....it sounds like we're
> > saying that:
> > - When we first ship a release with no out of band updates, Balrog should
> > return nothing in the response.
> 
> 
> Right, we already know what set shipped with Firefox. This implies fixing
> bug 1287191 and changing the client behavior of the "empty" response
> (`<addons></addons>`) that currently causes built-in system add-ons to be
> disabled.
> 
> 
> > - If we ship a new System Addon to that version, Balrog should _only_ return
> > that new system addon in the response.
> > - If we ship an update to a built in system addon, Balrog should _only_
> > return that new version for that system addon in the response.
> > - If we ship an update to a built in system addon and a new System Addon,
> > Balrog should return both of those in the response (but not any built in
> > system addons that don't have updates).
> 
> 
> I'm not sure about the above. I think we might want to continue serving all
> updates, and fall back to built-in for any unspecified updates. We should be
> keeping track of which updates are being served to which Firefox versions,
> IMHO.
> 
> Is this problematic?

Hm, I think I'm confused! I thought that after comment #12 we thought that putting just the name of built in addons was the same as leaving them out, it sounds like I misunderstood that. It's not a problem to continue including them, I'm just clarifying. So, let's try those last three points again....
- If we ship a new System Addon to that version, Balrog will return the new addon with url+hash and only the names of the built in addons.
- If we ship an update to a built in System Addon, Balrog will return the updated addon with url+hash and only the names of the other built in addons
- If we ship an update to a built in System Addon and a new System Addon, Balrog will return the updated addon with url+hash, the new addon with url+hash, and just the names of the other built in System Addons.

If that's still wrong let's sync up on Vidyo :).

Comment 16

[Robert Helmer [:rhelmer]]

(In reply to Ben Hearsum (:bhearsum) from comment #15)
> Hm, I think I'm confused! I thought that after comment #12 we thought that
> putting just the name of built in addons was the same as leaving them out,
> it sounds like I misunderstood that. It's not a problem to continue
> including them, I'm just clarifying. So, let's try those last three points
> again....
> - If we ship a new System Addon to that version, Balrog will return the new
> addon with url+hash and only the names of the built in addons.
> - If we ship an update to a built in System Addon, Balrog will return the
> updated addon with url+hash and only the names of the other built in addons
> - If we ship an update to a built in System Addon and a new System Addon,
> Balrog will return the updated addon with url+hash, the new addon with
> url+hash, and just the names of the other built in System Addons.
> 
> If that's still wrong let's sync up on Vidyo :).

Sorry, you had it right before, I don't think there's a point to specifying the built-in add-ons since we know what shipped with the version of Firefox we're targeting for updates.

My only amendment was that it should always specify all updates.

Comment 17

[Robert Helmer [:rhelmer]]

Changing summary back, after chatting w/ Osmose we should just have the server specify a list of built-in add-ons too. This is less change from where we are now, and resolves concerns around being sure of what was shipped with Firefox (enforced in the client by bug 1292029).

tl;dr let's do comment 15.

Comment 18

[Robert Helmer [:rhelmer]]

Currently, system add-ons require re-downloading because Firefox only uses either the "default" or "updates" location, there is no way to mix-and-match between them.

A simple hack would be to simply copy the built-in XPIs to the update location, but I think we can do better: the addon manager supports precedence for install locations. This is the reason that users can install an add-on and have it override a built-in system add-on with the same ID. This feature is also used by "temporary" add-ons.

We still want to download and install updates as a set - that is, if download, verification or installation of any update fails, then we should roll back to the previous update. However, instead of switching install locations on restart, we can just allow the "updates" location to have precedence over the "default" location.

This also paves the way for doing restartless installs and updates of system add-ons. Since we're changing the current "switch install location on application restart" behavior, and we have a "delayed upgrade" feature (bug 1231172, coming soon for web extensions in bug 1279012) we should allow for restartless installs and updates (bug 1204156).

Before we do this, let's add better tests for the precedence feature (bug 557710).

Comment 19

[Robert Helmer [:rhelmer]]

I split comment 18 out to bug 1295732.

I think we might not actually need to keep this bug, I think we'll get the same effect from fixing the little ones that have been split off.

Comment 20

[Robert Helmer [:rhelmer]]

I think the discussion in here has been helpful but the work is covered by other bugs which are a bit more well-scoped and actionable like bug 1204158 and bug 1295732.