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

RESOLVED FIXED

Status

--
major
RESOLVED FIXED
17 years ago
10 years ago

People

(Reporter: mozilla-bugs, Assigned: mscott)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

17 years ago
Summary: namespace prefs keep growing. → namespace prefs keep growing.

Comment 1

17 years ago
do you have the check_all_folders pref set to on.
I do and I get this weird pref set to. On win2k
Severity: minor → normal
OS: Linux → All
Hardware: PC → All
(Reporter)

Comment 2

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

Comment 3

17 years ago
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?
Summary: namespace prefs keep growing. → namespace prefs keep growing when "mail.check_all_imap_folders_for_new" is on
(Reporter)

Comment 4

17 years ago
> What machine are you on?

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

Updated

17 years ago
Blocks: 160644

Comment 5

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

17 years ago
this bug keeps cauing me to get "mailbox folder does not exists" IMAP error alerts
Severity: normal → major

Comment 7

17 years ago
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!
Summary: namespace prefs keep growing when "mail.check_all_imap_folders_for_new" is on → namespace prefs keep growing when checking for mail in multiple folders is on due to cached connections

Comment 8

16 years ago
Created attachment 107654 [details] [diff] [review]
patch for test

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

Comment 9

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

Comment 10

16 years ago
No matter. Thank you anyway. :-)

Comment 11

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

Updated

16 years ago
Attachment #107654 - Flags: review?(bienvenu)

Comment 12

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

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

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

16 years ago
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?
Attachment #107654 - Flags: review?(bienvenu) → review+

Comment 16

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

Updated

16 years ago
QA Contact: huang → gchan

Comment 17

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

sr=Henry

Pls add the comments when checking in according to David's suggestion.
Attachment #107654 - Flags: superreview+

Comment 18

16 years ago
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.
Attachment #107654 - Flags: approval1.3?
Comment on attachment 107654 [details] [diff] [review]
patch for test

a=asa (on behalf of drivers) for checkin to 1.3final.
Attachment #107654 - Flags: approval1.3? → approval1.3+

Comment 20

16 years ago
Patch in.

=> Resolved
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.