Difficulty finding trash folder when using an IMAP server directory

VERIFIED FIXED

Status

VERIFIED FIXED
17 years ago
11 years ago

People

(Reporter: mkmail, Assigned: Henry.Jia)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
Let me take a look at this bug. CCing to myself.
Is this the same as bug 27002?

Updated

17 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Or perhaps the same as bug 80858?...

(Reporter)

Comment 4

17 years ago
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.

Updated

17 years ago
QA Contact: gayatri → meehansqa
(Assignee)

Updated

17 years ago
Blocks: 160644
(Assignee)

Comment 5

17 years ago
Created attachment 94009 [details] [diff] [review]
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.
(Assignee)

Comment 6

17 years ago
Taking. :-)

Add mscott to CC list.
Assignee: mscott → Henry.Jia
Keywords: review
Hardware: PC → All
(Assignee)

Comment 7

17 years ago
Created attachment 94013 [details] [diff] [review]
patch for review

'INBOX' should be case insensitive.

Pls r=/sr=.
Attachment #94009 - Attachment is obsolete: true

Comment 8

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

Comment 9

17 years ago
Created attachment 94288 [details] [diff] [review]
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.
Attachment #94013 - Attachment is obsolete: true
(Assignee)

Comment 10

17 years ago
Add navin & cavin to the CC list.

Pls r=. Thx.
Status: NEW → ASSIGNED

Comment 11

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

r=naving
Attachment #94288 - Flags: review+

Comment 12

17 years ago
This should also be related to bug 24064

Comment 13

17 years ago
+              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));
(Assignee)

Comment 14

17 years ago
Created attachment 96670 [details] [diff] [review]
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=.
Attachment #94288 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #96670 - Flags: review+

Comment 15

17 years ago
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?
(Assignee)

Comment 16

17 years ago
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

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

ok, thx, I think I see.
Attachment #96670 - Flags: superreview+
(Assignee)

Comment 18

17 years ago
In trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 19

16 years ago
marking as VERIFIED
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.