Closed Bug 1237956 Opened 10 years ago Closed 9 years ago

Files uploaded to server have their filename changed

Categories

(Firefox for Android Graveyard :: General, defect, P1)

Product:
Firefox for Android Graveyard
Component:
General
Version:
43 Branch
Type:
defect

Tracking

(fennec+, firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
fennec + ---
firefox54 --- verified

People

(Reporter: the.decryptor, Assigned: jwu)

Details

Attachments

(1 file)

Bug 1237956 - Use temporary folder instead of modifying the filename while uploading,
59 bytes, text/x-review-board-request
sebastian
: review+
Details
When you select a file to upload using the file picker, the filename has the string "tmp_XXXX-" (Where each X is a number between 0-9) prepended to it, and that is passed onto the server. This might be a base Android problem, as some file providers (e.g. Photos) seem to be immune to it and pass the correct filename, but in both cases Chrome doesn't show the "tmp" string.
Is there a specific site you are using for the photo uploads? Facebook, Twitter, Imgur?
I first noticed it on http://hostr.co (Imgur and Twitter both rename the files you upload), but even a plain page with a file input shows it. data:text/html,<!doctype html><input type="file"> If I select a file from the "Downloads" item in the sidebar of the "Open from" picker, any one of those files has the "tmp_" bit prepended.
Tested using: Device: Samsung Galaxy S6 Edge (Android 5.1.1) I've uploaded a photo with the title: "The-fourth-and-final-IMAX.." on Firefox using 43.0 and latest 46.0a1 on imgur.com and hostr.co and the image appears with "temp_1167-" in front of the title: http://i.imgur.com/zwURIJP.png On Chrome, image title is displayed correctly: http://i.imgur.com/3eIeJNE.png
OS: Android 6.0.1 (Samsung G935F) Browsers: Firefox 50.1.0 and Aurora 52.0a2 By accessing name property of selected file in input element via javascript, instead of getting name of chosen file, Firefox for Android returns tmp_#####-filename##########.extension (# = random number). If file being selected is "someimg.jpg" it returns "tmp_27350-someimg1842134916.jpg" and it is the name by which the file is being uploaded. The number between name and extension is randomized every time file is selected and it is 10 numbers wide (sometimes it is 9 numbers wide). The number after tmp_ is 5 numbers wide and randomized only when firefox is restarted, it is not randomized when page is reloaded or other file selected. This happens only in Firefox... I've tried Chrome, Samsung Internet, Opera, UC browser and Dolphin and they all return real file name.
Setting flag for triage: It would be interesting to investigate why we are doing this and if it is easy to fix.
tracking-fennec: --- → ?
tracking-fennec: ? → +
Priority: -- → P3
The behavior of uploading is 1. Copy file from other apps to local cache folder(/data/data/org.mozilla.firefox/cache/uploads/), 2. Ask Gecko to do the upload by filename(path + name) given I think the prefix(tmp_#####-) is used to indicate it's a temporary file and the suffix(##########) is used to prevent filename conflict. Maybe we can use a temporary folder instead of modifying the filename. Any suggestion? Sebastian, if you think this solution is acceptable, I can give it a try.
Flags: needinfo?(s.kaspari)
Some DXR links to the code would have been helpful. :) (In reply to Jing-wei Wu [:jwu] from comment #6) > The behavior of uploading is > 1. Copy file from other apps to local cache > folder(/data/data/org.mozilla.firefox/cache/uploads/), Can you see why from the code why we copy the file to the cache folder? > I think the prefix(tmp_#####-) is used to indicate it's a temporary file and > the suffix(##########) is used to prevent filename conflict. Maybe we can > use a temporary folder instead of modifying the filename. A temporary folder might be a solution. I wonder if we could avoid copying the file in the first place. Is it really needed?
Flags: needinfo?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #7) > A temporary folder might be a solution. I wonder if we could avoid copying > the file in the first place. Is it really needed? Yes, I think the file copy is necessary. Because the file content might be modified by other apps while uploading. For example, delete the file while uploading. Copy files into our cache folder can prevent this problem because other apps has no privilege to access them. I'll try to use a temporary folder to fix this issue.
Assignee: nobody → topwu.tw
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8828678 [details] Bug 1237956 - Use temporary folder instead of modifying the filename while uploading, https://reviewboard.mozilla.org/r/106018/#review107910 ::: mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java:201 (Diff revision 1) > @Override > public void onLoadFinished(Loader<Cursor> loader, Cursor cursor) { > if (cursor.moveToFirst()) { > String name = cursor.getString(0); > // tmp filenames must be at least 3 characters long. Add a prefix to make sure that happens > String fileName = "tmp_" + Process.myPid() + "-"; Looks like this line can be moved now, so that we only generate this file name if needed. ::: mobile/android/base/java/org/mozilla/gecko/FilePickerResultHandler.java:215 (Diff revision 1) > fileExt = name.substring(period); > - fileName += name.substring(0, period); > + tempFile = name.substring(0, period) + fileExt; This is weird: We first split the file name into extension and name and then we reassmble it again. Isn't this the same as tempFile = name? ::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/FileUtils.java:265 (Diff revision 1) > + final String folderName = "tmp_" + Process.myPid() + "-" + SystemClock.uptimeMillis() + "-"; > + for (int counter = 0; counter < TEMP_DIR_ATTEMPTS; counter++) { > + File tempDir = new File(baseDir, folderName + counter); File.createTempFile() uses random. Maybe worth copying the approach? https://android.googlesource.com/platform/libcore.git/+/android-4.2.2_r1/luni/src/main/java/java/io/File.java#996
Attachment #8828678 - Flags: review?(s.kaspari) → review+
Comment on attachment 8828678 [details] Bug 1237956 - Use temporary folder instead of modifying the filename while uploading, https://reviewboard.mozilla.org/r/106018/#review107910 > Looks like this line can be moved now, so that we only generate this file name if needed. The temp file name is still required if cursor.getString(0) returns null, but it should put into the if() block below. I'll send a new path for review. > This is weird: We first split the file name into extension and name and then we reassmble it again. Isn't this the same as tempFile = name? Yes, it's totally redundant and should be removed. Please help review the new patch. > File.createTempFile() uses random. Maybe worth copying the approach? > https://android.googlesource.com/platform/libcore.git/+/android-4.2.2_r1/luni/src/main/java/java/io/File.java#996 I rewrite createTempDir() based on File.createTempFile(), not sure if it meets your requirement. Also I create a test case in TestFileUtils.java to check this new method. Please help review the change and give me any advice.
(In reply to Jing-wei Wu [:jwu] from comment #12) > I rewrite createTempDir() based on File.createTempFile(), not sure if it > meets your requirement. Also I create a test case in TestFileUtils.java to > check this new method. Perfect! :) > Please help review the change and give me any advice. I already r+'ed the previous patch - so after updating the patch you can go ahead, push to try and land. :)
Priority: P3 → P1
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a4a6217867d0 Use temporary folder instead of modifying the filename while uploading, r=sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4a6217867d0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Verified as fixed on Nighty 54 (2017-30-01), on the following devices: - Motorola Razr - Android 4.4.4 - Huawei Honor 8 - Android 6.0 Uploaded photos and files on imgur.com and flickr.com and verified that the tmp prefix isn't added anymore.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: