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.
Created attachment 326853 [details] [diff] [review] 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 on attachment 326853 [details] [diff] [review] UpdateSummaryTotals moved to parent thx, Kent, yes, this looks right.
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
Target Milestone: --- → mozilla1.9.1a1
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
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.
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.
(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.
(or at least quieten the NS_ENSURE_SUCCESS -- essentially, right now NS_ERROR_FAILURE is an expected case)
(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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
(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.
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.
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.
Created attachment 328057 [details] [diff] [review] 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 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
Created attachment 328062 [details] [diff] [review] patch to checkin Added comment referencing bug 72871.
Attachment #328057 - Attachment is obsolete: true
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
(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.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.