1019054 - Uninstalling webapps doesn't seem to work properly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Device: Galaxy Q running Android 2.3 and an up-to-date nightly.

Steps: (I did this a few days ago, my memory might not be 100% spot on)
1. Go to Firefox marketplace and install the Loqui IM app. It installs fine as a regular android app
2. Run the Loqui app and try using it. I forget exactly what the problem there was but I realized it wasn't what I wanted, so I wanted to uninstall it
3. Go to the android settings, into applications, and uninstall Loqui. Everything seems fine at this point

At some point between then and now I got a notification in my Android notification tray saying "1 update downloaded \n Touch to install update for Loqui IM". Tapping on this notification doesn't do anything (i.e. the notification tray hides but nothing else happens; the notification itself remains there). I can clear the notification by using the Android notification tray "Clear" button but after some time the notification will re-appear.

If I go into Fennec's "Apps" page I see the app listed there, but there doesn't seem to be any way to remove it. Long-pressing on the app just invokes text selection mode.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Also, if I remember correctly, I think after I uninstalled Loqui the first thing that happened was the Firefox notified me it was downloading an update for the app. I don't think it gave me any option to cancel it or anything. It was probably only after that the the "update downloaded" notification started showing up.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

There seem to be circumstances under which it's possible to uninstall the native APK for a webapp without triggering removal of the webapp's record from DOMApplicationRegistry.  We need to fix whatever issues are causing that, but in the meantime (and probably even afterward, regardless of these bugs) I've come to think that we should reenable uninstallation directly from about:apps via the context menu for that page, which we disabled but have considered re-enabling in bug 965176.

And when you uninstall directly from about:apps, it should both uninstall the native package and remove the DOMApplicationRegistry record, so it's always possible to remove an app from about:apps, regardless of bugs that cause the native registry and DOMApplicationRegistry to get out of sync.

Thus my strategy here is to re-enable the menu (which was ifdefed off but never removed from the codebase) and then re-implement its uninstall item.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

This work in progress is just about ready, but there's one issue I still need to button up, this pair of exceptions when autouninstalling an app whose APK was uninstalled while Fennec wasn't running:

  8689     GeckoWebappManager  D  attempting to uninstall KitchenSink
  8689           GeckoWebapps  D  uninstall https://kitchensinkmyk.apk.cli.firefox.com/manifest.webapp
  8689           GeckoWebapps  D  cancelDownload https://kitchensinkmyk.apk.cli.firefox.com/manifest.webapp
  8689           GeckoWebapps  D  Could not find a download for https://kitchensinkmyk.apk.cli.firefox.com/manifest.webapp
  8689           GeckoWebapps  D  Saving /data/data/org.mozilla.fennec_myk/files/mozilla/webapps/webapps.json
…
  8689                  Gecko  I  *************************
  8689                  Gecko  I  A coding exception was thrown in a Promise resolution callback.
  8689                  Gecko  I  Full message: TypeError: app is undefined
  8689                  Gecko  I  See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
  8689                  Gecko  I  Full stack: this.DOMApplicationRegistry.updateDataStoreForApp/<@resource://gre/modules/Webapps.jsm:333:7
  8689                  Gecko  I  Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:863:11
  8689                  Gecko  I  this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:7
  8689                  Gecko  I  *************************
  8689                  Gecko  I  *************************
  8689                  Gecko  I  A coding exception was thrown in a Promise resolution callback.
  8689                  Gecko  I  Full message: TypeError: app is undefined
  8689                  Gecko  I  See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
  8689                  Gecko  I  Full stack: this.DOMApplicationRegistry.registerAppsHandlers/</<@resource://gre/modules/Webapps.jsm:312:11
  8689                  Gecko  I  this.DOMApplicationRegistry.registerAppsHandlers/<@resource://gre/modules/Webapps.jsm:304:9
  8689                  Gecko  I  Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:863:11
  8689                  Gecko  I  this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:742:7
  8689                  Gecko  I  *************************


I'd also like to run some more tests before I'm satisfied that I've exercised the changes enough.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: reenable/reimplement app uninstallation from about:apps

(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
> This work in progress is just about ready, but there's one issue I still
> need to button up, this pair of exceptions when autouninstalling an app
> whose APK was uninstalled while Fennec wasn't running:

This turned out to be a pair of discrete but identical bugs in DOMApplicationRegistry initialization, which didn't wait for asynchronous work in updateDataStoreForApp and registerAppsHandlers to complete before calling notifyAppsRegistryReady to resolve registryReady.

Since uninstallation from the registry happens on startup for apps whose APKs were uninstalled while Fennec wasn't running, that resulted in WebappManager.autoUninstall racing updateDataStoreForApp and registerAppsHandlers, since it would observe registryReady and then uninstall the app before updateDataStoreForApp and registerAppsHandlers tried to access it.

In the end, the fixes were simple: turn the asynchronous functions into tasks and yield until the asynchronous work is complete.


With fixes for those bugs (which are actually unrelated issues, I just happened to notice them while implementing this enhancement), the patch now behaves as expected: the user can uninstall an app from about:apps by long-pressing the app icon, whether or not there's an APK for the app installed on the device; and if the user uninstalls an APK while Fennec isn't running, then Fennec will remove it from the registry on startup without triggering DOMApplicationRegistry exceptions.


Note that the code in WebappManager.uninstall is incomplete, as it doesn't keep track of the message manager and notify it about the result of the request.  Since about:apps is WebappManager.uninstall's only consumer, and it doesn't care about the result, that omission doesn't affect this patch; but I added a TODO about implementing it and will file a followup bug to do so.


Requesting review from mfinkle for the mobile/android/ changes and Marco for the dom/apps/ changes.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8437312 [details] [diff] [review]
patch v1: reenable/reimplement app uninstallation from about:apps

Android parts look good. Bonus points for Monopoly-inspired comments.

Comment 6

[Marco Castelluccio [:marco]]

Comment on attachment 8437312 [details] [diff] [review]
patch v1: reenable/reimplement app uninstallation from about:apps

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

r+ with the comment about doUninstall addressed.

::: dom/apps/src/Webapps.jsm
@@ +327,5 @@
> +          return;
> +        }
> +        let app = this.webapps[aResult.id];
> +        app.csp = aResult.manifest.csp || "";
> +        app.role = aResult.manifest.role || "";

This is another place where we're modifying the registry silently (like we
did in _processManifestForIds).
I'm not sure why we're doing this, we should probably file a bug and ask
Fabrice.

::: mobile/android/modules/WebappManager.jsm
@@ +234,5 @@
> +        apkPackageName: app.apkPackageName,
> +      });
> +
> +      // We don't need to call DOMApplicationRegistry.doUninstall at this point,
> +      // because the APK uninstall listener will do it once the APK

If you're calling doUninstall once the APK is uninstalled...

@@ +239,5 @@
> +      // is uninstalled; and if the user cancels the APK uninstallation, then we
> +      // shouldn't remove the app from the registry anyway.
> +
> +      // But we should tell the requesting document the result of their request.
> +      // TODO: tell the requesting document if uninstallation succeeds or fails.

...you shouldn't need to send the result of the request here, as doUninstall
will do it.

Am I missing something?

Anyway I think we should send the reply when the application is actually
uninstalled, so when the APK is uninstalled.

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: make the comment accurate

(In reply to Marco Castelluccio [:marco] from comment #6)
> This is another place where we're modifying the registry silently (like we
> did in _processManifestForIds).
> I'm not sure why we're doing this, we should probably file a bug and ask
> Fabrice.

Indeed!  I filed this as bug 1023569 and requested info from Fabrice.


> ::: mobile/android/modules/WebappManager.jsm
> @@ +234,5 @@
> > +        apkPackageName: app.apkPackageName,
> > +      });
> > +
> > +      // We don't need to call DOMApplicationRegistry.doUninstall at this point,
> > +      // because the APK uninstall listener will do it once the APK
> 
> If you're calling doUninstall once the APK is uninstalled...
> 
> @@ +239,5 @@
> > +      // is uninstalled; and if the user cancels the APK uninstallation, then we
> > +      // shouldn't remove the app from the registry anyway.
> > +
> > +      // But we should tell the requesting document the result of their request.
> > +      // TODO: tell the requesting document if uninstallation succeeds or fails.
> 
> ...you shouldn't need to send the result of the request here, as doUninstall
> will do it.
> 
> Am I missing something?

Erm, actually the comment is inaccurate, as the APK uninstall listener doesn't actually call DOMApplicationRegistry.doUninstall.  Instead, it calls DOMApplicationRegistry.uninstall directly; which doesn't take a message/manager pair, just a manifest URL and success/failure handlers:

https://github.com/mozilla/gecko-dev/blob/db078bd435b42d687e0bcbee9c9c30c01294cdec/mobile/android/modules/WebappManager.jsm#L604-L612

To make it actually notify the requesting document, WebappManager.uninstall will need to store weak references to the message/manager pair associated with the request.  Then, when UninstallListener observes APK uninstallation and broadcasts Webapps:Autouninstall, which triggers a call to WebappManager.autoUninstall, that function will need to call DOMApplicationRegistry.doUninstall with the message/manager pair, if they're still defined at that point.

I've updated the comment to be accurate and describe what needs to be done (more or less; actual details will be worked out during implementation!).  It now reads:

      // We don't need to call DOMApplicationRegistry.doUninstall at this point,
      // because the APK uninstall listener will call autoUninstall once the APK
      // is uninstalled; and if the user cancels the APK uninstallation, then we
      // shouldn't remove the app from the registry anyway.

      // But we should tell the requesting document the result of their request.
      // TODO: tell the requesting document if uninstallation succeeds or fails
      // by storing weak references to the message/manager pair here and then
      // using them in autoUninstall if they're still defined when it's called;
      // and make EventListener.uninstallApk return an error when APK uninstall
      // fails (which it should be able to detect reliably on Android 4+),
      // which we observe here and use to notify the requester of failure.

I also filed bug 1023574 to implement this.


> Anyway I think we should send the reply when the application is actually
> uninstalled, so when the APK is uninstalled.

Erm, to make sure I understand: do you mean after the APK is uninstalled and also after the app is removed from the registry (which doesn't happen until after APK uninstallation); or do you mean after the APK is uninstalled but *before* the app is removed from the registry?

I'm guessing the former, in which case I agree!

Here's the version I'll commit.

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> Indeed!  I filed this as bug 1023569 and requested info from Fabrice.

Thanks!

> Erm, actually the comment is inaccurate, as the APK uninstall listener
> doesn't actually call DOMApplicationRegistry.doUninstall.  Instead, it calls
> DOMApplicationRegistry.uninstall directly; which doesn't take a
> message/manager pair, just a manifest URL and success/failure handlers:

Thank you for the clarification ;)

> > Anyway I think we should send the reply when the application is actually
> > uninstalled, so when the APK is uninstalled.
> 
> Erm, to make sure I understand: do you mean after the APK is uninstalled and
> also after the app is removed from the registry (which doesn't happen until
> after APK uninstallation); or do you mean after the APK is uninstalled but
> *before* the app is removed from the registry?
> 
> I'm guessing the former, in which case I agree!

Your guess is correct!

With the old comment "TODO: tell the requesting document ..." I thought you meant you wanted to send the reply exactly there (that is, just replacing the comment with the code), thus not waiting for the APK uninstallation to be finished. I now realize what you meant ;)
If you want you could write a more concise comment "TODO: tell the requesting document ... once the APK has been unininstalled".

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: resolves conflicts with bug 1021342

Erm, or rather here's the version I'll commit.  It resolves conflicts with bug 1021342, which raced this bug to fx-team.  It also removes the aboutApps.addToHomescreen string, which these two bugs make obsolete (since that bug removed the context menu entirely, and this bug re-adds it only for the "uninstall" item).  And it corrects the humorous comment to reflect that it's *uninstallation* that we're skipping in this case.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: remove last vestiges from merge conflict

The third (fourth?) time's the charm!  Upon further review and testing, I discovered two remaining vestiges of the old code that I missed when resolving the merge conflicts for the last patch.  This update removes those vestiges.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(Landed on fx-team with one additional merge conflict resolved, as bug 1019613 landed on fx-team since the last time I pulled upstream a few hours ago, and it introduced a trivial conflict in aboutApps.js.)

https://hg.mozilla.org/integration/fx-team/rev/207e26dcf2ad

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/207e26dcf2ad