831644 - 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

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Jason Smith [:jsmith]]

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

Comment 1

[Jason Smith [:jsmith]]

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.

Comment 2

[Julien Wajsberg [:julienw]]

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 ?

Comment 3

[Jason Smith [:jsmith]]

Note - a simple fix to this would be just disable the uninstall buttons while the app is being downloaded.

Comment 4

[Julien Wajsberg [:julienw]]

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.

Comment 5

[Jason Smith [:jsmith]]

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.

Comment 6

[Jason Smith [:jsmith]]

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.

Comment 7

[Julien Wajsberg [:julienw]]

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 ?

Comment 8

[Jason Smith [:jsmith]]

Good point. I'll investigate.

Comment 9

[[:fabrice] Fabrice Desré]

(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.

Comment 10

[Jason Smith [:jsmith]]

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.

Comment 11

[Jason Smith [:jsmith]]

Moving to core per Fabrice's comments.

Comment 12

[Jason Smith [:jsmith]]

Need info on Josh.

How likely could a user make the mistake of uninstalling an app during a download phase of an app?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 14

[Jason Smith [:jsmith]]

(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.

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: patch

Julien, that works in my local tests, but can you double check that this makes gaia happy?

Comment 16

[Julien Wajsberg [:julienw]]

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.

Comment 17

[Julien Wajsberg [:julienw]]

BTW I tried that with my app in http://everlong.org/mozilla/verybigpackaged/

Comment 18

[Julien Wajsberg [:julienw]]

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.

Comment 19

[Jason Smith [:jsmith]]

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?

Comment 20

[Julien Wajsberg [:julienw]]

I really think the uninstall-during-update case could happen in real life.

Comment 21

[[:fabrice] Fabrice Desré]

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.

Comment 22

[Julien Wajsberg [:julienw]]

Yep, should we do try to protect this in Gaia, Gecko, or both ?

Comment 23

[[:fabrice] Fabrice Desré]

Gecko only. I don't think gaia can do much about that.

Comment 24

[Josh Carpenter [:jcarpenter]]

(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.

Comment 25

[[:fabrice] Fabrice Desré]

(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.

Comment 26

[Julien Wajsberg [:julienw]]

Ok, I'll try again today.

I managed to uninstall while installing from the Settings app (in the permissions screen).

Comment 27

[Julien Wajsberg [:julienw]]

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.

Comment 28

[Julien Wajsberg [:julienw]]

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 29

[Julien Wajsberg [:julienw]]

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.

Comment 30

[Julien Wajsberg [:julienw]]

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.

Comment 31

[Julien Wajsberg [:julienw]]

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..

Comment 32

[[:fabrice] Fabrice Desré]

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.

Comment 33

[[:fabrice] Fabrice Desré]

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.

Comment 34

[Julien Wajsberg [:julienw]]

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 35

[Julien Wajsberg [:julienw]]

Comment on attachment 704058 [details] [diff] [review]
patch

clearing the r+, it doesn't work yet

Comment 36

[Julien Wajsberg [:julienw]]

(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 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 704058 [details] [diff] [review]
patch

Removing nomination since it doesn't sound like this is the right fix

Comment 38

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Fixes the hosted+appcache issues. I tested by repeatedly installing/uninstalling the Big hosted app from http://owapps.cloudfoundry.com/

Comment 39

[Julien Wajsberg [:julienw]]

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

Comment 40

[Julien Wajsberg [:julienw]]

Note also that I can't try the hosted+appcache update case because of Bug 834338.

Comment 41

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Now removing the appcache downloads from the download manager properly.

Comment 42

[Julien Wajsberg [:julienw]]

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.

Comment 43

[[:fabrice] Fabrice Desré]

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.

Comment 44

[Johnny Stenback (:jst)]

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.

Comment 45

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4825b19c9941

Comment 46

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/c812891df52f

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4825b19c9941