909547 - File Picker should avoid using temp files

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Our current file picker sometimes gets results back that aren't file uri's (I'm not sure when that is exactly.... probably depends on what the app returns I guess. Some of them may return content provider uris) When it does, it downloads the file to a tmp file using File.createTempFile(); That results in a random file name.

We should try a harder to maintain the name if we can in these cases. If there's a conflict with an existing file, we can try to make something unique. Otherwise we should just use the default.

createTempFile doesn't automatically schedule cleanup for the files, and deleteOnExit doesn't work well on Android either [1]. We should maintain a list of these files somewhere and delete them when we shutdown? Separate bug?

[1] https://developer.android.com/reference/java/io/File.html#deleteOnExit%28%29

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This just uses our normal temp directory to store these, and puts numbers after the name to ensure they remain unique. i.e.

file.png
file 1.png
file 2.png
etc.

We no longer have to worry about the 3-char minimum since we're not using createTempFile(). I'm not sure about cleanup of these files, but I don't think we're any worse off than we already were. i.e. createTempFile doesn't clean up after itself either. I'm vaguely hoping that Gecko periodically cleans out the tmp dir for us? but I'll file a separate bug for that.

Comment 2

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

Given the overhead of trying to find the right spot in the code to cleanup temp files, I think we should clean up temp files when we try to make a temp file. We should also strive to never need to add numbers to make a file unique. If we attempt to cleanup stale files before we write the new file, chances are good we won't have file 1.png.

Comment 3

[Wesley Johnston (:wesj)]

Hmmm... can we do that? If I open a webpage with multiple input[type="file"] nodes in it and attach multiple (different if you want) files, I'm not sure we can safely delete those temp files until the page, and all handles to those files, are dead? i.e. If gecko opens the file for reading when the file is added to the input, maybe we can safely call delete and assume it won't happen until the file is closed, but I'm not sure Gecko does that.

Comment 4

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

(In reply to Wesley Johnston (:wesj) from comment #3)
> Hmmm... can we do that? If I open a webpage with multiple input[type="file"]
> nodes in it and attach multiple (different if you want) files, I'm not sure
> we can safely delete those temp files until the page, and all handles to
> those files, are dead? i.e. If gecko opens the file for reading when the
> file is added to the input, maybe we can safely call delete and assume it
> won't happen until the file is closed, but I'm not sure Gecko does that.

I was making an assumption that we would use some kind of "is a file stale?" check. So we would only delete files that are X hours old, or something. We can't really delete files on shutdown. We shouldn't waste time during startup to delete files (even on a separate thread since this is a lie for single core devices). I figure the best time might be "on use".

There is a second idea: Use a background service. If we use a service, I would hope that are other tasks the JanitorService could do too.

Comment 5

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

Thinking... a service might not be run often enough to clean up files. We could still have unused files in the dir. Maybe we should be making new temp sub-folders so we are always sure the file name is untouched?

Comment 6

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

Comment on attachment 799164 [details] [diff] [review]
Patch v1

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

::: mobile/android/base/FilePickerResultHandler.java
@@ +62,5 @@
>              int period;
>              if (name == null || (period = name.lastIndexOf('.')) == -1) {
>                  String mimeType = cr.getType(uri);
> +                fileExt = GeckoAppShell.getExtensionFromMimeType(mimeType);
> +                fileName = name;

fileName can be set to null here, which I don't think you want.

@@ +67,2 @@
>              } else {
> +                fileExt = name.substring(period+1);

nit: spaces around the '+'

@@ +85,5 @@
> +
> +                // Replace the number on the end of the filename with a new one
> +                num++;
> +                fileName = fileName.replaceAll("\\s\\d+$", " " + Integer.toString(num));
> +                file = new File(tempDir, fileName + "." + fileExt);

I don't really like this approach of regex replacement. It seems unnecessarily brittle and complex to me, and if the filename originally ends in a number you could end up incrementing it. I would prefer having a "baseName" to which you just tack on an incrementing counter while you have collisions.

String baseName = ...
File file = new File(tempDir, baseName + "." + fileExt);
for (int suffix = 1; file.exists(); suffix++) {
    file = new File(tempDir, baseName + " " + suffix + "." + fileExt);
}
// use file for whatever

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

This used to get called only once on the gecko thread. If it is called multiple times concurrently from different threads we could end up trying to delete files that don't exist in the delTree call, which would be bad (although looking at the method docs it shouldn't throw any exceptions). Although that shouldn't happen with your current code (because FilePickerResultHandler should only run much after gecko startup) by making this function public you're introducing a footgun that somebody else will use it without realizing this. At the very least I would add a method-level comment warning about this.

Comment 7

[Wesley Johnston (:wesj)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> I don't really like this approach of regex replacement. It seems
> unnecessarily brittle and complex to me, and if the filename originally ends
> in a number you could end up incrementing it. I would prefer having a
> "baseName" to which you just tack on an incrementing counter while you have
> collisions.
> 
> String baseName = ...
> File file = new File(tempDir, baseName + "." + fileExt);
> for (int suffix = 1; file.exists(); suffix++) {
>     file = new File(tempDir, baseName + " " + suffix + "." + fileExt);
> }
> // use file for whatever

With this, if I attach the same file to a filepicker repeatedly I'll get something like:

file.png
file 1.png
file 1 1.png
file 1 1 1.png

Unless we retain some list of what filenames we control, we'll have to risk altering names. Maybe it would be better if I made the regex a little more special. Look for "(#)" patterns?

I'll try to split the getTmpDir stuff up to be safer. We have the delTree function duplicated in our code right now too, so maybe its time for a static FileUtils class.

Comment 8

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

(In reply to Wesley Johnston (:wesj) from comment #7)
> With this, if I attach the same file to a filepicker repeatedly I'll get
> something like:
> 
> file.png
> file 1.png
> file 1 1.png
> file 1 1 1.png

Ah, good point. I hadn't considered that case.

> Unless we retain some list of what filenames we control, we'll have to risk
> altering names. Maybe it would be better if I made the regex a little more
> special. Look for "(#)" patterns?

Yeah that might be better. Desktop tacks on (#) I believe so it would be nice to be consistent with that.

> I'll try to split the getTmpDir stuff up to be safer. We have the delTree
> function duplicated in our code right now too, so maybe its time for a
> static FileUtils class.

Sounds good.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Create a FileUtils class

This is a bit unrelated, but I noticed some duplicated code in here and decided to fix it. This creates a fileUtils class with the delete code used in GeckoProfile/GeckoLoader in it. Currently utils depends on mozglue, but I needed to invert that for this to work, so it also does that.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2 - Put files in a tmp directory

This fixes up the files by creating temp directories to put them in rather than trying to be clever with the names. Note file pickers are currently broken because of Bug 914381. Will land that fix soon (working on a test).

Comment 11

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

Comment on attachment 801920 [details] [diff] [review]
Patch 1/2 - Create a FileUtils class

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

I didn't realize you were going to gut mozglue quite so much. But I don't have any particular objections to this, I guess.

::: mobile/android/base/util/FileUtils.java
@@ +28,5 @@
> +
> +    public static File createTempDir(String basename, String extension, File baseDir)
> +            throws IOException {
> +        File dir = File.createTempFile(basename, extension, baseDir);
> +        if(!dir.delete()) {

space before (. Also, does delete() return false if the file doesn't exist? If so then this will return null most of the time, which I don't think you want.

Comment 12

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

Comment on attachment 801921 [details] [diff] [review]
Patch 2/2 - Put files in a tmp directory

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

r=me with comments addressed

::: mobile/android/base/FilePickerResultHandler.java
@@ +68,2 @@
>              } else {
> +                fileExt = name.substring(period+1);

nit: spaces around +

@@ +71,5 @@
>              }
> +
> +            File dir = FileUtils.createTempDir(fileName, "." + fileExt, GeckoLoader.getTmpDir(GeckoAppShell.getContext()));
> +            if (dir == null) {
> +                return "";

Maybe log an error here?

::: mobile/android/base/mozglue/GeckoLoader.java.in
@@ +133,5 @@
>          // profile home path
>          putenv("HOME=" + profilePath);
>  
> +        // clean up any old tmp dirs lying around
> +        cleanupOldTmpDir(context);

I don't think this comment is accurate. The cleanup is for the specific tmp dir we used to use before we had to rename it because it was world-readable. If we ever get to the state where we have no more users migrating across bug 844832, we should be able to take this code out.

Also it might be a good idea to move this call off the startup code path into a runnable that is scheduled for later. Less disk I/O on startup is good.

Comment 13

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)

> Also it might be a good idea to move this call off the startup code path
> into a runnable that is scheduled for later. Less disk I/O on startup is
> good.

Agreed. In fact Wes and I (and Brian) talked about possibly making a JanitorService that runs in the background and occasionally cleans up temp files. That would remove it from the app code altogether.

Comment 14

[Wesley Johnston (:wesj)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Comment on attachment 801920 [details] [diff] [review]
> Patch 1/2 - Create a FileUtils class
> 
> Review of attachment 801920 [details] [diff] [review]:
> -----------------------------------------------------------------
> I didn't realize you were going to gut mozglue quite so much. But I don't
> have any particular objections to this, I guess.

This got a bit more involved today with the Generating classes. i.e. the util/Clipboard.java code relies on the Generator which means util has another dependency on mozglue.

It feels nice to have that code in MozGlue too, since it really is our glue. I think I'm just going to toss this patch. It doesn't feel worth it to save this one little method, and either way its orthogonal to this patch.

Comment 15

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.