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

VERIFIED FIXED in Firefox 26

Status

defect
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: amac, Assigned: amac)

Tracking

unspecified
mozilla26
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox24 wontfix, firefox25 wontfix, firefox26 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [LeoVB+])

Attachments

(3 attachments, 9 obsolete attachments)

2.32 KB, patch
amac
: review+
Details | Diff | Splinter Review
33.18 KB, patch
amac
: review+
Details | Diff | Splinter Review
1.10 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
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.
Is this a regression from 1.01? Or does this likely already exist on 1.01?
It's happening on 1.01. In fact we found it on a ZTE open
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.
blocking-b2g: --- → leo?
Fabrice,

Please check if this issue is reproducible and comment on the UX
Flags: needinfo?(fabrice)
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).
Flags: needinfo?(fabrice)
(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 ;)
Assignee: nobody → amac
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).
Attachment #799519 - Flags: review?(fabrice)
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).
Attachment #799519 - Flags: review?(fabrice) → feedback+
Posted patch includes feedback requests (obsolete) — Splinter Review
Tests are coming on another patch, in 5 mins :)
Attachment #799519 - Attachment is obsolete: true
Attachment #800203 - Flags: review?(fabrice)
Posted patch part 2, tests (obsolete) — Splinter Review
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).
Attachment #800208 - Flags: review?(ferjmoreno)
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...
Attachment #800208 - Flags: review?(ferjmoreno) → review-
(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 ;)
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"...
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).
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.
Attachment #800208 - Attachment is obsolete: true
Attachment #800274 - Flags: review?(fabrice)
Lets leave customization to work on this!! Minus
blocking-b2g: leo? → -
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)
Attachment #800203 - Flags: review?(fabrice) → feedback+
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?
Attachment #800274 - Flags: review?(ferjmoreno)
Attachment #800274 - Flags: review?(fabrice)
Attachment #800274 - Flags: feedback+
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.
Attachment #800274 - Flags: review?(ferjmoreno) → feedback+
Posted patch Patches, third try (obsolete) — Splinter Review
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.
Attachment #800274 - Attachment is obsolete: true
Attachment #800886 - Flags: review?(ferjmoreno)
Attachment #800886 - Flags: review?(fabrice)
Posted patch bug900553_p1_02.patch (obsolete) — Splinter Review
I don't know if that's what you wanted with OS.File.move :).
Attachment #800203 - Attachment is obsolete: true
Attachment #800911 - Flags: review?(fabrice)
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.
Attachment #800911 - Flags: review?(fabrice) → review-
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...
Attachment #800886 - Flags: review?(fabrice)
Posted patch Part 1, V3. Actual changes (obsolete) — Splinter Review
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?
Attachment #800911 - Attachment is obsolete: true
Attachment #801106 - Flags: review?(fabrice)
That sound you hear is my head hitting my table.
Attachment #800886 - Attachment is obsolete: true
Attachment #800886 - Flags: review?(ferjmoreno)
Attachment #801109 - Flags: review?(ferjmoreno)
Attachment #801109 - Flags: review?(fabrice)
(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 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
Attachment #801109 - Flags: review?(ferjmoreno) → review+
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
Attachment #801106 - Flags: review?(fabrice) → review+
(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 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 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
Attachment #801109 - Flags: review?(fabrice) → review+
r=ferjm, r=fabrice
Try run at https://tbpl.mozilla.org/?tree=Try&rev=9eacd27be08e
Attachment #801109 - Attachment is obsolete: true
Attachment #802146 - Flags: review+
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
Attachment #802146 - Attachment is obsolete: true
Attachment #802270 - Flags: review+
Comment on attachment 802146 [details] [diff] [review]
Tests, including review comments

Unobsoleting this since I found the problem on windows
Attachment #802146 - Attachment is obsolete: false
Comment on attachment 802270 [details] [diff] [review]
Test, including review comments. Disabled for Windows

Obsoleting this one, fixed the problem on Windows
Attachment #802270 - Attachment is obsolete: true
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
Attachment #802396 - Flags: review?(fabrice) → review+
(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 ;)
(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.
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.
Flags: needinfo?(praghunath)
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+
blocking-b2g: - → leo?
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.
Flags: needinfo?(fabrice)
Keywords: verifyme
QA Contact: jsmith
I think the risk is really. No need to backport the tests though.
Flags: needinfo?(fabrice)
(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
marking blockingleo+
blocking-b2g: leo? → leo+
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?
Status: RESOLVED → VERIFIED
Flags: needinfo?(amac)
Keywords: verifyme
(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.
Flags: needinfo?(amac)
Whiteboard: [LeoVB+]
Flags: needinfo?(praghunath)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.