147995 - Difficulty finding trash folder when using an IMAP server directory

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Michael Klose]

Please note that this is not a user issue as the code is written in such a way
that the end user does not see this bug.

Code says more than a thousand words:

From function: nsImapProtocol::DiscoverMailboxSpec

            if (serverTrashName)
            {
              if (!PL_strcasecmp(nsPrefix, "INBOX.")) // need to special-case
this because it should be case-in
sensitive
              {
#ifdef DEBUG
                NS_ASSERTION (PL_strlen(serverTrashName) > 6,
                  "Oops.. less that 6");
#endif
                trashExists = ((PL_strlen(serverTrashName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
                  (PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
                  !PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX." */
                  !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
              }
              else
              {
                trashExists = (PL_strcmp(serverTrashName,
adoptedBoxSpec->allocatedPathName) == 0);
              }


The problem is the following. If using a Cyrus server (or a few others), the
personal namespace is "INBOX." and the serverTrashName is "INBOX/Trash" and
works properly. (I am not sure about the delimiter here, something I have to
check, but I think it is a / because the serverTrashName is in canonical namespace).

When using a server directory though and this server directory is "INBOX/", then
the serverTrashname (which is in canonical namespace) is not INBOX.Trash but
just simply "Trash".

As you can see in the above code, it will fail to find the Trash folder, because
the namespace *is* "INBOX." but the ServerTrashname simply "Trash".

Also, the above code makes a few assumptions it shouldn't.

It compares against INBOX.

* It assumes that INBOX is a personal namespace (which it doesn't have to be
* It assumes that . is a personal namespace delimiter

I am not sure if the check for "INBOX." should be there at all. If the server
name were properly set to a personal namespace, then we wouldn't have this problem.

This seems to me like a quick hack to get cyrus imap severs to somehow work at all.

The reason this still worked when having a server directory set to "INBOX." was
that evengthough the trash folder was not found, a new one is created later in
the code as "Trash". Add the server directory and this becomes "INBOX.Trash"
which is the TRash folder as we know it.

A quick hack to temporarily fix this is:

          if (serverTrashName)
            {
              
                  trashExists = (PL_strcmp(serverTrashName,
adoptedBoxSpec->allocatedPathName) == 0);
              
                          
                          if ((!trashExists) && (!PL_strcasecmp(nsPrefix,
"INBOX."))) // need to special-case t
his because it should be case-insensitive
              {
#ifdef DEBUG
                NS_ASSERTION (PL_strlen(serverTrashName) > 6,
                  "Oops.. less that 6");
#endif
                trashExists = ((PL_strlen(serverTrashName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
                  (PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
                  !PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX." */
                  !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
              }
      

But like I said, the check against "INBOX." shouldn't be there at all.

Assign this to me if you want. I am getting quite into the IMAP code.

Comment 1

[Henry Jia]

Let me take a look at this bug. CCing to myself.

Comment 2

[Scott Gifford (old account)]

Is this the same as bug 27002?

Comment 3

[Scott Gifford (old account)]

Or perhaps the same as bug 80858?...

Comment 4

[Michael Klose]

I don't consider this a dupe of the other two bugs.

This is just a bad code issue.

Because the trash folder is being "found" (or rather recreated) later on. But it
shouldn't try to recreate an exisiting trash folder.

Comment 5

[Henry Jia]

Attachment: patch for review

This is a logical error. After mozilla changes the 'Trash' folder name to
canonical format, it will never get the name with 'INBOX.' prefixed. If the
'IMAP Server Directory' setting is not set, the changed name will be
'INBOX/Trash'. If the setting is set, the changed name will be 'Trash' that
trigger the ASSERTION.

Comment 6

[Henry Jia]

Taking. :-)

Add mscott to CC list.

Comment 7

[Henry Jia]

Attachment: patch for review

'INBOX' should be case insensitive.

Pls r=/sr=.

Comment 8

[David :Bienvenu]

+              ns ? ns->GetDelimiter() : 0, &serverTrashName); 
0 will never work, right?

Comment 9

[Henry Jia]

Attachment: new patch for review

David, you have the sharpest eye. :-)

Indeed, if code goes here, there is no need to check ns. It must not be nsnull.
We have checked above.

Here is the new patch. Pls r=/sr=. Thx.

Comment 10

[Henry Jia]

Add navin & cavin to the CC list.

Pls r=. Thx.

Comment 11

[Navin Gupta]

Comment on attachment 94288 [details] [diff] [review]
new patch for review

r=naving

Comment 12

[Martin Sch]

This should also be related to bug 24064

Comment 13

[David :Bienvenu]

+              if (strlen(serverTrashName) >= 6 &&
+                  !PL_strncasecmp(serverTrashName, "INBOX/", 6)) //
case-insensitive
can't this just be:

 if (!PL_strncasecmp(serverTrashName, "INBOX/", 6)) ?

what is the purpose of this change? It looks superfluous to me. It seems to me
that the change that actually fixes this bug is that one line above, changing
INBOX. to INBOX/

-                trashExists = ((PL_strlen(serverTrashName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
-                  (PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 /*
nsCRT::strlen("INBOX.") */) &&
-                  !PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX." */
-                  !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));
+                trashExists = (strlen(adoptedBoxSpec->allocatedPathName) >= 6
&&              /* "INBOX/" */
+                    !PL_strncasecmp(adoptedBoxSpec->allocatedPathName,
serverTrashName, 6) && /* "INBOX/" */
+                    !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6));

Comment 14

[Henry Jia]

Attachment: new patch for review

A clearer patch. The patch does mainly compare 'serverTrashName' (in canonical
format) with 'INBOX/'.

Just a little change, carrying navin's r=.

Comment 15

[David :Bienvenu]

that looks like the same patch to me, but it removes these two lines

trashExists = ((PL_strlen(serverTrashName) > 6 /* nsCRT::strlen("INBOX.") */) &&
-                  (PL_strlen(adoptedBoxSpec->allocatedPathName) > 6 

which are actually needed because of  this line:
+                              !PL_strcmp(adoptedBoxSpec->allocatedPathName + 6,
serverTrashName + 6);

Again, do you really need to change anything more than the one line?

Comment 16

[Henry Jia]

The core of this patch is
-              if (!PL_strcasecmp(nsPrefix, "INBOX.")) // need to special-case
this because it should be case-insensitive
+              if (!PL_strncasecmp(serverTrashName, "INBOX/", 6)) //
case-insensitive
After setting 'IMAP Server Directory', 'serverTrashName' may not include 'INBOX/'.

I removes these two lines is because they are not needed. If code goes there, we
have executed the following code and the condition is ok

if (!PL_strncasecmp(serverTrashName, "INBOX/", 6))

and

!PL_strncasecmp(adoptedBoxSpec->allocatedPathName, serverTrashName, 6)

which implies the removed lines.

Comment 17

[David :Bienvenu]

Comment on attachment 96670 [details] [diff] [review]
new patch for review

ok, thx, I think I see.

Comment 18

[Henry Jia]

In trunk.

Comment 19

[Gregg Meehan]

marking as VERIFIED