441916 - Consolidate UpdateSummaryTotals into nsMsgDBFolder

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

A comment in nsImapMailFolder::UpdateSummaryTotals says:

// could we move this into nsMsgDBFolder, or do we need to deal
// with the pending imap counts?

In fact the code is duplicated in several places, and can be effectively moved. Since I've been reviewing handling of counts, I want to do this to make changes easier.

Comment 1

[Kent James (:rkent)]

Attachment: UpdateSummaryTotals moved to parent

Although there were subtle differences between the versions, as far as I can tell they were not significant. The pending messages are set to zero. The check for mIsServer in:

if (!mNotifyCountChanges || mIsServer)

only occurred in the IMAP version, but I believe is also valid in News and Local, right?

Comment 2

[David :Bienvenu]

Comment on attachment 326853 [details] [diff] [review]
UpdateSummaryTotals moved to parent

thx, Kent, yes, this looks right.

Comment 3

[Mark Banner (:standard8)]

Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.350; previous revision: 1.349
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.822; previous revision: 1.821
done
Checking in mailnews/imap/src/nsImapMailFolder.h;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.h,v  <--  nsImapMailFolder.h
new revision: 1.254; previous revision: 1.253
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.593; previous revision: 1.592
done
Checking in mailnews/local/src/nsLocalMailFolder.h;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.h,v  <--  nsLocalMailFolder.h
new revision: 1.166; previous revision: 1.165
done
Checking in mailnews/news/src/nsNewsFolder.cpp;
/cvsroot/mozilla/mailnews/news/src/nsNewsFolder.cpp,v  <--  nsNewsFolder.cpp
new revision: 1.300; previous revision: 1.299
done
Checking in mailnews/news/src/nsNewsFolder.h;
/cvsroot/mozilla/mailnews/news/src/nsNewsFolder.h,v  <--  nsNewsFolder.h
new revision: 1.103; previous revision: 1.102
done

Comment 4

[Rain [:rain1] (inactive)]

There are plenty of errors of this sort on the console now:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file D:/mozilla-build/msys/co/moz-expt/mailnews/base/util/nsMsgDBFolder.cpp, line 3286 (3283 on trunk)

Earlier the rv of ReadDBFolderInfo wasn't being tested, now it is.

Comment 5

[Kent James (:rkent)]

I'm seeing this now on startup - do you see it any other time?

It's being called from GetSubfolders/UpdateSummaryTotals, which then ignores the error return. So it is a false assertion. The problem would go away if we had force=TRUE - but Bienvenu added that in 1999 for some reason.

Any idea why the force=false is needed here (http://mxr.mozilla.org/mozilla/source/mailnews/local/src/nsLocalMailFolder.cpp#431):

   UpdateSummaryTotals(PR_FALSE);

We could also just ignore the error, but I think it would be better to understand what is going on.

Comment 6

[David :Bienvenu]

Calling it with force=true forces us to open the db - that change would cause us to open the db of every local folder during folder discovery, which would pretty much make the cache useless.

Comment 7

[Rain [:rain1] (inactive)]

(In reply to comment #5)
> We could also just ignore the error, but I think it would be better to
> understand what is going on.
> 

I stepped through the code a bit, and what's happening is that in normal operation, ReadDBFolderInfo() is entered:
http://mxr.mozilla.org/mozilla/source/mailnews/base/util/nsMsgDBFolder.cpp#496

Now none of the two if blocks are actually entered, so NS_ERROR_FAILURE is returned.

Absolutely nowhere else is the rv from ReadDBFolderInfo tested (verified using mxr), so it should be safe to change the NS_ERROR_FAILURE to NS_OK.

Comment 8

[Rain [:rain1] (inactive)]

(or at least quieten the NS_ENSURE_SUCCESS -- essentially, right now NS_ERROR_FAILURE is an expected case)

Comment 9

[Kent James (:rkent)]

(In reply to comment #7)

> 
> Absolutely nowhere else is the rv from ReadDBFolderInfo tested (verified using
> mxr), so it should be safe to change the NS_ERROR_FAILURE to NS_OK.
> 

I think it is safe compared to the previous behavior - but what I would like to understand is why we are calling it in cases where it does not work.

Comment 10

[Kent James (:rkent)]

I'm going to reopen this to keep it on the radar. I'm doing a lot of review right now of related code, and I want to consider what is the best approach to this as part of that review. I think for now we can leave the assertions as a temporary but harmless annoyance, but I'll get back to this.

Comment 11

[walter sheets]

Mark's checkin (comment #3) has the annoying side effect of expanding all of
the usenet account folders every time tb is started.  The pop3/imap folders
are not affected.

Anyone else seeing this?

Comment 12

[Kent James (:rkent)]

(In reply to comment #11)
> Mark's checkin (comment #3) has the annoying side effect of expanding all of
> the usenet account folders every time tb is started.  The pop3/imap folders
> are not affected.
> 
> Anyone else seeing this?
> 

I see this, and can confirm that backing out the patch removes the behavior. I'll look into it. Thanks for pointing this out.

Comment 13

[Kent James (:rkent)]

The news folder opening problem is occurring because of the "isServer" in the opening statement:

  if (!mNotifyCountChanges || mIsServer)

which was only present originally in the IMAP version. The mIsServer was added by bienvenu in 2001 as part of a patch dealing with a crash and some IMAP folder display problems. I don't understand the issues with that patch well enough to understand why it is there. I guess I could try to special-case the IMAP folder - but this was supposed to be a simplification, and I'm reluctant to start adding back in complications like that.

So I'm leaning now toward backing out the patch and abandoning the bug, unless David Bienvenu has some other suggestion.

Comment 14

[David :Bienvenu]

worst case, you could remove the isServer check from the base class, override the method in nsImapMailFolder.cpp as follows:

return (mIsServer) ? NS_OK : nsMsgDBFolder::UpdateSummaryTotals(...)

I had a quick look at the bug with the patch that added the mIsServer check - I'd be a little leary of removing that check.

Comment 15

[Kent James (:rkent)]

Attachment: isServer IMAP only, removed NS_ENSURE_SUCCESS

This is an addon patch. I moved the isServer check to the IMAP version, and changed the NS_ENSURE_SUCCESS to an if (NS_SUCCEEDED(...)) on the action section, and just returned the error.

Comment 16

[David :Bienvenu]

Comment on attachment 328057 [details] [diff] [review]
isServer IMAP only, removed NS_ENSURE_SUCCESS

thx, Kent.

I usually don't like putting bug #'s in comments, but since cvs history isn't going to help us here so much, maybe you could add a comment in the nsImapMailFolder override about the bug that we were worried about -  bug 72871

Comment 17

[Kent James (:rkent)]

Attachment: patch to checkin

Added comment referencing bug 72871.

Comment 18

[Mark Banner (:standard8)]

Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.352; previous revision: 1.351
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.826; previous revision: 1.825
done
Checking in mailnews/imap/src/nsImapMailFolder.h;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.h,v  <--  nsImapMailFolder.h
new revision: 1.256; previous revision: 1.255
done

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> Mark's checkin (comment #3) has the annoying side effect of expanding all of
> the usenet account folders every time tb is started.

SeaMonkey 2.0a1pre has/had this regression too.