Last Comment Bug 1281347 - [tracking] Support revision numbers for system add-on updates and only require updated add-ons to be specified by server
: [tracking] Support revision numbers for system add-on updates and only requir...
Status: NEW
triaged
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: Robert Helmer [:rhelmer]
:
: Andy McKay [:andym]
Mentors:
Depends on: 1292031 1273709 1282891 1282898 1292029 1295732
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-21 17:30 PDT by Michael Kelly [:mkelly,:Osmose]
Modified: 2016-08-19 10:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
doc_updates.diff (11.08 KB, patch)
2016-06-21 17:30 PDT, Michael Kelly [:mkelly,:Osmose]
no flags Details | Diff | Splinter Review
doc_updates.diff (11.20 KB, patch)
2016-06-27 09:59 PDT, Michael Kelly [:mkelly,:Osmose]
rhelmer: feedback+
bhearsum: feedback+
Details | Diff | Splinter Review

Description User image Michael Kelly [:mkelly,:Osmose] 2016-06-21 17:30:04 PDT
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
Comment 1 User image Ben Hearsum (:bhearsum) 2016-06-22 11:23:07 PDT
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 User image Ben Hearsum (:bhearsum) 2016-06-24 06:49:32 PDT
(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.
Comment 3 User image Michael Kelly [:mkelly,:Osmose] 2016-06-27 09:56:43 PDT
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.
Comment 4 User image Michael Kelly [:mkelly,:Osmose] 2016-06-27 09:59:32 PDT
Created attachment 8765518 [details] [diff] [review]
doc_updates.diff

Updated the misleading sentence that bhearsum pointed out.
Comment 5 User image Robert Helmer [:rhelmer] 2016-06-27 13:16:28 PDT
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 User image Michael Kelly [:mkelly,:Osmose] 2016-06-27 13:24:36 PDT
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 User image Ben Hearsum (:bhearsum) 2016-06-28 04:09:51 PDT
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 User image Michael Kelly [:mkelly,:Osmose] 2016-06-28 08:17:48 PDT
(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 User image Michael Kelly [:mkelly,:Osmose] 2016-06-28 14:51:21 PDT
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 User image Robert Helmer [:rhelmer] 2016-07-11 11:17:47 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.