uninstall synthetic APK when web page with privileges calls mozApps.mgmt.uninstall()

RESOLVED WONTFIX

Status

()

Firefox for Android
Web Apps
P1
normal
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: myk, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRuntime])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
When a web page with privileges calls navigator.mozApps.mgmt.uninstall() for an app, Fennec should uninstall the synthetic APK associated with the app.
We can use the PackageInstaller via an intent. 

Here is a reasonable Q&A on Stackoverflow. http://stackoverflow.com/questions/6813322/install-uninstall-apks-programmatically-packagemanager-vs-intents
Assignee: nobody → jhugman
(Reporter)

Updated

4 years ago
Priority: -- → P1
Created attachment 8365976 [details] [diff] [review]
WIP Patch

APKs are being uninstalled, and autouninstall (untouched in this patch) should remove the app from the web-app registry.

We're now trying to get the request callbacks to be called back.

I'm proposing we do this in response to the user clicking uninstall in the package manager, rather than repurposing the current UninstallListener. 

The choices are:
 1. give data to the always registered uninstall listener.
 2. send the request once the package manager returns.
 3. writing a smaller shortlived uninstall listener which only handles a single apk uninstall. This is how we do it in autoinstall.
Attachment #8365976 - Flags: feedback?(myk)
Attachment #8365976 - Flags: feedback?(mark.finkle)
(Reporter)

Comment 3

4 years ago
Comment on attachment 8365976 [details] [diff] [review]
WIP Patch

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

Overall looks good.  Just a few comments, mostly about stuff that has changed very recently on trunk (code moves quickly! until it stops moving, that is).

::: dom/apps/src/Webapps.jsm
@@ +3390,5 @@
>    },
>  
> +#ifdef MOZ_ANDROID_SYNTHAPKS
> +  doUninstallApk: function (aData, aMm) {
> +    let app = this.getAppByManifestURL(aData.manifestURL);

I would do the same thing we do for APK installation and put the entirety of the implementation into WebappManager.jsm, such that the only thing Webapps.jsm does is notify observers about webapps-uninstall-apk.

That reduces churn in the complex and fragile Webapps.jsm and isolates runtime-specific functionality in the runtime-specific WebappManager.jsm.  Nor does it prevent the implementation from doing anything it needs to do, like calling getAppByManifestURL, which is a public method, or calling aMm.sendAsyncMessage if an error occurs, since we can include aMm in the notification (cf. the new webapps-runtime-install and webapps-runtime-install-package notifications, which pass aMm via the "subject" parameter).

@@ +3398,5 @@
> +    }
> +
> +    dump("Uninstalling: " + JSON.stringify(app));
> +    aData.origin = app.origin;
> +    Services.obs.notifyObservers(null, "webapps-uninstall-apk", JSON.stringify(aData));

Let's call this webapps-runtime-uninstall for consistency with the new webapps-runtime-install and webapps-runtime-install-package notifications.

::: mobile/android/base/GeckoApp.java
@@ +671,2 @@
>                  GeckoAppShell.uninstallWebApp(message.getString("origin"));
> +                }

Most of these event handlers live in webapp/EventListener.java now, and this one should move there.  (The only exceptions are handlers that have to set mCurrentResponse.)

Also, this implementation is very simple (four short lines!), but I'd still create a method for it and have handleMessage call that method instead of putting the implementation into handleMessage itself.  That separates concerns consistently such that handleMessage is always responsible just for calling the event-specific handler (expanding message properties as needed to pass to the handler), while each event has a corresponding handler that implements the relevant function.

::: mobile/android/modules/WebappManager.jsm
@@ +198,5 @@
> +  },
> +
> +  postUninstall: function (aData) {
> +
> +  },

Do we need this?
Attachment #8365976 - Flags: feedback?(myk) → feedback+
Created attachment 8367650 [details] [diff] [review]
bug-957065-v2.patch

Addressed feedback in previous patch.
Attachment #8365976 - Attachment is obsolete: true
Attachment #8365976 - Flags: feedback?(mark.finkle)
Attachment #8367650 - Flags: feedback?(myk)
Attachment #8367650 - Flags: feedback?(mark.finkle)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8367650 [details] [diff] [review]
bug-957065-v2.patch

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

::: dom/apps/src/Webapps.jsm
@@ +1147,2 @@
>          this.doUninstall(msg, mm);
> +#endif

Looks good to me, but we need Fabrice to review this change.

@@ +2022,4 @@
>                       ": " + aError);
>      }.bind(this);
>  
> +    if (app.receipts && app.receipts.length > 0) {

This (and the identical change to doInstallPackage) is a fix for unrelated bug 964877, and my alternative is already inbound <https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7f622775b9>, so I would avoid fixing it here as well.

::: mobile/android/base/webapp/EventListener.java
@@ -14,3 @@
>  import org.mozilla.gecko.util.GeckoEventListener;
>  import org.mozilla.gecko.util.ThreadUtils;
> -import org.mozilla.gecko.WebAppAllocator;

Why stop importing this class?  It makes the references to it much longer and doesn't seem to provide any commensurate benefits.

@@ +98,3 @@
>                  uninstallWebApp(message.getString("origin"));
>              }
> +            }

Nit: indent the uninstallWebApp call (and the closing brace on the next line) one more level.

@@ +114,5 @@
> +            public void onActivityResult(int resultCode, Intent data) {
> +                if (resultCode == Activity.RESULT_OK) {
> +                    Log.i(LOGTAG, "Uninstall agreed by user");
> +                } else {
> +                    Log.i(LOGTAG, "Uninstall canceled by user");

Nit: it'd be nice for these messages to include the name of the app in question.

::: mobile/android/modules/WebappManager.jsm
@@ +145,5 @@
>      });
>    },
>  
> +  /**
> +   * Auto install is always initialized by Java

Nit: initialized -> initiated ?

@@ +150,5 @@
> +   * It is the second part of the install process that is started by
> +   * the installation of the Android APK, or at worse, the first time the app is run.
> +   * In the case of autoInstall in response to APK install, Java will keep track of the data
> +   * passed to it from the original mozApps.install() call.
> +   */

Nit: although code line widths can and should exceed 80 characters in Fennec when appropriate, it's still useful to keep comment widths down, as it's marginally easier to read narrower columns of text.

Also, I was confused by the term "Android APK", which isn't how we typically describe the APKs that install webapps.  I would use a term like "webapp APK" or simply "APK" here.

Finally, I understand what this is saying, because I'm intimately familiar with this implementation, but I'm not sure the next hacker to look at this code will feel the same.  So I would describe the two different install paths with additional verbiage, something like:

  /**
   * Auto install is always initiated by Java.
   * It is the second part of the install process that is started by
   * the installation of the webapp APK, either from a call to mozApps.install()
   * or via some install process that happens independently of Fennec
   * (f.e. via ADB or installation from an APK store like Google Play).
   *
   * When an APK is installed from mozApps.install(), Java keeps track of
   * the data provided by mozApps.install() and passes it to this method, which
   * then passes it to DOMApplicationRegistry.doInstall/doInstallPackage().
   *
   * When an APK is installed independently of Fennec, however, there is no
   * such data, so we have to construct the equivalent here.
   */

@@ +233,5 @@
> +  uninstall: function(aData) {
> +
> +    let app = DOMApplicationRegistry.getAppByManifestURL(aData.manifestURL);
> +    if (!app) {
> +      DOMApplicationRegistry.broadcastMessage("Webapps:Uninstall:Return:KO", aData);

webapps-runtime-uninstall gets the message manager in question, so it should pass that to this method, and this method should call sendAsyncMessage on the message manager instead of broadcasting the message.  See the implementation of *install* and *installPackage* for details.

@@ +238,5 @@
> +      return;
> +    }
> +
> +    if (this._testing) {
> +      // We don't have to do anything, as the registry does all the work.

This comment makes more sense attached to the "return" statement.  And "anything" should probably be "anything else", since we do do this one thing.
Attachment #8367650 - Flags: feedback?(myk) → feedback+
Comment on attachment 8367650 [details] [diff] [review]
bug-957065-v2.patch


>diff --git a/mobile/android/modules/WebappManager.jsm b/mobile/android/modules/WebappManager.jsm

>       }
>     });
>+    });
>+  },
>+

nit: indent issue?
Attachment #8367650 - Flags: feedback?(mark.finkle) → feedback+
Created attachment 8368657 [details] [diff] [review]
bug-957065-v3.patch

Addressed existing nits. Ready for review.
Attachment #8367650 - Attachment is obsolete: true
Attachment #8368657 - Flags: review?(wjohnston)
Attachment #8368657 - Flags: review?(myk)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8368657 [details] [diff] [review]
bug-957065-v3.patch

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

I can only give feedback, and my feedback is the same as before: looks good!  But still a few nits, and one substantive issue.

Also, let's have mfinkle do the review, as wesj has a humongous patch from me on his plate.

Finally, we need Fabrice to review the (trivial) change to Webapps.jsm.

::: mobile/android/base/webapp/EventListener.java
@@ +94,5 @@
> +            } else if (event.equals("WebApps:Uninstall")) {
> +                Log.i(LOGTAG, "Uninstalling " + message);
> +                if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {
> +                    uninstallApk(GeckoAppShell.getGeckoInterface().getActivity(), message);
> +                } else {

I really like this separation of the event.equals conditional from the AppConstants.MOZ_ANDROID_SYNTHAPKS one.  I wish I had done that for the other events.

@@ +99,2 @@
>                  uninstallWebApp(message.getString("origin"));
>              }

Nit: these two existing lines should still be indented one more level now.

@@ +117,5 @@
> +                if (resultCode == Activity.RESULT_OK) {
> +                    Log.i(LOGTAG, "Uninstall of " + packageName + " agreed by user");
> +                } else {
> +                    Log.i(LOGTAG, "Uninstall of " + packageName + " canceled by user");
> +                }

Nit: I wouldn't mind a comment here explaining that we don't do anything more here because UninstallListener automatically handles the next steps.

@@ +132,4 @@
>  
>      // Not used by MOZ_ANDROID_SYNTHAPKS.
>      public static void postInstallWebApp(String aTitle, String aURI, String aOrigin, String aIconURL, String aOriginalOrigin) {
> +        org.mozilla.gecko.WebAppAllocator allocator = WebAppAllocator.getInstance(GeckoAppShell.getContext());

Nit: now that you import org.mozilla.gecko.WebAppAllocator, you don't need to use its fully-qualified name here, right?

::: mobile/android/modules/WebappManager.jsm
@@ +233,3 @@
>          }
>        }
>      });

Nit: these three lines also need to be indented one more level now.  Are you perhaps using a 'git diff' configuration that excludes whitespace-only changes?

@@ +236,5 @@
> +    });
> +  },
> +
> +  // This is the function that is called when called from content.
> +  // navigator.mozApps.mgmt.uninstall().

Erm, did you perhaps mean "This is the function that is called when navigator.mozApps.mgmt.uninstall() is called from content."?

@@ +245,5 @@
> +    let app = DOMApplicationRegistry.getAppByManifestURL(aData.manifestURL);
> +    if (!app) {
> +      DOMApplicationRegistry.broadcastMessage("Webapps:Uninstall:Return:KO", aData);
> +      return;
> +    }

As I mentioned last time around, this should use send a message to the message manager that is specific to this message.  Now DOMApplicationRegistry passes it to notifyObservers' aSubject parameter:

  Services.obs.notifyObservers(mm, "webapps-runtime-uninstall", JSON.stringify(msg));

So BrowserApp.observe should pass it to WebappManager.uninstall:

      case "webapps-runtime-uninstall": {
        WebappManager.uninstall(JSON.parse(aData), aSubject);
        break;
      }

And then WebappManager.uninstall should use it to send the message:

  uninstall: function(aData, aMessageManager) {

    let app = DOMApplicationRegistry.getAppByManifestURL(aData.manifestURL);
    if (!app) {
      aMessageManager.sendAsyncMessage("Webapps:Install:Return:KO", aData);
      return;
    }
Attachment #8368657 - Flags: review?(wjohnston)
Attachment #8368657 - Flags: review?(myk)
Attachment #8368657 - Flags: review?(mark.finkle)
Attachment #8368657 - Flags: review?(fabrice)
Attachment #8368657 - Flags: feedback+
Comment on attachment 8368657 [details] [diff] [review]
bug-957065-v3.patch


>diff --git a/mobile/android/base/webapp/EventListener.java b/mobile/android/base/webapp/EventListener.java

>     // Not used by MOZ_ANDROID_SYNTHAPKS.
>     public static void postInstallWebApp(String aTitle, String aURI, String aOrigin, String aIconURL, String aOriginalOrigin) {
>-        WebAppAllocator allocator = WebAppAllocator.getInstance(GeckoAppShell.getContext());
>+        org.mozilla.gecko.WebAppAllocator allocator = WebAppAllocator.getInstance(GeckoAppShell.getContext());

Do you need the change to long-form? Not a big deal since we will likely be removing the code sometime, but just checking.

>diff --git a/mobile/android/modules/WebappManager.jsm b/mobile/android/modules/WebappManager.jsm

>+      DOMApplicationRegistry.getAll((apps) => {
>+        for (let app in apps) {
>+          if (aData.apkPackageNames.indexOf(apps[app].apkPackageName) > -1) {
>+            let appToRemove = apps[app];
>           dump("should remove: " + appToRemove.name);
>           DOMApplicationRegistry.uninstall(appToRemove.manifestURL, function() {
>             dump(appToRemove.name + " uninstalled");
>@@ -227,6 +233,34 @@ this.WebappManager = {
>         }
>       }
>     });
>+    });
>+  },

Just wondering if the two terminators at the same indent level is indicative of a syntax issue, or just "it worked out that way"
 
>+  // then the management of the profile etc done via autoUninstall.
>+  uninstall: function(aData) {
>+
>+    let app = DOMApplicationRegistry.getAppByManifestURL(aData.manifestURL);

nit: remove the blank line

Myk's feedback on the actual mechanics is better than I could do (so make sure you address them) and I don't see any red flags for other things.
Attachment #8368657 - Flags: review?(mark.finkle) → review+
Comment on attachment 8368657 [details] [diff] [review]
bug-957065-v3.patch

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

I discussed the patch with Myk and this patch is not calling DOMApplicationRegistry.uninstall() properly, since it's missing the message manager that will then let us propagate the uninstall result down to the DOM part.
Myk has some ideas on how to get that working though!
Attachment #8368657 - Flags: review?(fabrice)
Created attachment 8369613 [details] [diff] [review]
bug-957065-v4.patch

Updated patch, addressing feedback. 

The autouninstall has already landed (by someone else), to a better implementation than I was suggesting. This part has now disappeared from this patch.
Attachment #8368657 - Attachment is obsolete: true
Attachment #8369613 - Flags: review?(fabrice)
Attachment #8369613 - Flags: feedback?(myk)
(Reporter)

Comment 12

4 years ago
Comment on attachment 8369613 [details] [diff] [review]
bug-957065-v4.patch

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

::: mobile/android/chrome/content/browser.js
@@ +617,4 @@
>        function(aTarget) {
>          aTarget.muted = true;
>        });
> +

Nit: It'd be better not to make these whitespace-only changes to reduce the scope of the patch and avoid cluttering blame with inaccurate assignations.

::: mobile/android/modules/WebappManager.jsm
@@ +231,5 @@
> +      return;
> +    }
> +
> +    // Android will uninstall the APK, which will prompt a call to
> +    // WebappManager.autoUninstall, above.

As Fabrice mentioned, this needs a way to hold a reference to the message manager until autoUninstall gets called, so autoUninstall can propagate the result of the mozApps.mgmt.uninstall call back to the page that made the call.

For example, this could use a WeakMap <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap> to hold a reference to that message manager that wouldn't leak if the web page was unloaded before the uninstall completed.

Cancelling review request pending inclusion of such a mechanism to pass the result back to the calling page.
Attachment #8369613 - Flags: review?(fabrice)
Attachment #8369613 - Flags: feedback?(myk)
Attachment #8369613 - Flags: feedback+
(Reporter)

Updated

4 years ago
Whiteboard: [WebRuntime]
(Reporter)

Comment 13

4 years ago
James: are you still working on this one?
Flags: needinfo?(jhugman)
Not currently.
Flags: needinfo?(jhugman)
(Reporter)

Comment 15

4 years ago
(In reply to James Hugman [:jhugman] [@jhugman] from comment #14)
> Not currently.

Ok, I'm unassigning you.  Please do so yourself in the future, so folks know the status of the bug!
Assignee: jhugman → nobody
(Reporter)

Comment 16

2 years ago
Per bug 1235869, we're going to disable the Android web runtime, so we won't fix this bug in it.

(This is part of a bulk resolution of bugs in the Firefox for Android::Web Apps component, from which I attempted to exclude bugs that are not specific to the runtime, but it's possible that I included one accidentally.  If so, I'm sorry, and please reopen the bug!)
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.