834371 - Applying a packaged app update and immediately losing power to the phone - app update is lost

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

Build: B2G 18 1/24/2013
Device: Unagi

Steps:

1. Install a packaged app
2. Update it on the server
3. Check for updates
4. Apply the update
5. Immediately kill the power to the phone
6. Turn the phone on and try to check for updates again

Expected:

We should find an update available.

Actual:

No update is found. We appear to get stuck in the downloading phase, even though we didn't download the update.

  "{b1d6a06b-3013-4df4-ac58-808c20f7b7a8}": {
    "name": "Test WebAPI Permissions",
    "csp": "",
    "installOrigin": "http://mozqa.com",
    "origin": "app://{b1d6a06b-3013-4df4-ac58-808c20f7b7a8}",
    "receipts": [],
    "installTime": 1359054433516,
    "manifestURL": "http://mozqa.com/webapi-permissions-tests/simple_packaged_up
date/simple_packaged_update.manifest",
    "appStatus": 1,
    "removable": true,
    "id": "{b1d6a06b-3013-4df4-ac58-808c20f7b7a8}",
    "localId": 1022,
    "basePath": "/data/local/webapps",
    "progress": 119705,
    "installState": "updating",
    "downloadAvailable": true,
    "downloading": true,
    "readyToApplyDownload": false,
    "downloadSize": 17000,
    "lastUpdateCheck": 1359054433516,
    "etag": "\"804d-176-4d40d71c14ec0\"",
    "packageEtag": "\"804e-4365-4d400e45978c0\"",
    "manifestHash": "6d884b36d6bc8713bbd1828221b08815",
    "packageHash": "f972162820d90374406b1259d3d03ef6",
    "installerAppId": 1008,
    "installerIsBrowser": true,
    "lastCheckedUpdate": 1359054793298,
    "retryingDownload": false
  }

Comment 1

[Jason Smith [:jsmith]]

In one instance of testing this I also got a crash. Will get a crash report link shortly...

Comment 2

[Jason Smith [:jsmith]]

Argh. Socorro is having a problem with saving crash reports right now, so crash report link :(.

Comment 3

[Julien Wajsberg [:julienw]]

When I try a restart instead of killing the power of the phone, I get :

    "downloadAvailable": false,
    "downloading": false,

But I get an update as soon as I check.

I believe this is also wrong and we should have in both cases:

    "downloadAvailable": true,
    "downloading": false,

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: wip patch

This is a very simple & safe patch that just forces |downloading| to never be true at startup, so we always restart a failed download. Works fine when aborting a download, need to test more with updates.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 706016 [details] [diff] [review]
wip patch

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

need to reset installState too :

  "installState": "updating",

This approach should work for the "remove the battery" case, not for the "restart phone" case.

But the latter case is not critical though so I'd be ok with filing another bug for that, so that the first case could land for v1.

::: dom/apps/src/Webapps.jsm
@@ +150,5 @@
> +            // shutdown while downloading. Since we don't support resuming
> +            // downloads, just pretend that never happened and start from
> +            // scratch.
> +            if (app.downloading === true) {
> +              app.downloading = false;

why don't you just force app.downloading to false without testing ?

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Created attachment 706016 [details] [diff] [review]
> wip patch
> 
> This is a very simple & safe patch that just forces |downloading| to never
> be true at startup, so we always restart a failed download. Works fine when
> aborting a download, need to test more with updates.

Will the download restart on the next update ping? Or is the update broken indefinitely? Trying to determine whether we need this bug for v1.0.0

Comment 7

[Jason Smith [:jsmith]]

(In reply to Alex Keybl [:akeybl] from comment #6)
> (In reply to Fabrice Desré [:fabrice] from comment #4)
> > Created attachment 706016 [details] [diff] [review]
> > wip patch
> > 
> > This is a very simple & safe patch that just forces |downloading| to never
> > be true at startup, so we always restart a failed download. Works fine when
> > aborting a download, need to test more with updates.
> 
> Will the download restart on the next update ping? Or is the update broken
> indefinitely? Trying to determine whether we need this bug for v1.0.0

No. It's broken indefinitely entering into a bad state.

The patch on this bug is actually *fixing* the issue to allow you to recover to get the update ping. We won't get it right now because we are in a bad state.

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Now resetting |installState| to "installed" if we were in an "updating" state along with forcing |downloading| to false.

Works for me in both install and update cases.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Since bug 835606 will clear $TMP, we can't either just apply a staged update.

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 707732 [details] [diff] [review]
patch v3

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

works for me, thanks !
r=me

The following comment is not important, just apply it if you think that's better, as I do.

::: dom/apps/src/Webapps.jsm
@@ +143,5 @@
>  
> +            // Default installState to "installed", and reset if we shutdown
> +            // during an update.
> +            if (app.installState === undefined ||
> +                app.installState === "updating") {

I wonder if we should not do |if (app.installState !== 'pending')| instead, to guard against future possible installState ?

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Comment 12

[[:fabrice] Fabrice Desré]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-4 failures caused by something in this push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/220ee1b126c3

https://tbpl.mozilla.org/php/getParsedLog.php?id=19298749&tree=Mozilla-Inbound

206 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/webapps/test_bug_779982.html | Test timed out.

Comment 14

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09b4d9d69c42

Comment 15

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/406ad0db44e0

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09b4d9d69c42

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/229026fc55ce

Comment 18

[Jason Smith [:jsmith]]

Tested a bunch of restart and power off scenarios with a large packaged app update - looks good to me.