Closed Bug 933979 Opened 6 years ago Closed 6 years ago

Uninstalled webapps take up a lot of space

Categories

(Firefox for Android :: General, defect)

28 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
fennec + ---

People

(Reporter: bug.zilla, Assigned: wesj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131026030205

Steps to reproduce:

Firefox Nightly occupied 150mb on my device of which 120mb was data; the largest application by far. This caused the "low storage" warning and was not able to install any other apps.


Actual results:

Running du in a terminal session in the fennec directory showed that a number of uninstalled webapps occupied c. 110mb! A further 10mb was take up by un-submitted crash reports. I manually deleted these but for ordinary users this is a big problem.


Expected results:

Uninstalled webapps should leave no traces behind. Regular cleanup of un-submitted crash reports
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → 29+
It probably makes sense to handle pending crash reports and uninstalled webapp cruft in two separate bugs. I've filed bug 942612 about the abandoned pending crash report files.
Blocks: fatfennec
OS: Windows 7 → Android
Hardware: x86_64 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true
Is the web-app point an non-issue now with synth APKs?
(In reply to Aaron Train [:aaronmt] from comment #2)
> Is the web-app point an non-issue now with synth APKs?

Unsure, but synthetic APKs still create a profile directory in Fennec's storage, and Fennec might not delete that directory when a webapp is uninstalled, in which case it's still a problem.  We need to investigate further to know for sure.
Unassigning. We have bugs for these things, making this basically a meta bug.
Assignee: wjohnston → nobody
I don't see those bugs on the list of blockers of meta-bug 942609:

  https://bugzilla.mozilla.org/showdependencytree.cgi?id=942609&hide_resolved=1

Nevertheless, if that's a meta-bug, and this is a meta-bug, then we have one meta-bug too many.  So we should close this one.
Assignee: nobody → wjohnston
Summary: Uninstalled webapps and un-submitted crash reports take up a lot of space → Uninstalled webapps take up a lot of space
Ahh, you're right myk. We already have one. I removed the crash reports stuff from this bug since it was split into bug 942612. I fixed this webapp problem at one point. Maybe this was filed in the interim? It would be good to verify one way or the other....
Keywords: qawanted
tracking-fennec: 29+ → +
Keywords: qawanted
Attached patch Patch v1 (obsolete) — Splinter Review
We still have code to do this in the EventListener, but those events aren't fired anymore. Rather than refire them (and have to update that code to use packageNames in addition to origin) this just updates the UninstallListener to do the same thing.
Attachment #8396037 - Flags: review?(myk)
Comment on attachment 8396037 [details] [diff] [review]
Patch v1

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

::: mobile/android/base/webapp/UninstallListener.java
@@ +55,5 @@
>  
>          if (installedPackages.contains(packageName)) {
> +            ArrayList<String> uninstalledPackages = new ArrayList<String>();
> +            uninstalledPackages.add(packageName);
> +            doUninstall(context, uninstalledPackages);

The novice Java hacker in me sees this as an opportunity for polymorphism via a doUninstall variant that takes a single package name and constructs the array itself.  But that seems like overkill given the single caller!

@@ +69,5 @@
> +            int index = allocator.findOrAllocatePackage(packageName);
> +
> +            // if -1, nothing to do; we didn't think it was installed anyway
> +            if (index == -1)
> +                return;

Since Allocator.findOrAllocatePackage will allocate an index if there isn't one already, it won't return -1 if the package isn't allocated, only if it's run out of slots.  So we'll need to factor out the code that merely finds the package from that method and call it directly to ensure that we only get an index for a package that has already been allocated.

But also, allocation is distinct from installation (i.e. registration in the webapp registry), even if they should always happen together.  So it seems like we should still emit Webapps:AutoUninstall on packages that aren't allocated, just in case they're nevertheless "installed."

@@ +96,5 @@
> +        try {
> +            message.put("apkPackageNames", jsonPackages);
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Webapps:AutoUninstall", message.toString()));
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "JSON EXCEPTION " + e);

Nit: I know you just copied this line, but since we're changing it anyway, we should take the opportunity to make "JSON EXCEPTION" a more descriptive error message, like "error putting apkPackageNames".

@@ -75,5 @@
>          List<ApplicationInfo> packages = pm.getInstalledApplications(PackageManager.GET_META_DATA);
>          Set<String> allInstalledPackages = new HashSet<String>();
>  
>          for (ApplicationInfo packageInfo : packages) {
> -            //Log.i(LOGTAG, "Android package: " + packageInfo.packageName);

Nice catch!  No reason to leave stuff like this lying around.
Attachment #8396037 - Flags: review?(myk) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
I gave you you're polymorphism. Although it actually feels like this code should be mostly in the single string method. i.e. the array method should loop over things and call into that. Unfortunately, that would mean one Webapps:AutoUninstall call for every package, which seems awful. I decided this was the simpler solution.
Attachment #8396037 - Attachment is obsolete: true
Attachment #8396890 - Flags: review?(myk)
Comment on attachment 8396890 [details] [diff] [review]
Patch v2

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

(In reply to Wesley Johnston (:wesj) from comment #9)
> Created attachment 8396890 [details] [diff] [review]
> Patch v2
> 
> I gave you you're polymorphism. Although it actually feels like this code
> should be mostly in the single string method. i.e. the array method should
> loop over things and call into that. Unfortunately, that would mean one
> Webapps:AutoUninstall call for every package, which seems awful. I decided
> this was the simpler solution.

/me does his polymorphy happy dance!

::: mobile/android/base/webapp/UninstallListener.java
@@ +70,5 @@
> +        JSONArray jsonPackages = new JSONArray();
> +
> +        for (String packageName : packageNames) {
> +            // Although its unlikley that an app is not allocated, but is installed in Gecko, it
> +            // is possible. We always send the packageName to JS to be removed from Gecko's registry,

Nit: unlikley -> unlikely.

Also, was there something else you intended to add after the trailing comma?

@@ +77,5 @@
> +            int index = allocator.getIndexForApp(packageName);
> +
> +            // if -1, nothing to do; we didn't think it was installed anyway
> +            if (index == -1)
> +                return;

This *return* needs to be *continue*, or the first unallocated app will prevent others from being uninstalled.

I would also s/nothing to do/nothing more to do/, since we do at least try to remove the app from the registry.

@@ +80,5 @@
> +            if (index == -1)
> +                return;
> +
> +            // kill the app if it's running
> +            String targetProcessName = GeckoAppShell.getContext().getPackageName();

This raises a NullPointerException in GeckoAppShell.getContext:

  8370         AndroidRuntime  E  java.lang.RuntimeException: Unable to start receiver org.mozilla.gecko.webapp.UninstallListener: java.lang.NullPointerException
  8370         AndroidRuntime  E  at org.mozilla.gecko.GeckoAppShell.getContext(GeckoAppShell.java:2145)
  8370         AndroidRuntime  E  at org.mozilla.gecko.webapp.UninstallListener.doUninstall(UninstallListener.java:84)
  8370         AndroidRuntime  E  at org.mozilla.gecko.webapp.UninstallListener.onReceive(UninstallListener.java:57)


But you should be able to use the *context* argument passed into the method (here and below).
Attachment #8396890 - Flags: review?(myk) → review-
Attached patch Patch v3Splinter Review
I swear I changed that return to a continue. hg fail I guess.
Attachment #8396890 - Attachment is obsolete: true
Attachment #8398359 - Flags: review?(myk)
Comment on attachment 8398359 [details] [diff] [review]
Patch v3

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

Looks great!
Attachment #8398359 - Flags: review?(myk) → review+
https://hg.mozilla.org/mozilla-central/rev/44fb37f312d5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.