839068 - app install manager doesn't handle the uninstall of an app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* install an app
* stop the download
* uninstall the app

=> the app install manager still has handles on the app

This means that if we install the same app again, completely this time, we might get messages both from the app install manager and the update manager for the same app.

This is not blocking but I nom for tracking as this is easy to do.

Comment 1

[Julien Wajsberg [:julienw]]

another strange STR because of this :
* install an app
* uninstall it
* reinstall it
* cancel the download before the end of install

=> the download icons are not removed.
I've seen also a case where there were 2 progress bar (maybe because we receive 2 progress events ?), and one of these stays after the cancel (see screenshot).

Again, not a big deal, not something users will do often (even if some of our limitations force them to do that sometimes), and fixed by a reboot, but still this is easy to fix.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: screenshot

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Apply with |git am|

Correctly handle the uninstall event in the app install manager. Remove the
notification and the download icon if they're displayed too.
---
 apps/system/js/app_install_manager.js             |    9 +++
 apps/system/test/unit/app_install_manager_test.js |   73 +++++++++++++++------
 2 files changed, 62 insertions(+), 20 deletions(-)

The "normal" users won't see this problem much but we trigger this a lot when we're testing.

Comment 4

[Etienne Segonzac (:etienne)]

Comment on attachment 712254 [details] [diff] [review]
patch v1

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

::: apps/system/test/unit/app_install_manager_test.js
@@ +462,5 @@
>      }
>  
> +    var dispatchInstallEvent = dispatchEvent.bind(null, 'applicationinstall'),
> +        dispatchUninstallEvent = dispatchEvent.bind(null,
> +                                                    'applicationuninstall');

Not blocking but |dispatchUninstallEvent()| should do something like

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/update_manager_test.js#L311-314

to reflect the fact that we're not getting a full-fledged app object here.

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 712254 [details] [diff] [review]
patch v1

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

::: apps/system/test/unit/app_install_manager_test.js
@@ +462,5 @@
>      }
>  
> +    var dispatchInstallEvent = dispatchEvent.bind(null, 'applicationinstall'),
> +        dispatchUninstallEvent = dispatchEvent.bind(null,
> +                                                    'applicationuninstall');

ah right, completely forgot this. Will change the test to better reflect this.

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Etienne, is it better like that ?

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 712447 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a

User impact if declined: little impact normally _but_ there are some use cases where we said "bah the user can uninstall and reinstall the app". Jason will know which ones.
Also, this has an impact when we're testing because we're doing this a lot, and so we may see strange bugs because of that.

Testing completed: yes

Risk to taking this patch (and alternatives if risky): very low as we're not changing the logic at all on existing code paths, but merely filling a void. Also this is one of the most unit-tested part of Gaia.

Comment 8

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/9157646edfa2065d098c3c0bd4b5cf270a84c9fa

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 712447 [details] [diff] [review]
patch v2

given that this is a well-tested section in the code and it helps with the uninstalling process, we'll take the uplift.  let's get QA to verify that this is clearing up the issues found in known apps - will put Jason on the QA contact for this.

Comment 10

[Jason Smith [:jsmith]]

Only need verifyme in this case if it's already landed.

Comment 11

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 86c89f5eee72f3c3c7a19c5d22f75e82ef0b8b4f

Comment 12

[Julien Wajsberg [:julienw]]

John, I believe the v1.0.0 flag was incorrectly set to "fixed" ?

Comment 13

[John Ford [:jhford] CET/CEST Berlin Time]

yes, apologies.

Comment 14

[Jason Smith [:jsmith]]

Didn't really see any regressions with this patch, although there could potentially be an issue I'm seeing with the progress bar rendering incorrectly (size is known, but progress bar shows undetermined timeframe for download). Working on getting a STR on this.

Comment 15

[Jason Smith [:jsmith]]

Filed bug 844219 about the issue I found in comment 14.