Uninstalling an app while it's currently being downloading leaves Gaia's homescreen and download progress in a corrupted, out of sync state with the webapps registry

RESOLVED FIXED in Firefox 21

Status

defect
RESOLVED 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:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

7 years ago
Build: B2G 18 1/16/2013
Device: Unagi

Steps:

1. Install an app that requires a decent size download (ex: 20 MB)
2. Uninstall the app while it's being downloaded

Expected:

The download notification should disappear and stop. All references to the app icon information (Settings App Perm UI & Homescreen) should remove the app.

Actual:

We enter into a corrupted, out of sync state with the following characteristics:

* The app continues to download in the download notification with no end in sight
* The app disappears upon uninstall, but reappears not too later - probably when the download would have originally finished. Selecting to launch this app does nothing
* If I restart the phone, it does most of the cleanup of the problem, but not all of it. If I then uninstall app and try to install it again, then the icon is missing from the homescreen showing the app being downloaded
* After a second attempt at doing an uninstall and install again, we now return back to a clean state
Reporter

Comment 1

7 years ago
On one hand, this is bad because the data corruption on Gaia's side takes a lot of twisty turns to get yourself out of it. On another hand, you would have to take the option to uninstall the app while it's being downloaded.

Let's see what triage thinks.
Blocks: app-install
blocking-b2g: --- → tef?
I'd say this could be a platform problem too: when we try to uninstall an app, we should get a proper downloaderror event from the platform before the uninstall event.

But I think we'd have to do stuff in Gaia too, most notably to prevent any race condition (we never know if we'd receive the event in the same order than they're being sent) so we should at least remove the download notification when we receive the 'uninstall' event too.

So, I believe there is work in both part. Fabrice, what are your thoughts on this ?
Flags: needinfo?(fabrice)
Reporter

Comment 3

7 years ago
Note - a simple fix to this would be just disable the uninstall buttons while the app is being downloaded.
Jason, good idea, I like that :)

Then this is Homescreen.

As a side note I personally believe that we should be able to cancel the download from the icon and not only from the notification, but I think this is too late for that and we'll do this post-v1.
Component: Gaia::System → Gaia::Homescreen
Flags: needinfo?(fabrice)
Reporter

Comment 5

7 years ago
Actually, I reproduced this bug by doing this through the settings app, not the homescreen. I couldn't pull it off on the homescreen as I could not click the button.
Component: Gaia::Homescreen → Gaia::Settings
Reporter

Comment 6

7 years ago
So the *simple* fix to this bug would be to disable uninstall app button in the settings apps perm UI when the app is being downloaded.
I didn't even know that we could do that in Settings ;)

Jason, could you see if this is broken when we try to uninstall during an update too ?
Reporter

Comment 8

7 years ago
Good point. I'll investigate.
Keywords: qawanted
Reporter

Updated

7 years ago
QA Contact: jsmith
Assignee

Comment 9

7 years ago
(In reply to Julien Wajsberg [:julienw] from comment #2)
> I'd say this could be a platform problem too: when we try to uninstall an
> app, we should get a proper downloaderror event from the platform before the
> uninstall event.
> 
> But I think we'd have to do stuff in Gaia too, most notably to prevent any
> race condition (we never know if we'd receive the event in the same order
> than they're being sent) so we should at least remove the download
> notification when we receive the 'uninstall' event too.

I don't see why you would receive events out of order, since event delivery is synchronous.

> So, I believe there is work in both part. Fabrice, what are your thoughts on
> this ?

I'm not comfortable with just a gaia workaround. We must fix the gecko side.
Reporter

Comment 10

7 years ago
Tried uninstalling during updates. One word - Yikes. If you try to uninstall a packaged app while it's downloading an update, the app icon and download notification disappears, but the downloading icon and network activity still appears evident. This implies that we are *still* downloading resources even though the app has been uninstalled. This means for scenarios where say, I'm updating from a 17 KB --> 20 MB app, we're making unwarranted network connections that the user has very little control over (they'll have to kill the network connection or power off the phone).

Unlike the install scenario also - the update scenario is far easier to make the mistake of doing, as you can easily do this from the homescreen.

I'm arguing to block here.
Reporter

Updated

7 years ago
Keywords: qawanted
Reporter

Updated

7 years ago
Component: Gaia::Settings → DOM: Apps
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Reporter

Comment 11

7 years ago
Moving to core per Fabrice's comments.
Reporter

Comment 12

7 years ago
Need info on Josh.

How likely could a user make the mistake of uninstalling an app during a download phase of an app?
Flags: needinfo?(jcarpenter)
We're going to try not blocking on this, but we'd like UX information on how likely it is that someone will try to stop network traffic by uninstalling an app rather than going to the notification center and stopping the download there.
Flags: needinfo?(jcarpenter)
Reporter

Comment 14

7 years ago
(In reply to Jonas Sicking (:sicking) from comment #13)
> We're going to try not blocking on this, but we'd like UX information on how
> likely it is that someone will try to stop network traffic by uninstalling
> an app rather than going to the notification center and stopping the
> download there.

I still question and don't understand why we're even making a likelihood argument here for not blocking on this if we're choosing to block on the multiple apps per origin error issue - they carry the same severity. Both enter a same level of out of sync issue and carry the same level of work-around to get out of it. This has a higher level of likelihood than the multiple apps per origin error issue.
Flags: needinfo?(jcarpenter)
Assignee

Comment 15

7 years ago
Posted patch patch (obsolete) — Splinter Review
Julien, that works in my local tests, but can you double check that this makes gaia happy?
Assignee: nobody → fabrice
Attachment #704058 - Flags: review?(felash)
Comment on attachment 704058 [details] [diff] [review]
patch

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

I tried uninstalling an app from the Settings app while it was installing, and it was bad :)

+ the download was canceled because the network download indicator was not there anymore
- the icon was still there on the homescreen (but not in "downloading" mode so it means that it might have got the downloaderror event)
- the download notification + icon was still there in the System app, so here it seems we didn't get the downloaderror
- rebooting the phone, it doesn't work anymore, it stays on the "mozilla" black screen. Here are the logs :


E/GeckoConsole(  509): [JavaScript Error: "DOMApplicationRegistry: Could not read from /data/local/webapps/{82ca0012-8838-4124-aba5-5741da79e1ca}/manifest.json : [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIChannel.asyncOpen]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/NetUtil.jsm :: NetUtil_asyncOpen :: line 165"  data: no]
E/GeckoConsole(  509): [JavaScript Error: "DOMApplicationRegistry: Could not parse JSON: /data/local/webapps/marketplace/manifest.webapp TypeError: manifest is null
E/GeckoConsole(  509): [JavaScript Error: "manifest is null" {file: "resource://gre/modules/Webapps.jsm" line: 595}]
E/GeckoConsole(  509): [JavaScript Error: "TypeError: manifest is null" {file: "app://system.gaiamobile.org/shared/js/manifest_helper.js" line: 12}]

I believe we should check of the manifest is null in Gaia but still something was corrupted in Gecko.
Attachment #704058 - Flags: review?(felash) → review-
Comment on attachment 704058 [details] [diff] [review]
patch

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

::: dom/apps/src/Webapps.jsm
@@ +2256,5 @@
>          return;
>  
> +      // Check if we are downloading something for this app, and cancel the
> +      // download if needed.
> +      this.cancelDownload(app.manifestURL);

also, it seems I always get the error even when it's not downloading.
Reporter

Comment 19

7 years ago
Just so I get more perspectives before I go into triage again:

Julien and/or Fabrice, where do you stand on blocking vs. tracking vs. not tracking this bug?
I really think the uninstall-during-update case could happen in real life.
Assignee

Comment 21

7 years ago
I really think this should block.

The str the registry are interesting. I'll update this patch to fix that, but I also think we should just protect ourselves at startup and evict bogus entries from the DOM registry to not end up in the infamous black screen.
Yep, should we do try to protect this in Gaia, Gecko, or both ?
Assignee

Comment 23

7 years ago
Gecko only. I don't think gaia can do much about that.
(In reply to Jason Smith [:jsmith] from comment #3)
> Note - a simple fix to this would be just disable the uninstall buttons
> while the app is being downloaded.

I agree and am fine with this from a UX perspective, so long as the user retains their ability to cancel an update/download from the Notification Center. This strikes me as an acceptable quick-n-clean solution for v1. Ideally the disabled 
"Uninstall app" button from the App Permissions page would be accompanied by a explanatory message along the lines of  "This app is currently downloading a new version. The download must complete or be manually stopped before this app can be deleted."

(In reply to Jason Smith [:jsmith] from comment #12)
> Need info on Josh.
> 
> How likely could a user make the mistake of uninstalling an app during a
> download phase of an app?

We know users in our target markets are highly sensitive to the costs associated with bandwidth consumption, and it's reasonable to assume that some may try to "turn off the spigot" by uninstalling an app from the Home before it can finish downloading. But I also suspect that any user savvy enough to think of that approach will also know that they can cancel a download (for either an update or install) from the Notification Center. So I'd say we're fine preventing uninstall during update/install.
Flags: needinfo?(jcarpenter)
Assignee

Comment 25

7 years ago
(In reply to Julien Wajsberg [:julienw] from comment #17)
> BTW I tried that with my app in http://everlong.org/mozilla/verybigpackaged/

I can't reproduce your results with this page or any other. When doing the first download during install, the "uninstall" button in the settings app doesn't do anything, and I could not uninstall from the homescreen either. I updated gaia to 563a8beb3b9b7772a7e11bf076a3dbc16e7b034b.
Ok, I'll try again today.

I managed to uninstall while installing from the Settings app (in the permissions screen).
Flags: needinfo?(felash)
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Ok, this seems to basically work (I don't know why it wasn't yesterday) but I run in strange situations (maybe unrelated) where the download doesn't start, or that I can't uninstall from the Settings app.

I'd like to test a little more.
Flags: needinfo?(felash)
For packaged app, everything seems to be ok.

For hosted+appcache app, for the "uninstall while installing", I get 2 errors :

E/GeckoConsole(  563): Content JS INFO at app://system.gaiamobile.org/js/app_install_manager.js:188 in ai_handleDownloadError: downloadError event, error code is APP_CACHE_DOWNLOAD_ERROR
E/GeckoConsole(  563): Content JS INFO at app://system.gaiamobile.org/js/app_install_manager.js:188 in ai_handleDownloadError: downloadError event, error code is DOWNLOAD_CANCELED

Everything seems correct otherwise though. However I wonder if we could not have Bug 830294 for hosted+appcache apps too...

I couldn't try the "uninstall while updating" for an hosted+appcache app because... it doesn't update the files even if the appcache is changed. Houra another appcache bug :)
Comment on attachment 704058 [details] [diff] [review]
patch

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

r=me.

Will file new bugs for the other issues I ran into.

::: dom/apps/src/Webapps.jsm
@@ +2256,5 @@
>          return;
>  
> +      // Check if we are downloading something for this app, and cancel the
> +      // download if needed.
> +      this.cancelDownload(app.manifestURL);

Forget about this, I think I was in a strange state.
Attachment #704058 - Flags: review- → review+
I just got a bad behavior when uninstalling while installing an app that I uninstalled before already.

The bad behaviour that the icon was staying in the homescreen. I'm quite sure the problem is in Homescreen and not in Gecko so let's land this and handle the other problems in other bugs.
Ah, the bad behaviour also left the phone in a bricked state, the same one as in my comment 16. So I was not crazy...

Will try to reproduce, I'm still quite sure it's not because of your patch but..
Assignee

Comment 32

7 years ago
Comment on attachment 704058 [details] [diff] [review]
patch

[Approval Request Comment]
This is part of a set of patches that helps hardening against edge cases that may let the phone in a bad state. Risk is low compared to the reward.
Assignee

Comment 33

7 years ago
Comment on attachment 704058 [details] [diff] [review]
patch

[Approval Request Comment]
This is part of a set of patches that helps hardening against edge cases that may let the phone in a bad state. Risk is low compared to the reward.
Attachment #704058 - Flags: approval-mozilla-b2g18?
Fabrice, I think the patch doesn't work correctly for hosted+appcache apps after all. This may be because of the "double error" error.

Sometimes (but not all the time) the notification is left in System's utility tray; sometimes, an icon is left on the HomeScreen (not necessarily the same times). After a reboot it is always fixed.

Also after trying this a few times, I get each of both errors multiple times in the logcat. I can see also JS errors in system because he tries to remove the notification twice, which is consistent with the previous logs.

So I think you need to implement the same think as in Bug 830294 but for hosted+appcache apps first.
Comment on attachment 704058 [details] [diff] [review]
patch

clearing the r+, it doesn't work yet
Attachment #704058 - Flags: review+
(In reply to Julien Wajsberg [:julienw] from comment #34)

> So I think you need to implement the same think as in Bug 830294 but for
> hosted+appcache apps first.

That would fix the "double error" error (we get both DOWNLOAD_CANCELED and APP_CACHE_DOWNLOAD_ERROR errors), but maybe not the "we get the error multiple times" thing which might be something else.
Comment on attachment 704058 [details] [diff] [review]
patch

Removing nomination since it doesn't sound like this is the right fix
Attachment #704058 - Flags: approval-mozilla-b2g18?
Assignee

Comment 38

7 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Fixes the hosted+appcache issues. I tested by repeatedly installing/uninstalling the Big hosted app from http://owapps.cloudfoundry.com/
Attachment #704058 - Attachment is obsolete: true
Attachment #705970 - Flags: review?(felash)
This seems to be way better.

I still see a small thing with the following STR:
* install a hosted+appcache app 
* wait for a complete install
* uninstall it

=> we get the download_canceled notification

Related adb logcat :

I/Gecko   (  460): -*-*- Webapps.jsm : uninstall http://everlong.org
I/Gecko   (  460): -*-*- Webapps.jsm : cancelDownload http://everlong.org/mozilla/selfupdatinghosted/manifest.webapp
I/Gecko   (  460): -*-*- Webapps.jsm : [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIOfflineCacheUpdate.cancel]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/Webapps.jsm :: cancelDownload :: line 882"  data: no]
I/Gecko   (  460): -*-*- Webapps.jsm : Saving /data/local/webapps/webapps.json
I/Gecko   (  460): -*-*- Webapps.jsm : Saving /data/local/webapps/webapps.json
E/GeckoConsole(  460): Content JS INFO at app://system.gaiamobile.org/js/app_install_manager.js:188 in ai_handleDownloadError: downloadError event, error code is DOWNLOAD_CANCELED


Interestingly we don't get this for packaged app :

I/Gecko   (  460): -*-*- Webapps.jsm : uninstall app://{16d73d9f-b185-418a-bf8d-e2c28dbd8e14}
I/Gecko   (  460): -*-*- Webapps.jsm : cancelDownload http://everlong.org/mozilla/bigpackaged/manifest.webapp
I/Gecko   (  460): -*-*- Webapps.jsm : Could not find a download for http://everlong.org/mozilla/bigpackaged/manifest.webapp
I/Gecko   (  460): -*-*- Webapps.jsm : Saving /data/local/webapps/webapps.json
Note also that I can't try the hosted+appcache update case because of Bug 834338.
Assignee

Comment 41

7 years ago
Posted patch patch v3Splinter Review
Now removing the appcache downloads from the download manager properly.
Attachment #705970 - Attachment is obsolete: true
Attachment #705970 - Flags: review?(felash)
Attachment #705991 - Flags: review?(felash)
Comment on attachment 705991 [details] [diff] [review]
patch v3

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

r=me

we should not forget to test the appcache update case when Bug 834338 is fixed.
Attachment #705991 - Flags: review?(felash) → review+
Assignee

Comment 43

7 years ago
Comment on attachment 705991 [details] [diff] [review]
patch v3

[Approval Request Comment]
This is part of a set of patches that helps hardening against edge cases that may let the phone in a bad state. Risk is low compared to the reward.
Attachment #705991 - Flags: approval-mozilla-b2g18?
Comment on attachment 705991 [details] [diff] [review]
patch v3

a=jst, low risk, fixes obvious bustage in cases that people will no doubt run into.
Attachment #705991 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Assignee

Comment 46

7 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c812891df52f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

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