1281347 - [tracking] Support revision numbers for system add-on updates and only require updated add-ons to be specified by server

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

[Osmose [:osmose, :mkelly]]

Attachment: doc_updates.diff

After a long discussion on the mailing list[1] and in-person at MozLondon, the gofaster group has decided to make two changes to the system add-on update process:

1. If an update response does not contain all the default system add-ons that shipped with Firefox, the update process should be aborted. This will help guard against mistakes in specifying all system add-ons in an update, and will also force us to always be explicit about what system add-on version combinations are cleared to be used together.

2. Update responses should have a "revision number" that tracks which revision of the update response is being sent out. The revision number should increase whenever the update response changes, and Firefox should store the revision number of the last update response it received, and ignore any update responses with a revision lower than the stored number. This helps resolve some issues around the first change where system add-ons being partially rolled out would be accidentally downgraded.

rhlemer is going to handle implementation on the Firefox side of these changes, and bhearsum is going to handle implementation on the Balrog side. I've attached a diff of the spec changes to the documentation for system add-on updates. This bug is mostly a tracker for these two implementations.

[1] https://mail.mozilla.org/pipermail/gofaster/2016-May/000341.html

Comment 1

[[e-mail] (:bhearsum)]

Comment on attachment 8764082 [details] [diff] [review]
doc_updates.diff

Review of attachment 8764082 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/docs/SystemAddons.rst
@@ +217,5 @@
> +    <addons revision="5">
> +      <addon id="[e-mail]" URL="https://ftp.mozilla.org/pub/system-addons/hello/[e-mail]" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> +      <addon id="[e-mail]" URL="https://ftp.mozilla.org/pub/system-addons/pocket/[e-mail]" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> +    </addons>
> +  </updates>

Are we going to be returning this old revision, or returning an empty response like the text above this says)? I think we said we wanted the former, so that people who get on the wrong side of the dice roll get the latest available stable thing.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum (:bhearsum) from comment #1)
> Comment on attachment 8764082 [details] [diff] [review]
> doc_updates.diff
> 
> Review of attachment 8764082 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/extensions/docs/SystemAddons.rst
> @@ +217,5 @@
> > +    <addons revision="5">
> > +      <addon id="[e-mail]" URL="https://ftp.mozilla.org/pub/system-addons/hello/[e-mail]" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> > +      <addon id="[e-mail]" URL="https://ftp.mozilla.org/pub/system-addons/pocket/[e-mail]" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> > +    </addons>
> > +  </updates>
> 
> Are we going to be returning this old revision, or returning an empty
> response like the text above this says)? I think we said we wanted the
> former, so that people who get on the wrong side of the dice roll get the
> latest available stable thing.

Whoops, meant to needinfo you.

Comment 3

[Osmose [:osmose, :mkelly]]

Comment on attachment 8764082 [details] [diff] [review]
doc_updates.diff

Review of attachment 8764082 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/docs/SystemAddons.rst
@@ +217,5 @@
> +    <addons revision="5">
> +      <addon id="[e-mail]" URL="https://ftp.mozilla.org/pub/system-addons/hello/[e-mail]" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> +      <addon id="[e-mail]" URL="https://ftp.mozilla.org/pub/system-addons/pocket/[e-mail]" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> +    </addons>
> +  </updates>

Whoops. The text above about an empty version is the wrong text. You're right, we want the former.

Comment 4

[Osmose [:osmose, :mkelly]]

Attachment: doc_updates.diff

Updated the misleading sentence that bhearsum pointed out.

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8765518 [details] [diff] [review]
doc_updates.diff

Review of attachment 8765518 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/docs/SystemAddons.rst
@@ +109,4 @@
>     the update response, remove all the  **update** add-ons and finish.
> +6. If the set of add-ons specified in the update response does not include all
> +   of the **default** add-ons (versions do not have to match), abort the update
> +   process.

If the version does not have to match then the SHA does not as well, correct? Might want to call that out specifically.

Also I want to point out that someone (or something, like malware/adware) dropping a XPI into the system addon install location in the application will prevent updates from happening. Bug 1277920 is the right fix for that though.

Comment 6

[Osmose [:osmose, :mkelly]]

Comment on attachment 8765518 [details] [diff] [review]
doc_updates.diff

Review of attachment 8765518 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/docs/SystemAddons.rst
@@ +109,4 @@
>     the update response, remove all the  **update** add-ons and finish.
> +6. If the set of add-ons specified in the update response does not include all
> +   of the **default** add-ons (versions do not have to match), abort the update
> +   process.

I think what I really want to say is that the add-on IDs must match. Does that sound right?

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 8765518 [details] [diff] [review]
doc_updates.diff

Review of attachment 8765518 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me. Should these doc updates land at the same time as the client side code changes to help avoid confusion, or does it not matter much for Nightly?

Comment 8

[Osmose [:osmose, :mkelly]]

(In reply to Ben Hearsum (:bhearsum) from comment #7)
> This looks fine to me. Should these doc updates land at the same time as the
> client side code changes to help avoid confusion, or does it not matter much
> for Nightly?

I figured rhelmer would include these changes in the client side patch; IMO the in-tree docs should always be in sync with the code.

Comment 9

[Osmose [:osmose, :mkelly]]

rhelmer: Can you test the behavior of current Firefox against the new response format? Based on the spec I'm pretty sure always specifying all add-ons in the response won't break anything, but I'm wondering if adding the revision numbers in will break anything.

If not, then it seems like we should have Balrog update first to add revision numbers and such, and then land the client-side Firefox change. That way existing clients will still get updates and just ignore the revision numbers harmlessly (except in the case of a new version rollout of a system add-on, which we can either just avoid or preserve the old server behavior of sending an empty response until the client updates land on the channel we want to roll out to).

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Michael Kelly [:mkelly,:Osmose] from comment #9)
> rhelmer: Can you test the behavior of current Firefox against the new
> response format? Based on the spec I'm pretty sure always specifying all
> add-ons in the response won't break anything, but I'm wondering if adding
> the revision numbers in will break anything.
> 
> If not, then it seems like we should have Balrog update first to add
> revision numbers and such, and then land the client-side Firefox change.
> That way existing clients will still get updates and just ignore the
> revision numbers harmlessly (except in the case of a new version rollout of
> a system add-on, which we can either just avoid or preserve the old server
> behavior of sending an empty response until the client updates land on the
> channel we want to roll out to).

OK tested this and can confirm that `revision` attribute on the `<addons>` tag is ignored by the current client. I agree with the assessment above, adding the revision number anytime is fine but we don't want to change the "empty response" throttling behavior until the client is ready.

Comment 11

[Robert Helmer [:rhelmer]]

Ben and I just discussed and although this would be useful for throttled roll-outs, in practice we haven't actually needed it (add-ons either do their own a/b implementation locally, or we use Telemetry Experiments first).