303571 - Can not move subfolders to IMAP "root" folder (maildir)

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Gianluca Sforna]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050720 Fedora/1.0.6-1.1.fc3 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050720 Fedora/1.0.6-1.1.fc3 Firefox/1.0.6

I am running a Dovecot IMAP server, with mailboxes in Maildir format.
With thunderbird I can create new folders and also drag and drop them into other
folders.
However, the move operation fails if destination is the root folder and not
another existent folder.



Reproducible: Always

Steps to Reproduce:
1. Create a folder "test1"
2. Create another folder "test2"
3. move test2 into test1: success
4. try to move back test2 to the top level


Actual Results:  
thunderbird complains:
The current command did not succeed. The mail server responded: Mailbox exists..



Expected Results:  
the test2 folder is put back at the top level

I suspected Dovecot was to blame for the problem, but then I tried the same
operation with Evolution and it worked as expected.
Moreover I used ethereal to capture the IMAP connection and it recorded the
following (wrong) command:
rename: "test2.test1" "test2.test1"
with the obvious response:
NO Mailbox exists.

Also recorded at redhat bugzilla:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=157425

Comment 1

[David :Bienvenu]

can you generate an imap protocol log of a session where you do this? Substitute
IMAP for "protocol" in these instructions.

http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html#imap

I tried this with a cyrus server, which only allows folders to be sub-folders of
the inbox, and it worked fine.

Comment 2

[David :Bienvenu]

I should note that I only tried this on a trunk build, not a 1.0x build.

Comment 3

[Gianluca Sforna]

Attachment: Sample session log, with two move commands, the second one failing

Comment 4

[Gianluca Sforna]

(In reply to comment #1)
> can you generate an imap protocol log of a session where you do this? 
I guess so... :)

> I tried this with a cyrus server, which only allows folders to be sub-folders of
> the inbox, and it worked fine.
So I think you are moving the folder from the 3rd level to 2nd. i.e. from a
starting point (sorry for the ugly ascii art...):

--account_name
 +--Inbox
   +--test2
     +--test3

you drag test3 into Inbox resulting in:

--account_name
 +--Inbox
   +--test2
   +--test3

this also works for me (see attached log, line 617, "Inbox" is replaced by "test")

What it does not work is dragging test2 to "account_name", which I expected to
produce something like:

--account_name
 +--Inbox
 |  +--test2
 +--test3

Which I tried later on (log line 667).
        

Comment 5

[Gianluca Sforna]

Just a brief note to say the bug is still present after the last fc3 update.
Now I am running thunderbird version 1.0.7-1.1.fc3 (20050929).

It should be nice if at least the bug is recognized by the developers by
removing the "UNCONFIRMED" state...

Comment 6

[Adam Guthrie]

Gianluca, you really need to test this on trunk for it to be valid.
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-trunk/

Comment 7

[Gianluca Sforna]

(In reply to comment #6)
> Gianluca, you really need to test this on trunk for it to be valid.
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-trunk/

Thanks for having a look at this. I downloaded and tested the trunk version
1.6a1 (20051016) and the bug is still present.

I am updating the bug version info.

Comment 8

[Magnus Melin [:mkmelin]]

*** Bug 321672 has been marked as a duplicate of this bug. ***

Comment 9

[jakemaul]

I can confirm that this bug exists in Thunderbird 1.0.7.

It also still exists in latest-trunk: version 1.6a1 (20051229).

It has existed since at least Thunderbird 0.9.x.

My log looks like the one posted... the failed move is attempting to rename a folder to the same name... it SHOULD be attempting to rename 'test.test2' to 'test2' (obviously). Without seeing the code, it looks like Tbird has a special case for top-level folder moves, and it's b0rked. My guess is it was only tested on an IMAP server that forces all folders to be subfolders of inbox, as the other comment implied...

There has been no activity on this issue (or this bug report), so I'm 'confirming' it.

Comment 10

[jakemaul]

(In reply to comment #9)
> There has been no activity on this issue (or this bug report), so I'm
> 'confirming' it.

Or at least I would have, but I'm not sufficiently 'empowered'...

Comment 11

[jakemaul]

Hello? Anyone? Thie bug still exists in trunk version 1.6a1 (20060204).

Going on 6 months now, and this bug is still UNCONFIRMED, despite several user confirmations on different stable versions, different trunk versions, and different platforms. What's the holdup?

By the way... Eudora 7.0.1.0 works correctly. So if there was any doubt about whether or not Thunderbird is at fault, this info ought to pretty much kill that...

Comment 12

[Gianluca Sforna]

well, I just checked and the current assignee for this bug has 1573 other bugs assigned to him, and this count is only for bugs in state UNCONFIRMED... no wonders this bug is not high on a priority list :(

Comment 13

[jakemaul]

Attachment: (possibly) the offending file

The presumed offending function begins on line 6253.

Comment 14

[jakemaul]

I think the problem is in the function nsImapProtocol::OnMoveFolderHierarchy(...) in the file mailnews/imap/src/nsImapProtocol.cpp.

For anyone watching who doesn't have a copy of the source lying around to look at, I've attached the file. The function begins on line 6253.

Comment 15

[jakemaul]

I've tracked this problem down a bit more...

Basically... mozilla/mailnews/imap/src/nsImapUrl.cpp has a default folder delimiter character '/'. Dovecot defaults to '.'. Fear not, it also features functions to get and set this value. This value is checked and set for virtually every operation the user performs. Normally, this happens with no problem.

When moving a folder to the top-level, however, it doesn't. In the file I posted, in nsImapProtocol::OnMoveFolderHierarchy(), there is a line that looks like:
m_runningUrl->GetOnlineSubDirSeparator(&onlineSubDirSeparator)
GetOnlineSubDirSeparator stores the value on m_onlineSubDirSeparator (from nsImapUrl.cpp) into the variable onlineSubDirSeparator. The problem is it returns '/', when it SHOULD be returning ".". m_onlineSubDirSeparator is not being set correctly before being checked.

This causes a logical failure later on in the same function (4 lines, to be exact) when we try to find the name of the 'leaf' folder ('test3', not 'test.test3'). Since it's looking for the wrong delimiter, it find the whole name (test.test3) to be the leaf folder name.

The final step is to append the 'leaf' name onto the new root name. In the correct case, if you're moving a folder to the root, then you'll be appending the leaf name (test3) onto an empty string, so the full newBoxName would be just 'test3', as desired. But since leafName actually contains the FULL name, it appends 'test.test3'... and there it is.

There are several solutions, but most of them will break when an IMAP server uses '/' as the separator. I could make the default separator '.', and then it works. Or I could hard-code it in right in this function. The real problem is, the delimiter for this m_runningUrl is never correctly set, and so it uses the default. But I don't know how to fix it...

Comment 16

[Gianluca Sforna]

(In reply to comment #15)
> 
> When moving a folder to the top-level, however, it doesn't. In the file I
> posted, in nsImapProtocol::OnMoveFolderHierarchy(), there is a line that looks
> like:
> m_runningUrl->GetOnlineSubDirSeparator(&onlineSubDirSeparator)
> GetOnlineSubDirSeparator stores the value on m_onlineSubDirSeparator (from
> nsImapUrl.cpp) into the variable onlineSubDirSeparator. The problem is it
> returns '/', when it SHOULD be returning ".". m_onlineSubDirSeparator is not
> being set correctly before being checked.

Well, some days ago I compiled a debug version and discovered exactly what you describe here. The only difference is that I did not dare to add a comment on the bug... 

> 
> There are several solutions, but most of them will break when an IMAP server
> uses '/' as the separator. I could make the default separator '.', and then it
> works. Or I could hard-code it in right in this function. The real problem is,
> the delimiter for this m_runningUrl is never correctly set, and so it uses the
> default. But I don't know how to fix it...
> 

I thought that there could be another part in the code base responsible for querying the server and discover the correct separator, but I had no time to investigate further. Please note this could very well be OUTSIDE the responsibility of the current bug assignee.

I think what we really need here is some help from a REAL developer...

Comment 17

[jakemaul]

(In reply to comment #16)
> Well, some days ago I compiled a debug version and discovered exactly what you
> describe here. The only difference is that I did not dare to add a comment on
> the bug... 

> I thought that there could be another part in the code base responsible for
> querying the server and discover the correct separator, but I had no time to
> investigate further. Please note this could very well be OUTSIDE the
> responsibility of the current bug assignee.

The only place this seems to be done is in nsImapUrl::ParseFolderPath(). But that's only ever called from within nsImapUrl.

> I think what we really need here is some help from a REAL developer...

Indeed. It'd be nice if SOMEBODY @mozilla.org would at least comment...

Comment 18

[jakemaul]

Another month goes by...

(In reply to comment #17)
> (In reply to comment #16)
> > Well, some days ago I compiled a debug version and discovered exactly what you
> > describe here. The only difference is that I did not dare to add a comment on
> > the bug... 
> 
> > I thought that there could be another part in the code base responsible for
> > querying the server and discover the correct separator, but I had no time to
> > investigate further. Please note this could very well be OUTSIDE the
> > responsibility of the current bug assignee.
> 
> The only place this seems to be done is in nsImapUrl::ParseFolderPath(). But
> that's only ever called from within nsImapUrl.
> 
> > I think what we really need here is some help from a REAL developer...
> 
> Indeed. It'd be nice if SOMEBODY @mozilla.org would at least comment...
> 

Comment 19

[David :Bienvenu]

I'm trying to get access to a dovecot server like this in order to test this.

Comment 20

[Gianluca Sforna]

(In reply to comment #19)
> I'm trying to get access to a dovecot server like this in order to test this.
> 

Only 3 words: THANKS, THANKS, THANKS...

If you need it, I can provide you with an email account here. just ask

Comment 21

[David :Bienvenu]

Attachment: proposed fix

the interesting change is in nsImapMailFolder.cpp - the other changes are just getting rid of stuff that's not used.

That change is basically to get the hierarchy delimiter from the first child folder; the root folder doesn't exist, in IMAP, so we need to get a real folder. The first child is a good bet...