Closed Bug 675448 Opened 9 years ago Closed 5 years ago

"Save as ..." multiple mails fails because of long pathnames and doesn't notice the user

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 38.0

People

(Reporter: robin.ladiges, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I selected several mails in my in-box, right-clicked on them and choosed "Save as...". In the prompting dialog I selected a directory (Windows XP, NTFS partition) and pressed "Ok".


Actual results:

The dialog diapeared, and the selected mails got saved in the choosen directory (*.eml). After checking the directory I noticed, that not all of my mails got saved. A few mails were missing.

Digging further I noticed, that it depends on the directory at which I'm saving into. Manually moving the created files to the directory where I wanted them caused the Windows Explorer to throw an exception which said that the filename was invalid or too long.

Example (redacted with X, length retained):
Thunderbird has no problem saving the file "[Fwd  Rechnung für Artikel ReNr XXXXX XXXXXX XXXXXXXX IAS Giga 4 x RJ45 Switch + 1 x DSL _ Cable + AP( Artikelnummer  XXXXXXXXXXXXX )...] - XXXXXXXXXXXXX (XXXXXXXXXXXXXXXXXXX) - 2009-05-18 1129.eml" (197 characters) in the directory "C:\Dokumente und Einstellungen\XXXXXXX\Desktop\emails\1234567" (61 characters), but fails (without noticing) to the directory "C:\Dokumente und Einstellungen\XXXXXXX\Desktop\emails\12345678" (62 characters).

This is very likely because of the 255-chars filesize limit of the underlying NTFS-filesystem. (the full filepath without the file extension is in the example 255 and 256 characters long)

Bug 310925 is similar to this (but this one isn't about attachments). It may be a problem with every "Save as..." of multiple items. The "Save as..." of a single item does correctly show a error message.


Expected results:

Thunderbird should check the return value of the system call it is calling (POSIX open() ???) to check if the call was a success, and show a error message, to notice the user which files couldn't be saved, if it wasn't.

It would be nice to analyse the error number (errno==ENAMETOOLONG in my specific case) if a error occurs, and deliver a user-friendly (localised) error message.
Summary: "Save as ..." multiple mails fails because of long filenames / pathnames and doesn't notice the user → "Save as ..." multiple mails fails because of long pathnames and doesn't notice the user
Related:

When saving a single mail, and the subject of the mail contains characters which are not allowed (e.g.: colon :, semikolon ;, question mark ?) in a filename, the unallowed characters get removed in the file saving dialog.

When saving multiple files, the unallowed characters will not get removed. In this situation the mail doesn't get saved (the system call fails because of the unallowed characters), and the user doesn't get a notice about it.

The expected result would be to automatically remove any unallowed characters from the filenames, before giving them as a parameter to the systemcall, like when saving a single mail.

This may be filled in as a new bug, but the effect that the user gets no notice about the failure suites here.
(In reply to comment #1)
> Related:
> 
> When saving a single mail, and the subject of the mail contains characters
> which are not allowed (e.g.: colon :, semikolon ;, question mark ?) in a
> filename, the unallowed characters get removed in the file saving dialog.
> 
> When saving multiple files, the unallowed characters will not get removed.
> In this situation the mail doesn't get saved (the system call fails because
> of the unallowed characters), and the user doesn't get a notice about it.
> 
> The expected result would be to automatically remove any unallowed
> characters from the filenames, before giving them as a parameter to the
> systemcall, like when saving a single mail.
> 
> This may be filled in as a new bug, but the effect that the user gets no
> notice about the failure suites here.

Ignore that last comment. Tests with other mails with unallowed characters in the subject shows me, that the characters in fact get removed. But it reproducable fails without an error on multiple other mails (with a colon : in the subject, but the problem is a too long From field which gets part off the filename when saving multiple mails).
I can confirm this bug.

Initially reported to me by a coworker, i did a quick test exporting a folder with thousand of messages and was able to reproduce it, some emails slipped without any notice.

Specifically, i created an output folder with the name "D:\bbbbbbbbbb" and exported (save as) a mailbox with 3458 emails, that folder on disk had then 3457 emails saved. Renaming that folder to "D:\b" and exporting them again showed all of them were saved properly. Digging into which email slipped, found it was one with exactly 255 characters (254 + \0)

I'm using Windows 7 32-bits, while my coworker is using OSX. Both of us with Thunderbird 31.3.0

I think at the very least TB should check and inform the user if there was some problem saving to disk any email, as OP originally suggested, ultimately checking if the file exists on disk since i'd be quite surprised if the codebase doesn't perform already the required checks against the used I/O functions.

If you need any further info/test from us please let me know it, thank you.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Version: 5.0 → 31
I have a recollection this was fixed semi-recently?
Component: OS Integration → Backend
Product: Thunderbird → MailNews Core
I can re-confirm that the reported behavior is still observable, now with an Lubuntu 14.04 operating system and an ext4 file system (255 byte filenames).

User Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.3.0
Build ID: 20141128200248
See Also: → 1093035
See Also: 1093035
See Also: → 1093035
Assignee: nobody → mkmelin+mozilla
Attached patch bug675448_long_filename.patch (obsolete) — Splinter Review
Ok so the problem is we don't check the rv at the end of the loop before going to next. I made the automatic file names cap themselves in the middle so we don't go above 255 chars which normally is what file systems support.

It doesn't fix it for IMAP which apparently don't have any error checking at all for this. Fixing that would be a larger project, and this is all very ugly anyway (file handling not even async for pop, error handling with alert from backend etc).
Attachment #8550756 - Flags: review?(kent)
Status: NEW → ASSIGNED
Hardware: x86 → All
Comment on attachment 8550756 [details] [diff] [review]
bug675448_long_filename.patch

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

While this patch is an improvement over the current situation (with an error check, and a file length limit which may be helpful on other operating systems) it does not solve the issue of the OP. This bug is about a path length limit of 255 characters on Windows, while the patch is only concerned with filename length.

A little Yahooing got me to http://support.microsoft.com/kb/177665/EN-US/ which implies that a 255 character path limit is a limitation of Win32 programs.

So how would you like to proceed Magnus? It would be good to make forward progress with this. The full path though is not specified at the javascript level but only in nsMessenger.cpp, so the fix would need to be there. It would not be hard to truncated file name there to limit path the 255 chars on Windows systems, and filename to 255 on other systems (if appropriate).
Please note the issue i was reporting happened saving a filename with 254 characters under a folder whose name had 10 characters ("bbbbbbbbbb")

So, the patch won't prevent my issue, but at least it'd notify me that the email wasn't saved to disk, it's a good progress, thanks :)
Attached patch bug675448_long_filename.patch (obsolete) — Splinter Review
Ah yes...
Attachment #8550756 - Attachment is obsolete: true
Attachment #8550756 - Flags: review?(kent)
Attachment #8554161 - Flags: review?(kent)
Comment on attachment 8554161 [details] [diff] [review]
bug675448_long_filename.patch

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

Need one more update
Attachment #8554161 - Flags: review?(kent)
Attached patch bug675448_long_filename.patch (obsolete) — Splinter Review
Attachment #8554161 - Attachment is obsolete: true
Attachment #8554203 - Flags: review?(kent)
Comment on attachment 8554203 [details] [diff] [review]
bug675448_long_filename.patch

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

I tried this and it worked for me, so overall this is a good direction. But I think I should look it over once more when you deal with the possible underflow of the "x" variable.

::: mailnews/base/src/nsMessenger.cpp
@@ +953,5 @@
> +/**
> + * Adjust the file name, removing characters from the middle of the name if
> + * the the name would otherwise be too long - too long for the what file
> + * systems usually support.
> + */

Nit "- too long for the what file" remove the "the"

@@ +971,5 @@
> +    rv = aFile->GetLeafName(leafName);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    uint32_t x = MAX - (path.Length() - leafName.Length()); // x = max leaf size
> +    nsAutoString trunactedLeaf;

Nit: spelled incorrectly, use truncatedLeaf

Not a nit:
If you had a file system that supported path lengths greater than 255, then at this point you could end up with negative x, which would overflow to a very large value.

I'm not really sure of a simple way to deal with this. One simple possibility would be to simply fail if path.Length() - leafName.length > 255

@@ +1398,5 @@
> +    if (NS_FAILED(rv)) {
> +      NS_IF_RELEASE(saveListener);
> +      Alert("saveMessageFailed");
> +      return rv;
> +    }

Bonus points for using nsRefPtr<> with saveListener so that all of these NS_IF_RELEASE calls are not needed.
Attachment #8554203 - Flags: review?(kent) → review-
Attachment #8554203 - Attachment is obsolete: true
Attachment #8556678 - Flags: review?(kent)
Comment on attachment 8556678 [details] [diff] [review]
bug675448_long_filename.patch

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

LGTM.

::: mailnews/base/src/nsMessenger.cpp
@@ +951,5 @@
>  #define TEXT_FILE_EXTENSION ".txt"
>  
> +/**
> + * Adjust the file name, removing characters from the middle of the name if
> + * the the name would otherwise be too long - too long for what file systems

One more nit: "the the"
Attachment #8556678 - Flags: review?(kent) → review+
https://hg.mozilla.org/comm-central/rev/a55857b15908 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Duplicate of this bug: 711016
Blocks: 1093035
See Also: 1093035
You need to log in before you can comment on or make changes to this bug.