Closed Bug 1096971 Opened 5 years ago Closed 5 years ago

When updating an app, if it fails (network goes down) you need to restart the device in order to use the app

Categories

(Core Graveyard :: DOM: Apps, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 verified)

VERIFIED FIXED
mozilla36
blocking-b2g 2.0+
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- verified

People

(Reporter: mbarone976, Assigned: amac)

References

Details

Attachments

(3 files, 1 obsolete file)

Tested on Flame 2.0 and master.

STR
  1. Install an app from a store. The app *must* be privileged and have a specific origin (like app://whatever.com).
  2. Update the app on the store
  3. Check for updates and apply the update.
  4. Before the update is downloaded completely, remove the network from the device (stopping the wifi for example).

  5. At a later point (before it gives the network error), reactivate the network, and cancel the update.
  6. Resume the update

ACTUAL RESULT
The app seems to be installed as expected but when you open the app the message "Required update: There is an update with new features....." is shown. Also the message "There was an error downloading the update" is shown. The user can't use the app until you restart the device; now is when the app works as expected.

EXPECTED RESULT
The app should be updated correctly without restarting the device.
Assignee: nobody → amac.bug
There seem to be several errors here:

1 On one hand, when the app is finally installed, for some reason we're not updating correctly the etag/hash. So when checkForUpdates is called again, a new (bogus) update is offered.

2 Then, when the user tries to download the package, the actual zip is detected as being the same we already have and so we (correctly) send a applied event ([1]). But then in the next line we throw a PACKAGE_UNCHANGED. Which ends sending a downloaderror event at [2]. So we're telling Gaia that the download was a success, and applied, and that it was an error at the same time. Which seems kinda schizophrenic, all things considered.

Now, we could fix this in Gaia by checking if the error was PACKAGE_UNCHANGED, but since we're already saying that the download was a success, I think it would be better to not send the downloaderror when the package is unchanged.

WDTY, Fabrice?

(oh, BTW, this fails also since 2.0 at least).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#3264
[2] http://mxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#3945
Flags: needinfo?(fabrice)
Yep, it looks strange to say "everything work catastrophically"! So yes, we should not send downloaderror in this case.
I'm not sure how we can test that though.
Flags: needinfo?(fabrice)
This fixes half of the problem (the easy half): when the package downloaded is the same as the one we had, we won't fire a downloaderror on the content. I also updated the test to fail in case we did.
Test run is at: 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=41f133ffcd84
Attachment #8523775 - Flags: review?(fabrice)
Attachment #8523775 - Flags: review?(fabrice) → review+
And this fixes the second problem. The problem exists because when a download fails we were deleting the .staged attribute, but the staged-update file still existed (because the download is still available, and it's a transient error supposedly). So when the user retries the download the .staged.etag and .staged.manifestHash are not recalculated, and so even if the update is applied correctly the system still thinks that the remote manifest is different from the local one (and so the update is offered again).

Added a test to check for this error condition also (downloading/applying an update after it failed once).
Attachment #8524620 - Flags: review?(fabrice)
[Blocking Requested - why for this release]: Fatal error, we have detected the platform fault with Loop application but it will happen for any application that interrupts his downloading from the Market Place because a network disconnection.
blocking-b2g: --- → 2.0?
Comment on attachment 8524620 [details] [diff] [review]
Part 2, V1. Don't erase the staged attributes if the download is still available

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

::: dom/apps/tests/file_packaged_app.sjs
@@ +61,5 @@
>      var packageName = "test_packaged_app_" + packageVersion + ".zip";
>  
>      setState("packageName", packageName);
>      var packagePath = "/" + gBasePath + "file_packaged_app.sjs?" +
> +                      (allowCancel?"allowCancel&": "") +

nit: spaces around the operators.
Attachment #8524620 - Flags: review?(fabrice) → review+
r=fabrice
Attachment #8524620 - Attachment is obsolete: true
Attachment #8526898 - Flags: review+
Part 2 touches the same files than part1, and they should be applied in order.
Keywords: checkin-needed
Comment on attachment 8523775 [details] [diff] [review]
Part 1, V1. Don't fire downloaderror when the package wasn't changed on an update

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When a package update fails for a transient problem (like a network error, network going down, etc) after the network comes up and the package is updated successfully, it'll be unlaunchable until the phone is rebooted. The user doesn't get any indication of this (the UI shown is as if the package wasn't updated successfully)
Testing completed: The patch includes tests, and also manual tests.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:none
Attachment #8523775 - Flags: approval-mozilla-b2g32?
Comment on attachment 8526898 [details] [diff] [review]
Part 2, V2. With nits fixed

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When a package update fails for a transient problem (like a network error, network going down, etc) after the network comes up and the package is updated successfully, the package update will *still* be offered (so if the app checks for updates it'll offer it again). This is compounded by the other problem fixed in this patch (the update will be offered even after a reboot).
Testing completed: The patch includes tests, and also manual tests.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:none
Attachment #8526898 - Flags: approval-mozilla-b2g32?
https://hg.mozilla.org/mozilla-central/rev/71a0e053308c
https://hg.mozilla.org/mozilla-central/rev/8e0063fd3c5e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
[Triage] fixed in m/c already per comment#13, not blocking 2.0 per the abnormal use case and current 2.0 phase.
blocking-b2g: 2.0? → ---
oteo/:amac, is this 2.0 specific regression or has this been always busted int he past releases as well?
Flags: needinfo?(oteo)
Flags: needinfo?(amac.bug)
This code was rewritten completely for 2.0. The actual code that I changed didn't exist in 1.4 (it would launch an uncatched exception which would be logged, but that would be transparent for Gaia so for the user there would not be any problems).
Flags: needinfo?(amac.bug)
Flags: needinfo?(oteo)
[Blocking Requested - why for this release]:

Clearing my needinfo, as Antonio has already confirmed that this is a regression and in 1.4 this bug was not happening
blocking-b2g: --- → 2.0?
Depends on: 1105560
Yeojin, can you verify that this issue has been fixed?
QA Whiteboard: [QAnalyst-Triage-]
Flags: needinfo?(ychung)
Tested with Flame
master (KK 188 based)
Gecko-6d3a321
Gaia-689bdc8

It works well. If the user lost the internet connection and then resume the connection, the update is updated as expected and you don't have to restart the device. When the device finish to update the app, the message "There was an error downloading the update" is displayed. I understand that this is the expected behavior.
Flags: needinfo?(ychung)
Marking as verified fixed based on Comment 19.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage-] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
[Triage] per comment#16 we may "treat" this one as a regression however not reported from partner, also fixed already in mc/, thus de-nom..
blocking-b2g: 2.0? → ---
[Blocking Requested - why for this release]: this is a blocker and reported by a partner. please approve the uplift to the branch we are about to commercialize.
blocking-b2g: --- → 2.0?
[Triage]

Thanks Beatriz, I see, so at least we need this in 2.0 as well.

Also per comment#1 & comment#17 update flags, and tag for 2.0.

qawanted for 2.1 and 2.0M.
blocking-b2g: 2.0? → 2.0+
status-b2g-v2.1: --- → ?
This is the B2G32 version of the patch (without backporting the tests, and folded into a single patch).
Attachment #8536510 - Flags: review+
Was there a b2g34 approval request coming for this too?
Flags: needinfo?(amac.bug)
Flags: in-testsuite+
It should be applied to b2g34 also, indeed. I believe the m-c one should apply. Let me know if it doesn't so I can rebase it. Thanks!
Flags: needinfo?(amac.bug)
Re-read my comment. You need to *request approval* for each branch.
Flags: needinfo?(amac.bug)
Comment on attachment 8523775 [details] [diff] [review]
Part 1, V1. Don't fire downloaderror when the package wasn't changed on an update



[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: When a package update fails for a transient problem (like a network error, network going down, etc) after the network comes up and the package is updated successfully, it'll be unlaunchable until the phone is rebooted. The user doesn't get any indication of this (the UI shown is as if the package wasn't updated successfully)
Testing completed: The patch includes tests, and also manual tests.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch:none
Flags: needinfo?(amac.bug)
Attachment #8523775 - Flags: approval-mozilla-b2g34?
There you go ;)
Attachment #8523775 - Flags: approval-mozilla-b2g34?
Attachment #8523775 - Flags: approval-mozilla-b2g34+
Attachment #8523775 - Flags: approval-mozilla-b2g32?
Attachment #8523775 - Flags: approval-mozilla-b2g32+
Attachment #8526898 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.