Open Bug 1549424 Opened 6 years ago Updated 2 years ago

"unknown error" problem when attempting to save more than 16,000 temp files with the same name

Categories

(Toolkit :: Downloads API, defect, P3)

defect

Tracking

()

People

(Reporter: jason_q, Unassigned)

Details

Attachments

(1 file)

Attached image IMG_7725.jpg

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36

Steps to reproduce:

Repeatedly save new versions of a file with the same name tens of thousands of times.

Actual results:

Eventually subsequent downloads with the same filename fail, a 0 byte .part file is created in the temp directory but then an error message says that the file cannot be saved because an unknown error occurred. After acknowledging the error dialog the 0 byte .part file disappears and no new file is saved. I suspect an integer counter is incremented each time a new file of the same name is added to the temp directory, and in extreme cases such as this one the user is hitting the integer limit of the variable.

Expected results:

An additional file should have been successfully saved.

Summary: "unknown problem" error when attempting to save more than 16,000 temp files with the same name → "unknown error" problem when attempting to save more than 16,000 temp files with the same name

Does the browser get restarted, and are there still 16k files on disk at this point?

Flags: needinfo?(jason_q)

Yes. The temp files are the result of years of downloads in this user's case.

Flags: needinfo?(jason_q)

Also, none of the tools to clear the cache or downloaded files had any effect on these files. It was only when I deleted the temp files myself that normal function was restored.

(In reply to jason_q from comment #2)

Yes. The temp files are the result of years of downloads in this user's case.

So are they suffixed with -16000.pdf ? Or a random sequence of alphanumeric characters (more or less - base64, to be specific) ? Or something else?

Flags: needinfo?(jason_q)

(In reply to jason_q from comment #3)

Also, none of the tools to clear the cache or downloaded files had any effect on these files.

Right, this isn't a cache, and deleting downloads from the download manager doesn't delete the files, I think. I understand that's unfortunate in this case, but I think it's probably expected in other cases (certainly the cache side of things - if you got told to clear your cache to avoid issues with cookies or cached broken content, you'd be pretty unhappy if all your downloads got removed...)

I regret that I didn't note the maximum number before deleting them, but each file was suffixed with an incremented integer value. So the first was file.pdf, then file-1.pdf, file-2.pdf up to somewhere around file-16000.pdf.

Flags: needinfo?(jason_q)

This yak is too funny looking not to shave.

https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadPaths.jsm#103

This search tries up to 10000, but then falls back. Seems like the 16k limit has to be in the fallback. Looking there shows: https://searchfox.org/mozilla-central/source/xpcom/io/nsLocalFileCommon.cpp#143 which is also 10k, and that matches the loop: https://searchfox.org/mozilla-central/source/xpcom/io/nsLocalFileCommon.cpp#244

So I'm not sure why it failed at 16k files. We'd expect 10k.

Honestly I'm not sure what a fundamental fix for this would be. Just increasing the number might cause the user to have an insane amount of files in a single dir, which isn't something most filesystems like, and which might lead to downloads becoming progressively slower.

I think the bug is that https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadPaths.jsm#97 doesn't check the return value for that call. It is explicitly documented to return NS_ERROR_FILE_TOO_BIG if it can't find a unique name. So that call in DownloadPaths should propagate the error and present an understandable message.

Component: File Handling → Downloads API
Product: Firefox → Toolkit
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true

I cannot say for certain that it was 16,000 and not 10,000. I was in a hurry trying to solve a strange problem for a user and the number of files didn't seem important until I'd had some time to think about the problem.

Thanks, I believe we found the underlying code then.

If I can offer a user's perspective I don't think an improved error message will be sufficient. It is beyond the ability of most users to navigate to their temp directory and manage the files there. Whatever the fix the browser needs to take care of this for the user. I think a few better solutions, not being at all familiar with the code base and what other functions depend on this behavior might be:

  1. if the error dialog offered to remove all of the files
  2. if the error dialog offered to remove all files beyond a certain age, perhaps 30 days
  3. if the browser used a timestamp or a combination of a timestamp and random value to generate reliably unique names
  4. if the browser detected that it's hit the 10,000 file limit and began using a different naming scheme

I suspect this problem happens with some regularity. In this particular case the user is downloading inventory tracking labels to print them, a task that can be done hundreds of times a day. Obviously the application generating the labels could be a little nicer and generate unique file names itself but that is outside the user's control.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)

I think the bug is that https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadPaths.jsm#97 doesn't check the return value for that call.

Sorry, by "that call", do you mean the call to createUnique?

It is explicitly documented to return NS_ERROR_FILE_TOO_BIG if it can't find a unique name. So that call in DownloadPaths should propagate the error and present an understandable message.

xpcom APIs that return non-NS_OK throw an exception in JS. So DownloadPaths.jsm doesn't need to do anything: its consumers would need to try...catch and do specific handling for that exception. However, given that the 10k limit exists in multiple places I guess it'd need to be increased in all of those places and/or failure handled for all of those cases, in order for it to make a serious difference...

Also, ideally this code should be written in a way where we don't do exists() calls on 10k files, twice, to find the right download filename...

Flags: needinfo?(gpascutto)

Sorry, by "that call", do you mean the call to createUnique?

Yes, that will report the failure to actually find a unique name.

However, given that the 10k limit exists in multiple places I guess it'd need to be increased in all of those places and/or failure handled for all of those cases, in order for it to make a serious difference...

From the code I listed above, the JS will try 10k times, and then hand off to CreateUnique, which will try very similar logic to try another 10k times (this is obviously already very suboptimal!). So, I don't think you need to fix the cases in the JS, just make sure that when the CreateUnique handoff fails, you handle that.

Actually, given that the JS tries (1), (2), (3) and the CreateUnique call tries -1, -2, -3, maybe the limit that OP hit was closer to 20k :-)

It looks like the retrying in the JS code could be removed as CreateUnique already has that logic, but that may be a more invasive change unless you're very confident around this code.

Flags: needinfo?(gpascutto)

It is beyond the ability of most users to navigate to their temp directory and manage the files there

Your solution (4) is equivalent to just increasing the limit to 10k, 100k. I'm not sure if the performance issue with a large amount of files is relevant to a modern NTFS (or ext4/XFS) installation, but people use Firefox in all kinds of configurations including network shares and USB sticks with FAT32 etc. That's why I'd be hesitant to potentially allow putting 100k+ files in a single dir. (3) can fail in situations where there are limits on file name length and has some of the same problems of (4). And if what you say above is true, then (1) or (2) have a large potential to cause catastrophic data loss ("It was talking about temp files, where have all my past downloads gone?!")

An option may be to create a randomly named subdir. Maybe that's easier than trying to word a coherent enough error message that a user has a chance of figuring out what's going wrong :-)

An option may be to create a randomly named subdir.

The logic in that case would be to go directly to CreateUnique (which will work 10k times), and if it returns NS_ERROR_FILE_TOO_BIG, try to save the download in randomly named subdir (we can't use that logic in CreateUnique as it deals with different use cases). If you're on a FAT32 system, Firefox will run into this error again once you've saved 65k files instead of 10k. Doesn't seem very optimal either :-/

Well as I said I have no knowledge of the code base or the intricacies you guys are managing. However, my option 1 was meant to be specific to files of that same name, so something like:

"You've reached the limit for temporary files of this name [filename]. Would you like to remove the existing [filename] files to make room?"

I can see scenarios where even that might result in catastrophe, but at least it's a path forward and the user could always click "no" and still end up with a better understanding of the problem than they'd have now.

still end up with a better understanding of the problem than they'd have now.

I agree there, anything better than "unknown error"! A message like "Could not save X.pdf in c:\blah\downloaddir because a file with that name already exists." (when the 10k auto-renames fail) would give an exact clue what the problem is. Even if the user isn't capable of navigating there themselves, at least a Google Search or asking for help will give a chance of resolving it.

Well that certainly would have saved me some time. I still think it'd be better if the browser could handle it but I'm beginning to understand the complexity involved and the caution required in making such a change.

This is an edge case, anyway, but showing a better error would be nice.

Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: