User-Agent: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.8) Gecko/20100214 Ubuntu/9.10 (karmic) Firefox/3.5.8 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4 When Thunderbird create a new imap account, it looks if special folder already exist and create them if this is not the case. However, the search seems to be case sensitive so Thunderbird doesn't use a folder call "TRASH" and create a new one (called "Trash"). This result in duplicate of folder which are a pain to correct for the user. Reproducible: Always Steps to Reproduce: 1. Create an imap account on http://www.laposte.net/ (French provider) 2. Thunderbird succeed Actual Results: Unfortunately, it has created a new Trash folder and doesn't use the existing one Expected Results: Thunderbird should use the "TRASH" folder that already exist This problem should be corrected for every others special (junk, send, draft,...) folders, not only the trash.

Can you provide us with an imap log ? (see https://wiki.mozilla.org/MailNews:Logging for instructions)

-1336176640[1c5818c0]: 2f2f600:imap.laposte.net:A:CreateNewLineFromSocket: * LSUB () "." "INBOX.TRASH" -1336176640[1c5818c0]: ReadNextLine [stream=1862d4b8 nb=37 needmore=0] -1336176640[1c5818c0]: 2f2f600:imap.laposte.net:A:CreateNewLineFromSocket: 5 OK Completed (0.000 secs 5 calls) -1336176640[1c5818c0]: 2f2f600:imap.laposte.net:A:SendData: 6 list "" "INBOX" -1336176640[1c5818c0]: ReadNextLine [stream=1862d4b8 nb=35 needmore=0] -1336176640[1c5818c0]: 2f2f600:imap.laposte.net:A:CreateNewLineFromSocket: * LIST (\HasChildren) "." "INBOX" -1336176640[1c5818c0]: ReadNextLine [stream=1862d4b8 nb=37 needmore=0] -1336176640[1c5818c0]: 2f2f600:imap.laposte.net:A:CreateNewLineFromSocket: 6 OK Completed (0.000 secs 6 calls) -1336176640[1c5818c0]: 2f2f600:imap.laposte.net:A:SendData: 7 list "" "INBOX.Trash" extracts from the log. David I have a usable account on laposte.net for testing if needed. It seems that we donct detect TRASH as being the Trash.

Issue is most likely that IMAP server is creating IMAP.TRASH, and we're asking the server for IMAP.Trash. There's no way to ask the server case-insensitively for a folder. Maybe the backend could do a case-insensitive compare for the existing trash folder so that we wouldn't try to list it...

Thanks for your log Ludovic, Maybe some tests should be done, for instance looking for the Trash as: Trash, trash, TRASH,... The sent folder can be search as: Sent, sent, SENT, OUTBOX (name of this folder on laposte.net), ... The draft folder can be search as: Draft, draft, DRAFT, Drafts, drafts, DRAFTS (don't know why this one is plurial by default in Thunderbird?) Etc Etc... It would really help average user to configure their imap account!

Note that I wonder if I should open a new bug to signal that Thunderbird create a new "Drafts" folder whereas a "DRAFT" folder already exist on the server (still on laposte.net). What is your felling?

(In reply to comment #6) Basically, case-sensitive/case-insensitive folder name issue on special folders is same as Trash or TRASH issue of this bug, although trash particular issues are involved in this bug. There is no need to open separated bug for other folders, unless developer will request us(different patch for each folder may be required). > Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4 Both "trash" displayed at folder pane is "Trash" and "TRASH" in English? Or translated name? General way to specifiy folder of all upper case character. (1) Trash At Server Settings, IMAP delete model choice, choose "move to trash", select "INBOX.TRASH"(INBOX->TRASH) as folder for trash(folder on which Empty Trash is executed). And, change back to other delete model if required. (2) Junk At Junk Settings, choose INBOX->JUNK at Others: (3) Sent etc. At Copies&Folders, choose INBOX->folder_of_uppercase at Others: However, if localized folder name by Tb is displayed at folder pane, localized string is written in mail.server.serverN.trash_folder_name by (1). And, if name space is used, mismatch between namespace and trash_folder_name set by (1) can happen. Both are known bugs. If INBOX.TRASH is required for trash folder when namespace="INBOX." is used, specify next via Config Editor, mail.server.serverN.trash_folder_name=TRASH restart Tb, and never touch trash folder selection UI, please. If both INBOX.Xyz and INBOX.XYZ are already created on server, Tb may hide one of two, because of internal case-insensitive comparison in some places. If not hidden, delete INBOX.Xyz by Tb, unsubscribe INBOX.Xyz by Tb if required. If hidden, delete INBOX.Xyz at server. > Expected Results: > Thunderbird should use the "TRASH" folder that already exist If server supports XLIST, and server returns * XLIST (\Trash) "." "INBOX.TRASH", Tb pobably uses INBOX.TRASH.

I already know the workaround to make it works, I have written a long explanation on French forums (here): http://www.geckozone.org/forum/viewtopic.php?f=4&t=69304 But I am sure that only good user can use it. Then, the draft folder for Laposte is singular (DRAFT), not plural (Drafts). Unfortunately, it seems that Laposte.net doesn"t support XLIST. Note that Orange, which is the first ISP in France, also has the same folder structure.

(In reply to comment #8) > Then, the draft folder for Laposte is singular (DRAFT), not plural (Drafts). > Unfortunately, it seems that Laposte.net doesn"t support XLIST. > Note that Orange, which is the first ISP in France, also has the same folder > structure. Enhancement of autoconfig may be a solution. > https://wiki.mozilla.org/Thunderbird:Autoconfiguration > https://live.mozillamessaging.com/autoconfig/ > https://live.mozillamessaging.com/autoconfig/orange.fr Add support of IMAP, add support of special folder names commonly used by server. <incomingServer type="imap"> <hostname>imap.orange.fr</hostname> <port>143</port> <socketType>plain</socketType> <username>%EMAILLOCALPART%@%EMAILLDOMAINPART%</username> <authentication>plain</authentication> > <imap_folders> > <imap_Trash>TRASH</imap_Trash> => mail.server.serverN.trash_folder_name=TRASH > <imap_Sent>SENT</imap_Sent> => mail.server.serverN.fcc_folder_name(New) > <imap_Drafts>DRAFT</imap_Drafts> => mail.server.serverN.drafts_folder_name(New) > </imap_folders> </incomingServer>

I wonder if this bug shouldn't block: Bug 422814 - (autoconfig) Make account configuration quick, easy, and more secure Because it would really made account configuration easier!

Bug 422814 is long fixed, and it thus can't block that bug. This is an RFE. We already have a bug filed for this. => DUPME

Bug 475139, also filed by caméléon, asked for the same thing. That's the one I was thinking of. Marking that one as dup, because this one is more targetted. Also compare bug 471174.

Compare bug 476260: If laposte.net would support the XLIST extension to IMAP (which they currently don't), we would detect the folders automatically. I also think that's the technically better way to do it instead of autoconfig files.

I sure it would be better if laposte.net (and orange.fr, first ISP in France) support XLIST, but actually they don't and I assume they are not alone... So I understand that your preference would be to fix it via autoconfig? Or should we wait that our ISP support XLIST?

My preference would be to use XLIST. I think IMAP extensions are the correct way to handle this problem, and it turns out there is one, and we even support it. However, that may not be feasible in this case, given that - XLIST is not a standard - Not many ISPs support it I think we may need a workaround like this one for the next 2-3 years.

(In reply to comment #16) > However, that may not be feasible in this case, given that > - XLIST is not a standard > - Not many ISPs support it > I think we may need a workaround like this one for the next 2-3 years. XLIST, or something similar, is on track to become a standard. But yeah, we're going to need a workaround for the next couple years. Case-insensitive folder name matches helps a little, but not in the case where the server has localized the folder names.

> XLIST, or something similar, is on track to become a standard Great! Do you have any reference? > server has localized the folder names. Yes, that's very common. Or different terms like "Sent" vs. "Sent Mail" vs. "Sent Items" and "Draft" vs. "Drafts".

(In reply to comment #18) > > XLIST, or something similar, is on track to become a standard > > Great! Do you have any reference? I can't find the actual draft, but there's a non-working link to it here: http://www.ietf.org/mail-archive/web/morg/current/msg00248.html

Here you go working link http://www.ietf.org/id/draft-ietf-morg-list-specialuse-01.txt

Thanks for the link Nicolas. Anyway, it would very be great to have a workaround soon. Note that there are already some specials folders names listed into the google document used to start the autoconfig work at: https://spreadsheets.google.com/ccc?key=0AtLNtYDDyKsucDQ5U1czMm5OWVgwb3RrUmMzVVpVSkE&hl=en Look at colomn S, T, U, V.

For information, I have just helped a new user concerned by this issue on geckozone, for the ISP Free.fr : http://www.geckozone.org/forum/viewtopic.php?p=569579#p569579

Wouldn't be great if during the autosetup of the account there is an extra button that will allow the user to manually configured the folders (trash/sent/junk/draft/archive/etc..) if one of the standard ones is not found? I am using an italian ISP (email.it) and they call: - Drafts: Bozze - Trash: trash - junk: anti-spam Thunderbird always creates an INBOX.Trash (and sometime also INBOX.trash) folder inside the current INBOX. mail.server.server2.trash_folder_name in pref.js is set to: INBOX/trash

There already is such a button, it's called "Manual Config" (or Advanced Config soon). We won't add more widgetry for this.

It would be great if it downloads a list of the folders so that the first configuration can be done right away instead than having to click ok, manage folder subscriptions, make some changes (if no changes are made then it doesn't "remember" the folders in account settings), click ok, go back into account settings and finally set the folders.

FYI. Bug 546728 is tryng to find another solution for "case insensitivity of IMAP server" part of this bug.

I have just translated the sumo article "Thunderbird and Yahoo" and discover that Yahoo IMAP is also impacted... User need to change manually : - Draft folder (Thunderbird looks for Drafts but it is called "Draft") - Junk folder (called "Bulk Mail" on Yahoo Server) So user need around 10 steps to completely configure their Yahoo imap account, which is really problematic in my opinion... Link to the knowledge base article: https://support.mozillamessaging.com/fr/kb/Thunderbird+and+Yahoo Any chance to see this bug fixed in a near futur?

draft-ietf-morg-list-specialuse became standart now http://tools.ietf.org/html/rfc6154

XLIST IMAP is bug 476260

FWIW, this functionality is implemented as RFC 5464 annotation, so this issue depends upon https://bugzilla.mozilla.org/show_bug.cgi?id=337330

RFC6154 now implement in dovecot 2.1 http://hg.dovecot.org/dovecot-2.1/rev/32d1a903d42d

I wonder if this bug shouldn't block bug 160644 ? Any thought?

I got hired by Patrick to implement this. No exact time schedule yet.

(In reply to Ben Bucksch (:BenB) from comment #37) > I got hired by Patrick to implement this. No exact time schedule yet. Good to hear that!!!!!!!!!!! I would be happy to test any patch on this subject with some of our French ISP, which are a nightmare to set up with IMAP...

cameleon, sorry to disappoint, but they don't support SPECIAL-USE / RFC 6154. I checked imap.laposte.net, imap.orange.fr and imap.free.fr. But feel free to do advocacy there.

Summary of IRC discussion with bienvenu on 2011-12-20: 1. By default, we have [x] Show only subscribed folders turned on. This is mostly for UW-IMAP, which is no longer maintained, but may have a few die-hard users left. If that pref is on (default), we don't do LIST, but LSUB, in folder discovery. With LSUB, servers don't send the extended flags. With extended LIST, they do send the flags. 2. Folder discovery (LIST etc.) happens only on the first IMAP connection during the TB session, not on every IMAP connection we open for folders. This is good. 3. We need to store the SPECIAL USE capability in a flags variable, but we're already using all 32bit of it. We can extend it to 64bits, but we need to store it (or at least parts of it) in a pref, and that's limited to 32 bits. Summary of my investigation: 1. Thunderbird's response parser for LSUB, LIST and extended LIST is one and the same, so if we get the flags in response to any of them, we'd use the flags. 2. Thunderbird already parses all (or almost all) of the flags defined by the SPECIAL USE spec. 3. But by default, we don't get these flags, because we to LSUB. 4. Extended LIST also returns \Subscribed, so that would be a replacement for LSUB on servers that support it. My testing showed: If I unselect "[ ] Subscribed folders only" *before* I click on the Inbox for the very first time, the following happens: 1. Sent: Thunderbird uses the folder marked \Sent by the server. That folder gets the special icon, and is used for storing sent copies. However, it is detected only when I send an email (before, there is no Sent folder at all), due to lazy creation. 2. Drafts: same as Sent 3. Archives: Doesn't work. Thunderbird ignores the server flag and uses its own Archive folder. 4. Trash: Doesn't work. Trash is not created lazily, but immediately at first connect (no idea why). There's some special handling for GMail, too. 5. Junk: Doesn't work, because Junk filtering isn't enabled by default. Once you do enable it explicitly in prefs, you automatically also set the folder (because the folder pref UI is right next to the enable UI), so the folder is saved explicitly and thus overrides the automatic detection. TODO: 1. Cap: Parse the new server capability, and store it in the flags. Extend the flags field as necessary. 2. Flags: Ensure that we parse and store all relevant flags in the spec 3. LSUB: If the server supports EXTENDED-LIST or SPECIAL-USE, use the extended LIST command (which should return both \Subscribed and \Sent flags) instead of LSUB. 3.b. Maybe even change the "subscribed only" pref to (default) false. 4. Folder prefs: Figure out how when to honor server flags and when user override. User override should be possible, but only as intentional act, not by merely clicking "OK" on a pref dialog that has the folder pref on it. That would solve the Junk folder case, and also prevent the other folders to be accidentally changed, too. Either: 4.a. Make the pref UI so that it has "default" (not a concrete folder) as default setting, or a checkbox (default off) to enable the folder selector, 4.b. Make the pref UI show the (concrete) server-detected folder by default, and not save the folder to prefs unless it's different from the default. I'm leaning towards this solution. 5. Trash: Figure out what's going on, and fix it. Need input from bienvenu here 6. Archives: ditto

I fixed several issues with Trash just recently, having to do with case-insensitive handling of trash folder. Yes, the imap backend code aggressively tries to create the trash folder, if the delete model is not imap delete. See bug 546728. When I initially did XLIST, I got it working with gMail Trash, so I'm pretty sure that worked at one point (i.e., we won't create a second trash folder with a brand new gmail account). Although perhaps your definition of "doesn't work" is that the flag doesn't show up correctly right away? does list extended allow you to list only folders with a particular flag set? If so, we could list extended for \Subscribed.

> does list extended allow you to list only folders with a particular flag set? Yes. LIST-EXTENDED is defined in RFC 5258 <http://www.apps.ietf.org/rfc/rfc5258.html> Section 3.1 <http://www.apps.ietf.org/rfc/rfc5258.html#sec-3.1> states: > 3.1 Initial List of Selection Options > SUBSCRIBED - causes the LIST command to list subscribed names > ... > This option defines a new mailbox attribute, "\Subscribed", that indicates > that a mailbox name is subscribed to. The "\Subscribed" attribute > MUST be supported and MUST be accurately computed when the > SUBSCRIBED selection option is specified. You use it like so: <http://www.apps.ietf.org/rfc/rfc5258.html#sec-5> LIST (SUBSCRIBED) "" "*" or LIST (SUBSCRIBED) "" "*" RETURN (SUBSCRIBED) The first is the "selection" = filter of folders, the latter after RETURN is which flags should be returned for the resulting folders. --- SPECIAL-USE works the same and explicitly builds on RFC 5258. At least here, LIST (SPECIAL-USE) "" "*" is equivalent to LIST (SPECIAL-USE) "" "*" RETURN (SPECIAL-USE) (Dovecot did this wrong, but Timo fixed it within minutes.) --- We already support XLIST, which is a non-standard way to do SPECIAL-USE. Google explicitly states <https://developers.google.com/google-apps/gmail/imap_extensions> that they want to drop support for XLIST in favor of the standard. Unfortunately, as it stands, GMail supports neither SPECIAL-USE nor LIST-EXTENDED: imap.gmail.com:99 * CAPABILITY IMAP4rev1 UNSELECT IDLE NAMESPACE QUOTA ID XLIST CHILDREN X-GM-EXT-1 XYZZY SASL-IR AUTH=XOAUTH :-( Time to rattle Google GMail contacts?

This changes the capability variable from 32bit to 64 bit (across the code). I save it in 2 int prefs now: The current "capability" pref remains the same, and the new flags are in a creatively named "capability2" pref. I'm not unsure about the GetCapabilityPref() implementation, maybe I should directly read the pref. bienvenu, opinion? Also, not sure my bit pushing is correct. Given that the variable appears in so many places, and we already have |eIMAPCapabilityFlag| as type, I think I should make a typedef |eIMAPCapabilityFlags| instead of using raw PRUint64 everywhere. I'll do that in v2, unless you veto.

Now with typedef PRUint64 eIMAPCapabilityFlags

weird, this doesn't compile on windows, and gives unpleasant warnings: nsImapIncomingServer.cpp nsImapIncomingServer.cpp c:\builds\tbirdhq\mailnews\imap\src\nsImapCore.h(158) : warning C4341: 'kHasExte ndedListCapability' : signed value is out of range for enum constant c:\builds\tbirdhq\mailnews\imap\src\nsImapCore.h(158) : warning C4309: 'initiali zing' : truncation of constant value c:\builds\tbirdhq\mailnews\imap\src\nsImapCore.h(160) : warning C4341: 'kHasSpec ialUseCapability' : signed value is out of range for enum constant c:\builds\tbirdhq\mailnews\imap\src\nsImapCore.h(160) : warning C4309: 'initiali zing' : truncation of constant value c:/builds/tbirdhq/objdir-tb/mailnews/imap/src/../../../../mailnews/imap/src/nsIm apIncomingServer.cpp(425) : error C2373: 'nsImapIncomingServer::GetCapabilityPre fA' : redefinition; different type modifiers c:\builds\tbirdhq\mailnews\imap\src\nsImapIncomingServer.h(132) : see de claration of 'nsImapIncomingServer::GetCapabilityPrefA' c:/builds/tbirdhq/objdir-tb/mailnews/imap/src/../../../../mailnews/imap/src/nsIm apIncomingServer.cpp(425) : error C2373: 'nsImapIncomingServer::SetCapabilityPre fA' : redefinition; different type modifiers c:\builds\tbirdhq\mailnews\imap\src\nsImapIncomingServer.h(134) : see de claration of 'nsImapIncomingServer::SetCapabilityPrefA' c:/builds/tbirdhq/objdir-tb/mailnews/imap/src/../../../../mailnews/imap/src/nsIm apIncomingServer.cpp(427) : error C2373: 'nsImapIncomingServer::GetCapabilityPre fB' : redefinition; different type modifiers c:\builds\tbirdhq\mailnews\imap\src\nsImapIncomingServer.h(133) : see de claration of 'nsImapIncomingServer::GetCapabilityPrefB' c:/builds/tbirdhq/objdir-tb/mailnews/imap/src/../../../../mailnews/imap/src/nsIm apIncomingServer.cpp(427) : error C2373: 'nsImapIncomingServer::SetCapabilityPre fB' : redefinition; different type modifiers c:\builds\tbirdhq\mailnews\imap\src\nsImapIncomingServer.h(135) : see de claration of 'nsImapIncomingServer::SetCapabilityPrefB'

need to use NS_IMETHOD in nsImapIncomingServer.h NS_IMETHOD GetCapabilityPrefA(PRInt32 *aCap); NS_IMETHOD GetCapabilityPrefB(PRInt32 *aCap); NS_IMETHOD SetCapabilityPrefA(PRInt32 aCap); NS_IMETHOD SetCapabilityPrefB(PRInt32 aCap); which would mean exposing the two parts of the prefs in the idl or don't use NS_IMPL_SERVERPREF_INT

Comment on attachment 612369 [details] [diff] [review] Capability bitflags 64bit, v2 minusing since it doesn't build on windows. I'd really like to avoid the warning as well, which could mean having two separate enums for pref a and b

> signed value is out of range for enum constant OK, that's going to get ugly... The enums are integers. On 32bit, ints are of course 32bit. Now, I read people citing the C++0x spec that enums should use the smallest built-in integer type that fits all values. Given that C++0x has "long long" (64bit), the compiler should have a 64bit built-in int that it can use, and should use that here for our enum. Apparently, though, the VC++ compiler you use is either not supporting long long or too dumb to follow the spec and use the long long for the enum as it should. Now, fixing that is going to get ugly. Because we use the cap variable in a lot of code places, and all that code assumes that we can store the cap in one variable, and can simply do |if (caps & kSomeCap)|. If we split this into 2 variables, all that's going to break. I see only 2 clean and easy ways out: 1) find a way to make VC++ create a 64bit enum, as it should (maybe need a C++ expert here) 2) reorganize the caps constants so that certain values are in the first caps var and others are in the second, and they don't overlap, and the split is clear and obvious. Not sure that's a good path, though. Anybody has insight on 1) or knows somebody?

(In reply to Ben Bucksch (:BenB) from comment #48) > > signed value is out of range for enum constant > > OK, that's going to get ugly... > > The enums are integers. On 32bit, ints are of course 32bit. Now, I read > people citing the C++0x spec that enums should use the smallest built-in > integer type that fits all values. Given that C++0x has "long long" (64bit), > the compiler should have a 64bit built-in int that it can use, and should > use that here for our enum. Apparently, though, the VC++ compiler you use is > either not supporting long long or too dumb to follow the spec and use the > long long for the enum as it should. > > Now, fixing that is going to get ugly. Because we use the cap variable in a > lot of code places, and all that code assumes that we can store the cap in > one variable, and can simply do |if (caps & kSomeCap)|. If we split this > into 2 variables, all that's going to break. > > I see only 2 clean and easy ways out: > 1) find a way to make VC++ create a 64bit enum, as it should (maybe need a > C++ expert here) > 2) reorganize the caps constants so that certain values are in the first > caps var and others are in the second, and they don't overlap, and the split > is clear and obvious. Not sure that's a good path, though. They have to overlap, or there's no point to splitting them up, if by overlap, you mean two enums having the same value. I think this is the path to take, to be honest. Or, since there's no need to persist things like special use, we don't need to do any of this - just have methods on the parser, get/setsupportsSpecialUse, implement it as a packedbool, set it from the parser, and you're done. All of the other hoo-hah has to do with persisting the capability across sessions. >

I don't worry much about persistence. We can easily find solutions there (store only some caps, reformat for storage, whatever). I worry about all the code in the parser that uses the caps. I don't know about packedbool. Tell me more, or give me a URL, please.

> I worry about all the code in the parser that uses the caps. Specifically, I don't want to change all the function signatures that take/return caps (apart from changing the typedef eIMAPCapabilityFlags), and I don't want to change the ton of code which assumes it can do |if (caps & kSomeCap)| with any arbitrary cap.

> I don't know about packedbool. Tell me more, or give me a URL, please. Oh, I think I know what you mean. If you have 28 packedbool vars in a row as member vars, they occupy only 4 byte, not 28 byte. But that doesn't help us, because we still have to change *all* the code that checks for caps. There's a ton of that, and I'm not going to change all that, both for time and risk.

(In reply to Ben Bucksch (:BenB) from comment #50) > I don't worry much about persistence. We can easily find solutions there > (store only some caps, reformat for storage, whatever). I worry about all > the code in the parser that uses the caps. > I don't know about packedbool. Tell me more, or give me a URL, please. ah, I guess packed bools have gone away, though they're just bit fields, the same as saying bool foo :1; It's just a space savings, fairly irrelevant when you're talking about at most around 400 bytes, assuming you have 20 imap accounts with five open connections :-) since there's no code that currently uses the special use flag, I don't see how there can be any code you have to worry about changing.

I chatted for an hour with bienvenu. I would really like to keep the caps consistent, not use one solution (bitflag) for some caps (the old/existing ones) and another solution (e.g. bool and accessor methods) for other caps (the new ones). Similarly, I don't think 80 accessor methods for 40 bits are meaningful. I proposed 3 solutions for the caps variable. 1) find a way to trick VC++ into compiling my last patch. 2) Use |typedef eCapabilityFlag PRUint64;| and |const PRUint64 kSomeCap = 0x001;|, instead of |enum { kSomeCap = 0x001; } eCapabilityFlag;| 3) make a struct/class eCapabilityFlag { packed bool someCap = false; packed bool otherCap = false; ... } bienvenu picked 2), and I agree. As for the preference, we currently store all caps as bitfield in an int pref "capablity". The only ones we're interested in, however, are acl and quota caps (because we want to know whether to enable the corresponding UI or not). So, we agreed on storing these separately as explicit bool prefs, like so: server.capCache.acl = true (default false) server.capCache.quota = true (default false)

> 2) Use |typedef eCapabilityFlag PRUint64;| and |const PRUint64 kSomeCap = 0x001;|, > instead of |enum { kSomeCap = 0x001; } eCapabilityFlag;| > bienvenu picked 2), and I agree. > The only ones we're interested in, however, are acl and quota caps (because we want > to know whether to enable the corresponding UI or not) bienvenu, you were spot-on with quota and acl: function FillInFolderProps() http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#5920 How can you remember such details of arcane aspects of the code? I'm impressed. However, the pref also might be implicitly (maybe unintentionally) used in SetupWithURL(): The only caller of GetCapabilityPref() http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapIncomingServer.cpp#179 calls SetCapabilityForHost() directly afterwards, and we then call GetCapabilityForHost() in SetupWithURL(): http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#757 Now, if I were to keep GetCapabilityPref() and let it read only the quota and acl bits and init the caps bitflags with these 2 bits only, everything else nulled, and then call SetCapabilityForHost() with that, I might overwrite the full caps bitflag that we already read, so the next IMAP connection would have the caps (almost) emptied and would fail. I didn't test this, but I fear that. So, I think the correct approach is to get rid of the function GetCapabilityFlags and its use as well, and only store acl and quota specifically, and then read the acl and quota prefs (call GetAcl and GetQuota) directly in function FillInFolderProps(). In other words, I'll never read the caps bitflags from prefs anymore. Probably that's what you meant all along.

As discussed.

This is patch v2 (not v3) plus: > 2) Use |typedef eCapabilityFlag PRUint64;| and |const PRUint64 kSomeCap = 0x001;|, > instead of |enum { kSomeCap = 0x001; } eCapabilityFlag;|

This is v3 + small changes. The difference between v4 and v5 is the atomic pref changes discussed. However, this patch crashes when I go to Account Settings | Server Settings. Patch v4 doesn't crash. I read the code, and tried to investigate by eliminating selected changes, but I can't find the cause of the crash. :-(

- fix 64bit literal constants on Windows - remove imapSink:SetCapability()

Comment on attachment 614175 [details] [diff] [review] Capability bitflags 64bit, v6 bienvenu told Rookie Ben to do |make| on the toplevel, not just make -C mailnews/. After that, the crash is gone. And so are 2-3 hours. Gotta love build systems. Thanks, bienvenu! So, this is ready for review.

Comment on attachment 614175 [details] [diff] [review] Capability bitflags 64bit, v6 getting close... nsIImapHostSessionList Get and SetCapabilityForHost shouldn't be needed anymore. I think nsIImapServerSink needs a method to set the quota and acl values, so that the backend ui thread can write out those prefs. I would do this explicitly, instead of indirectly via nsImapIncomingServer::SetCapability. I don't think nsImapIncomingServer::SetCapability is actually called with your patch. Code inspection makes me think that, and not hitting the breakpoint I set there would seem to confirm this. Though the cacheCapa.acl pref is mysteriously set for a couple of my imap accounts, but not nearly all of them. So what I was thinking was nsIImapServerSink::SetACLAndQuotaCapabilities(in bool hasACL, in bool hasQuota); Doing this explicitly, instead of passing in all the flags, makes it harder for us to commit the sin of persisting other capabilities. I think the place you would call this is in nsImapProtocol::CommitCapability(). nit - no space after ( - if (! (capability & kACLCapability)) + if ( !haveACL)

> I think nsIImapServerSink needs a method to set > I don't think nsImapIncomingServer::SetCapability is actually called with your patch. Correct, see comment 60, that's because you asked me to remove CommitCapability() - m_imapServerSink->SetCapability(GetServerStateParser().GetCapabilityFlag()); I'll just re-add that, if you don't mind, because you asked me to do it via nsIImapServerSink, and that's precisely what this did. I think SetACLAndQuotaCapabilities() is not an ideal name, because we may have the need to save other caps later. We would still do this atomically in bool prefs, but I wouldn't want to call it SetACLAndQuotaAndKnownBrokenAuthMethodCapabilities() later, so I think SetCapabilities() and saving acl and quota as bool prefs in there is just fine.

Again, we shouldn't be persisting server capabilities at all. That's why I don't want to make it easy.

> nsIImapHostSessionList Get and SetCapabilityForHost shouldn't be needed anymore. What about this? http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#757

(In reply to Ben Bucksch (:BenB) from comment #65) > > nsIImapHostSessionList Get and SetCapabilityForHost shouldn't be needed anymore. > > What about this? > http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol. > cpp#757 Isn't that used for the "use starttls if available" setting, which we (you?) got rid of because it was insecure?

> Isn't that used for the "use starttls if available" setting, which we (you?) > got rid of because it was insecure? Correct, Correct (it was me), and correct. However, I only removed it from the UI. People might still have it in their settings, and our outlook migrator sets it (unfortunately). It is insecure insofar as an attacker can always downgrade you with an active attack, but it's still somewhat better to use STARTTLS if available than using plaintext. So, while I don't want people to use that setting, I don't want to drop those who do into the deep water just like that. If we want that setting to continue to work at all, we need a way to get the information of the first IMAP to the second one somehow.

FWIW, the proper solution to that is to reconfigure accounts regularly and almost completely automatically. hm, maybe we can do that: If we are on "STARTTLS if available", and the STARTTLS connection works, we change the pref to "always STARTTLS". Would you support that?

- Re-added imapServerSink:SetCapability() as it was before. I didn't change the name to SetACLAnd, because it already exists as SetCapability() before my patch. We didn't change its purpose, we just improved the way we store things. - !haveACL fixed - Get/SetCapabilityForHost() not removed.

This patch premanently forces STARTTLS, if the settings was "STARTTLS if available", and STARTTLS actually worked. I just change the pref in this case. This allows to remove Get/SetCapabilityForHost(), which this patch also does. I don't know whether it actually works (e.g. really uses STARTTLS on the second IMAP connection, at the moment of upgrade).

(In reply to Ben Bucksch (:BenB) from comment #68) > FWIW, the proper solution to that is to reconfigure accounts regularly and > almost completely automatically. hm, maybe we can do that: If we are on > "STARTTLS if available", and the STARTTLS connection works, we change the > pref to "always STARTTLS". Would you support that? Yes - by works, do you mean the server advertises support for STARTTLS, i.e., the capability flag was set? If so, yes, that makes sense, and is easy. The pref says "if available", which to me means the server advertises it, not it actually worked :-)

> by works, do you mean the server advertises support for STARTTLS, > i.e., the capability flag was set? In my patch "Upgrade trySTARTTLS to alwaysSTARTTLS - v1", I change the pref when we successfully established an STARTTLS connection.

Filed bug 744676 about this upgrade STARTTLS.

Comment on attachment 614242 [details] [diff] [review] Capability bitflags 64bit, v7 bienvenu: ping review

sorry, the starttls patch was not nearly as independent of this patch as was claimed, and that patch is stalled out a bit.

One issue is that if the user has picked tryStartTLS, then it's important that the host session list capability get initialized from the pref, so we'll try starttls for the first connection. Otherwise, we'll start the connection in plain mode, notice that the server supports STARTTLS, kill the connection, and restart it. Upgrading to always using STARTTLS in that situation will fix that. I would dearly love to get rid of the host session list capability stuff, but there's a bit of work before we can get there. Since I've given up on that for now, I think I can fix the issue above in this patch w/o too much trouble.

this expands on your patch by upgrading tls if available if starttls succeeds (but doesn't downgrade to plain, since that's of limited use, and I haven't gotten that right yet).

Comment on attachment 618137 [details] [diff] [review] upgrade tryStartTLS if starttls succeeds bienvenu, isn't this patch pretty exactly attachment 614242 [details] [diff] [review] plus attachment 614243 [details] [diff] [review] above? Insofar, r=BenB, yes. Only thing I don't know is when we use SetupWithUrl() and when ProcessCurrentURL(), but I trust you that the new STARTTLS code will do the right thing.

> isn't this patch pretty exactly attachment 614242 [details] [diff] [review] plus attachment 614243 [details] [diff] [review] above? Confirmed, it's *exactly* the same. I've tested the patch with "STARTTLS if available" settings, and indeed STARTTLS is used for first and secondary connections. However, STARTTLS if available is not upgraded to STARTTLS (always). Maybe we don't trigger the pref saving?

Sorry, please ignore the last 2 comments. I got the wrong file, and apparently interdiff also has serious bugs. I was discussing with bienvenu, suggesting to get rid of any caps saving whatsoever, neither in server.capability pref nor in getCapabilitiesForHost(). That avoids the dovecot problem which doesn't send STARTTLS cap anymore once we are on TLS. But it means we need to upgrade or downgrade all "STARTTLS if available" users to either "STARTTLS always" or "plain", based on whether STARTTLS worked or not.

bienvenu, this is what I meant. This works for the upgrade case, but the downgrade doesn't, I don't know why, maybe we don't run into the code.

I'll see if I can find a server that doesn't support STARTTLS to test this with.

I'll send you hostname and credentials

ok, this fixes downgrade of tls if available for various failure cases. I tried the two test servers you gave me, Ben, and they both worked for me (downgrade happened). So I think we're pretty close here.

(In reply to Ben Bucksch (:BenB) from comment #39) > cameleon, sorry to disappoint, but they don't support SPECIAL-USE / RFC 6154. > I checked imap.laposte.net, imap.orange.fr and imap.free.fr. As Yahoo! is also impacted by this issue, I would like to know if it supports SPECIAL-USE / RFC 6154 ? Do you know any popular server that support this RFC?

> Do you know any popular server that support this RFC? No. You can find out yourself https://developer.mozilla.org/en/Thunderbird/Autoconfiguration/FileFormat/HowTo#How_to_probe_mail_servers If the CAPABILITY lists "SPECIAL-USE", then the server supports it, otherwise not.

So should we filled a new bug asking that IMAP auto-detection of specials folders use more than one name? For instance, it could check for: - sent, sent mails (Gmail), outbox (laposte.net, orange.fr, sfr.fr) - trash, - draft, drafts (Gmail) - junk, spam (Gmail), bulk mail (Yahoo!), quarantaine (laposte.net, orange.fr, sfr.fr) I don't see any reason why we have made a special case for Gmail and we don't try to improve the situation for the others servers... At least, Thunderbird shouldn't create a new folder when it didn't succeed to find some special folder on the server, but instead it should ask the user to select it manually.

cameleon, this bug is only about suppporting the SPECIAL-USE standard. It's not about making other servers work that do not support the standard. I've filed bug 752484 for a solution for other ISPs that do not support SPECIAL-USE yet.

(In reply to caméléon from comment #87) > > I don't see any reason why we have made a special case for Gmail and we > don't try to improve the situation for the others servers How have we made a special case for Gmail? They support an imap extension (XLIST) that tells us what the special folders are, and we use that extension. There is now a standards-based extension to do the same thing, and this bug is about supporting that more standard but less popular (in terms of users, at this point) extension.

Ok, thanks for David and Ben for your clarifications. I really hope we can make imap configuration easier, and I understand that fixing this bug won't immediately fix my original demand (see the original bug description), but bug 752484 could be a good workaround. And in a long term, all servers may implements SPECIAL-USE or XLIST and things should be easier!

cameleon, ah, I didn't realize that we had morphed your bug. I hope bug 752484 helps.

Comment on attachment 619599 [details] [diff] [review] fix fallback for various starttls errors Works nicely, thanks! I've read the code changes you've made, and they make sense to me. I've also tested: 1) a server supporting STARTTLS properly, it was upgraded to STARTTLS always 2) a server without cap STARTTLS, it was downgraded to plain 3) a server with cap STARTTLS, but TLS not properly set up and failing to init, it was downgraded to plain Very nice. Thanks! r=BenB I've make some minor comment changes, attaching patch.

interdiff actually works here :) https://bugzilla.mozilla.org/attachment.cgi?oldid=619599&action=interdiff&newid=623329&headers=1

Comment on attachment 614242 [details] [diff] [review] Capability bitflags 64bit, v7 Actually, your patch last patch still contains exactly this patch here, and the of the patch is attachment 623339 [details] [diff] [review] (Patch v3) for bug 744676. The patch here is mostly mechanical and large, while bug 744676 is a significant behaviour change. For proper hg history, I'll separate them again.

Actually, this is part that was in your patch.

Comment on attachment 623341 [details] [diff] [review] Capability bitflags 64bit, v7b Commited as http://hg.mozilla.org/comm-central/rev/a3307f98beb6 The trySTARTTLS part was commited as http://hg.mozilla.org/comm-central/rev/9d5996744bca . Together, they are patch v11.

Comment on attachment 623329 [details] [diff] [review] Capability bitflags 64bit, v11 incl. downgrade TrySTARTTLS retroactive r+

So what's the status of this bug? Am i correct we now parse so we know the capability, but it's actually not used yet?

Correct. In other words, TODO step 1 at the bottom of comment 40 is done.

FWIW, I'd appreciate help with TODO steps 4 and 5 in comment 40.

There's somebody working on LIST-EXTENDED in bug 495318. That would be step 2 and 3 in the TODO list. I also had a small patch for that myself, but I notice I had not attached it yet. Will do soon - if nothing else, to possibly help the other guy.

(In reply to Ben Bucksch (:BenB) from comment #101) > FWIW, I'd appreciate help with TODO steps 4 and 5 in comment 40. Bug 800035 is my idea for your step 4. (In reply to Ben Bucksch (:BenB) from comment #102) > There's somebody working on LIST-EXTENDED in bug 495318. That would be step 2 > and 3 in the TODO list. Yup. That would be me. > I also had a small patch for that myself, but I notice I had not attached it > yet. Will do soon - if nothing else, to possibly help the other guy. I think I have it figured out. I wouldn't mind seeing your patch though. --- Also, while all of this is fresh in my head from doing LIST-EXTENDED, I have some ideas for you here that hopefully I'll get around to posting in the near future. If I am going to add to what you have done here already, I take it that I just need one patch or the other, not both, right?

For information, it's used by: * Gmail: https://developers.google.com/gmail/imap_extensions#special-use_extension_of_the_list_command: "Gmail supports the IMAP LIST Extension for Special-Use Mailboxes, which provides new attributes for special folders" (XLIST is deprecated ) * MS Office 2013 "Outlook 2013 implements the IMAP LIST extension specified in [RFC6154] as the XLIST command." http://msdn.microsoft.com/en-us/library/ee624430%28v=EXCHG.80%29.aspx (may be same for Microsoft Exchange Server) * Android: not yet, https://code.google.com/p/android/issues/detail?id=40436 * K9-mail (android application): https://github.com/k9mail/k-9/search?q=RFC6154&source=cc * Roundcube (webmail): not yet http://trac.roundcube.net/ticket/1487830 * CiderWebmail (Webmail): work in progress https://github.com/CiderWebmail/CiderWebmail/issues/24 * Horde (web groupware): http://www.horde.org/apps/imp/docs/RFCS * Cyrus IMAP server: work in progress for 2.5 http://cyrusimap.web.cmu.edu/mediawiki/index.php/RoadMap * Dovecot POP/IMAP server: since August 2010 http://dovecot.2317879.n4.nabble.com/MUAs-creating-different-quot-Sent-quot-folders-td7348.html

So, this turned out to be a case of tying up all the loose ends. The spec suggests All and Archive are different, but we don't use them differently; I wasn't sure whether to use separate flags or not. The patch has the extra flag added but I can remove it if you like. (Having added the new flag I then have to tell the IMAP mail folder about it of course.) The IMAP parser already knows whether the server supports special use so it's just a case of sending the return (special-use) flag when we list extended. Then of course it's just a case of getting the parser to recognise the standard tokens as well as the XLIST tokens. I've tested this against a trial version of Communigate 6.1 which defaults to saving sent mail in "Sent Items" and Thunderbird correctly picks this up.

> The spec suggests All and Archive are different, but we don't use them differently Actually, we do: "Archive" is a manual action that a user does to get the email out of the way. Archived emails are only those that are already processed and no longer interesting. "All" should contain all the user's mail, no matter which folder it is in. This is in practice a GMail-only feature.

r+ on the patch. Only one comment: This seems wrong: // Treat the GMail all mail folder as the archive folder. - if (m_boxFlags & kImapAllMail) + if (m_boxFlags & (kImapAllMail | kImapArchive)) newFlags |= nsMsgFolderFlags::Archive; We should not use "All mail" as "Archive" folder. But given that it was like that before your change, this should not stop the patch.

(In reply to Ben Bucksch from comment #40) > 1. Sent: Thunderbird uses the folder marked \Sent by the server. That folder > gets the special icon, and is used for storing sent copies. However, it is > detected only when I send an email (before, there is no Sent folder at all), > due to lazy creation. > 2. Drafts: same as Sent As long as the folder already exists as the special folder on the server and doesn't exist yet on the client then the preferences will be changed so that the special folder on the server is used. > 3. Archives: Doesn't work. Thunderbird ignores the server flag and uses its > own Archive folder. Couldn't test this as I don't have an IMAP server that supports the \Archive flag, but the same function is used as for Sent and Drafts so it should work. > 4. Trash: Doesn't work. Trash is not created lazily, but immediately at > first connect (no idea why). There's some special handling for GMail, too. If you don't already have a Trash folder, then the server's Trash folder is used. If you have a GMail server then your preferences are updated too. If you don't then if you already have a Trash folder or change your preference then the server specified folder will be ignored. What should probably happen is that the preference should be updated to the server's Trash folder if the folder referred to by the preference does not exist, in the same way as for the Drafts/Sent folder. > 5. Junk: Doesn't work, because Junk filtering isn't enabled by default. Once > you do enable it explicitly in prefs, you automatically also set the folder > (because the folder pref UI is right next to the enable UI), so the folder > is saved explicitly and thus overrides the automatic detection. Currently there seems to be no detection of the server's Junk folder.

Trash: When I looked into it, Trash was created by Thunderbird early, not lazily. (Unlike Sent.) So Trash always existed.

> Attachment #8531690 [details] [diff] - Flags: review+ Can we get this commited?

After the discovery process, any missing copies and folder settings are automatically updated to point to a folder discovered with the appropriate type. However this wasn't applied to the Junk or Trash folders, except in the case of GMail's Trash folder. This version not only works on all servers, but also saves any missing junk or trash settings to preferences too. I also improved CheckSpecialFolder to work better in the face of invalid prefs.

Comment on attachment 8531690 [details] [diff] [review] Tie up loose ends I think it would be nice to get this in as a separate patch, for readability. Thus, un-obsoleting.

During this year, Rondcube 1.1 (the coming soon version) support this RFC: http://trac.roundcube.net/ticket/1487830#comment:9 For Cyrus it will be in the next version (2.5) I don't understand if K9-mail use-it or not: https://github.com/k9mail/k-9/search?q=RFC6154&source=cc I hope Thunderbird give this functionality soon.

Comment on attachment 8532559 [details] [diff] [review] Persist trash and spam folders in preferences Review of attachment 8532559 [details] [diff] [review]: ----------------------------------------------------------------- Aside from a couple of nits, this looks fine to me. ::: mailnews/imap/src/nsImapIncomingServer.cpp @@ +1547,5 @@ > + nsCOMPtr<nsIMsgImapMailFolder> imapFolder(do_QueryInterface(trashFolder)); > + int32_t boxFlags; > + imapFolder->GetBoxFlags(&boxFlags); > + if (boxFlags & kImapXListTrash) > + continue; Nit: new code should have { } around one-line bodies @@ +1638,5 @@ > nsCOMPtr<nsIMsgFolder> folder; > nsCOMPtr<nsIMsgFolder> rootMsgFolder; > nsresult rv = GetRootFolder(getter_AddRefs(rootMsgFolder)); > NS_ENSURE_SUCCESS(rv, false); > + nsCOMPtr<nsIMsgFolder> foundExistingFolder; You've changed that variable from bool to nsCOMPtr; I'd prefer it be renamed (perhaps 'existingFolder' as the old code used a few lines later). @@ +1666,1 @@ > return foundExistingFolder; Because you changed the type of foundExistingFolder, you're now relying on implicit cast to 'bool' in the return. I'd prefer an explicit return (existingFolder != nullptr); to make it obvious that you're intentionally returning 'it exists', not something that's expected to be a meaningful pointer.

Comment on attachment 8531690 [details] [diff] [review] Tie up loose ends https://hg.mozilla.org/comm-central/rev/8fa32d043062

Comment on attachment 8532559 [details] [diff] [review] Persist trash and spam folders in preferences https://hg.mozilla.org/comm-central/rev/83b4178ffd06

Is everything landed about this bug, can we mark it as resolved fixed?

I'd say we can call this FIXED. Not everything on the laundry list in comment 40 is done, e.g. Trash folder created and left behind unused, but some of these have specific bugs filed, e.g. bug 800035.