829934 - Implement a hash verifier for webapp/mini manifest

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Julien Wajsberg [:julienw]]

Currently in FirefoxOS we only rely on ETag to know if a file has changed. This means that if the server does not support ETag, it will always return a status 200 and we will report an update to the user.

We should implement a hash verifier as the appcache mechanism does (see [1]) so that servers without ETag support (and/or Last-Modified support when Bug 823648 is fixed) does not trigger an update available for each check.

[1] https://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp#1727

Comment 1

[Julien Wajsberg [:julienw]]

Asking for blocking-basecamp because I don't know the new tracking flag.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I think we should do this. This is the most complete way to fix bug 829853 and should eliminate cases where we would tell the user there's an update available when there isn't.

Comment 3

[Alex Keybl [:akeybl]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Yeah, I think we should do this. This is the most complete way to fix bug
> 829853 and should eliminate cases where we would tell the user there's an
> update available when there isn't.

Fabrice & Jonas - what's the risk and level of effort here? We're considering this a blocker for now, but if the risk is too high we may not be able to fix this till our first update.

Comment 4

[[:fabrice] Fabrice Desré]

Level of effort is low. The risk is low to moderate. I think we should really take that.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: part 1 : package OS.File xpt

We can't currently use OS.File on b2g and android because the xpt is not packaged.

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: Part 2: add manifest and package hashes

We add 2 hashes: one for the manifest, and one for packages.

I kept the same logic, being that even if a hosted app manifest didn't change, we check for appcache updates. For packaged apps, we only check the package if the update manifest has changed.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's really not good that we're doing so much sync IO here. You should be able to use |new File(somensIFile)| and then use FileReader to do the IO asynchronously instead.

Comment 8

[[:fabrice] Fabrice Desré]

I don't mind changing to use FileReader, but all the OS.File calls I use in this patch are doing async IO off the main thread.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ooOOoo. Carry on, ignore me :)

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 703630 [details] [diff] [review]
part 1 : package OS.File xpt

Review of attachment 703630 [details] [diff] [review]:
-----------------------------------------------------------------

That looks like bug 803165 + bug 823502. Am I missing something?

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 703633 [details] [diff] [review]
Part 2: add manifest and package hashes

Review of attachment 703633 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Fabrice!

r=me with the comments addressed, please.

::: dom/apps/src/Webapps.jsm
@@ +1127,5 @@
>      }
>    },
>  
> +  // Returns the MD5 hash of the file.
> +  computeFileHash: function(aFile, aCallback) {

Name the function, please.

@@ +1131,5 @@
> +  computeFileHash: function(aFile, aCallback) {
> +    Cu.import("resource://gre/modules/osfile.jsm");
> +    const CHUNK_SIZE = 16384;
> +
> +    // return the two-digit hexadecimal code for a byte

nit: comments starting with capital letter and . at the end, please. Also the same for the rest of comments introduced in the patch.

@@ +1136,5 @@
> +    function toHexString(charCode) {
> +      return ("0" + charCode.toString(16)).slice(-2);
> +    }
> +
> +    let ch = Cc["@mozilla.org/security/hash;1"]

nit: a var named 'ch' while reading a file might be a confusing name (char?). Could you change it to something more descriptive, please? You name an instance of Ci.nsICryptoHash like 'hasher' a few lines later.

@@ +1143,5 @@
> +    ch.init(ch.MD5);
> +
> +    OS.File.open(aFile.path, { read: true }).then(
> +      function opened(file) {
> +        let readChunk = function readChunk() {

Does this need to be declared as a variable? (just using a named function should suffice, unless you meant to bind it?).

@@ +1144,5 @@
> +
> +    OS.File.open(aFile.path, { read: true }).then(
> +      function opened(file) {
> +        let readChunk = function readChunk() {
> +          debug("In readChunk");

nit: remove this debug message, please.

@@ +1146,5 @@
> +      function opened(file) {
> +        let readChunk = function readChunk() {
> +          debug("In readChunk");
> +          file.read(CHUNK_SIZE).then(
> +            function readsuccess(array) {

nit: readSuccess

@@ +1152,5 @@
> +              ch.update(array, array.length);
> +              if (array.length == CHUNK_SIZE) {
> +                readChunk();
> +              } else {
> +                let hash = ch.finish(false);

nit: maybe add a comment explaining the 'false' parameter: "We are passing false to get the produced hash as binary data instead of a base 64 encoded string"

@@ +1154,5 @@
> +                readChunk();
> +              } else {
> +                let hash = ch.finish(false);
> +                // convert the binary hash data to a hex string.
> +                aCallback([toHexString(hash.charCodeAt(i)) for (i in hash)].join(""));

nit: lines shorter than 80 chars, please.

@@ +1157,5 @@
> +                // convert the binary hash data to a hex string.
> +                aCallback([toHexString(hash.charCodeAt(i)) for (i in hash)].join(""));
> +              }
> +            },
> +            function readerror() {

nit: readError

@@ +1166,5 @@
> +        }
> +
> +        readChunk();
> +      },
> +      function openerror() {

nit: openError

@@ +1174,5 @@
> +    );
> +  },
> +
> +  // Returns the MD5 hash of the manifest.
> +  computeManifestHash: function(aManifest) {

Name the function, please.

@@ +1186,5 @@
> +    let hasher = Cc["@mozilla.org/security/hash;1"]
> +                   .createInstance(Ci.nsICryptoHash);
> +    hasher.init(hasher.MD5);
> +    hasher.update(data, data.length);
> +    let hash = hasher.finish(false);

Same nit as the previous call to finish, add a comment about the 'false' parameter.

@@ +1435,5 @@
>            app.lastCheckedUpdate = Date.now();
>            if (app.origin.startsWith("app://")) {
> +            if (oldHash != hash) {
> +              updatePackagedApp.call(this, manifest);
> +            }

else { we should probably notify a 'downloadapplied' event, just like we do for the 304 case for packaged apps }

@@ +2035,5 @@
>              cleanup("NETWORK_ERROR");
>              return;
>            }
>  
> +          self.computeFileHash(zipFile, function(aHash) {

nit: name the function, please

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccacf79352fc

Comment 13

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0498a12f4e79

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.