1272672 - make system addon downgrades/uninstalls explicit rather than implicit

Status: RESOLVED INVALID

(Toolkit :: Add-ons Manager, defect)

Description

[[e-mail] (:bhearsum)]

My understanding of the current client System Addon update code is that if a specific addon is absent in the response from Balrog, it will be uninstalled entirely (though I've been told this may have been changed to "revert to built in version"). This has a few rough edges that have been identified recently:
* As soon as we ship an update to any System Addon, we must include _all_ System Addons in the update response. This means we're required to add metadata to Balrog for built-in System Addons, not just out of band updates.
* Per-System Addon throttling is going to be very difficult right now. We'll soon have the ability to set per-addon throttle rates, but it will result in responses that may only not include all possible system addons.
* If we accidentally configure the Balrog rules in such a way that an addon isn't returned, we will accidentally downgrade people.

I'd like to propose that we change the client to do nothing when an addon is absent from the Balrog response, which would address all of the above points. If uninstalling entirely or reverting to the built in version are behaviours we need to support, we can add specific instructions for that in the response, eg:
<updates>
<addons>
  <addon id="[e-mail]" action="uninstall">
</addons>
</updates>



This was discussed on the Go Faster list, and everyone who responded was in agreement so far.

Comment 1

[Robert Helmer [:rhelmer]]

(In reply to Ben Hearsum (:bhearsum) from comment #0)
> My understanding of the current client System Addon update code is that if a
> specific addon is absent in the response from Balrog, it will be uninstalled
> entirely (though I've been told this may have been changed to "revert to
> built in version"). This has a few rough edges that have been identified
> recently:

I've been spending some time on this lately, and the currnt behavior is that an empty add-on set (`<update><addons/></update>`) causes the built-in versions to be disabled.

I am pretty sure we never want to do this, so I am considering removing support for this behavior completely, and just treating that the same as we treat a "blank" update (`<update></update>`).

(In reply to Ben Hearsum (:bhearsum) from comment #0)
> I'd like to propose that we change the client to do nothing when an addon is
> absent from the Balrog response, which would address all of the above
> points.

Right now the client only honors one location at a time - either the "built-in" location or the "updates" location, which is why we have the current behavior.

It sounds like what most people want and expect is something similar to the way our "temporary" add-on support works - add-ons in the default location are overridden by the add-on in the temporary location, per-addon-ID.

> If uninstalling entirely or reverting to the built in version are
> behaviours we need to support, we can add specific instructions for that in
> the response

The more I think about it, an "uninstall" type of feature is likely to be harmful and isn't really useful. It'd be better to either ship a full Firefox build with the feature removed, or ship a no-op XPI if it was truly urgent. I can't think why removing a feature would be this urgent though.

I think these two are problems that we need to solve:

(In reply to Ben Hearsum (:bhearsum) from comment #0)
> * As soon as we ship an update to any System Addon, we must include _all_
> System Addons in the update response. This means we're required to add
> metadata to Balrog for built-in System Addons, not just out of band updates.
> * Per-System Addon throttling is going to be very difficult right now. We'll
> soon have the ability to set per-addon throttle rates, but it will result in
> responses that may only not include all possible system addons.

Are these both covered by bug 1281347 now?

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Robert Helmer [:rhelmer] from comment #1)
> I think these two are problems that we need to solve:
> 
> (In reply to Ben Hearsum (:bhearsum) from comment #0)
> > * As soon as we ship an update to any System Addon, we must include _all_
> > System Addons in the update response. This means we're required to add
> > metadata to Balrog for built-in System Addons, not just out of band updates.

Do we even want to address this at this point? I was under the impression that it was a feature, not a bug, that we wanted to always include all System Addons in the response. Maybe bug 1281347 solves this in the sense that it codifies that, but I don't think it will remove the need to include all System addons in the response.

> > * Per-System Addon throttling is going to be very difficult right now. We'll
> > soon have the ability to set per-addon throttle rates, but it will result in
> > responses that may only not include all possible system addons.

One of the dep bugs from bug 1281347 will make this possible, yeah.

Probably should've closed this after https://bugzilla.mozilla.org/show_bug.cgi?id=1281347 was opened, in retrospect...