1201059 - (dlc-sync) Implement simple package service client for downloadable content (Kinto Sync)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1200291 we are implementing a simple package registry for downloadable content. This bug tracks the work needed to implement a package service client to download/synchronize a list of available assets from a server component.

Comment 1

[Gervase Markham [:gerv]]

Does this need to be Firefox for Android-specific? Being able to download e.g. dictionaries on demand on desktop would be very useful. It can help avoid licensing problems, for one. (We can't ship GPLed dictionaries.)

Gerv

Comment 2

[Richard Newman [:rnewman]]

Our background services on Android are typically small chunks of Java written (naturally) against Android APIs -- Gecko is too heavy to start up just to make an HTTP request.

All of the same pressures around up-front installer size apply on desktop, of course.

The hosted package service is based on Kinto, and files will be hosted on a CDN; we can and should consume those on desktop, too. Desktop will presumably use kinto.js to start, and move to a proper background service when we start doing that. But not in this bug :D

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review commit: https://reviewboard.mozilla.org/r/34989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34989/

Comment 4

[Tarek Ziadé (:tarek)]

That's a very interesting use-case Gerv. Like Richard said, the Kinto lib is also shipping in Desktop, so if you have a list of files you would like to see added in Firefox on demand, please e-mail me and we can look at this case

Comment 5

[Gervase Markham [:gerv]]

Hi Tarek,

You would need to talk to Pike about dictionaries. From my perspective, there are various files and lists in Firefox that I'd like to see be remotely updateable, but I have a vague recollection rbarnes may be developing some generic information update service?

Gerv

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34989/diff/1-2/

Comment 7

[Richard Barnes [:rbarnes]]

We've been working on a generic security policy update service, based on Kinto.  So this is consistent.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34989/diff/2-3/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34989/diff/3-4/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, this is the first version for review.

I moved some tasks to follow-up bugs. This is behind a switchboard flag and doesn't affect anyone for now.

Comment 11

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

https://reviewboard.mozilla.org/r/34989/#review37673

I reviewed what I could, it looks good to me.

PS: This is my first review through the reviewboard. I hope it will land properly :)

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:136
(Diff revision 4)
> +
> +            final int responseCode = connection.getResponseCode();
> +
> +            if (responseCode != HttpURLConnection.HTTP_OK) {
> +                if (responseCode == 401) {
> +                    // A 410 Gone error response can be returned if the client version is too old,

Typo: 410

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:156
(Diff revision 4)
> +                    // If the HTTP status is not OK (<200 or >=400), the response contains a JSON
> +                    // response.
> +                    logErrorResponse(connection);
> +
> +                    throw new RecoverableDownloadContentException(
> +                            RecoverableDownloadContentException.SERVER,

As you did elsewhere: *// Unrecoverable: Client errors 4xx - Unlikely that this version of the client will ever succeed.*

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:168
(Diff revision 4)
> +                    throw new UnrecoverableDownloadContentException("(Unrecoverable) Download failed. Response code: " + responseCode);
> +                }
> +            }
> +
> +            return fetchJSONResponse(connection).getJSONArray("data");
> +        } catch (JSONException | IOException e) {

nitpick: since you have KINTO_KEY_ID and KINTO_KEY_DELETED you could have KINTO_KEY_DATA

::: mobile/android/tests/background/junit4/resources/dlc_sync_single_font.json:17
(Diff revision 4)
> +      "attachment": {
> +        "mimetype":"application/x-gzip",
> +        "size":548720,
> +        "hash":"960be4fc5a92c1dc488582b215d5d75429fd4ffbee463105d29992cd792a912e",
> +        "location":"https://kinto-ota.dev.mozaws.net/attachments/0d28a72d-a51f-46f8-9e5a-f95c61de904e.gz",
> +        "filename":"CharisSILCompact-R.ttf.gz"

nit: in the deployed service, we said we would remove the root url from records

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Updated patch plz!

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34989/diff/4-5/

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

@Mathieu: I addressed your review comments. Reviewboard re-flagged you for review. If everything's okay then check the "Ship it" checkbox and reviewboard will mark the review as completed.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

https://reviewboard.mozilla.org/r/34989/#review39249

Biggest comment: the catalog should be keyed by ID. By the time you hit your third or fourth method where you walked a list checking for ID equality, you should have switched to

http://developer.android.com/reference/android/support/v4/util/ArrayMap.html

!

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1948
(Diff revision 5)
>  
>                  if (AppConstants.MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE) {
> +                    // TODO: Better scheduling of sync action (Bug 1257492)
> +                    DownloadContentService.startSync(this);
> +
>                      DownloadContentService.startVerification(this);

Are you confident that these two intents will be delivered serially?

::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadContentService.java:27
(Diff revision 5)
>   * Service to handle downloadable content that did not ship with the APK.
>   */
>  public class DownloadContentService extends IntentService {
>      private static final String LOGTAG = "GeckoDLCService";
>  
>      private static final String ACTION_STUDY_CATALOG = AppConstants.ANDROID_PACKAGE_NAME + ".DLC.STUDY";

A little block comment explaining what each does would be nice.

::: mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java:18
(Diff revision 5)
>  import org.mozilla.gecko.dlc.catalog.DownloadContent;
>  import org.mozilla.gecko.dlc.catalog.DownloadContentCatalog;
>  
> +import java.util.regex.Pattern;
> +
> +import ch.boye.httpclientandroidlib.util.TextUtils;

Wrong TextUtils.

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:47
(Diff revision 5)
> +     */
> +    private static final String CATALOG_ENDPOINT = "https://firefox.settings.services.mozilla.com/v1/buckets/fennec-dlc/collections/catalog/records";
> +
> +    @Override
> +    public void perform(Context context, DownloadContentCatalog catalog) {
> +        Log.d(LOGTAG, "Synchronizing catalog..");

s/\.\././

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:114
(Diff revision 5)
> +            throws RecoverableDownloadContentException, UnrecoverableDownloadContentException {
> +        HttpURLConnection connection = null;
> +
> +        try {
> +            StringBuilder url = new StringBuilder(CATALOG_ENDPOINT);
> +            url.append("?");

:(

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:141
(Diff revision 5)
> +            // TODO: Add support for Next-Page header (Bug 1257495)
> +
> +            final int responseCode = connection.getResponseCode();
> +
> +            if (responseCode != HttpURLConnection.HTTP_OK) {
> +                if (responseCode == 401) {

Move below the 500 block.

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:168
(Diff revision 5)
> +
> +                    throw new RecoverableDownloadContentException(RecoverableDownloadContentException.SERVER,  "Response code: " + responseCode);
> +                } else {
> +                    // HttpsUrlConnection: -1 (No valid response code)
> +                    // Successful 2xx: We don't know how to handle anything but 200.
> +                    // Redirection 3xx: HttpClient should have followed redirects if possible. We should not see those errors here.

Does it really?

I can't imagine it would handle 301 correctly. And we have discovered during service decommissionings that we'd really like to handle permanent redirects. Can we do so here? (Follow-up bug that shouldn't be ignored, please.)

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:191
(Diff revision 5)
> +
> +        try {
> +            inputStream = new BufferedInputStream(connection.getInputStream());
> +            ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
> +            IOUtils.copy(inputStream, outputStream);
> +            return new JSONObject(outputStream.toString("UTF-8"));

Now that we're Java 7, can we start using

```
java.nio.charset.StandardCharsets.UTF_8
```

and avoid the `UnsupportedEncodingException` wherever we use charsets? (Throughout)

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:210
(Diff revision 5)
> +            return false;
> +        }
> +
> +        catalog.update(content);
> +
> +        Log.i(LOGTAG, "Updated content: " + content);

Remove?

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentBuilder.java:11
(Diff revision 5)
> +package org.mozilla.gecko.dlc.catalog;
> +
> +import org.json.JSONException;
> +import org.json.JSONObject;
> +
> +import ch.boye.httpclientandroidlib.util.TextUtils;

Wrong TextUtils.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java:79
(Diff revision 5)
> +    }
> +
> +    public synchronized List<DownloadContent> getContentToDelete() {
> +        awaitLoadingCatalogLocked();
> +
> +        List<DownloadContent> deleteContent = new ArrayList<>();

`final`

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java:135
(Diff revision 5)
> +
> +        this.content.add(content);
> +        this.hasCatalogChanged = true;
> +    }
> +
> +    public synchronized void update(DownloadContent content) {

For sanity's sake, how about we change the argument names here to `changedContent`?

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java:140
(Diff revision 5)
> +    public synchronized void update(DownloadContent content) {
> +        awaitLoadingCatalogLocked();
> +
> +        for (int i = 0; i < this.content.size(); i++) {
> +            DownloadContent existingContent = this.content.get(i);
> +            if (TextUtils.equals(content.getId(), existingContent.getId())) {

Why is `this.content` not a `Map<String, DownloadContent>`?

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java:163
(Diff revision 5)
> +        Iterator<DownloadContent> iterator = this.content.iterator();
> +
> +        while (iterator.hasNext()) {
> +            DownloadContent currentContent = iterator.next();
> +            if (TextUtils.equals(currentContent.getId(), content.getId())) {
> +                iterator.remove();

Similarly, this is way easier if you have a smarter collection!

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/34989/#review39249

I switched to ArrayMap and it's indeed much nicer. :)

Somehow I didn't consider this because there's no structure that is perfect for all ways I access the collection.

> Are you confident that these two intents will be delivered serially?

I am pretty sure that Android will deliver the Intents in order. But I also think that it doesn't really matter much here. Overall I'd like to move this away and use some better scheduling mechanisms (bug 1257492 and bug 1260406).

> Wrong TextUtils.

Damn. This happens so often.

> Does it really?
> 
> I can't imagine it would handle 301 correctly. And we have discovered during service decommissionings that we'd really like to handle permanent redirects. Can we do so here? (Follow-up bug that shouldn't be ignored, please.)

bug 1260459

> Remove?

They are helpful for debugging right now. But I changed the level to verbose.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34989/diff/5-6/

Comment 18

[Richard Newman [:rnewman]]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

https://reviewboard.mozilla.org/r/34989/#review39667

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:193
(Diff revisions 5 - 6)
>  
>          try {
>              inputStream = new BufferedInputStream(connection.getInputStream());
>              ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
>              IOUtils.copy(inputStream, outputStream);
> -            return new JSONObject(outputStream.toString("UTF-8"));
> +            return new JSONObject(new String(outputStream.toByteArray(), StandardCharsets.UTF_8));

Okay, I take it back. If `BAOS` doesn't have a `Charset`-aware variant (and it doesn't), let's suffer the `"UTF-8"` tax.

The reason: `toByteArray` makes a copy of the buffer, and `BAOS.toString` doesn't.

Using `StandardCharsets` avoids the possibility of an exception, but it adds yet another copy of the input data to our memory footprint.

http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/io/ByteArrayOutputStream.java

::: mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java:212
(Diff revisions 5 - 6)
>              return false;
>          }
>  
>          catalog.update(content);
>  
> -        Log.i(LOGTAG, "Updated content: " + content);
> +        Log.v(LOGTAG, "Updated content: " + content);

This string concatenation will be done even with `Log.v`, right? Either add a debug logging flag so the compiler can get rid of the concatenation, or just kill the line altogether.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java:89
(Diff revisions 5 - 6)
>  
> -        return scheduledContent;
> +        return filteredContent;
>      }
>  
> -    public synchronized boolean hasScheduledDownloads() {
> -        awaitLoadingCatalogLocked();
> +    public boolean hasScheduledDownloads() {
> +        return filterByState(DownloadContent.STATE_SCHEDULED).size() > 0;

Use `!.isEmpty()`.

::: mobile/android/base/java/org/mozilla/gecko/dlc/catalog/DownloadContentCatalog.java:96
(Diff revisions 5 - 6)
>  
> -    public synchronized void add(DownloadContent content) {
> +    public synchronized void add(DownloadContent newContent) {
>          awaitLoadingCatalogLocked();
>  
> -        this.content.add(content);
> -        this.hasCatalogChanged = true;
> +        content.put(newContent.getId(), newContent);
> +        hasCatalogChanged = true;

I miss my `this`! :)

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/34989/#review39667

> I miss my `this`! :)

I bet you miss it less if you see the code in all its beautiy with IDE coloring. ;)

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34989/diff/6-7/

Comment 21

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

https://reviewboard.mozilla.org/r/34989/#review39731

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/a8cb004e06e7c61c6ca3676e2c0d33773882e702
Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8cb004e06e7

Comment 24

[Chenxia Liu [:liuche]]

When updating experiments for bug 1270641, I noticed that this experiment calls Experiments.isInExperimentLocal() - is that intentional?

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/SyncAction.java#237

isInExperimentLocal is only used for Onboarding, because we don't have time to listen for a response from the Switchboard server:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/util/Experiments.java#97

Perhaps this was intended to be Switchboard.isInExperiment()?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2318

I think we're no longer doing this work, but I wanted to point this out in case it affects anything.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Chenxia Liu [:liuche] from comment #24)
> When updating experiments for bug 1270641, I noticed that this experiment
> calls Experiments.isInExperimentLocal() - is that intentional?

Oh, that's absolutely an error. Thanks for finding this! I filed bug 1276577 for fixing this.