Closed Bug 1201059 (dlc-sync) Opened 5 years ago Closed 4 years ago

Implement simple package service client for downloadable content (Kinto Sync)

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

(Depends on 9 open bugs, Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
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
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
Status: NEW → ASSIGNED
Depends on: 1248596
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
Flags: needinfo?(gerv)
Depends on: 1249001
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
Flags: needinfo?(gerv) → needinfo?(rlb)
Depends on: 1249248
Depends on: 1249249
Depends on: 1249251
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/
We've been working on a generic security policy update service, based on Kinto.  So this is consistent.
Flags: needinfo?(rlb)
Summary: Implement simple package service client → Implement simple package service client for downloadable content (Kinto Sync)
Depends on: 1257459
Depends on: 1257492
Depends on: 1257495
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/
Attachment #8719784 - Attachment description: MozReview Request: Bug 1201059 - (WIP) Synchronize catalog of downloadable content from Kinto instance. r? → MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r?rnewman,leplatrem
Attachment #8719784 - Flags: review?(rnewman)
Attachment #8719784 - Flags: review?(mathieu)
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/
Attachment #8719784 - Attachment description: MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r?rnewman,leplatrem → MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r?rnewman,mathieu
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 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
Attachment #8719784 - Flags: review?(mathieu)
Comment on attachment 8719784 [details]
MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu

Updated patch plz!
Attachment #8719784 - Flags: review?(rnewman)
Attachment #8719784 - Flags: review?(rnewman)
Attachment #8719784 - Flags: review?(mathieu)
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/
@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.
Attachment #8719784 - Flags: review?(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!
Depends on: 1260406
Depends on: 1260459
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 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/
Attachment #8719784 - Flags: review?(rnewman)
Attachment #8719784 - Flags: review?(rnewman) → review+
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`! :)
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 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/
Attachment #8719784 - Attachment description: MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r?rnewman,mathieu → MozReview Request: Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu
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
Attachment #8719784 - Flags: review?(mathieu) → review+
https://hg.mozilla.org/integration/fx-team/rev/a8cb004e06e7c61c6ca3676e2c0d33773882e702
Bug 1201059 - Synchronize catalog of downloadable content from Kinto instance. r=rnewman,mathieu
https://hg.mozilla.org/mozilla-central/rev/a8cb004e06e7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Alias: dlc-sync
Depends on: 1271352
Depends on: fennec-dlc
Blocks: fennec-dlc
No longer depends on: fennec-dlc
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.
Flags: needinfo?(s.kaspari)
Depends on: 1276577
(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.
Flags: needinfo?(s.kaspari)
Depends on: 1276587
Depends on: 1277183
You need to log in before you can comment on or make changes to this bug.