1104176 - Race condition when uninstalling apps leaves activities registered

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

STR: 

1. Install any app (1) that defines an activity.
2. Install any app (2) that uses that activity and check that when doing new MozActivity(whatever) the .onsuccess is called.
3. Uninstall the (1) app.
4. Try calling the activity again from the (2) app.

EXPECTED:
The activity invocation fails and the onerror handler is invoked.

ACTUAL: 

Sometimes (depending on your luck you can get anything from a 25 to a 100% fail chance) the .onsuccess is called (the (1) application isn't launched since it isn't installed).

CAUSE: 

The unregistering of the activities depends on reading the app manifest, which is launched asynchronously at [1]. And then we erase the directory at [2]. And sometimes the directory is erased before the file is read, and so the activities are kept around. 

[1] http://mxr.mozilla.org/mozilla-b2g32_v2_0/source/dom/apps/src/Webapps.jsm#3577

[2] http://mxr.mozilla.org/mozilla-b2g32_v2_0/source/dom/apps/src/Webapps.jsm#3584

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

[Blocking Requested - why for this release]:

Note that bug 1000315 fixed this in 2.1, so this only happens on 2.0. And the effects for the user are quite unfortunate (from activities that will show not installed apps forever, to in the loop case, the standalone UI directly stopping working since it believes the app to be installed).

Comment 2

[Wesly Huang (EPM)]

Hi Antonio: 

Would you help address more about the scenario that a end user will meet? For example what STR he/she need to perform then meet what problem? (can't install app anymore?)

Per 1000315 seems there is a huge work for fixing it, and it's what we try to avoid for 2.0 branch. Unless we all agreed this is a huge impact to end user.

[Triage] de-nom for now.

Comment 3

[Antonio Manuel Amaya Calvo (:amac)]

Oh, I wasn't proposing to take the patch for bug 1000315. That does a lot more things that fix this (and in fact I think this was fixed as a side effect). The fix for this is much simpler and I should have a path uploaded in a few hours.

The scenario for an end user is:

STR: Install some app that offers and activity and then uninstall it at a later point.

What will happen: If the activity was offered only for that app, any app that uses that activity will fail silently (the user will press share, for example, and nothing will happen). If the activity is offered by more apps, then the dialog to select the concrete app will come up, showing the installed apps *and* the uninstalled one). Actually since the app isn't really installed, I don't know if the dialog will show correctly or fail completely (since at some points it tries to get information for the app, like the name, that won't be present). Reinstalling the app should work (the activity registration will fire an error that AFAIK will be ignored happily by everyone). But uninstalling it again can fail again.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V1. B2G32 only patch

As I said, the fix is quite simple actually. Just wait for the activities to be unregistered before erasing the directory.

I didn't add new tests for this because this is not failing in 2.1 or 2.2, and I don't think it's worth adding tests just for 2.0

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8528272 [details] [diff] [review]
V1. B2G32 only patch

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

::: dom/apps/src/Webapps.jsm
@@ +3580,2 @@
>      if (supportSystemMessages()) {
> +      activitiesUnregistered = this._readManifests([{ id: id }]).then((aResult) => {

nit: s/(aResult)/aResult

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

Attachment: V2. B2G32 only patch, with nits addressed

r=fabrice

Comment 7

[Antonio Manuel Amaya Calvo (:amac)]

Comment on attachment 8530910 [details] [diff] [review]
V2. B2G32 only patch, with nits addressed

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: Randomly, when uninstalling apps that have registered activities, the activities will not be unregistered. That means that everything that uses or try to use that activitie from that point onwards will behave incorrectly (it will assume the uninstalled app is still present). In some cases (like the loop standalone UI) this might break the user experience completely (the standalone UI just stops working, for example).
Testing completed: Manual.
Risk to taking this patch (and alternatives if risky): Pretty low, the change is very small and it just delays the erase of the app until the activity data is read so the activities can be unregistered.
String or UUID changes made by this patch: None


This patch is *only* for B2G32 since that part of the code had a extensive rewrite for 2.1 which incidentally fixed this.

Comment 8

[Maria Angeles Oteo (:oteo)]

[Blocking Requested - why for this release]: Already covered by Antonio in comment 7

Comment 9

[Wesly Huang (EPM)]

Thanks Antonio's explanation in comment#3 and the work here, I support this one is good to fix for 2.0 as well providing the bad user experience user might meet, and the low risk of this fix.

tend to tag in coming 2.0 Triage.

Comment 10

[Wesly Huang (EPM)]

[Triage] tag for 2.0 as comment#9 above.

Comment 11

[Beatriz Rodríguez [:brg]]

Bhavana, Could you please help with the approval to b2g32? Thanks in advance.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/7d0adaf6c447

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/7d0adaf6c447