148043 - namespace prefs keep growing when checking for mail in multiple folders is on due to cached connections

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Aleksey Nogin]

My prefs.js currently contains

user_pref("mail.server.server1.namespace.personal",
"\"INBOX.\",\"INBOX.\",\"INBOX.\",\"INBOX.\",\"INBOX.\"");
user_pref("mail.server.server1.namespace.public",
"\"shared.\",\"shared.\",\"shared.\",\"shared.\",\"shared.\"");

I keep seeing this on different RedHat machines with both Courier IMAP and MS
Exchange. From time to time I would clean up the prefs (e.g. leave only one
copy) and it'll start growing again. Seems to still be happening with recent
trunk builds.

Comment 1

[Henrik Gemal]

do you have the check_all_folders pref set to on.
I do and I get this weird pref set to. On win2k

Comment 2

[Aleksey Nogin]

Yes, I do have user_pref("mail.check_all_imap_folders_for_new", true) !

Comment 3

[Henrik Gemal]

Also seen on Cyrus IMAP server.

I'm only seeing this on an slow machine 350Mhz P2. Not on my 900Mhz P3. What
machine are you on?

Comment 4

[Aleksey Nogin]

> What machine are you on?

Dual PPro-200; PII-266; PIII-450

Comment 5

[Henrik Gemal]

this bug is quite annoying. could be the source of some of the imap problems
users sometimes see, like selecting wrong folders, imap failures, etc...

I keep seeing this bug for the last year

20060816

Comment 6

[Henrik Gemal]

this bug keeps cauing me to get "mailbox folder does not exists" IMAP error alerts

Comment 7

[Henrik Gemal]

the bug is both caused by having:
user_pref("mail.check_all_imap_folders_for_new", true)
and by setting "check folder on startup" via the folder property

it *have* something to do with using multiple connections, since my IMAP log 
shows something like:

1040 - * NAMESPACE (("INBOX." ".")) NIL (("" "."))
1040 - 3 OK Completed
1040 - 4 lsub "" "INBOX.*"
724  - * NAMESPACE (("INBOX." ".")) NIL (("" "."))
724  - 3 OK Completed
724  - 4 lsub "" "INBOX.*"
1224 - * NAMESPACE (("INBOX." ".")) NIL (("" "."))
1224 - 3 OK Completed
1224 - 4 lsub "" "INBOX.*"

so I'm actually having 3 connections to my IMAP just start Mozilla.

when having max_cached_connections set to 2 the pref gets set to:
""\"INBOX.\",\"INBOX.\""

which proofs my theory!

Comment 8

[philip zhao]

Attachment: patch for test

Aleksey and henrik , please test it. It has work in my account.

Comment 9

[Aleksey Nogin]

I no longer have a setup to test this, sorry.

Comment 10

[philip zhao]

No matter. Thank you anyway. :-)

Comment 11

[Henrik Gemal]

Looks like a great fix. I dont build mozilla myself, so I can test. But you
should seek review and get this baby in asap...

I have to turn off the "override namespace" because of this bug.

Comment 12

[David :Bienvenu]

Good job, Phillip.

Two things:

If the namespace is the same (which is what we're checking, right), we should
just return, no need to delete the old one and add the new one.

But more importantly, we shouldn't be issuing the namespace command for each
connection.

We could make a change like this:
    m_hostSessionList->GetNamespacesOverridableForHost(GetImapServerKey(),
nameSpacesOverridable);
    m_hostSessionList->GetGotNamespacesForHost(GetImapServerKey(),
haveNameSpacesForHost); 

  // mscott: VERIFY THIS CLAUSE!!!!!!!
    if (nameSpacesOverridable && !haveNameSpacesForHost)
    {
--->>>>    m_hostSessionList->SetGotNamespacesForHost(GetImapServerKey(), PR_TRUE); 
      Namespace();
    }

then, we'd only get the namespace for the one connection, which is where we want
to end up, I think. Though now that I think about it, we'd have a race condition
between the connections, and the second/third/... connections might end up
without valid namespaces because the first connection hadn't retrieved it yet.
So, I think the best fix is the first one I described - if the new namespace
exists, just return.

Comment 13

[philip zhao]

Thx, David. I know what you mean.

But I still have a question.

If the originally created m_NamespaceList has more than one namespaces which is
same with what we want to add, should we let same namespaces exits in the
m_NamespaceList or delete them?(I prefer to delete any same ones)

For example, in this bug's condition, the original m_NamespaceList before we
sent 'namespace' command and add new namespace is "","","","","Shared
Folders/User/","Shared Folders/User/","Shared Folders/User/","Shared
Folders/User/", and when we add namespace "", we should delete all same
namespaces.(as in attachment 107654 [details] [diff] [review])

Of course ,if the original namespaceList is right, just return when we meet a
same namespace will work well.

Pls confirm it and I will add a new patch soon.

Comment 14

[David :Bienvenu]

Phillip, I see your point, yes, we still want to delete all the namespaces that
come from the prefs when we get a namespace from the server so we don't want to
return before finishing the loop. So, I think your fix is the simplest way of
solving both problems.

So, why don't you just add a comment to your fix saying that? Something like:

// if we find existing namespace(s) that matches the new one, we'll just remove
// the old ones and let the new one get added when we've finished checking for
// matching namespaces or namespaces that came from prefs.

r=bienvenu, if you put a comment like that in there.

Comment 15

[David :Bienvenu]

Comment on attachment 107654 [details] [diff] [review]
patch for test

actually, are we sure that GetPrefix will return a non-null pointer? Would we
have already crashed because the constructor does a PL_strdup of prefix?

Comment 16

[philip zhao]

Thanks, David. I will pay attention later.

To the failure of PL_strdup in constructor, I find it will still return (char*)0
as the m_Prefix when there are no space to malloc. So the call
PL_strcmp(ns->GetPrefix(),nspace->GetPrefix()) won't fail here. At other places
we must pay enough attention. Is it right? If not, pls tell me to have a change.

Thank you! David.

Comment 17

[Henry Jia]

Comment on attachment 107654 [details] [diff] [review]
patch for test

sr=Henry

Pls add the comments when checking in according to David's suggestion.

Comment 18

[Henry Jia]

Comment on attachment 107654 [details] [diff] [review]
patch for test

This is an annoying bug. The patch is small. It just make a strict check. The
risk is small.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 107654 [details] [diff] [review]
patch for test

a=asa (on behalf of drivers) for checkin to 1.3final.

Comment 20

[Henry Jia]

Patch in.

=> Resolved