Closed Bug 546728 Opened 14 years ago Closed 12 years ago

Thunderbird never finishes "copying to sent folder" on IMAP servers with case-insensitive folders

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 14.0

People

(Reporter: Parasyte, Assigned: Bienvenu)

References

()

Details

(Keywords: imap-interop, Whiteboard: [has protocol log] )

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a2pre) Gecko/20100217 Shredder/3.2a1pre

We run IceWarp Merak (by a series of unfortunate events; it was grandfathered into our infrastructure) which uses case-insensitive folder names.  That could be a Windows-only thing, I don't know for sure.

The problem is that Thunderbird, by default, looks for a folder named "Sent" to store email on the server when using IMAP.  By default, Merak creates a folder named "sent" in IMAP mailboxes.

When sending any mail with an IMAP connection, Thunderbird gets confused by this subtle issue, and then "hangs" with the "copying to sent folder" message and progress bar sitting indefinitely.  The operating can be canceled with the Cancel button.

Attached for developer convenience is two complete log files using:
NSPR_LOG_MODULES=imap:5,smtp:5,nsSocketTransport:5,nsHostResolver:5,timestamp

The first one is where it "hangs" and the second where it works flawlessly.  These logs confirm the bug is a case-sensitivity issue in Thunderbird; TB tries to create a "Sent" folder since it can't find one ("sent" is not accepted), and the server complains that "Sent" already exists (even though it is really "sent").  Thunderbird then does something totally unexpected: *nothing*  No error or warning message, no attempt to inform the user of what happened.

Merak logic: "Sent" == "sent"
Thunderbird logic: "Sent" != "sent"

The *only* difference between the two runs that produced these logs is that I changed the preference:
"mail.identity.id1.fcc_folder"

From: "imap://jason.oster@webmail.campnavajo.com/Sent"
To:   "imap://jason.oster@webmail.campnavajo.com/sent"

This is 100% reproducible.

Reproducible: Always




See also:
Bug 206408 - spin when copying message to "Sent" folder
Bug 250657 - Hangs (times out) at 100% when "Copying message to Sent folder" using IMAP and SSL, but the message is successfully copied to the sent folder.
Bug 275707 - Unable to save sent mail in Send folder every now and then
Bug 290059 - After sending mail, Thunderbird sometimes gets stuck moving the message to the "sent" folder
Bug 304173 - hanging for 10 minutes when "copying to Sent mail folder"
Bug 314730 - thunderbird never stops copying mail into sent-file until aborted
Bug 315680 - stuck copying mail to sent folder
Bug 325340 - Thunderbird hangs when copying mail to "sent" folder
Attached file Full log files
Full log files:

* Hang-up (incorrect case)
* Working (correct case)
It is important to note that the message is actually *sent correctly* ... it just does not get saved into the IMAP "Sent" folder.
Version: unspecified → Trunk
See also:
Bug 413240 - after sending message, "Copying to sent folder" doesn't finish
=> NEW based on clear STR, logs, no dupes found.

(In reply to comment #3)
> See also:
> Bug 413240 - after sending message, "Copying to sent folder" doesn't finish

This bug is actually quite different, as it's on POP3, not IMAP at all.
Severity: critical → major
Status: UNCONFIRMED → NEW
Component: General → Networking: IMAP
Ever confirmed: true
Keywords: imap-interop
Product: Thunderbird → MailNews Core
QA Contact: general → networking.imap
Whiteboard: [has protocol log]
Version: Trunk → 1.9.1 Branch
This bug has slightly better UX with recent TB 3.1 nightly builds.  Now we get a notification box which basically says the folder could not be created because it already exists.  But the "copying to sent" dialog still spins.
RFC 3501, Section 5.1 is pretty clear about how to handle case-sensitivity on IMAP folders (mailboxes).  Thunderbird is breaking spec here.

Also note that this is not limited to only the Sent folder, but also Drafts, and basically *any* folder on the IMAP server (except Inbox? Hmm, haven't tried that).


I tested this bug on a Dovecot server (implements case-sensitive folders) and the folder creation succeeds, creating both "sent" and "Sent" folders; however, the Thunderbird folder tree shows both as "Sent" which is confusing.

Trying to work on this bug, but I'm very unfamiliar with the code.  What I know so far:

/mailnews/compose/src/nsMsgCopy.cpp:
  nsMsgCopy::CreateIfMissing()

* When Thunderbird knows the folder exists, the call to (*folder)->GetParent(getter_AddRefs(parent)); succeeds (parent is non-null). This is what we want, no matter what case the folder name is.
* When Thunderbird cannot find the folder, the call fails; parent is null.

I don't have any idea where that comes from or how it gets there.  (Tracing it in a debugger is like following a maze.)


Some possibly related bugs:
Bug 160643 - case sensitive issue of the special 'IMAP Server Directory' setting - 'INBOX'
Bug 222223 - imap: trash case sensitive?
Version: 1.9.1 Branch → 1.9.2 Branch
Attached patch Proposed patch (obsolete) — Splinter Review
Andrew, this is a patch which I would like to have looked at more closely.  I don't think it's ready for review yet.

This fixes the problem by checking that the response to the IMAP "list" command was successful (eg. responded with "OK").  You can see in the logs from Comment #1, TB cannot find the folder, so it issues an "ensureExists", then a LIST on the folder.  Even though LIST returns OK, TB attempts a CREATE; failure.


A:ProcessCurrentURL:imap://jason%2Eoster@webmail.campnavajo.com:993/ensureExists%3E/Sent:  = currentUrl
A:SendData: 11 list "" "Sent"
A:CreateNewLineFromSocket: 11 OK LIST Completed

A:SendData: 12 create "Sent"


I'm wondering if this fix is "good enough" or if it would be better to make FolderVerifiedOnline() case-insensitive?  The latter sounds like a bad idea for case-sensitive servers.
Attachment #445219 - Flags: feedback?(bugmail)
Comment on attachment 445219 [details] [diff] [review]
Proposed patch

bienvenu is the king of IMAP and should be the one looking at such things.  However, he is also the king of a lot of other stuff too, especially 3.1-release related things, so it might be a bit before he can give full feedback...
Attachment #445219 - Flags: feedback?(bugmail) → feedback?(bienvenu)
Comment on attachment 445219 [details] [diff] [review]
Proposed patch

so, many imap servers don't return an error if you LIST a non-existent folder. You need to make sure that the LIST actually LISTs the folder we were trying to list, which is what the existing code was attempting to do. We list the folder, and then check that it was verified online, meaning we got back the folder we tried to list.
Attachment #445219 - Flags: feedback?(bienvenu) → feedback-
I'm not sure how to deal with this issue. The IMAP RFC basically says imap servers can do pretty much whatever they want with case, w/o any kind of hints about what it does (e.g., a capability response that describes what the server does w.r.t. case) , and the client has to deal with it. This is very convenient for the server that just wants to spit out the contents of files in the file system, but not so convenient for the client.

We could try to do case-insensitive lookups for special folders like Trash and Sent, and just deal with the users who want case-sensitivity even for special folders. Or we could try a case-sensitive lookup, and if that fails, try a case-sensitive one. That might be the easiest way out.
Attached patch Better proposed patch (obsolete) — Splinter Review
(In reply to comment #10)
> Or we could try a case-sensitive lookup, and if that fails, try a
> case-sensitive one. That might be the easiest way out.

[Assuming that was meant to say, "... if that fails, try a case-INsensitive one."]

That was what I originally wanted, but had trouble finding the code that does the case-sensitive lookup.  I think I've got it in this patch, now.

The previous patch was all-around bad, being too deep in the "failure case"; Even when it "successfully" allowed TB to copy the message into the folder, it also could also create additional subscriptions and local storage.  In my tests, one of each "sent" and "Sent" subscriptions and sew local folders.  (Testing on Linux, with a case-sensitive file system.)

The new patch implements the case-insensitive lookup as a fall-back to the original case-sensitive lookups.  As a result, folders can be found case-insensitively, and it doesn't further add subscriptions or local storage.

Maybe a test is wanted for this?  I could hack on the fakeserver xpcshell stuff for it.
Attachment #445219 - Attachment is obsolete: true
Attachment #445426 - Flags: feedback?(bienvenu)
(In reply to comment #10)

RFC is very clear on case sensitivity in non-INBOX mailbox names: No position.
> http://tools.ietf.org/html/rfc3501#section-5.1
> 5.1. Mailbox Naming
>(snip)
> In particular, this specification takes no position on case
>   sensitivity in non-INBOX mailbox names. 
"5.1. Mailbox Naming" merely says after it;
> Some server implementations
>   are fully case-sensitive; others preserve case of a newly-created
>   name but otherwise are case-insensitive; and yet others coerce names
>   to a particular case.
> Client implementations MUST interact with any of these.
(Note: merely "MUST interact". never "MUST treat it as server and user wants".)
> If a server implementation interprets non-INBOX mailbox
>   names as case-insensitive, it MUST treat names using the
>   international naming convention specially as described in section 5.1.3.
And, it seems that 5.1.3 merely says "sent returned as mbox name is sent at server" and "Sent returned as mbox name is Sent at server".

(In reply to comment #11)

Is "to lookup/do existent check for Trash, Sent, ... always" needed?
As main cause of problem is that case of special folder name in prefs setting is different from case of folder name which server really uses.
If server uses case insensitive name, server responds one of next to create;
  create "Sent" => (a) OK/Sent, (b) OK/sent, (c) NO/already exists
If (c), it's impossible to know real folder name at server. I think server returns (d) "sent" to LIST "Sent" or (e) "sent" to SELECT "Sent".
If (c) or (d) or (e) is reflected on prefs setting for special folder name or path, problem won't occur, because Tb uses "sent" here after.
It's reason why manual prefs change is always solution of the problem.

Followup actions are needed.
(i) If SUBSCRIBE Sent is previously isseued for (b), UNSUBSCRIBE is required. 
(ii) Tb on OS of case insensitive file system.
  If file of Sent.msf, ... is already created, rename it to sent.msf, ...
(iii) Tb on OS of case sensitive file system.
  - If Sent.msf, ... exists, rename to sent.msf. 
    (automatic rename won't produce any problem?)
  - If all of sent, Sent, sEnt, seNt, senT, SEnt, SeNt, SenT, SENt, SEnT,
    SeNT, SENT on case sensitive IMAP server should be supported correctly,
    all case insensitive comparison should be removed.

David, what do you think? Is such change possible?
Bug 558659 is for different solution(utilize autoconfig) of same as/similar to problem. xref.
Blocks: RFC6154
No longer blocks: RFC6154
Attached patch de-bitrotted patch (obsolete) — Splinter Review
I've de-bitrotted this, since it has taken me forever to look at this.
Attachment #445426 - Attachment is obsolete: true
Attachment #603124 - Flags: feedback?(dbienvenu)
Attachment #445426 - Flags: feedback?(dbienvenu)
Comment on attachment 603124 [details] [diff] [review]
de-bitrotted patch

yeah, this seems like a reasonable thing to do.
Attachment #603124 - Flags: feedback?(dbienvenu) → feedback+
Attachment #603124 - Flags: review?(dbienvenu)
Assignee: nobody → dbienvenu
I'll request a try server build with this patch, but it fixes the trash case for me, at least in the xpcshell test context.
Attachment #603124 - Attachment is obsolete: true
Attachment #609117 - Flags: review?(neil)
Attachment #603124 - Flags: review?(dbienvenu)
Comment on attachment 609117 [details] [diff] [review]
fix for trash folder, with unit test

>-  rv = rootMsgFolder->GetChildWithURI(aURI, true, false, getter_AddRefs(msgFolder));
>-
>-  // If we couldn't find the folder as is, check if we need to prepend the
>-  // personal namespace
>-  if (!msgFolder)
>-  {
>-    GetUriWithNamespacePrefixIfNecessary(kPersonalNamespace, aURI, folderUriWithNamespace);
>-    if (!folderUriWithNamespace.IsEmpty())
>-    {
>-      namespacePrefixAdded = true;
>-      rv = rootMsgFolder->GetChildWithURI(folderUriWithNamespace, true, false, getter_AddRefs(msgFolder));
>-    }
>-    else
>-      rv = rootMsgFolder->GetChildWithURI(aURI, true, false, getter_AddRefs(msgFolder));
I wonder why this call duplicates the one above...

Assuming (case sensitive), (prefixed case sensitive), (case insensitive), (prefixed case insensitive) is the correct order in which to look for folders, shouldn't you cache the uri with namespace to avoid fetching it twice?
(In reply to David :Bienvenu from comment #17)
> try server build with latest patch here:
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/
> bienvenu@nventure.com-4cbd6dd23624/

Thiq time the TRASH folder is used (no more Trash folder creation) with an imap.laposte.net account! Thanks David, great work!
(In reply to neil@parkwaycc.co.uk from comment #18)
> Comment on attachment 609117 [details] [diff] [review]
> fix for trash folder, with unit test
> 
> >-  rv = rootMsgFolder->GetChildWithURI(aURI, true, false, getter_AddRefs(msgFolder));
> >-
> >-  // If we couldn't find the folder as is, check if we need to prepend the
> >-  // personal namespace
> >-  if (!msgFolder)
> >-  {
> >-    GetUriWithNamespacePrefixIfNecessary(kPersonalNamespace, aURI, folderUriWithNamespace);
> >-    if (!folderUriWithNamespace.IsEmpty())
> >-    {
> >-      namespacePrefixAdded = true;
> >-      rv = rootMsgFolder->GetChildWithURI(folderUriWithNamespace, true, false, getter_AddRefs(msgFolder));
> >-    }
> >-    else
> >-      rv = rootMsgFolder->GetChildWithURI(aURI, true, false, getter_AddRefs(msgFolder));
> I wonder why this call duplicates the one above...
Different uri is passed in the two calls.
> 
> Assuming (case sensitive), (prefixed case sensitive), (case insensitive),
> (prefixed case insensitive) is the correct order in which to look for
> folders, shouldn't you cache the uri with namespace to avoid fetching it
> twice?
The namespace prefix isn't optional. We don't try it both ways (prefixed and non-prefixed). We either try prefixed + case sensitive and then prefixed and case-insensitive, OR, no prefix, case sensitive, no prefix case insensitive.
bienvenu, are you trying both names for all folders or just for the special folders? I'd prefer the latter.
(In reply to Ben Bucksch (:BenB) from comment #21)
> bienvenu, are you trying both names for all folders or just for the special
> folders? I'd prefer the latter.

mainly just the special folders, but the IMAP RFC essentially says we should do both, since it says we can't assume case-sensitivity or case-insensitivity, and the server won't tell us what it does.
(In reply to David Bienvenu from comment #20)
> (In reply to comment #18)
> > (From update of attachment 609117 [details] [diff] [review])
> > >-  rv = rootMsgFolder->GetChildWithURI(aURI, true, false, getter_AddRefs(msgFolder));
> > >-
> > >-  // If we couldn't find the folder as is, check if we need to prepend the
> > >-  // personal namespace
> > >-  if (!msgFolder)
> > >-  {
> > >-    GetUriWithNamespacePrefixIfNecessary(kPersonalNamespace, aURI, folderUriWithNamespace);
> > >-    if (!folderUriWithNamespace.IsEmpty())
> > >-    {
> > >-      namespacePrefixAdded = true;
> > >-      rv = rootMsgFolder->GetChildWithURI(folderUriWithNamespace, true, false, getter_AddRefs(msgFolder));
> > >-    }
> > >-    else
> > >-      rv = rootMsgFolder->GetChildWithURI(aURI, true, false, getter_AddRefs(msgFolder));
> > I wonder why this call duplicates the one above...
> Different uri is passed in the two calls.
> > 
> > Assuming (case sensitive), (prefixed case sensitive), (case insensitive),
> > (prefixed case insensitive) is the correct order in which to look for
> > folders, shouldn't you cache the uri with namespace to avoid fetching it
> > twice?
> The namespace prefix isn't optional. We don't try it both ways (prefixed and
> non-prefixed). We either try prefixed + case sensitive and then prefixed and
> case-insensitive, OR, no prefix, case sensitive, no prefix case insensitive.

I see one call with aURI passed, then if that fails another call with folderUriWithNamespace if there is one, otherwise another call with aURI...
(In reply to neil@parkwaycc.co.uk from comment #23)

> I see one call with aURI passed, then if that fails another call with
> folderUriWithNamespace if there is one, otherwise another call with aURI...

oh, higher up in the code, yes, you're right. I'm pretty sure the third call is superfluous. I'll remove it. I'll think about if the code should be reworked to avoid figuring out the uri with namespace twice.
I've removed the duplicate call to GetMsgFolderFromUri, cleaned up the long lines, and fix the param names to start with a. I don't think caching the uri with namespace is important - for almost all servers, the names will be in the right case to begin with, and the vast majority of trips through the code, the folder will exist, so the first call will succeed.

Oh, and I've switched the args to the method so the out parameter is last, and got rid of the default value for the case insensitive pref, since I think it makes the code a bit more readable.
Attachment #609117 - Attachment is obsolete: true
Attachment #610545 - Flags: review?(neil)
Attachment #609117 - Flags: review?(neil)
Comment on attachment 610545 [details] [diff] [review]
clean up patch a bit, remove unneeded code

>   bool namespacePrefixAdded = false;
[I still think that the code is a bit confusing because this variable is set if we discovered a namespace and so if the folder turns out not to exist then it needs to be created with the namespace. But if we happen to find the folder case insensitively without the namespace then this will still be set, although it happens not to actually matter because we don't use it in that case.]
Attachment #610545 - Flags: review?(neil) → review+
http://hg.mozilla.org/comm-central/rev/b3466dca8bc6 - I changed GetExistingMsgFolder to set namespacePrefix added to false at the top of the method so it should be more accurate.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Version: 1.9.2 Branch → Trunk
> I don't think caching the uri with namespace is important 

I think it would be good to store the correct URI on disk, though, to avoid that we make the same discovery over and over again, potentially hundreds of times. Opening a folder is an expensive op on the server, and there's no point in doing that again and again and again.
(In reply to Ben Bucksch (:BenB) from comment #28)
> > I don't think caching the uri with namespace is important 
> 
> I think it would be good to store the correct URI on disk, though, to avoid
> that we make the same discovery over and over again, potentially hundreds of
> times. Opening a folder is an expensive op on the server, and there's no
> point in doing that again and again and again.

Not sure I understand - this has nothing to do with opening a folder on the server - we're just grovelling through our local in memory folder tree.
OK, sorry, I misunderstood then and retract.
Hi gents,

Just done some additional tests with earlybird, and I am afraid that there are still some issues, especially with the French locale of Earlybird.
- download earlybird fr
- from a fresh profile, create a new mail account on imap.laposte.net
- thunderbird create the account but with two different Trash folders (called "Corbeille" in French) appear!
- after a refresh, the "Corbeille" folder takes the nice trash icons, and the second one stay there and is renamed "Trash".

Sorry, I think there is still some works to be done here :-(
(In reply to caméléon from comment #31)
> Hi gents,
> 
> Just done some additional tests with earlybird, and I am afraid that there
> are still some issues, especially with the French locale of Earlybird.
> - download earlybird fr
> - from a fresh profile, create a new mail account on imap.laposte.net
> - thunderbird create the account but with two different Trash folders
> (called "Corbeille" in French) appear!
> - after a refresh, the "Corbeille" folder takes the nice trash icons, and
> the second one stay there and is renamed "Trash".
> 
> Sorry, I think there is still some works to be done here :-(

When it worked for you, I guess you were using the English localization...but I think you should take your issue to a new bug, since this bug isn't about our handling of localizations.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: