1197720 - Create background service for downloading fonts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Quote from the meta bug:
"Build a small chunk of code, probably in an Android service, to download fonts from a CDN into that 'alongside' location. This takes some care: we need to worry about captive portals, MITM, cert pinning, filesystems being full, memory pressure notifications, retry/interruption, etc."

Comment 1

[Richard Newman [:rnewman]]

CCing some of the gofaster/updater folks: this is the tip of the spear for OTA assets, and we'll run into many of the problems we'll need to solve around signing and pinning.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

Bug 1197720 - Create background service for downloading fonts.

This is a very early version of the patch and as you can see it still contains a
bunch of TODO flags. However it works and the main purpose is to discuss the idea
and basic architecture before continuing to finalize the patch and tackling all
edge cases.

My idea was that DownloadContentService should act as a very simple service that
just downloads additional content and notifies another component when done. Right
now it downloads an archive and extracts it to a location that has been configured.
In a later revision it might not even do the extracting part. Because different
content might require different actions.

FontsHelper is only temporary connecting the bits here. This is where I see the
package registry to plug in later (bug 1200291). In this regard I'm not 100% sure
if this archive approach is the best way to continue or whether we want to track every
font as a package itself. This will affect whether we track versions of an installed
package or just checksums of local content.

Comment 3

[Richard Newman [:rnewman]]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)

> if this archive approach is the best way to continue or whether we want to
> track every font as a package itself. This will affect whether we track versions of an
> installed package or just checksums of local content.

A few thoughts:

* Downloading assets individually makes sense. We will definitely have more than one (hyph, locale, fonts), and we can't jam all of them together. We would like to degrade gracefully: if we download the regular font weight first, and fail on the rest, we're doing OK.

* Downloading assets together makes sense if we get compression wins from doing so. I don't know if we do.

* We might still use a delta for incremental downloads. We can iterate later.

* Integrity checks will involve verifying the hash of the downloaded file. That can be before or after decompression, if we use it rather than transport compression. The hash will be supplied by Kinto.

I think the compression win is the decider. If it's not big, download each font individually. If it's big, then let's ship "fonts" as a whole.

Comment 4

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/17775/#review15881

This is a good POC. As you know, it needs to be generalized: at some point we need to shift from a boolean to tracking richer state -- the set of assets we should have, the set we actually have, and the current state of each, resulting in some queued downloads and potentially some queued deletions to manage the on-disk assets.

It might be worth grabbing a twig so you can land this and iterate. I'd like to avoid Nightly users having multiple copies of fonts on disk or downloading each time we make a step…

::: mobile/android/base/BrowserApp.java:2002
(Diff revision 1)
> +                FontsHelper.scheduleFontsDownloadIfNeeded(this);

How delayed is DelayedStartup these days? We probably should consider starting the fetch sooner rather than later.

::: mobile/android/base/dlc/DownloadContentService.java:50
(Diff revision 1)
> +        public DownloadFailedException(String messahe) {

message

::: mobile/android/base/dlc/DownloadContentService.java:66
(Diff revision 1)
> +    private HttpClient client;

final?

::: mobile/android/base/dlc/DownloadContentService.java:72
(Diff revision 1)
> +            .build();

Headers! (UA in particular)

::: mobile/android/base/dlc/DownloadContentService.java:96
(Diff revision 1)
> +            temporaryFile.delete();

Delete this in a finally block.

::: mobile/android/base/dlc/DownloadContentService.java:131
(Diff revision 1)
> +                throw new DownloadFailedException("Download failed. Status code: " + status);

I think in this case we still need to exhaust the stream. See some of the horrors in BaseResource.

::: mobile/android/base/dlc/DownloadContentService.java:200
(Diff revision 1)
> +        if (stream != null) {

Invert conditional, early return.

::: mobile/android/base/dlc/DownloadContentService.java:210
(Diff revision 1)
> +        if (stream != null) {

Same.

::: mobile/android/base/dlc/FontsHelper.java:20
(Diff revision 1)
> +    private static final String PREFERENCE_FILE = "fonts_dlc";

If we can, we should use the main app prefs file. Each additional file is an additional FD, and needs to be read/written separately. Using just one will help avoid startup regressions.

::: mobile/android/base/dlc/FontsHelper.java:21
(Diff revision 1)
> +    private static final String KEY_FONTS_DOWNOADED = "fonts_downloaded";

s/DOWNOADED/DOWNLOADED

Comment 5

[Richard Newman [:rnewman]]

Something else to think about, that might be relevant at this stage: progress notifications. I can imagine a post-first-run-tour screen that blocks on downloading critical assets (e.g., locales). That implies progress monitoring.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/17775/#review15881

> Delete this in a finally block.

I'm only deleting the temporary file after successful download to reuse it in case of error and only request the missing pieces using a range header. But this code is not written yet.

> I think in this case we still need to exhaust the stream. See some of the horrors in BaseResource.

Are you refering to HttpEntity.consumeContent()? I remember using this in the past but in our current ch.boye version it's deprecated and stating "Please use standard java convention to ensure resource deallocation by calling {@link InputStream#close()} on the input stream returned by {@link #getContent()}".

Comment 7

[Richard Newman [:rnewman]]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)

> Are you refering to HttpEntity.consumeContent()? I remember using this in
> the past but in our current ch.boye version it's deprecated and stating
> "Please use standard java convention to ensure resource deallocation by
> calling {@link InputStream#close()} on the input stream returned by {@link
> #getContent()}".

Yeah, that's what EntityUtils.consume does:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/net/BaseResource.java?offset=0#429

https://dxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/ch/boye/httpclientandroidlib/util/EntityUtils.java#81

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #3)
> if we download the regular font weight first, and fail on the rest, we're doing OK.

This is a very good point and after my investigation in bug 1195332 we could significantly improve the experience as soon as we have a serif font available.


> * Downloading assets together makes sense if we get compression wins from
> doing so. I don't know if we do.
> [..]
> I think the compression win is the decider. If it's not big, download each
> font individually. If it's big, then let's ship "fonts" as a whole.

Currently the difference is 7957132 bytes (~7.6 MB) of raw fonts vs 2833189 bytes (~ 2.7 MB) for the archive. This is a big improvement. However for comparison I compressed every font individually and ended up with 2833431 bytes (~ 2.7 MB) total. So this is almost the same. And maybe we can get the same improvement by transferring the data gzip encoded without much work needed from our side. So I'd continue to track individual files instead of archives for now. We should be able to extend this later if needed.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Richard: For landing this in Nightly (eventually): Should I try to get the local part of the package registry (Bug 1200291) completely defined and implemented without the server side component? The goal would be that later exactly the same registry would be used and updated from the server side. This implies that the previously downloaded fonts will be treated as the same that are in the server list (Not sure how yet). Or do you see any intermediate steps that could be landed sooner? I'd like to avoid doing something like writing shared preferences (like in the first POC iteration) and then abandon them later again.

Comment 10

[Richard Newman [:rnewman]]

Up to you. I think that it's worth thinking about what the schema will be — we figured some out already:

https://etherpad.mozilla.org/fennec-ota-assets

If you're relatively confident in the schema, do the local work now and make sure it works, then move on to updating it.

I agree that it's not worth doing too much throw-away work.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

Bug 1197720 - Create background service for downloading fonts.

This is a very early version of the patch and as you can see it still contains a
bunch of TODO flags. However it works and the main purpose is to discuss the idea
and basic architecture before continuing to finalize the patch and tackling all
edge cases.

My idea was that DownloadContentService should act as a very simple service that
just downloads additional content and notifies another component when done. Right
now it downloads an archive and extracts it to a location that has been configured.
In a later revision it might not even do the extracting part. Because different
content might require different actions.

FontsHelper is only temporary connecting the bits here. This is where I see the
package registry to plug in later (bug 1200291). In this regard I'm not 100% sure
if this archive approach is the best way to continue or whether we want to track every
font as a package itself. This will affect whether we track versions of an installed
package or just checksums of local content.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

Bug 1200291 - Implement simple package registry for downloadable content.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8664181 [details]
MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

Bug 1200291 - Implement simple package registry for downloadable content.

This patch introduces a local representation of a catalog of downloadable content.
DownloadContentService now handles multiple actions. For now these are:

* Verify: Going through the catalog and looking for installable content. Verifying
  that downloaded content is still in place and up to date (checksum).

* Download: Downloading content that has been scheduled for download during the
  "verify" step.

Later this service would handle additional actions to update the catalog from a
server component. For now the catalog is static and bootstraped once on first launch
(DownloadContentBootstrap).

This version handles some but not all edge cases (see TODO markers).

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8664181 [details]
MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

Looking for feedback on this WIP version. See commit message for details.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8664181 [details]
MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

https://reviewboard.mozilla.org/r/19933/#review18059

f+.

My main concern is around efficiency: I'd like for us to use a single flat file if we can, and for us to not hit the filesystem if we can avoid it. Other than that, roll on!

::: configure.in:3953
(Diff revision 2)
> +# Whether to download additional content (fonts, hyphenation dictionaries, ..) at runtime.

Doesn't this raise a question of definition?

This either means "never download stuff, always include it", or "never download stuff, live without it".

Is that a general rule? Can we document it here?

::: mobile/android/base/dlc/DownloadContentBootstrap.java:25
(Diff revision 2)
> +        return Arrays.asList(

Does it make sense to have a special case here for constrained builds? E.g., to only download the -R variants?

::: mobile/android/base/dlc/DownloadContentHelper.java:75
(Diff revision 2)
> +        return HttpClientBuilder.create()

File a bug for proxy support.

::: mobile/android/base/dlc/DownloadContentHelper.java:208
(Diff revision 2)
> +    private static String bytesToHex(byte[] bytes) {

Use Utils.byte2Hex.

::: mobile/android/base/dlc/DownloadContentService.java:84
(Diff revision 2)
> -        } catch (NoSuchAlgorithmException | UnsupportedEncodingException e) {
> +                    verifyDownloadedContent(content);

Is it necessary to do this on every run? Can we do a full disk-touching verify in response to a more sophisticated hook, like a missing font? Or can we trigger that verify in DelayedStartup, so the lifecycle would look like this?

  Application.onCreate
    -- Check catalog (~in-memory)
    -- Download if needed
  GeckoApp.onCreate
  ...
  GeckoApp.onResume
  ... DelayedStartup:
    -- Check disk contents to see if we missed any

::: mobile/android/base/dlc/LocalCatalogStorage.java:36
(Diff revision 2)
> +            SQLiteDatabase db = database.getReadableDatabase();

I expect the catalog to be small, and I expect opening and maintaining a database on first run, pretty much in the hot path of Application.onCreate, to be expensive (both disk access and memory usage).

I have a strong preference for this being stored in memory and synchronously persisted to disk after a write, just like SharedPreferences. (Indeed, you might opt to actually back this with SharedPreferences…)

f

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8664181 [details]
MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

*shakes fist at reviewboard*

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #15)
> ::: configure.in:3953
> (Diff revision 2)
> > +# Whether to download additional content (fonts, hyphenation dictionaries, ..) at runtime.
> 
> Doesn't this raise a question of definition?

Yeah, this flag should only control the service so I renamed it to MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE.


(In reply to Richard Newman [:rnewman] from comment #15)
> Does it make sense to have a special case here for constrained builds? E.g.,
> to only download the -R variants?

That's a good idea. I wonder how we would make this decision if the catalog is not bootstrapped but synchronized from a server? Instead of modifying the bootstrap catalog I'd like to implement this decision making. 


For everything else I'll upload a new patch later today.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

Bug 1197720 - Create background service for downloading fonts.

This is a very early version of the patch and as you can see it still contains a
bunch of TODO flags. However it works and the main purpose is to discuss the idea
and basic architecture before continuing to finalize the patch and tackling all
edge cases.

My idea was that DownloadContentService should act as a very simple service that
just downloads additional content and notifies another component when done. Right
now it downloads an archive and extracts it to a location that has been configured.
In a later revision it might not even do the extracting part. Because different
content might require different actions.

FontsHelper is only temporary connecting the bits here. This is where I see the
package registry to plug in later (bug 1200291). In this regard I'm not 100% sure
if this archive approach is the best way to continue or whether we want to track every
font as a package itself. This will affect whether we track versions of an installed
package or just checksums of local content.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8664181 [details]
MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

Bug 1200291 - Implement simple package registry for downloadable content.

This patch introduces a local representation of a catalog of downloadable content.
DownloadContentService now handles multiple actions. For now these are:

* Verify: Going through the catalog and looking for installable content. Verifying
  that downloaded content is still in place and up to date (checksum).

* Download: Downloading content that has been scheduled for download during the
  "verify" step.

Later this service would handle additional actions to update the catalog from a
server component. For now the catalog is static and bootstraped once on first launch
(DownloadContentBootstrap).

This version handles some but not all edge cases (see TODO markers).

Comment 20

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1200291 - Use a flat file instead of SQLite database for catalog.

Bug 1200291 - Use a flat file instead of SQLite database for catalog.

This patch replaces the database catalog with a flat file implementation based on
AtomicFile and inspired by SharedPreferencesImpl.

The "verify" step of the DownloadContentService is now split into two steps triggered
at different times:

* "Study": Scan the catalog for "new" content available for download.
* "Verify": Validate downloaded content. Does it still exist and does it have the correct checksum?

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8664181 [details]
MozReview Request: Bug 1200291 - Implement simple package registry for downloadable content.

What is reviewboard doing..

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8667416 [details]
MozReview Request: Bug 1200291 - Use a flat file instead of SQLite database for catalog.

This is the next step. Depending on your feedback I'm going to fold all this in a patch for final review. And take care of the last needed steps (e.g. final URLs on live servers).

I filed follow-up bugs for the last open TODOs in code. I think we could tests this on Nightly without those last ones fixed. Even though I'd work on those bugs after this.

I'll look into getting reftests working without shipping fonts in the APK next (bug 1197716).

Comment 23

[Richard Newman [:rnewman]]

https://reviewboard.mozilla.org/r/20741/#review19197

::: configure.in:3956
(Diff revision 1)
> -MOZ_ARG_DISABLE_BOOL(android-download-content,
> +MOZ_ARG_DISABLE_BOOL(android-download-conten-service,

s/conten-/content-

::: mobile/android/base/dlc/DownloadContentHelper.java:113
(Diff revision 1)
> -                //       Currently we do not update the catalog so there's no way to recover from an outdated URL.
> +                // misbehaving server.

File a follow-up that blocks leaving Nightly: failure counters. Someone installs an ancient version of Firefox and this gets a 404 or 500 on every request until the end of times? Nah.

::: mobile/android/base/dlc/DownloadContentService.java:48
(Diff revision 1)
> +    private DownloadContentCatalog catalog;

final

::: mobile/android/base/dlc/DownloadContentService.java:113
(Diff revision 1)
> +        Log.d(LOGTAG, "Verifying catalog..");

..

This looks pretty good to me. Fold and final!

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

Bug 1197720 - Introduce background service for downloadable content. r?rnewman

This patch introduces a background service for downloading content from a
catalog (bug 1200291). This catalog ships with the application and contains
only fonts in this first version (MOZ_ANDROID_EXCLUDE_FONTS).

For now this service is disabled by default (MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE).

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

I don't have the final URLs yet (bug 1212353) but I folded the patches for review anyways. The URLs should not block me from getting this patch in a state that is ready for landing. I don't want to start with the follow-up bugs as long as this patch is not finished yet.

Things that have changed since the last round of patches:
* The file name of the downloadable content is now part of the catalog (To be more flexible with the download URLs)
* I tried to be a bit smarter when handling the HTTP status codes (recoverable vs. unrecoverable errors). But I filed bug 1215106 for implementing failure counters).

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

https://reviewboard.mozilla.org/r/17775/#review21065

::: mobile/android/base/GeckoApplication.java:160
(Diff revision 4)
> +        if (AppConstants.MOZ_INSTALL_TRACKING) {

What's going on here?

::: mobile/android/base/dlc/DownloadContentHelper.java:108
(Diff revision 4)
> +        HttpGet request = new HttpGet(source);

Nit: final

::: mobile/android/base/dlc/DownloadContentHelper.java:112
(Diff revision 4)
> +            int status = response.getStatusLine().getStatusCode();

final (throughout)

::: mobile/android/base/dlc/DownloadContentHelper.java:148
(Diff revision 4)
> +            IOUtils.safeStreamClose(outputStream);

Don't use safeStreamClose here. Some writes will fail *on close* and not earlier -- you want to close the output stream *inside* the try so that you delete the temporary file and handle the failure!

::: mobile/android/base/dlc/DownloadContentHelper.java:158
(Diff revision 4)
> +            MessageDigest md = MessageDigest.getInstance("MD5");

I have a fairly high level concern here.

We know from Bug 959652 that this can be expensive: we'll init the Java crypto framework in order to do this. You should think about whether we have an alternative mechanism for computing hashes -- they don't need to be cryptographically (in)secure like MD5.

Right now the expense might be hidden like FHR, but it might poke its head out later.

::: mobile/android/base/dlc/DownloadContentService.java:176
(Diff revision 4)
> +                DownloadContentHelper.download(client, content.getLocation(), temporaryFile);

Can we verify that there's space on disk before we do this work? Follow-up is fine.

::: mobile/android/base/dlc/DownloadContentService.java:189
(Diff revision 4)
> +                DownloadContentHelper.copy(temporaryFile, destinationFile);

I think this could be improved in a few ways.

If the file is on the same mount point -- that is, the temporary location and Firefox aren't on different disks -- then you can just use `temporaryFile.renameTo(destinationFile)` and be done. Copying in this case uses double the disk space!

If you roll that logic into `copy`, and you rename it to `move`, you can have the fallback be to copy-and-delete… and do that work in the success path within `move`. Tada!

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #26)
> ::: mobile/android/base/GeckoApplication.java:160
> (Diff revision 4)
> > +        if (AppConstants.MOZ_INSTALL_TRACKING) {
> 
> What's going on here?

Wtf. This looks like some merge/rebase artifact.


(In reply to Richard Newman [:rnewman] from comment #26)
> Don't use safeStreamClose here. Some writes will fail *on close* and not
> earlier -- you want to close the output stream *inside* the try so that you
> delete the temporary file and handle the failure!

Very good catch! I totally missed that one.


(In reply to Richard Newman [:rnewman] from comment #26)
> ::: mobile/android/base/dlc/DownloadContentHelper.java:158
> (Diff revision 4)
> > +            MessageDigest md = MessageDigest.getInstance("MD5");
> 
> I have a fairly high level concern here.
> 
> We know from Bug 959652 that this can be expensive: we'll init the Java
> crypto framework in order to do this. You should think about whether we have
> an alternative mechanism for computing hashes -- they don't need to be
> cryptographically (in)secure like MD5.
> 
> Right now the expense might be hidden like FHR, but it might poke its head
> out later.

I just read bug 959652 and linked bugs. I definitely would like to avoid this startup/initialization penalty. Did you have any specific hashing algorithms in mind? Bug 959652 uses NativeCrypto.sha1. The current implementation takes a byte array. I'd like to avoid reading the whole file into memory first. I can look into other simple and fast hashing mechanisms too, but I'd like to get your feedback first. :)

> Can we verify that there's space on disk before we do this work? Follow-up
> is fine.

I thought I already had filed a bug for that but I didn't. I filed bug 1220145.


(In reply to Richard Newman [:rnewman] from comment #26)
> I think this could be improved in a few ways.
> 
> If the file is on the same mount point -- that is, the temporary location
> and Firefox aren't on different disks -- then you can just use
> `temporaryFile.renameTo(destinationFile)` and be done. Copying in this case
> uses double the disk space!
> 
> If you roll that logic into `copy`, and you rename it to `move`, you can
> have the fallback be to copy-and-delete… and do that work in the success
> path within `move`. Tada!

Good point. I actually used renameTo() before and then changed it to just copy to workaround the "mount point" issue. But yeah, using copy just as a fallback makes much more sense.

Comment 28

[Richard Newman [:rnewman]]

(In reply to Sebastian Kaspari (:sebastian) from comment #27)

> I just read bug 959652 and linked bugs. I definitely would like to avoid
> this startup/initialization penalty. Did you have any specific hashing
> algorithms in mind? Bug 959652 uses NativeCrypto.sha1. The current
> implementation takes a byte array. I'd like to avoid reading the whole file
> into memory first. I can look into other simple and fast hashing mechanisms
> too, but I'd like to get your feedback first. :)

It would be relatively straightforward to extend NativeCrypto to do either MD5 or SHA-1 for files, or chunk by chunk. (Obviously the file needs to be read, just not necessarily by this piece of code or in one go.)

Changing the hash function here presumably imposes a change upstream; it would be worth finding out why MD5 was chosen.

I was going to suggest a non-cryptographic hash function, but it occurs to me that we quite probably *do* want a cryptographic hash. mgoodwin, you probably have an opinion here, or know who does. If we're doing signing of the Kinto payloads, do we need to worry about the strength of the hash function for the downloaded asset files themselves?

Comment 29

[Mark Goodwin [:mgoodwin]]

(In reply to Richard Newman [:rnewman] from comment #28)
> I was going to suggest a non-cryptographic hash function, but it occurs to
> me that we quite probably *do* want a cryptographic hash. mgoodwin, you
> probably have an opinion here, or know who does. If we're doing signing of
> the Kinto payloads, do we need to worry about the strength of the hash
> function for the downloaded asset files themselves?

Depends what you're using it for - but there's little reason to still be using MD5 for anything where the check matters.

Even if the collection is signed, if we have a weak hash for the asset there's no assurance it is what we think it is. For fonts, it may be this doesn't matter (platform vulnerabilities around fonts are a possibility, of course) - but if we're likely to be using the same mechanism for other things, a better algorithm would certainly be advisable (these days, that means not worse than sha-256).

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1197720-NativeCrypto.patch

This is a small optimization for NativeCrypto.sha256update() (added in bug 1221040). Instead of always using all bytes in the byte array it let's the caller define the number of bytes to update from.

In Java we will call this method with byte array buffers we use to read from a stream. Java does not
guarantee to completely fill the buffer (and can't at the end of a stream). Passing the number of bytes
to read from the array to NativeCrypto avoids copying bytes between arrays of various lengths.

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

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

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll land the reviewed NativeCrypto patch now. This will make testing and implementing the other features easier because I can continue to use "disable-compile-environment".

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/49811ce73e27ef75cf48b3bf2a69e652595e46bc
Bug 1197720 - NativeCrypto (SHA-256): Let caller define number of bytes in 'str' to update from. r=snorp

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49811ce73e27

Comment 35

[Richard Newman [:rnewman]]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

https://reviewboard.mozilla.org/r/17775/#review24237

This is looking pretty good.

::: mobile/android/base/dlc/DownloadContentHelper.java:142
(Diff revisions 4 - 5)
> +            inputStream.close();

Can we close the input stream first? I think it's less likely to throw.

Or use safeStreamClose?

::: mobile/android/base/dlc/DownloadContentHelper.java:162
(Diff revisions 4 - 5)
> +            byte[] ctx = NativeCrypto.sha256init();

Can this return null? My guess is that it can if it can't allocate the buffer, no?

::: mobile/android/base/dlc/DownloadContentHelper.java:282
(Diff revisions 4 - 5)
> +    /* package-private */ static String createDownloadUrl(DownloadContent content) {

s/Url/URL

::: mobile/android/base/dlc/DownloadContentService.java:178
(Diff revisions 4 - 5)
> +                final String url = DownloadContentHelper.createDownloadUrl(content);

:%s,Url,URL,g

::: mobile/android/base/dlc/catalog/DownloadContentBootstrap.java:16
(Diff revision 5)
> +    // TODO: These are all just test URLs!!

Ahem.

::: mobile/android/base/dlc/catalog/DownloadContentCatalog.java:173
(Diff revision 5)
> +            content = DownloadContentBootstrap.createInitialDownloadContentList();

What about dropping the on-disk content?

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

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

Comment 37

[Richard Newman [:rnewman]]

Comment on attachment 8654954 [details]
MozReview Request: Bug 1197720 - Introduce background service for downloadable content. r?rnewman

https://reviewboard.mozilla.org/r/17775/#review24389

Great work.

::: mobile/android/base/AppConstants.java.in:370
(Diff revision 6)
> +    true;

You want to pref this off by default, right? Either set this to false, or you need to touch all the mozconfigs.

::: mobile/android/base/dlc/DownloadContentHelper.java:208
(Diff revision 6)
> +                throw new IOException("Destination directory does not exist and cannot be created: " + destinationFile);

If this includes the profile directory, then we shouldn't log the whole file path -- it'll leak the profile directory hash to the system log.

Please audit!

::: mobile/android/base/dlc/DownloadContentService.java:72
(Diff revision 6)
> +        if (DownloadContentHelper.ACTION_STUDY_CATALOG.equals(intent.getAction())) {

We use Java 7 sourcelevel, so you can rephrase this as a switch.

::: mobile/android/base/dlc/DownloadContentService.java:97
(Diff revision 6)
> +                catalog.markAsIgnored(content);

Maybe just skip? We don't want to have to unignore later…

Think about it; you decide.

::: mobile/android/base/dlc/catalog/DownloadContentCatalog.java:121
(Diff revision 6)
> +        new Thread(LOGTAG + "-Persist") {

Think about using an executor (probably a SingleThreadExecutor) for this module.

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #37)
> ::: mobile/android/base/AppConstants.java.in:370
> (Diff revision 6)
> > +    true;
> 
> You want to pref this off by default, right? Either set this to false, or
> you need to touch all the mozconfigs.

But isn't it automatically "false" because MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE is not defined in any mozconfig file?


(In reply to Richard Newman [:rnewman] from comment #37)
> ::: mobile/android/base/dlc/DownloadContentHelper.java:208
> (Diff revision 6)
> > +                throw new IOException("Destination directory does not exist and cannot be created: " + destinationFile);
> 
> If this includes the profile directory, then we shouldn't log the whole file
> path -- it'll leak the profile directory hash to the system log.
> 
> Please audit!

For the current set of fonts this should not point to the profile directory. However in the future we might have downloadable content that needs to go somewhere into the profile directory. I'll remove file paths from the exceptions / log messages. They have been useful for debugging but I don't think we will need them in production.


(In reply to Richard Newman [:rnewman] from comment #37)
> ::: mobile/android/base/dlc/DownloadContentService.java:97
> (Diff revision 6)
> > +                catalog.markAsIgnored(content);
> 
> Maybe just skip? We don't want to have to unignore later…

That's a very good point. I was planning to unignore content whenever it is updated in the catalog. However with the online catalog it will be possible to receive a new catalog with unknown content before the app update, that knows how to handle that content, is installed. I'll just skip and also remove the log here: I don't want to log this on every app start if we always skip.


(In reply to Richard Newman [:rnewman] from comment #37)
> ::: mobile/android/base/dlc/catalog/DownloadContentCatalog.java:121
> (Diff revision 6)
> > +        new Thread(LOGTAG + "-Persist") {
> 
> Think about using an executor (probably a SingleThreadExecutor) for this
> module.

This is borrowed from SharedPreferencesImpl. But I don't like those lose threads too. All critical access is synchronized but a SingleThreadExecutor will add another level of safety.

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76814513ae91

Comment 40

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/b8249000cec9855cf25054abe8870a25bf0bbc3c
Bug 1197720 - Introduce background service for downloadable content. r=rnewman

Comment 41

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #38)
> (In reply to Richard Newman [:rnewman] from comment #37)
> > ::: mobile/android/base/dlc/catalog/DownloadContentCatalog.java:121
> > (Diff revision 6)
> > > +        new Thread(LOGTAG + "-Persist") {
> > 
> > Think about using an executor (probably a SingleThreadExecutor) for this
> > module.
> 
> This is borrowed from SharedPreferencesImpl. But I don't like those lose
> threads too. All critical access is synchronized but a SingleThreadExecutor
> will add another level of safety.

I decided to do this not yet. But I might look into this in a follow-up. The tricky thing here is when to shutdown the thread(executor). Using a timeout is hard because we are downloading things and it's impossible to guess the time upfront. Introducing a public method to DownloadContentCatalog to shutdown the internal thread pool is also not nice.

Comment 42

[Carsten Book [:Tomcat]]

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