900553 - When the minimanifest changes but the package doesn't we get on a update loop

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

STR:

1. Install any packaged app from a remote source.
2. Modify the remote minimanifest (just add a random space), but don't modify the zip
3. Search for updates
4. When an update for the app is offered, apply it.
5. The update will disappear from the list.
6. Search for updates.

Expected: 

No updates are offered.

Actual:

The update is offered again.

What is happening is that since the package (zip) is the same, it's considered already applied but the local manifest copy isn't updated, so every time it searches for an update it will be offered again.

Comment 1

[Jason Smith [:jsmith]]

Is this a regression from 1.01? Or does this likely already exist on 1.01?

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

It's happening on 1.01. In fact we found it on a ZTE open

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

I'm noming this for leo because we've found this problem on the latest debug build provided by the partner for a leo device. 

The effect for users is:

* The device offers an update for the HERE Maps app.
* The user accepts the update. 
* The device downloads the 10mb. 
* The update *isn't* installed (because the package hasn't actually changed).
* The next time an update search is executed (next day, next week, next time a manual search is done), goto the first step, rinse and repeat.

Comment 4

[Preeti Raghunath(:Preeti)]

Fabrice,

Please check if this issue is reproducible and comment on the UX

Comment 5

[[:fabrice] Fabrice Desré]

I didn't try to repro but I'm 100% sure this bug exists because we don't unstage the manifest if we don't update also the package.

I also don't think we should block on that for 1.1 since that can be worked around if the right manifests are preloaded on the device (this is not the case currently).

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> I didn't try to repro but I'm 100% sure this bug exists because we don't
> unstage the manifest if we don't update also the package.
> 
> I also don't think we should block on that for 1.1 since that can be worked
> around if the right manifests are preloaded on the device (this is not the
> case currently).

The problem with that is that the control we have about how the device builds are done is tenuous at best... and I can have a patch for that in a little while anyway ;)

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: When the package is the same we have, update the minimanifest hash

This just clears the app.staged when the package is the same than we had, This way at least we download it only once and all not flat-rate users will be slighty happier.

Note that this leaves a problem open, which is if whoever updates the app updates the minimanifest first, and then we try downloading the app between him updating the minimanifest and him updating the package. Which has a low probability so it'll happen for sure in the middle of a critical demo. But it also has a easy solution: Recommend updating first the package AND then the minimanifest (which they should do anyway to avoid offering partial file downloads).

Comment 8

[[:fabrice] Fabrice Desré]

Comment on attachment 799519 [details] [diff] [review]
When the package is the same we have, update the minimanifest hash

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

That also needs tests before landing.

::: dom/apps/src/Webapps.jsm
@@ +2409,5 @@
> +                // hasn't. Let's clear this, then so we don't keep offering
> +                // a bogus update to the user
> +                app.manifestHash = app.staged.manifestHash;
> +                app.etag = app.staged.etag || app.etag;
> +                app.staged = {};

You also need to move the staged manifest to be unstaged (like https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1219).

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: includes feedback requests

Tests are coming on another patch, in 5 mins :)

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: part 2, tests

Tests.

If you apply this patch without applying the other one, the last test fails (that's expected because the other patch fixes that behavior).

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 800208 [details] [diff] [review]
part 2, tests

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

Antonio, we need a new "update_package_manifest_only" test, not some invasive changes to the install test. Let's not end up with a single mega test file...

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #11)
> Comment on attachment 800208 [details] [diff] [review]
> part 2, tests
> 
> Review of attachment 800208 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Antonio, we need a new "update_package_manifest_only" test, not some
> invasive changes to the install test. Let's not end up with a single mega
> test file...

Actually I didn't make a update_package_manifest_only test. I did test for updating an app normally (which didn't exist), for updating an app with only the manifest (same) and then for checking if an app was still missing. I can move that to a new file... but I'm still going to have a big file since I use a lot of Fernando's test infrastructure (so I'm going to do a lot of C&P). Not to mention that to update an app I have to install it which is what the last test of the original file did ;)

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

Actually I didn't make a update_package_manifest_only test. I did test for updating an app normally (which didn't exist), for updating an app with only the manifest (same) and then for checking if an app was still updateable I meant. And the original file is supposedly "Packaged apps installation and update"...

Comment 14

[[:fabrice] Fabrice Desré]

Then maybe we should preinstall a packaged app in the test profile to not have to run the install part in update tests (we have that for other apps tests, using hosted apps).

Comment 15

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 2, tests on a separate file this time

Installing the app on the test (even after separating it) only adds 30 lines of code. Preinstalling the app would actually mean more work (since I still would have to find a way to create a correct and incorrect update for the app).

Same as with the older one the last test (and only the last test) fails if the other patch isn't applied.

Comment 16

[Preeti Raghunath(:Preeti)]

Lets leave customization to work on this!! Minus

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 800203 [details] [diff] [review]
includes feedback requests

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

::: dom/apps/src/Webapps.jsm
@@ +2405,5 @@
>                app.installState = "installed";
>                app.readyToApplyDownload = false;
> +              if (app.staged && app.staged.manifestHash) {
> +                // If we're here then the manifest has changed but the package
> +                // hasn't. Let's clear this, then so we don't keep offering

nit: Let's clear this, so we don't...

@@ +2418,5 @@
> +                staged.append("staged-update.webapp");
> +
> +                if (staged.exists()) {
> +                  staged.moveTo(dir, "update.webapp");
> +                }

My bad for not telling you, but we're moving all new file manipulation code to use OS.File, which has a move() method (see https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread)

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 800274 [details] [diff] [review]
Part 2, tests on a separate file this time

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

I'd like to take another look, and also get ferjm review.

::: dom/apps/tests/file_packaged_app.sjs
@@ +31,5 @@
>    // the application package and return.
>    if ("setVersion" in query) {
>      var version = query.setVersion;
>      setState("version", version);
> +    var pckVersion = ("dontUpdatePackage" in query) ? version -1 : version;

nit: not sure why, but 'pckVersion' hurts my eyes (it looks too much like hungarian notation I guess). Bonus points for a better name :)

::: dom/apps/tests/test_packaged_app_update.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id={821589}

Update the bug number.

@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id={821589}
> +-->
> +<head>
> +  <title>Test for Bug {821589} Packaged apps installation and update</title>

Here also, update both the title and the bug number.

@@ +10,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id={821589}">Mozilla Bug {821589}</a>

same here.

@@ +26,5 @@
> +var gSJS = "http://test/" + gSJSPath;
> +var gAppName = "appname";
> +var gApp = null;
> +
> +var launchableValue = undefined;

no need for |= undefined|

@@ +216,5 @@
> +  var request = gApp.checkForUpdate();
> +  request.onerror = mozAppsError;
> +  request.onsuccess = function(event) {
> +    var expectingDownload = aExpected ? "": "not";
> +    ok(gApp.downloadAvailable === aExpected, 

nit: trailing ws

@@ +264,5 @@
> +          downloadSize: 0,
> +          size: 0,
> +          readyToApplyDownload: false,
> +        };
> +        checkAppState(gApp, 2, expected, true, false, next);

That doesn't hurt to test that, but in we do this tests already in the install specific one. You can just call next() in ondownloadsuccess

@@ +342,5 @@
> +      } else {
> +        next();
> +      }
> +    };
> +

This is test and the previous one only differ by a couple of parameters. Can you refactor that to share the code?

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 800274 [details] [diff] [review]
Part 2, tests on a separate file this time

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

Thanks Antonio! Tests FTW!

Maybe we can move the common parts of test_packaged_app_(install/update).html to a single file :). I'm thinking about 'setAppVersion', 'checkAppState', etc.

Also, I understand if you don't want to do this in this patch, but it would be great if we could also add tests for cases like:

- checkForUpdate on an app with 'pending' state. It should throw PENDING_APP_NOT_UPDATABLE error.
- checkForUpdate on an app with 'downloading' state. It should throw APP_IS_DOWNLOADING error.

In the future it would be great if we could manually modify the apps registry to check cases like testing updates on preinstalled apps.

::: dom/apps/tests/file_packaged_app.sjs
@@ +31,5 @@
>    // the application package and return.
>    if ("setVersion" in query) {
>      var version = query.setVersion;
>      setState("version", version);
> +    var pckVersion = ("dontUpdatePackage" in query) ? version -1 : version;

'packageVersion' could work.

Also, space after minus sygn in "version -1", please.

@@ +42,5 @@
>  
> +    if (version == pckVersion) {
> +      // Create the application package.
> +      var zipWriter = Cc["@mozilla.org/zipwriter;1"]
> +                        .createInstance(Ci.nsIZipWriter);

nit: keep the old indentation, please.

::: dom/apps/tests/test_packaged_app_update.html
@@ +80,5 @@
> +  var dontUpdate = "";
> +  if (aDontUpdatePackage) {
> +    dontUpdate = "&dontUpdatePackage=1";
> +  }
> +  var url = gSJS + "?setVersion=" + aVersion + dontUpdate;

Another way to say the same:

var url = gSJS + "?setVersion=" + aVersion;
if (aDontUpdatePacakge) {
  url += "&dontUpdatePackage=1";
}

@@ +208,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +function checkForUpdate(aExpected) {
> +  var miniManifestURL = gSJS +
> +      "?getManifest=true&appToUpdate";

nit: no need to split the above line in two.

@@ +242,5 @@
> +  },
> +  function() {
> +    ok(true, "== TEST == Install packaged app");
> +    var miniManifestURL = gSJS +
> +                          "?getManifest=true&appToUpdate";

Can you add a comment about the addition of "appToUpdate", please?

Also, another nit, no need to split the line in two.

@@ +278,5 @@
> +  function() {
> +    setAppVersion(3, next);
> +  },
> +  function() {
> +

nit: no need for extra lines. The same for the rest of the steps.

Comment 20

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Patches, third try

I made Fernando's requested changes, moved all the common part into a common file, and added one of the two extra tests he wanted to do. I don't know how to leave an app hanging on the downloading state indefinitely... and it's slighty out of this bug scope anyway.

Comment 21

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: bug900553_p1_02.patch

I don't know if that's what you wanted with OS.File.move :).

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 800911 [details] [diff] [review]
bug900553_p1_02.patch

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

::: dom/apps/src/Webapps.jsm
@@ +2416,5 @@
> +                let dirPath = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id],
> +                                               true, true).path;
> +                // We don't really mind much if this fails.
> +                OS.File.move(dirPath + "/staged-update.webapp",
> +                             dirPath + "/update.webapp");

Never append a path using string literals, because the path separator is not the same everywhere (I'm looking at you, windows). OS.Path.join() is what you want.

Comment 23

[[:fabrice] Fabrice Desré]

Comment on attachment 800886 [details] [diff] [review]
Patches, third try

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

::: dom/apps/tests/test_packaged_app_install.html
@@ +6,5 @@
>  <head>
>    <title>Test for Bug {821589} Packaged apps installation and update</title>
>    <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="text/javascript" src="test_packaged_app_common.js"></script>

You forgot to include this one in your patch...

Comment 24

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1, V3. Actual changes

I did look for a function like that but didn't find it. And there even was an example on the page you linked me! Blind as a bat, I am.

BTW, / works also on Windows system calls (c:\adir/anotherdir/aFile actually works correctly). And it has done so since... at least since before DOS 3.2 which was the first one I used :). 

Where it doesn't work is on some command line utils which use / as a option separator.

Is the 

let dirPath = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id],
                               true, true).path;
correct or there is other way of getting the webapps directory base now?

Comment 25

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Patches, this time with all files. I hope.

That sound you hear is my head hitting my table.

Comment 26

[Marco Castelluccio [:marco]]

(In reply to Antonio Manuel Amaya Calvo from comment #24)
> Is the 
> 
> let dirPath = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id],
>                                true, true).path;
> correct or there is other way of getting the webapps directory base now?

There's _getAppDir(id), but in this case I think you want the third parameter |false|, because the directory is guaranteed to be existing.

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 801109 [details] [diff] [review]
Patches, this time with all files. I hope.

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

Thanks Antonio!

r=me with the comments addressed, please.

::: dom/apps/tests/test_packaged_app_common.js
@@ +1,3 @@
> +var PackagedTestHelper = (function PackagedTestHelper() {
> +
> +  'use strict';

Can you take this out of the function, please?

Also, use double quotes. Here and in the rest of the patch, please.

@@ +1,4 @@
> +var PackagedTestHelper = (function PackagedTestHelper() {
> +
> +  'use strict';
> +

nit: no need for extra line.

::: dom/apps/tests/test_packaged_app_install.html
@@ +28,5 @@
>  function checkAppInstallError(aMiniManifestURL, aExpectedError) {
>    var req = navigator.mozApps.installPackage(aMiniManifestURL);
>    req.onsuccess = function() {
>      ok(false, "We are supposed to throw " + aExpectedError);
>      finish();

There is no finish() anymore, you'd need to use PackagedTestHelper.finish() instead. The same applies for the whole patch.

@@ +39,3 @@
>      } else {
>        ok(false, "Got unexpected " + aError);
>        finish();

PackagedTestHelper.finish()

@@ +54,4 @@
>    };
>    req.onerror = function(evt) {
>      ok(false, "Got unexpected " + evt.target.error.name);
>      finish();

PackagedTestHelper.finish()

@@ +66,5 @@
>    var req = navigator.mozApps.checkInstalled(aMiniManifestURL);
>    req.onsuccess = function(evt) {
>      ok(true, "The app is installed");
> +    PackagedTestHelper.checkAppState(evt.application, aVersion, aExpectedApp,
> +                                    aLaunchable, false, aCb);

nit: indentation

@@ +71,4 @@
>    };
>    req.onerror = function() {
>      ok(false, "The app is not installed");
>      finish();

PackagedTestHelper.finish()

@@ +80,5 @@
>  var steps = [
>    function() {
>      // Set up
> +    PackagedTestHelper.launchableValue =
> +      SpecialPowers.setAllAppsLaunchable(true);

nit: no need to split the line.

@@ +193,1 @@
>          finish();

PackagedTestHelper.finish();

@@ +218,5 @@
>      };
>    },
>    function() {
>      ok(true, "all done!\n");
>      SimpleTest.finish();

PackagedTestHelper.finish()

::: dom/apps/tests/test_packaged_app_update.html
@@ +27,5 @@
> +}
> +
> +var miniManifestURL;
> +
> +var launchableValue;

Can't we use PackagedTestHelper.launchableValue?

@@ +43,5 @@
> +                               PackagedTestHelper.mozAppsError;
> +  request.onsuccess = function(event) {
> +    var expectingDownload = aExpected ? "": "not ";
> +    ok(lApp.downloadAvailable === aExpected,
> +       "Download should " + expectingDownload + "be available");

nit: space before "be"

@@ +54,5 @@
> +}
> +
> +function checkLastAppState(aMiniManifestURL, aExpectedReady, aExpectedDownload,
> +                           aExpectedVersion, aCb) {
> +  ok(true, aExpectedReady?"App downloaded":"App download applied");

nit: spaces before and after "?" and ":"

@@ +71,5 @@
> +  PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, aExpectedVersion,
> +                                   expected, true, false, aCb);
> +}
> +
> +

nit: no need for extra lines.

@@ +84,5 @@
> +
> +    var ondownloadsuccesshandler =
> +      checkLastAppState.bind(undefined, miniManifestURL,
> +                             aExpectedReady, false, aPreviousVersion,
> +                             function () {

nit: no space before parenthesis

@@ +177,5 @@
> +    });
> +  },
> +  function() {
> +    ok(true, "all done!\n");
> +    SimpleTest.finish();

PackagedTestHelper.finish()

@@ +183,5 @@
> +];
> +
> +PackagedTestHelper.setSteps(steps);
> +// appToUpdate added to the URL so we get a unique URL for this app.
> +// Unique in this case meaning different fromt the ones used on the

typo: from

Comment 28

[[:fabrice] Fabrice Desré]

Comment on attachment 801106 [details] [diff] [review]
Part 1, V3. Actual changes

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

r=me with nit addressed.

::: dom/apps/src/Webapps.jsm
@@ +2436,5 @@
> +                app.staged = {};
> +                // Move the staged update manifest to a non staged one.
> +
> +                let dirPath = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id],
> +                                               true, true).path;

nit: let dirPath = self._getAppDir(id).path

Comment 29

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #28)
> Comment on attachment 801106 [details] [diff] [review]
> Part 1, V3. Actual changes
> 
> Review of attachment 801106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit addressed.
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +2436,5 @@
> > +                app.staged = {};
> > +                // Move the staged update manifest to a non staged one.
> > +
> > +                let dirPath = FileUtils.getDir(DIRECTORY_NAME, ["webapps", id],
> > +                                               true, true).path;
> 
> nit: let dirPath = self._getAppDir(id).path

This is going to do main-thread io, we should avoid that and set the third parameter as false here.

Comment 30

[[:fabrice] Fabrice Desré]

Comment on attachment 801109 [details] [diff] [review]
Patches, this time with all files. I hope.

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

r=me with comments addressed. Please push on try with the updated patch before landing.

::: dom/apps/tests/file_packaged_app.sjs
@@ +42,5 @@
>  
> +    if (version == packageVersion) {
> +      // Create the application package.
> +      var zipWriter = Cc["@mozilla.org/zipwriter;1"]
> +                      .createInstance(Ci.nsIZipWriter);

Nit: align .createInstance with ["@mozilla....

::: dom/apps/tests/test_packaged_app_common.js
@@ +1,1 @@
> +var PackagedTestHelper = (function PackagedTestHelper() {

Please add a MPL header.

@@ +208,5 @@
> +    setSteps: function (aSteps) {
> +      steps = aSteps;
> +    },
> +    next: next,
> +    go: go,

can we rename that to "start" please?

::: dom/apps/tests/test_packaged_app_update.html
@@ +187,5 @@
> +// Unique in this case meaning different fromt the ones used on the
> +// install tests
> +miniManifestURL = PackagedTestHelper.gSJS + "?getManifest=true&appToUpdate";
> +
> +addLoadEvent(PackagedTestHelper.go);

s/go/start

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 801109 [details] [diff] [review]
Patches, this time with all files. I hope.

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

r=me with comments addressed. Please push on try with the updated patch before landing.

::: dom/apps/tests/file_packaged_app.sjs
@@ +42,5 @@
>  
> +    if (version == packageVersion) {
> +      // Create the application package.
> +      var zipWriter = Cc["@mozilla.org/zipwriter;1"]
> +                      .createInstance(Ci.nsIZipWriter);

Nit: align .createInstance with ["@mozilla....

::: dom/apps/tests/test_packaged_app_common.js
@@ +1,1 @@
> +var PackagedTestHelper = (function PackagedTestHelper() {

Please add a MPL header.

@@ +208,5 @@
> +    setSteps: function (aSteps) {
> +      steps = aSteps;
> +    },
> +    next: next,
> +    go: go,

can we rename that to "start" please?

::: dom/apps/tests/test_packaged_app_update.html
@@ +187,5 @@
> +// Unique in this case meaning different fromt the ones used on the
> +// install tests
> +miniManifestURL = PackagedTestHelper.gSJS + "?getManifest=true&appToUpdate";
> +
> +addLoadEvent(PackagedTestHelper.go);

s/go/start

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Part 1. Actual changes, including review comments

Try run at https://tbpl.mozilla.org/?tree=Try&rev=9eacd27be08e

Comment 33

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Tests, including review comments

r=ferjm, r=fabrice
Try run at https://tbpl.mozilla.org/?tree=Try&rev=9eacd27be08e

Comment 34

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Test, including review comments. Disabled for Windows

r=fabrice, r=ferjm

I disabled the tests on windows because navigator.mgmt.applyDownload is failing there.

The only difference between this patch and the reviewed one is that I added a 

if (navigator.platform.startsWith("Win")) {
  // These tests don't work at Windows currently because
  // navigator.mgmt.applyDownload fails
  ok(true, "Tests skipped on Windows");
  SimpleTest.finish();
} else {
  old code
} 
To the test.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=fb0ca82e239b

Comment 35

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 802146 [details] [diff] [review]
Tests, including review comments

Unobsoleting this since I found the problem on windows

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 802270 [details] [diff] [review]
Test, including review comments. Disabled for Windows

Obsoleting this one, fixed the problem on Windows

Comment 37

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: Windows fix, closes the application.zip file after hashing

Comment 38

[Marco Castelluccio [:marco]]

I've written another test for packaged app updates, that is more similar to the other update tests (that is, it tests the app by loading it in an iframe, to make sure the package is really updated). So I hope I didn't write it for nothing :D

Comment 39

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #38)
> I've written another test for packaged app updates, that is more similar to
> the other update tests (that is, it tests the app by loading it in an
> iframe, to make sure the package is really updated). So I hope I didn't
> write it for nothing :D

That's not testing more if the package is really updated since you rely on the same DOM api. You are mostly testing that getSelf() returns the correct app ;)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e00feb59df10
https://hg.mozilla.org/integration/b2g-inbound/rev/56ec76b4037b
https://hg.mozilla.org/integration/b2g-inbound/rev/ecc322e666c8

Comment 41

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #39)
> (In reply to Marco Castelluccio [:marco] from comment #38)
> > I've written another test for packaged app updates, that is more similar to
> > the other update tests (that is, it tests the app by loading it in an
> > iframe, to make sure the package is really updated). So I hope I didn't
> > write it for nothing :D
> 
> That's not testing more if the package is really updated since you rely on
> the same DOM api. You are mostly testing that getSelf() returns the correct
> app ;)

I'm accessing to the {PACKAGE}/index.html file, so if it didn't change the test would fail.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e00feb59df10
https://hg.mozilla.org/mozilla-central/rev/56ec76b4037b
https://hg.mozilla.org/mozilla-central/rev/ecc322e666c8

Comment 43

[Wayne Chang [:wchang]]

Hi Preeti,

What do you mean by this?

(In reply to Preeti Raghunath(:Preeti) from comment #16)
> Lets leave customization to work on this!! Minus

Leo will likely need to pick this up for their first MR coming soon, which is still on v1-train.

Comment 44

[Beatriz Rodríguez [:brg]]

This is very important to guarantee a good update process of the devices in the market(afecting not only leo but all of them). We do need this in v1-train.
Thanks Wayne.
Nominating again... please set it to leo+

Comment 45

[Jason Smith [:jsmith]]

Fabrice - Can you give you a risk assessment on taking this patch to 1.1?

I'm going to verify this on trunk to assess if there's any fallout from this patch.

Comment 46

[[:fabrice] Fabrice Desré]

I think the risk is really. No need to backport the tests though.

Comment 47

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #46)
> I think the risk is really. No need to backport the tests though.

Clarified in IRC - risk is really low

Comment 48

[Tony Chung [:tchung]]

marking blockingleo+

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3655fe17b75b

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/3655fe17b75b

Comment 51

[Jason Smith [:jsmith]]

Tested this on a 1.2 build - this sort of works. When the mini-manifest changes but not the zip file, I do end up seeing an update being available. When I apply the update and check again, I won't see another update available. As a result, this patch appears to fix the issue with the update loop, but it won't prevent the initial app update check from being shown, even though there probably isn't value to issue the update.

Antonio - Thoughts?

Comment 52

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Jason Smith [:jsmith] from comment #51)
> Tested this on a 1.2 build - this sort of works. When the mini-manifest
> changes but not the zip file, I do end up seeing an update being available.
> When I apply the update and check again, I won't see another update
> available. As a result, this patch appears to fix the issue with the update
> loop, but it won't prevent the initial app update check from being shown,
> even though there probably isn't value to issue the update.
> 
> Antonio - Thoughts?


That sort of works is actually the expected behavior... and the only behavior we can implement. We cannot know that the zip hasn't changed until we actually download the zip (well, we can using the etag but that's already implemented so if it's being actually downloaded is because either we didn't had an etag or the etag changed although the file didn't). And we don't download the zip until the user tells us to do so by selecting to apply the update. So even if, as you say, there isn't any value in issuing the update the problem is that we don't know that until after we've downloaded the file, and by then it's too late :)

The problem before was that even after downloading the zip the new manifest data wasn't being updated and so we would get offered the update again (and then download again and on and on). And that's what the patch fixes.