301714 - subscribe, unsubscribe and delete add extra '/' to mailbox name

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Timo Sirainen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050105 Galeon/1.3.19 (Debian package 1.3.19-4)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20050105 Galeon/1.3.19 (Debian package 1.3.19-4)

Just because CREATE command requires directory-like mailboxes to be created as
"CREATE foo/", it doesn't mean that it should be done with SUBSCRIBE,
UNSUBSCRIBE or DELETE commands. Dovecot doesn't really like it:

20 create "helloworld/"

20 OK Create completed.

21 subscribe "helloworld/"

21 NO Invalid mailbox name: helloworld/
..
9 delete "helloworld/"

9 NO Invalid mailbox name



Reproducible: Always

Steps to Reproduce:
1. Unset "Server supports folders that contain sub-folders and messages"
2. Set "Show only subscribed folders"
3. Try creating and then deleting folder-only folders

Actual Results:  
Server gives error

Expected Results:  
No server errors

Comment 1

[zug_treno]

Related to Core bug 29926?

Comment 2

[OstGote!]

*** This bug has been marked as a duplicate of 29926 ***

Comment 3

[Ben Bucksch (:BenB)]

I don't really understand this bug. Please describe clearly what TB is doing wrong, based on the IMAP spec. The gut of your report seems to be:

> it doesn't mean that it should be done with SUBSCRIBE,
> UNSUBSCRIBE or DELETE commands. Dovecot doesn't really like it:

but I can't make sense of it what's wrong with that.

Given that this is about the protocol, please describe actual and expected.

From the information you given here, to me, the dovecot reply seems wrong, it looks like this is a dovecot bug. If I successfully created a folder "foo/", I should be able to subscribe to it, no?

Also, I assume the "/" is in fact the configured or negotiated folder separator and not hardcoded.

The only thing that I could fish out of this is that CREATE command accepts a folder separator as hint that this is a 'folder containing folders', but the folder separator is not part of the name, thus the SUBSCRIBE to the name plus folder separator fails with dovecot. I don't think that's wrong per-se, though, and does not seem to fail with other servers.

It'd be good to cite the spec that this is wrong and dovecot is correct. Esp. given that you are cited as the author of dovecot, it would be easy for you to fix that, so please give justification.

Comment 4

[Ben Bucksch (:BenB)]

John G. Myers in bug 29926 comment 10 does agree with you, though, so I'll assume you're right.

Comment 5

[Timo Sirainen]

Yes, by '/' I mean hierarchy separator.

OK, again steps to reproduce:

1. Unset "Server supports folders that contain sub-folders and messages"
2. Set "Show only subscribed folders"
3. Right-click mailbox list and select "New folder..."
4. Select "Folders-only"
5. Give a mailbox name, such as "testbox"
6. Click OK.

Expected IMAP commands:

1 CREATE testbox/
2 SUBSCRIBE testbox

Actual IMAP commands TB currently sends:

1 CREATE testbox/
2 SUBSCRIBE testbox/

Although subscriptions dialog doesn't allow subscribing or unsubscribing from folders-only mailboxes anyway. And actually latest UW-IMAP even responds to such requests with:

1 subscribe mailbox/
* NO CLIENT BUG DETECTED: subscribe of non-mailbox directory mailbox/
1 OK SUBSCRIBE completed

Perhaps there should be no SUBSCRIBE command sent at all?

Anyway, besides SUBSCRIBE the same problem also happens with deleting the mailbox. It should send DELETE "testbox", not DELETE "testbox/".

> The only thing that I could fish out of this is that CREATE command accepts a
> folder separator as hint that this is a 'folder containing folders', but the
> folder separator is not part of the name, thus the SUBSCRIBE to the name plus
> folder separator fails with dovecot. I don't think that's wrong per-se, though,
> and does not seem to fail with other servers.

Yes, this is what I meant. '/' is a hint for the CREATE command, it's not part of the mailbox name. Using it in SUBSCRIBE testbox/ would mean you're trying to subscribe to a mailbox named "testbox/", not to "testbox" which is what really was created.

I think it's just a coincidence that it happens to work with most servers due to how they're implemented. It may even cause problems. For example with UW-IMAP it's possible to subscribe to both "testbox/" and "testbox". Unsubscribing from one of them doesn't remove the other subscription.

I've also "fixed" it with Dovecot, but since I feel it's a violation of IMAP protocol I've made it an optional workaround that needs to be explicitly enabled.

Comment 6

[Ben Bucksch (:BenB)]

OK, thanks, that's clearer.

FWIW, on Unix filesystem, / is also the folder separator, but rm -rf foo/ is fully legal.

REOPENing and CONFIRMing.

Comment 7

[Timo Sirainen]

Right, that's why DELETE "mailbox/" also just happens to work on UNIX filesystem-based IMAP servers (including Dovecot). They directly use stat("mailbox/") and rmdir("mailbox/") calls which don't care about the trailing '/'. I still wouldn't rely on it working though. :)

Comment 8

[WADA:World Anti-bad-Duping Agency]

As i wrote in bug 537626 comment #8, cause is RFC violation by Tb.
 foo/ is valid in create command/list response/lsub response
 foo/ is invalid for delete/subscribe command, because "mailbox name" isrequired

Comment 9

[WADA:World Anti-bad-Duping Agency]

FYI.
(copy of a part of bug 537626 comment #8 by Timo Sirainen)
> In Dovecot there's tb-extra-mailbox-sep imap_client_workaround that deals with
> Thunderbird's bugginess of adding these extra slashes to mailbox names all around.

Comment 10

[WADA:World Anti-bad-Duping Agency]

Sorry. It was bug 537626 comment #6 by Timo Sirainen.

Comment 11

[William Blunn]

OK, so nearly three years ago this bug became confirmed, and the importance is "major", but ... no activity.

Without wishing to sound like a voice from the peanut gallery ...

Would it be possible for this bug to get some airtime with the Thunderbird development team (if there is such a thing) ?

Comment 12

[Willie]

I have never done any Thunderbird development before and figured that this would be a good bug with which to learn how to do it.

So, I'm just posting here to say that I'm going to work on it -- although I have no idea of the complete procedures to follow.

Here's where I've gotten so far:
I've compiled Shredder
I've verified that (part of) the original bug described by Timo still exists.
* Thunderbird seems to properly delete now, so that part is fixed.
* Thunderbird will properly describe if you do it manually, but the auto-subscribe that occurs after creation is still invalid

So now, I guess my next step is to find where that "auto-subscribe" functionality exists in the code and figure out how to properly submit a patch.

Additional note: If the setting "Server supports folders that contain sub-folders and messages" is set then Thunderbird seems to generate proper IMAP commands.  Not that this was part of the original bug, but I thought I'd check either way.

Comment 13

[Leho Kraav (:macmaN @lkraav)]

Jump on #maildev on freenode too if you haven't already.

Comment 14

[Ben Bucksch (:BenB)]

> figure out how to properly submit a patch.

1. if you got your source via hg, you just do
hg diff > bug307714-fix-v1.diff
2. Then you click "Add an attachment" above and check the "patch" checkbox.
3. While attaching, you mark review as ? and ":bienvenu" in the textfield as reviewer.

Comment 15

[Willie]

Attachment: Proposed fix for bug 301714

A new folder is created and subscribed to.  This folder does one thing: it makes sure that when we subscribe, we never have the hierarchy delimiter at the end of the subscription request.  While it is allowed in some cases for creating folders (RFC 3501, 6.3.3), it is not valid for the SUBSCRIBE command.

Now Thunderbird will subscribe to folders it creates using the proper IMAP commands.  Also, if they have improperly unchecked "Server supports folders that contain sub-folders and messages", it's no big deal because we will still subscribe to the folder for them anyway.

However, this fix brings to light another bug which could use some discussion.  Normally, Thunderbird does not (for some reason) let you manually subscribe (using the Subscribe dialog) to these "folders only" mailboxes.  Perhaps this is because the server would return a \Noselect flag?

With this patch, however, they will be subscribed.  Not a huge deal except for that on a restart of Thunderbird (or a relisting of the folders), the folders switch from being in an italicized font to the regular font and Thunderbird will try to SELECT the mailboxes when the user clicks on the folder.  This same behavior would occur if another mail client was to SUBSCRIBE to the mailbox and then we start Thunderbird though, so it's not an issue introduced by this patch -- it just becomes more obvious because we'll see it more.

Unchecking "show only subscribed folders" fixes that problem, so it's really an issue somewhere in the listing code as to whether to show a folder as a \Noselect mailbox or not -- somehow "show only subscribed folders" is overriding some logic there.  I haven't dug into it though since that's not specifically addressed by this bug.  Should I search/open up another bug for this issue and we can set this bug #301714 as resolved?

Comment 16

[Timo Sirainen]

(In reply to comment #15)

> With this patch, however, they will be subscribed.  Not a huge deal except
> for that on a restart of Thunderbird (or a relisting of the folders), the
> folders switch from being in an italicized font to the regular font and
> Thunderbird will try to SELECT the mailboxes when the user clicks on the
> folder.  This same behavior would occur if another mail client was to
> SUBSCRIBE to the mailbox and then we start Thunderbird though, so it's not
> an issue introduced by this patch -- it just becomes more obvious because
> we'll see it more.

Typically IMAP servers won't show any flags when using LSUB command. This may partially be because it's ambiguous if a \NoSelect entry means that this is a subscribed \NoSelect mailbox, or because it has a subscribed child mailbox and we're listing only %. But I guess it's mainly because it's extra work for most servers to find out the flags.

I've recently added a setting to Dovecot that makes it return \NoSelect with LSUB. It's disabled by default because it's extra work for Dovecot and few other clients actually care about it. http://hg.dovecot.org/dovecot-2.0/rev/0405f4c507c9

Comment 17

[Willie]

(In reply to comment #16)
> Typically IMAP servers won't show any flags when using LSUB command. This
> may partially be because it's ambiguous if a \NoSelect entry means that this
> is a subscribed \NoSelect mailbox, or because it has a subscribed child
> mailbox and we're listing only %. But I guess it's mainly because it's extra
> work for most servers to find out the flags.

That is probably what is happening then.  Thunderbird is issuing an LSUB "" "*" to populate it's folder list in this case.  If a folder is listed in LSUB then it is assuming that it is a selectable folder.  That may also be why they made the decision to not allow users to manually subscribe to \Noselect mailboxes... it helps the assumption break less often.

There is another problem with the way this is set up as well.  Say I create a "Folders Only" mailbox.  We'll call it FO.  If it's not subscribed, and has no subfolders yet, Thunderbird removes it from its list of folders when it restarts (because it's not subscribed).

So I cannot easily create new folders under my FO folder.  The two workarounds:
1) Have Thunderbird list all mailboxes (not just subscribed) and then create it.
2) Create a new folder called FO/newfolder (This assumes that the hierarchy separator is / and that the user is aware of that fact.)

Unfortunately, I'm not familiar enough with the code yet to know what the suggested fix would be for all of this.  One could remove the assumption that an  LSUB membership means that a folder is selectable, but since Thunderbird may still need to know which folders are selectable, it'd still have to issue a LIST command of some sort if it wanted to know (for the italicized font, knowing whether to show the "New subfolder..." menu option, whether to SELECT the folder when chosen, etc.)

Comment 18

[Willie]

(In reply to comment #17)
> There is another problem with the way this is set up as well.  Say I create
> a "Folders Only" mailbox.  We'll call it FO.  If it's not subscribed, and
> has no subfolders yet, Thunderbird removes it from its list of folders when
> it restarts (because it's not subscribed).

Replying to myself: with the proposed patch, this wouldn't be a problem anymore since the folder WOULD be subscribed.  So the real concern would just be the last paragraph -- the assumption that a folder listed in LSUB is selectable (treated as a "messages only" folder).

Comment 19

[Wayne Mery (:wsmwk)]

Willie, are you still waiting for a reply to comment 18?

Comment 20

[Willie]

Mostly still waiting on the requested review from the patch in comment 15.
However, I worry that the trunk has moved so far forward since then that it may need to be rewritten unless the two functions it is called from have remained the same.

Comment 21

[:aceman]

Can you please check it and update?

Comment 22

[Ludovic Hirlimann [:Usul]]

and if the code patch still applies please ping bienvenu for review

Comment 23

[David :Bienvenu]

Attachment: cleaned up, unbitrotted patch

Willie, thx for the original patch. I've redone it a bit to take advantage of the nsCString class. I also fixed the online hierarchy delimiter code to be a little less crazy - hierarchy delimiters are always 8 bit characters, so there's no need for the string allocations and frees that were introduced as part of the external linkage work. Could you try this patch to make sure it still fixes your original issue? Thx!

Comment 24

[[e-mail]]

(In reply to David :Bienvenu from comment #23)
> Willie, thx for the original patch. I've redone it a bit to take advantage
> of the nsCString class. I also fixed the online hierarchy delimiter code to
> be a little less crazy - hierarchy delimiters are always 8 bit characters,
> so there's no need for the string allocations and frees that were introduced
> as part of the external linkage work.

Actually the online delimiter was always a string even back in the 1.7 era, but then again back then the hierarchy delimiter was a PRUnichar for some reason...

Comment 25

[[e-mail]]

Comment on attachment 598443 [details] [diff] [review]
cleaned up, unbitrotted patch

So, when creating a folder that contains folders, we create "Foo/", but then we have to remove the "/" once to auto-subscribe to it (which is possibly wrong anyway), and then again to list it?

>+  readonly attribute char onlineDelimiter;
Can we not call this hierarchyDelimiter to match nsMsgImapMailFolder? (The compiler will do all the appropriate thunking for us, and the idl compiler won't mind as long as the two interfaces don't inherit from each other.)

>+  if (onlineDelimiter  != kOnlineHierarchySeparatorUnknown
>+      && onlineDelimiter != hierarchyDelimiter)
Nit: move the && to the end of the previous line (and also above)

Comment 26

[David :Bienvenu]

(In reply to [e-mail] from comment #25)
> Comment on attachment 598443 [details] [diff] [review]
> cleaned up, unbitrotted patch
> 
> So, when creating a folder that contains folders, we create "Foo/", but then
> we have to remove the "/" once to auto-subscribe to it (which is possibly
> wrong anyway), and then again to list it?
Yes.
> 
> >+  readonly attribute char onlineDelimiter;
> Can we not call this hierarchyDelimiter to match nsMsgImapMailFolder? (The
> compiler will do all the appropriate thunking for us, and the idl compiler
> won't mind as long as the two interfaces don't inherit from each other.)

The c++ compiler doesn't like the fact that nsImapMailFolder implements both interfaces. Were you suggesting getting rid of the method from nsIMsgImapMailFolder?

Comment 27

[David :Bienvenu]

Attachment: fix up formatting

I cleaned up the line formatting a bit. I couldn't think of a way of combining the hierarchy delimiter attributes that wasn't worse than what I have now. I can't have it in both interfaces, and removing it from one would require quite a few other changes...I could add a seperate folder sink proxy for nsIMsgImapMailFolder methods, with new sync runnable methods, but that seemed overkill. And making all the callers use the folder sink interface, even if they're already on the UI thread would be a bit ugly.

Comment 28

[[e-mail]]

(In reply to David Bienvenu from comment #26)
> The c++ compiler doesn't like the fact that nsImapMailFolder implements both
> interfaces.
It doesn't? Admittedly, I haven't tried patching the files in question, but I can't see what reason it might have to complain.

Comment 29

[David :Bienvenu]

nsImapUndoTxn.cpp
c:\builds\tbirdhq\mailnews\imap\src\nsImapMailFolder.h(325) : error C2535: 'nsre
sult nsImapMailFolder::GetHierarchyDelimiter(char *)' : member function already
defined or declared
        c:\builds\tbirdhq\mailnews\imap\src\nsImapMailFolder.h(324) : see declar
ation of 'nsImapMailFolder::GetHierarchyDelimiter'
c:\builds\tbirdhq\mailnews\imap\src\nsImapMailFolder.h(325) : error C2535: 'nsre
sult nsImapMailFolder::GetHierarchyDelimiter(char *)' : member function already
defined or declared
        c:\builds\tbirdhq\mailnews\imap\src\nsImapMailFolder.h(324) : see declar
ation of 'nsImapMailFolder::GetHierarchyDelimiter'

Comment 30

[[e-mail]]

Ah, it's those NS_IMPL_NSIFOO macros, I forgot about them...

Comment 31

[[e-mail]]

Comment on attachment 598704 [details] [diff] [review]
fix up formatting

[There are at least two ugly ways to properly share the onlineDelimiter attribute, but, sadly, no nice ways. Stupid C++]

>+    m_runningUrl->SetOnlineSubDirSeparator (onlineDelimiter);
Nit: no space before (

>+  if (onlineDelimiter  != kOnlineHierarchySeparatorUnknown &&
Nit: double space

>+void nsImapProtocol::RemoveHierarchyDelimiter(nsCString &mailboxName)
>+{
>+  char onlineDelimiter[2] = {0, 0};
>+  if (m_imapMailFolderSink)
>+    m_imapMailFolderSink->GetOnlineDelimiter(&onlineDelimiter[0]);
Out of interest, does it make any difference whether you use the imap mail folder sink or the running URL?

Comment 32

[David :Bienvenu]

(In reply to [e-mail] from comment #31)
 = {0, 0};
> >+  if (m_imapMailFolderSink)
> >+    m_imapMailFolderSink->GetOnlineDelimiter(&onlineDelimiter[0]);
> Out of interest, does it make any difference whether you use the imap mail
> folder sink or the running URL?

Yes, I think it does. The folder persists its hierarchy delimiter, so it's good after the first time it's set. The running URL sets it when the url is running, and sometimes it sets it from the folder's persisted delimiter.

Comment 33

[David :Bienvenu]

fixed - http://hg.mozilla.org/comm-central/rev/4821565a8f24, Willie, thx for the original patch.