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

RESOLVED WONTFIX

Status

()

Toolkit
Add-ons Manager
RESOLVED WONTFIX
11 months ago
2 months ago

People

(Reporter: mkelly, Assigned: rhelmer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
Created attachment 8764082 [details] [diff] [review]
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
Attachment #8764082 - Flags: feedback?(rhelmer)
Attachment #8764082 - Flags: feedback?(bhearsum)

Comment 1

11 months ago
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="loop@mozilla.org" URL="https://ftp.mozilla.org/pub/system-addons/hello/loop@mozilla.org-2.0.xpi" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> +      <addon id="pocket@mozilla.org" URL="https://ftp.mozilla.org/pub/system-addons/pocket/pocket@mozilla.org-1.0.xpi" 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

11 months ago
(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="loop@mozilla.org" URL="https://ftp.mozilla.org/pub/system-addons/hello/loop@mozilla.org-2.0.xpi" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> > +      <addon id="pocket@mozilla.org" URL="https://ftp.mozilla.org/pub/system-addons/pocket/pocket@mozilla.org-1.0.xpi" 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.
Flags: needinfo?(mkelly)
(Reporter)

Comment 3

11 months ago
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="loop@mozilla.org" URL="https://ftp.mozilla.org/pub/system-addons/hello/loop@mozilla.org-2.0.xpi" hashFunction="sha512" hashValue="abcdef123" size="1234" version="1.0"/>
> +      <addon id="pocket@mozilla.org" URL="https://ftp.mozilla.org/pub/system-addons/pocket/pocket@mozilla.org-1.0.xpi" 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.
(Reporter)

Comment 4

11 months ago
Created attachment 8765518 [details] [diff] [review]
doc_updates.diff

Updated the misleading sentence that bhearsum pointed out.
Attachment #8764082 - Attachment is obsolete: true
Attachment #8764082 - Flags: feedback?(rhelmer)
Attachment #8764082 - Flags: feedback?(bhearsum)
Flags: needinfo?(mkelly)
Attachment #8765518 - Flags: feedback?(rhelmer)
Attachment #8765518 - Flags: feedback?(bhearsum)
(Assignee)

Updated

11 months ago
Attachment #8765518 - Flags: feedback?(rhelmer) → feedback+
(Assignee)

Comment 5

11 months ago
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.
(Reporter)

Comment 6

11 months ago
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

11 months ago
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?
Attachment #8765518 - Flags: feedback?(bhearsum) → feedback+
(Reporter)

Comment 8

11 months ago
(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.

Updated

11 months ago
Depends on: 1282891

Updated

11 months ago
Depends on: 1282898
(Reporter)

Comment 9

11 months ago
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).
Flags: needinfo?(rhelmer)

Updated

11 months ago
Whiteboard: triaged
(Assignee)

Comment 10

11 months ago
(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.
Flags: needinfo?(rhelmer)
(Assignee)

Updated

10 months ago
Assignee: nobody → rhelmer
(Assignee)

Updated

10 months ago
Summary: Support revision numbers for system add-on updates and fail on missing default system add-ons → [tracking] Support revision numbers for system add-on updates and fail on missing default system add-ons
(Assignee)

Updated

10 months ago
Depends on: 1273709
(Assignee)

Updated

10 months ago
Depends on: 1292029
(Assignee)

Updated

10 months ago
Depends on: 1292031
(Assignee)

Updated

9 months ago
Depends on: 1295732
(Assignee)

Updated

9 months ago
Summary: [tracking] Support revision numbers for system add-on updates and fail on missing default system add-ons → [tracking] Support revision numbers for system add-on updates and only require updated add-ons to be specified by server
(Assignee)

Comment 11

2 months ago
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).
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.