Closed Bug 1104176 Opened 10 years ago Closed 10 years ago

Race condition when uninstalling apps leaves activities registered


(Core Graveyard :: DOM: Apps, defect)

32 Branch
Windows 7
Not set


(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 unaffected, b2g-v2.2 unaffected)

2.2 S3 (9jan)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- unaffected
b2g-v2.2 --- unaffected


(Reporter: amac, Assigned: amac)




(1 file, 1 obsolete file)


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.

The activity invocation fails and the onerror handler is invoked.


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


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. 


[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).
blocking-b2g: --- → 2.0?
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.
blocking-b2g: 2.0? → ---
Flags: needinfo?(amac.bug)
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.
Flags: needinfo?(amac.bug) → needinfo?(wehuang)
Attached patch V1. B2G32 only patch (obsolete) — Splinter Review
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
Attachment #8528272 - Flags: review?(fabrice)
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
Attachment #8528272 - Flags: review?(fabrice) → review+
Attachment #8528272 - Attachment is obsolete: true
Attachment #8530910 - Flags: review+
Comment on attachment 8530910 [details] [diff] [review]
V2. B2G32 only patch, with nits addressed

NOTE: Please see 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.
Attachment #8530910 - Flags: approval-mozilla-b2g32?
[Blocking Requested - why for this release]: Already covered by Antonio in comment 7
blocking-b2g: --- → 2.0?
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.
Flags: needinfo?(wehuang)
[Triage] tag for 2.0 as comment#9 above.
blocking-b2g: 2.0? → 2.0+
Bhavana, Could you please help with the approval to b2g32? Thanks in advance.
Flags: needinfo?(bbajaj)
Attachment #8530910 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Flags: needinfo?(bbajaj)
Keywords: checkin-needed
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
Blocks: Woodduck
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.