942611 - Stop Firefox Mobile from leaking Storage space by leaking file uploads

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jonathan Watt [:jwatt]]

Spinning this off as a sub-bug of bug 942609. It seems that when Firefox Mobile uploads files it first copies them to the root of its data directory (/data/data/org.mozilla.firefox/). For background see:

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2013-November/000388.html

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This works AFAICT, but I'm not very familiar with this type of code. Any feedback at all is appreciated.

The general approach here was just to allow code to post an intent to the service telling it that it needs a particular "task" done. i.e.

Intent cleanupIntent = FileCleanup.createIntent(context, dir, regex, age);
context.startService(cleanupIntent);

The service gets the intent, pushes it into a PendingIntent and schedules it with the AlarmService. When the alarms runs, we use the intent to generate a runnable, and start it. i.e. Android does the job of storing data in the pending intent rather than us managing a list of files to purge/tasks to run.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8346319 [details] [diff] [review]
Patch

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

The general right idea but I think you're adding an extra layer of abstraction that you don't need to - namely just have the CrashReporter and FilePicker... set the alarms themselves rather than starting a Service to do so. More details below.

If it's helpful for me to jump on Vidyo to explain a bit better, let me know.

::: mobile/android/base/CrashReporter.java
@@ +114,5 @@
> +    public static File getPendingDir(Context context) {
> +        return new File(context.getFilesDir(), PENDING_SUFFIX);
> +    }
> +
> +    public static File getSubmittedDir(Context context) {

Should these be public?

@@ +140,5 @@
>          moveFile(extrasFile, mPendingExtrasFile);
>  
> +        // Schedule cleanup after 12 hours
> +        Intent cleanupIntent = FileCleanup.createIntent(this, pendingDir, null, FileCleanup.TWELVE_HOURS);
> +        startService(cleanupIntent);

Rather than starting a service to register the alarm to start the cleanup service, why don't we just register the alarm directly, right now. We can call out to a static method of FileCleanup to keep the code modular.

Side note since you may have been following the services' example: we do that in a BroadcastReceiver so we can register the event when the device is started.

::: mobile/android/base/FilePickerResultHandler.java
@@ +87,5 @@
>              String path = file.getAbsolutePath();
> +
> +            // Schedule cleanup after 12 hours
> +            Intent cleanupIntent = FileCleanup.createIntent(context, file.getParentFile(), null, FileCleanup.TWELVE_HOURS);
> +            context.startService(cleanupIntent);

Like CrashReporter, maybe this should set the alarm directly.

::: mobile/android/base/background/fennec/CleanupService.java
@@ +29,5 @@
> +  private static final String WORKER_THREAD_NAME = LOGTAG + "Worker";
> +
> +  /* Actions to register and run tasks */
> +  public static final String ACTION_CLEANUP = "cleanup";
> +  public static final String ACTION_REGISTER = "register";

I'd prefer this as an enum where you can override .equals to do String comparison against a String member variable, e.g. "cleanup" (though that sucks because .equals can only go one way, so maybe it'd be better just to have a getter for the member String variable).

@@ +53,5 @@
> +      final PendingIntent pending = convertToPendingIntent(intent);
> +
> +      final AlarmManager alarm = getAlarmManager();
> +      final long now = System.currentTimeMillis();
> +      alarm.set(AlarmManager.RTC, now + intent.getLongExtra(TASK_DELAY, GlobalConstants.MILLISECONDS_PER_DAY), pending);

Rather than returning a default value of one day, I think it would be better form to force callers to set a value by checking `hasExtra(...)` and dropping the intent if it doesn't have a value. (this is circumvented if the callers just registered the alarm themselves).

@@ +59,5 @@
> +    } else if (CleanupService.ACTION_CLEANUP.equals(intent.getAction())) {
> +      // The alarm is firing, run the scheduled task
> +      Runnable task = getTaskForIntent(intent);
> +      if (task != null) {
> +        task.run();

I know we discussed this so I'm sorry this realization didn't come to me sooner, but using Runnable in this way is equivalent to just extending the Service class, except adding a little unneeded code. Instead, I think we should have multiple services like FileCleanupService (as opposed to `FileCleanup implements Runnable`).

FileCleanupService can still implement runnable if you want to use it outside of the service context, however - just have `onHandleIntent()` call `run()`.

@@ +69,5 @@
> +   * was found. */
> +  private Runnable getTaskForIntent(final Intent intent) {
> +    Runnable task = null;
> +
> +    TaskType type = TaskType.valueOf(TaskType.class, intent.getStringExtra(TASK_TYPE));

I believe there's actually a `TaskType.valueOf(String)` method, even though the javadoc doesn't say it.

@@ +87,5 @@
> +    Context context = this.getApplicationContext();
> +
> +    Intent i = null;
> +
> +    TaskType type = TaskType.valueOf(TaskType.class, intent.getStringExtra(TASK_TYPE));

Same as above.

@@ +94,5 @@
> +      default:
> +        i = FileCleanup.createIntent(context, intent, ACTION_CLEANUP);
> +    }
> +
> +    final PendingIntent pending = PendingIntent.getService(context, 0, i, PendingIntent.FLAG_CANCEL_CURRENT);

PendingIntents set by an AlarmManager may cancel each other out. For specifics, see the opening text of [1]. Essentially, it comes down to PendingIntents having to differ in values other than extras (e.g. use a different `Action`) or a different requestCode to PendingIntent.get*.

So here we'd having CrashReporter's and FilePicker...'s PendingIntent cancelling each other out.

Additionally, each one cancels its own out so every time Fennec is started from the OOM state (recreating CrashReporter), the files will be cleared twelve hours from now, which means if Fennec is periodically cleared, this will never be cleaned up. Does that seem okay? Additionally, I imagine this could potentially run while Fennec is running - is that also okay?

[1]: https://developer.android.com/reference/android/app/PendingIntent.html

::: mobile/android/base/moz.build
@@ +101,5 @@
>      'animation/Rotate3DAnimation.java',
>      'animation/ViewHelper.java',
>      'ANRReporter.java',
>      'AppNotificationClient.java',
> +    'background/BackgroundService.java',

I'm not familiar with the moz.build stuff but are you sure it's necessary? It wasn't needed before...

::: mobile/android/base/util/FileUtils.java
@@ +7,5 @@
> +import java.io.File;
> +import java.io.IOException;
> +
> +public class FileUtils {
> +    public static boolean delete(File file) throws IOException {

See bug 925435 on unifying file-directory deletion methods.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8346319 [details] [diff] [review]
Patch

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

::: mobile/android/base/CrashReporter.java
@@ +117,5 @@
> +
> +    public static File getSubmittedDir(Context context) {
> +        return new File(context.getFilesDir(), SUBMITTED_SUFFIX);
> +    }
> +    

WS

::: mobile/android/base/background/fennec/CleanupService.java
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.background.fennec;

Background is entirely imported from Git, so this needs to either (a) move into background services, and not consume most o.m.g.* classes, or (b) shift to a different package. Sorry that this seems arbitrary, but it'll avoid some bites later.

If you wish you can keep all of the intent handling and such in a .background service (and benefit from the power management code etc. that these will accrue), and decouple the actual functionality (which will be growing) into o.m.g.cleanup classes. You seem to be half-way there, so rock on.

Decoupling can probably best be accomplished by... sending an intent, so I encourage this division: scheduling in background, action in omg.cleanup.

@@ +7,5 @@
> +import org.mozilla.gecko.background.BackgroundService;
> +import org.mozilla.gecko.background.common.GlobalConstants;
> +import org.mozilla.gecko.CrashReporter;
> +import org.mozilla.gecko.util.FileUtils;
> +import org.mozilla.gecko.mozglue.GeckoLoader;

These three imports are unused.

@@ +53,5 @@
> +      final PendingIntent pending = convertToPendingIntent(intent);
> +
> +      final AlarmManager alarm = getAlarmManager();
> +      final long now = System.currentTimeMillis();
> +      alarm.set(AlarmManager.RTC, now + intent.getLongExtra(TASK_DELAY, GlobalConstants.MILLISECONDS_PER_DAY), pending);

Safer to used ELAPSED_REALTIME here. RTC can jump forward and backward:

http://opensignal.com/reports/timestamps/

::: mobile/android/base/background/fennec/FileCleanup.java
@@ +20,5 @@
> +import java.io.FilenameFilter;
> +import java.util.ArrayList;
> +
> +/* A basic runnable task that will delete files in a directory. Calls can pass in a dir, a filter
> + * for file names to be removed, and a maximum age that the file should reach before deleting it */

Javadoc format, plz:

/**
 * A basic runnable task that will delete files in a directory.
 * Callers can pass in a directory, a filter for file names to be removed,
 * and a maximum age for files.
 **/

@@ +24,5 @@
> + * for file names to be removed, and a maximum age that the file should reach before deleting it */
> +public class FileCleanup implements Runnable {
> +  final private static String LOGTAG = "GeckoFileCleanup";
> +  // The age at which files are deleted. Defaults to 12 hours old
> +  public static final long TWELVE_HOURS = 1000*60*60*12;

TWELVE_HOURS_MILLIS, and spaces around arithmetic ops here, cos we're on our own line.

But note that you can just use AlarmManager.INTERVAL_HALF_DAY instead.

@@ +28,5 @@
> +  public static final long TWELVE_HOURS = 1000*60*60*12;
> +
> +  private static final String DIR_EXTRA = "directory";
> +  private static final String FILE_EXTRA = "fileExtra";
> +  private static final String TIME_EXTRA = "time";

EXTRA_FOO

@@ +30,5 @@
> +  private static final String DIR_EXTRA = "directory";
> +  private static final String FILE_EXTRA = "fileExtra";
> +  private static final String TIME_EXTRA = "time";
> +
> +  final private long mAge;

I think the age should be part of the filter. See below.

@@ +44,5 @@
> +      mName = name;
> +    }
> +
> +    public File getDir() {
> +      return mDir;

I think this should be part of the Runnable, not the filter.

@@ +73,5 @@
> +    return createIntent(context, 
> +                       intent.getStringExtra(DIR_EXTRA),
> +                       intent.getStringExtra(FILE_EXTRA),
> +                       intent.getIntExtra(TIME_EXTRA, 0),
> +                       action);

Trailing whitespace and indenting.

@@ +89,5 @@
> +
> +    return i;
> +  }
> +
> +  /* Runnable */

@Override

@@ +103,5 @@
> +
> +    if (files == null) {
> +      return;
> +    }
> +    

WS.

@@ +107,5 @@
> +    
> +    final long now = System.currentTimeMillis();
> +    for (String file : files) {
> +      if (mFilter.accept(dir, file)) {
> +        File f = new File(mFilter.getDir(), file);

Break this stuff out into FileUtils? What you're doing here is writing 

  void deltree(File directory, FilenameFilter filter, long deleteOlderThan, boolean recurse);

@@ +108,5 @@
> +    final long now = System.currentTimeMillis();
> +    for (String file : files) {
> +      if (mFilter.accept(dir, file)) {
> +        File f = new File(mFilter.getDir(), file);
> +        if (f.lastModified() > now - mAge) {

Compute the oldest timestamp once.

Furthermore, if you use FileFilter instead of FilenameFilter in your definition of deltree, and adjust your filter class accordingly, you can just do:

  FileUtils.deltree(dir, new NameAgeFilter(oldest, namePattern), true);

Submit the appropriate filter -- name, age, name and age, or neither -- according to the input you get. Then this whole class becomes:

  * Build the right filter based on an intent.
  * Wrap up a runnable that closes over the destination directory and the filter to use, and just calls FileUtils.deltree.

@@ +110,5 @@
> +      if (mFilter.accept(dir, file)) {
> +        File f = new File(mFilter.getDir(), file);
> +        if (f.lastModified() > now - mAge) {
> +          try {
> +            FileUtils.delete(f);

This will fail for directories. Recurse or document.

http://docs.oracle.com/javase/6/docs/api/java/io/File.html#delete%28%29

@@ +120,5 @@
> +
> +  /* Creates a filter from an intent */
> +  static FileCleanup fromIntent(Intent intent) {
> +    FileCleanupFilter filter = new FileCleanupFilter(new File(intent.getStringExtra(DIR_EXTRA)),
> +                                                     intent.getStringExtra(FILE_EXTRA));

If there's no filename pattern, the filter does nothing, so let's just skip the creation of the filter (and its use above) and directly pass the dir to the FileCleanup instance.

Also, this is a great place to validate TIME_EXTRA and the directory, too.

::: mobile/android/base/mozglue/GeckoLoader.java.in
@@ +108,5 @@
>          }
>          file.delete();
>      }
>  
> +    public static File getTmpDir(Context context) {

Move to FileUtils?

@@ +113,5 @@
>          File tmpDir = context.getDir("tmpdir", Context.MODE_PRIVATE);
>          // check if the old tmp dir is there
>          File oldDir = new File(tmpDir.getParentFile(), "app_tmp");
>          if (oldDir.exists()) {
>              delTree(oldDir);

Oh hey! We have a deltree!

::: mobile/android/base/util/FileUtils.java
@@ +7,5 @@
> +import java.io.File;
> +import java.io.IOException;
> +
> +public class FileUtils {
> +    public static boolean delete(File file) throws IOException {

How does this compare to delTree in GeckoLoader?

@@ +12,5 @@
> +        // Try to do a quick initial delete
> +        if (file.delete())
> +            return true;
> +
> +        if (file.isDirectory()) {

isDirectory isn't expensive, as far as I can tell -- you're already going to hit the SecurityManager to do the delete. Just do this first and get rid of the first clause, unless you know something I don't!

Comment 4

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #2)

> I know we discussed this so I'm sorry this realization didn't come to me
> sooner, but using Runnable in this way is equivalent to just extending the
> Service class, except adding a little unneeded code. Instead, I think we
> should have multiple services like FileCleanupService (as opposed to
> `FileCleanup implements Runnable`).
> 
> FileCleanupService can still implement runnable if you want to use it
> outside of the service context, however - just have `onHandleIntent()` call
> `run()`.

To add to this: I agree; each of the cleanup things should be its own little service inside omg.cleanup. That's the decoupling I was referring to.

I don't mind if you do either of these two suggestions:

* As Michael said, have each service register its own alarms.
* As I suggested, if you want to combine scheduling somehow in a way that allows you to introduce logic that AlarmManager does not, then keep that scheduling/registering service, and put it in omg.background.

They're both going to trigger standalone services via intents, so the former is easier.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> The general approach here was just to allow code to post an intent to the service telling it that
> it needs a particular "task" done.

I thought it through it some more and found some benefits to "tasks" that I did not previously mention:
  * Less code overhead than extending Service (perhaps mitigated by subclassing Service and extending that instead)
  * Abstracting away Services so developers who need to write background tasks don't actually need to learn how Services work
  * (as rnewman mentioned in comment 4) Managing custom scheduling between various tasks (e.g. if you looked at the FHR PruneService, the dependency on deleting data from the database before vacuuming)

Of course, these benefits come at the loss of control that is dependent on the Service - i.e. when does my task get run? This might be fine for some tasks that just needed to get run eventually.

Comment 6

[Richard Newman [:rnewman]]

Bear in mind that if we use intents and services, the only coupling is to Android. If we do some kind of task stuff ourselves, then we're either doing some dirty hacks with strings, or we need to tightly couple the scheduler to the tasks (which is what this patch does).

(Remember that the services don't stay running, so we can't just keep a reference to a runnable when we set the alarm.)

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

OK, this makes the FileCleanup into its own service class. I also made it a Runnable and a Parcelable, so that the actual class implementation is actually pretty small, but it also means we have to do some evil things to make this look pretty (i.e. we're creating Service's of our own):

1.) create a FileCleanup object,
2.) Call runDelayed()
3.) FileCleanup dispatches a pending intent with the Parceled version of itself via the alarm service
4.) Some time later, the Alarm fires and creates a new FileCleanup service.
5.) That object then creates a different FileCleanup object using its stored parcel
6.) The .run() method on that fileCleanup is called.

Since its a Service, you normally wouldn't do things like this. I think I should split out a separate Service part here.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8349099 [details] [diff] [review]
Patch v2

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

Stepping back for a moment.

We seem to be trying to solve two problems here:

* Crash reports sticking around.
* Uploaded files sticking around.

The approach you're taking is to pickle actions and queue them up -- "I'm uploading a file; twelve hours from now, delete files in this directory that are older than twelve hours", and you're using a service with an alarm to make that happen.

That'll probably work, but has downsides:

* You're vulnerable to clock drift.
* Your service might not run when you want it to run.
* You're accruing a little complexity here in order to make stuff happen 'later'.

The thing I'm having a hard time with, though, is when and why we're queuing up actions.

Consider uploaded files. We know the files you've chosen to upload. We know that once we've made that copy, it's to upload immediately. We know that if Firefox dies, the file is orphaned.

So, in the spirit of defense-in-depth:

* trigger a cleanup service when our activity finishes, if we uploaded any files
* and when the upload completes (presumably we already delete the file in that case)
* and let's copy those files into the Android temp dir rather than our own dir, allowing Android to get rid of those files if we always crash.

Similarly, we already have a crash report uploader service that runs in the background. It should be cleaning up the successfully uploaded reports, and it should also be pruning really old reports, regardless of whether they were uploaded -- and I thought it already did so (at least on desktop). If it doesn't, we should fix that.

So: yes to doing deletions in services, but "hold up, wha?" to queuing up deletion tasks. Am I misunderstanding something after not getting enough sleep?

::: mobile/android/base/FilePickerResultHandler.java
@@ +85,5 @@
>              }
>              fos.close();
>              String path = file.getAbsolutePath();
> +
> +            // Schedule cleanup after 12 hours

Taking a step back: this doesn't make sense to me. We buffer twelve hours of completed uploads!

Why not fire that intent when they close the browser, or close that tab, or...?

::: mobile/android/base/CleanupService.java
@@ +49,5 @@
> +    runDelayed(context, GlobalConstants.MILLISECONDS_PER_DAY);
> +  }
> +
> +  public void runDelayed(Context context, long time) {
> +    runDelayed(context, time, UUID.randomUUID().toString());

The UUID bit seems like a bad idea, for two reasons:

* It's overkill. We don't need a random ID here; we only need a unique one. Just increment an AtomicInteger.
* We might well want to cancel or supersede these pending intents, and the only way to cancel one is to reconstruct the pending intent. No randomness!

::: mobile/android/base/FileCleanup.java
@@ +16,5 @@
> +import java.io.FilenameFilter;
> +
> +/*
> + * A basic runnable task that will delete files in a directory.
> + */ 

WS

@@ +26,5 @@
> +  final private File mDir;
> +  final private String mFilter;
> +
> +  /*
> +   * This constructor is only used by the intent service when creating this class. 

WS

@@ +63,5 @@
> +  }
> +
> +  public static final Parcelable.Creator<FileCleanup> CREATOR = new Parcelable.Creator<FileCleanup>() {
> +      public FileCleanup createFromParcel(Parcel in) { return new FileCleanup(in); }
> +      public FileCleanup[] newArray(int size) { return new FileCleanup[size]; }

Spend the extra six lines :D

@@ +65,5 @@
> +  public static final Parcelable.Creator<FileCleanup> CREATOR = new Parcelable.Creator<FileCleanup>() {
> +      public FileCleanup createFromParcel(Parcel in) { return new FileCleanup(in); }
> +      public FileCleanup[] newArray(int size) { return new FileCleanup[size]; }
> +  };
> +

Oh hey! A free line! :D

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8349099 [details] [diff] [review]
Patch v2

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

I agree with rnewman's comments in comment 8 that we can attach this cleanup to other Services.

Spoke with wesj on IRC and forgoing review with his permission.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #8)
> * and let's copy those files into the Android temp dir rather than our own
> dir, allowing Android to get rid of those files if we always crash.

It doesn't seem like Android actually has a temp dir/files. See [1]. Also, we might not want to leak our files to upload outside of our data directory in any case. For the moment, I assume we don't receive any intents for uploading that only we can receive, thus anyone can retrieve these files if they just write an Android app to accept them, but this may not be the case in the future (though we could always deal with that if it happens later).

[1]: https://groups.google.com/forum/#!topic/android-developers/tZrxqZQLh98

Comment 11

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #10)

> It doesn't seem like Android actually has a temp dir/files. See [1]. Also,
> we might not want to leak our files to upload outside of our data directory
> in any case. 

http://developer.android.com/reference/android/content/Context.html#getCacheDir%28%29

?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

My reasoning in comment 10 was a bit confusing because it nonsensically implied the existence of a /tmp style dir, sorry.

However, you mentioned:

> * and let's copy those files into the Android temp dir rather than our own dir, allowing Android to get
> rid of those files if we always crash.

But, via the docs:

> Note: you should not rely on the system deleting these files for you; you should always have a reasonable
> maximum, such as 1 MB, for the amount of space you consume with cache files, and prune those files when 
> exceeding that space.

As far as I know, this still gets put in Firefox's /data directory though, so there are no privacy concerns. Overall, this is a better approach than putting the files into who-knows-which dir, but we still have to manage it ourselves.

Comment 13

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

Firefox creates it's own tmp folder, app_tmp, puts that in the environment and that is what Gecko uses when code asks for the "TmpD" folder.

Comment 14

[Richard Newman [:rnewman]]

(In reply to Michael Comella (:mcomella) from comment #12)

> > Note: you should not rely on the system deleting these files for you; you should always have a reasonable
> > maximum, such as 1 MB, for the amount of space you consume with cache files, and prune those files when 
> > exceeding that space.
> 
> As far as I know, this still gets put in Firefox's /data directory though,
> so there are no privacy concerns. Overall, this is a better approach than
> putting the files into who-knows-which dir, but we still have to manage it
> ourselves.

Yeah, I'm not proposing *relying* on it, but it's free defense in depth: files in this directory might be automatically cleaned up by Android (or can be manually removed by "Clear cached data" in Android Settings for the app, independently of the rest of your data).

If we screw up for some reason, that's nice to have.

Furthermore, this might change how our space is reported in settings, which will make us look less fat.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8349099 [details] [diff] [review]
Patch v2

Waiting for a new rev, so clearing flag.

Comment 16

[Wesley Johnston (:wesj)]

(In reply to Richard Newman [:rnewman] from comment #8)
Sorry. I got distracted about this and let it slide:

> * You're vulnerable to clock drift.
> * Your service might not run when you want it to run.

I don't think I care about either of these things. We're not trying to be precise. Just hope that it might happen later. Hence, the Service doesn't just delete "this" file, it deletes anything thats old enough. If you like to upload files and then instantly turn your phone off constantly, this won't help you...

> * trigger a cleanup service when our activity finishes, if we uploaded any
> files

The activity might be closed while the tab that's using this file (your email for instance) is still open in Gecko. In fact, if you're emailing multiple files, you might hop in and out of fennec a few times. Activity pause/end notifications don't seem like the right place to do this.

> * and when the upload completes (presumably we already delete the file in
> that case)

This isn't necessarily an upload. You might be using a local image editor for instance. We (currently, in java) have no idea when the page is done with the file. We could try to attach to the dtor for this in Gecko and notify ourselves when all references are gone, but I decided that was a bit... overkill.

Even with this, you might open something and leave the tab open for 12 hours. Maybe a smarter version would watch for when the tab that requested it is destroyed... (tying the FilePicker request to a particular tab is also not trivial. We do get a window though, so we might be able to correlate it...). I don't think we'll get that if we're killed by Android though, so we likely still need a backup (or maybe we just delay again if Gecko is running when the service runs...)

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

We can use this to fix bug 845338 too.

Comment 18

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

We should also use this to fix bug 966707

Comment 20

[Wesley Johnston (:wesj)]

This should be helped by bug 970506. i.e. we only do this copy if we really have to now, which for me was none of the file picker apps I installed. We still need to clean up our old mess though.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 1/3

OK, Three pronged approach here. Step one is to start using the Android CacheDir. That gives us some chance that Android will do this cleanup for us. I'm not trying to exhaustively switch everything over here, just this one instance for now...

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 2/3

This is likely more risky. We have no guarantee that Android will cleanup the Cache dir. This TRIES to clean it up ourselves, either when the Tab's location changes or when its closed.

I played for awhile with hooking something up in DOMFile to auto-clean these up, but there are a lot of caveots there. Like, we don't actually know how the DOMFile was made and if there is a temp. We don't always create DOMFile's for these (they're lazily made when the FilePicker requests them). etc.

I just decided this was easier, and hopefully Android's own vacuuming will help as well.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch 3/3 - Cleanup old stuff

This piece brings back in the old FileUtils.java and tries to cleanup those files in onPause(). I left a 24 hour time limit in, just to be careful, but we could probably remove it.

I need to test this a bit too :)

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8385631 [details] [diff] [review]
Patch 1/3

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

::: mobile/android/base/FilePickerResultHandler.java
@@ +29,5 @@
>  import java.io.InputStream;
>  import java.io.IOException;
>  import java.util.Queue;
>  
>  class FilePickerResultHandler implements ActivityResultHandler {

FPRH's first constructor is never used, so it can die. Its second constructor is only called from one place...

@@ +183,5 @@
>                  }
>  
>                  // Now write the data to the temp file
>                  try {
> +                    File cacheDir = GeckoAppShell.getContext().getCacheDir();

... and that place has a context available. Just compute this and make it a final member inside the constructor:

  private final File cacheDir;

  public FilePickerResultHandler(FilePicker.ResultHandler handler, Context context) {
    this.cacheDir = new File(context.getCacheDir(), UPLOADS_DIR);
    this.handler = handler;
  }

Comment 25

[Richard Newman [:rnewman]]

Comment on attachment 8385635 [details] [diff] [review]
Patch 2/3

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

::: mobile/android/base/FilePicker.java
@@ +203,5 @@
>      /* Allows the user to pick an activity to load files from using a list prompt. Then opens the activity and
>       * sends the file returned to the passed in handler. If a null handler is passed in, will still
>       * pick and launch the file picker, but will throw away the result.
>       */
>      protected void showFilePickerAsync(String mimeType, final ResultHandler handler) {

This implementation is never called. Consider just making the tabId version the only one, and define what -1 means.

::: mobile/android/base/FilePickerResultHandler.java
@@ +87,5 @@
>          final FragmentActivity fa = (FragmentActivity) GeckoAppShell.getGeckoInterface().getActivity();
>          final LoaderManager lm = fa.getSupportLoaderManager();
>          // Finally, Video pickers and some file pickers may return a content provider.
>          try {
> +            /*

?

@@ +177,4 @@
>                      fileExt = "." + GeckoAppShell.getExtensionFromMimeType(mimeType);
>                  } else {
>                      fileExt = name.substring(period);
>                      fileName += name.substring(0, period);

Idea: prefix names (or add an intermediate directory) to correspond to some process-bound ID? This would allow us to -- in a follow-up -- conclusively identify files created by a previous incarnation of the app, which should be ripe for cleanup.

@@ +197,5 @@
>                      }
>                      fos.close();
>  
> +                    tempFile = file.getAbsolutePath();
> +                    Log.i(LOGTAG, "Created  " + tempFile);

Delete this before we ship. It leaks a filename.

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8385638 [details] [diff] [review]
Patch 3/3 - Cleanup old stuff

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

::: mobile/android/base/GeckoApp.java
@@ +2060,5 @@
>                  // until here to register the prune service.
>                  GeckoPreferences.broadcastHealthReportPrune(context);
> +
> +                // If we haven't done it before, cleanup any old files in our old temp dir
> +                if (prefs.getBoolean(GeckoApp.PREFS_CLEANUP_TEMP_FILES, true)) {

Can we get and set this before line 2056, reuse the editor and save a file write, and then use the fetched value here?

@@ +2061,5 @@
>                  GeckoPreferences.broadcastHealthReportPrune(context);
> +
> +                // If we haven't done it before, cleanup any old files in our old temp dir
> +                if (prefs.getBoolean(GeckoApp.PREFS_CLEANUP_TEMP_FILES, true)) {
> +                    File tempDir = GeckoLoader.getGREDir(GeckoAppShell.getContext());

sAppContext? Or just use `this`.

@@ +2062,5 @@
> +
> +                // If we haven't done it before, cleanup any old files in our old temp dir
> +                if (prefs.getBoolean(GeckoApp.PREFS_CLEANUP_TEMP_FILES, true)) {
> +                    File tempDir = GeckoLoader.getGREDir(GeckoAppShell.getContext());
> +                    FileUtils.delTree(tempDir, new FileUtils.NameAndAgeFilter(null, 1000*60*60*24), false);

Can we lift that constant?

Comment 27

[Wesley Johnston (:wesj)]

I added the PID to the files so that we could later try and use it to find old ones. Good idea I think :)


https://hg.mozilla.org/integration/fx-team/rev/8abcaed46e51
https://hg.mozilla.org/integration/fx-team/rev/8d358f011e50
https://hg.mozilla.org/integration/fx-team/rev/4166c73bace4

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8abcaed46e51
https://hg.mozilla.org/mozilla-central/rev/8d358f011e50
https://hg.mozilla.org/mozilla-central/rev/4166c73bace4
https://hg.mozilla.org/mozilla-central/rev/e67fd6177feb