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

VERIFIED FIXED in Firefox 21

Status

defect
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: jsmith, Assigned: fabrice)

Tracking

Trunk
mozilla21
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

7 years ago
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
  }
Reporter

Comment 1

7 years ago
In one instance of testing this I also got a crash. Will get a crash report link shortly...
blocking-b2g: --- → tef?
Reporter

Comment 2

7 years ago
Argh. Socorro is having a problem with saving crash reports right now, so crash report link :(.
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,
Assignee

Comment 4

7 years ago
Posted patch wip patch (obsolete) — Splinter Review
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.
Assignee: nobody → fabrice
Attachment #706016 - Flags: feedback?(felash)
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 ?
Attachment #706016 - Flags: feedback?(felash) → feedback-
(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
Reporter

Comment 7

7 years ago
(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.
blocking-b2g: tef? → tef+
Assignee

Comment 8

7 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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.
Assignee

Comment 9

7 years ago
Posted patch patch v3Splinter Review
Since bug 835606 will clear $TMP, we can't either just apply a staged update.
Attachment #706016 - Attachment is obsolete: true
Attachment #707726 - Attachment is obsolete: true
Attachment #707732 - Flags: review?(felash)
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 ?
Attachment #707732 - Flags: review?(felash) → review+
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
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.
Assignee

Updated

7 years ago
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Reporter

Updated

7 years ago
Keywords: verifyme
Reporter

Updated

7 years ago
QA Contact: jsmith
Reporter

Comment 18

7 years ago
Tested a bunch of restart and power off scenarios with a large packaged app update - looks good to me.
Status: RESOLVED → VERIFIED
Keywords: verifyme

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.