311774 - Newsgroup unread counts don't reflect filter actions

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect, P2)

Description

[Gudmund Areskoug]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Thunderbird version 1.0.7 (20050923)

When using the "Delete the message" option to "kill file", i. e. delete message
headers, messages with deleted headers still affect indication of how many
unread messages the folder/newsgroup has.

Now, the NG "unread messages" indicator shows unread messages, although there
are none, save the "deleted" ones, tricking the user to go read "new" messages
when there aren't any. Rather annoying.

Reproducible: Always

Steps to Reproduce:
1. Create a message filter in a newsgroup using the "Delete" option by e. g.
right-clicking the "From: " address and using "Create Filter from Message" and
ticking "Delete the message"
2. Receive messages from the filtered-out sender
3. Watch how many unread messages are indicated for the newsgroup
4. Go to the NG, count how many unread messages are in there

Actual Results:  
5. Leave the NG after making sure there are no more unread messages
6. Watch the number of unread messages pop back up after a while, note how many
it shows
7. Go to NG again, note how many unread messages are actually shown inside the NG


Expected Results:  
Messages that were "deleted" by the filter should not affect the "unread"
message count/indication.

Perhaps the "Delete the message" option in message filters should also mark the
"deleted" messages as "read" by default.

I tried adding "Mark the message as read" to the filter as a workaround.
Apparently doesn't work.

Could be related to bug https://bugzilla.mozilla.org/show_bug.cgi?id=285039

Comment 1

[Joshua Cranmer [:jcranmer]]

Gudmund (or anyone else): do you still see this problem with TB 2.0.0.*, trunk TB, or related versions of Seamonkey? If not, I will resolve this bug as INCO in 3 weeks.

Comment 2

[Gudmund Areskoug]

Have hardly used Newsgroups in a long time, in large part due to the various bugs in TB, but now I've tested this bug again, by going into the mozilla.support.firefox and killing a few threads, preferably ones with posters I know to be prolific (smile, you're a guineapig ;) ).

I'm now using TB 2.0.0.12 (portable version), and at first thought you had it solved (at least sort of), with the "unread" figure briefly flickering by and disappearing at times.

But having TB e. g. indicating 6 unread in the NG when I'm outside it and shrinking the number to 4 once I click in the NG doesn't look encouraging.

Switching between showing ignored threads and not showing them revealed the difference in the figures corresponded to the number of new messages in the experiment threads.

Installing update to 2.0.0.14 now, will tell if that behaves the same.

Please note that finding an exact occasion where TB will indicate messages unread when there are *none* new that aren't ignored, will be a bit difficult now that I don't visit NGs regularly any more. Will check it all the same...

Comment 3

[Joshua Cranmer [:jcranmer]]

Gudmund, beware that we don't run the filters until you actually click in the NG, which is what updates the unread count. I would try to reproduce this myself, but between gobs of filters doing whatnot, and the phantom message bug 71390, my current setup has too many factors to isolate the existence of a specific bug.

It could also be a dupe of one the dependencies of bug 71728...

Comment 4

[Gudmund Areskoug]

Aha, so the killing/ignoring is a filter setting (which doesn't show up in the filtering dialogue). 

The problem (annoyance) is that the unread count fools you into thinking there's something new to be read, and then there's nothing in there for you to read. Depending on circumstances and newsgroup, this can happen quite a lot.

I don't know how NG servers tell NG clients whether and how many new messages there are. Depending on how that is done, there might perhaps be a way to assess which new messages are in the kill list before presenting the Unread figure. 

Perhaps pre-fetching headers might be a way, don't know, but that would eat some bandwidth, which might in turn also be undesired.

Comment 5

[Joshua Cranmer [:jcranmer]]

Independent of other bugs in message counts, there are two problems with newsgroup message counts that aren't really bugs:

1. We rely on the server to tell us the number of new messages, but this number is almost always an overestimate and would include in its count messages that were subsequently expired or canceled.

2. We don't run filters when you run "Get New Messages" like IMAP or POP does.

Any other reason for incorrect newsgroup counts is truly a bug. However, trying to diagnose the independence of message count bugs is difficult because of these problems...

Comment 6

[Joshua Cranmer [:jcranmer]]

Coming back to this as part of the closeme triage:

The commenter has not provided sufficient evidence to find a message count bug outside of the two sources mentioned in the previous comment. It is possible that this bug is a dupe of a dep of 71728, although my cursory examinations of open bugs seems to indicate that this is not the case.

Changing the summary to describe the problem better, blocking 71728 for triaging purposes, and moving to Core -> Networking: News since that's where all unread count problems are (hopefully).

Comment 10

[Joshua Cranmer [:jcranmer]]

I'll start work on this, since it's been agreed over IRC that downloading headers on the biff+etc. notifications would be OK. Fixing this should also simplify my life with respect to tracking newsgroup unread count bugs (it's a major source of error in my setup).

I also think that this may fix other bugs "by accident"--I just remembered that you have to reload to get new headers if biff fires while in the newsgroup, and the separation of filtering from biff may be why.

Comment 11

[Joshua Cranmer [:jcranmer]]

BTW, I want to set the milestone to 3.0rc1, but that's not possible until reorg pt. 2...

Comment 12

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 0.1

This is a start. I verified that the biff works properly in one common case (a password-less server), and it almost definitely works in the other common case (one single password). I have not tested what happens if the server requires different passwords for different newsgroups.

I also verified that this will fix another bug "by accident" (the one I mentioned in the previous comment).

What is yet needed:
* I would like to remove the code solely performed by the old biff code.
* tests are nice, but are presently difficult to work as it requires one of two bugs to be fixed first.

Comment 13

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 0.5

This patch adds tests (yay tests!), and also rips out some of the old code. Two things yet before I can request review:

* test_download seems to fail about 1 of every 10 invocations (it doesn't get around to actually running the commands)
* I need more test_filter modification to take into account the newly-added filterTestUtils.

Multithreaded testing sucks...

Comment 14

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 0.8

This doesn't have tests, because the tests that I have written randomly failed and none of my hacks to date got it to unrandomly fail. In short, multithreading + tests + lack of callbacks = yuck, yuck, yuck.

Comment 15

[David :Bienvenu]

Joshua, can you verify that this doesn't result in us holding the .msf files open for every newsgroup on the server after the process is done?

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 338168 [details] [diff] [review]
Patch, version 0.8

Cancelling reviews, there's a few more things that need to go on here.

Comment 17

[Joshua Cranmer [:jcranmer]]

One of the things I discovered is that this makes it really easy to overdo the max connection limit on server-side, so this means that we need some functionality to actually respect the maximum connection limit we claim to support.

Comment 19

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 1

I have verified that the msf files do not remain open after biff with this patch. There is also a test to make sure this actually works (which now reliably passes and has no leaks to boot *mutters about JS closures causing leaks*).

I have also decided to keep the preferences and strip out the old code.

Comment 20

[David :Bienvenu]

Comment on attachment 395058 [details] [diff] [review]
Patch, version 1

I'm going to see what Neil thinks of this since I believe he uses usenet more than I do.

Comment 21

[[e-mail]]

Comment on attachment 395058 [details] [diff] [review]
Patch, version 1

+  rv = list->FinishXOVERLINE(0,&status);
Nit: might as well add a space after the comma while you're here

>@@ -320,6 +320,25 @@ nsresult nsMsgNewsFolder::GetDatabase()
> }
> 
> NS_IMETHODIMP
>+nsMsgNewsFolder::GetDatabaseWithoutCache(nsIMsgDatabase **db)
This looks ugly. Can we at least share code with GetDatabase? e.g. via
nsresult OpenOrCreateDatabase(PRBool aAddListener, nsIMsgDatabase **db);
[I wonder why, say, IMAP doesn't seem to need this rigmarole?]

>-      rv = AutoCompact(aWindow);
>-      NS_ENSURE_SUCCESS(rv,rv);
>+      if (aWindow)
>+      {
>+        rv = AutoCompact(aWindow);
>+        NS_ENSURE_SUCCESS(rv,rv);
>+      }
[Part of another patch?]

There was a blank line with spaces in one of your test files somewhere.

Comment 22

[David :Bienvenu]

Comment on attachment 395058 [details] [diff] [review]
Patch, version 1

wasCached and flipping the sense would be more accurate - someone else may have the db open, so wasClosed isn't really right.

+  PRBool wasClosed = !mDatabase;
+  nsresult rv = GetDatabase();
+  NS_IF_ADDREF(*db = mDatabase);
+
+  // If the DB was not open before, close our reference to it now.
+  if (wasClosed && mDatabase)
+  {
+    mDatabase->RemoveListener(this);
+    mDatabase = nsnull;
+  }

(this appears in two places).

I still worry a little that this might cause a bit of a hiccup for people with high traffic newsgroups but xover is a ton faster than IMAP, so the hiccup might be more on the adding of headers to the local .msf file.

Comment 23

[David :Bienvenu]

thinking about this a bit more, I'm not convinced that you're not leaving the news db's open. The call to nsIMsgDatabase::SetMsgDatabase(nsnull) breaks some cycles between headers and the db, by calling nsIMsgDatabase::clearCachedHdrs - does that get called in your new code?

Comment 24

[Joshua Cranmer [:jcranmer]]

(In reply to comment #21)
> >@@ -320,6 +320,25 @@ nsresult nsMsgNewsFolder::GetDatabase()
> > }
> > 
> > NS_IMETHODIMP
> >+nsMsgNewsFolder::GetDatabaseWithoutCache(nsIMsgDatabase **db)

> This looks ugly. Can we at least share code with GetDatabase? e.g. via
> nsresult OpenOrCreateDatabase(PRBool aAddListener, nsIMsgDatabase **db);

I'm not sure what you mean here-- the latter already calls the former...

> >-      rv = AutoCompact(aWindow);
> >-      NS_ENSURE_SUCCESS(rv,rv);
> >+      if (aWindow)
> >+      {
> >+        rv = AutoCompact(aWindow);
> >+        NS_ENSURE_SUCCESS(rv,rv);
> >+      }
> [Part of another patch?]

That, I think, was a vestige from an earlier version of this patch.

(In reply to comment #23)
> thinking about this a bit more, I'm not convinced that you're not leaving the
> news db's open. The call to nsIMsgDatabase::SetMsgDatabase(nsnull) breaks some
> cycles between headers and the db, by calling nsIMsgDatabase::clearCachedHdrs -
> does that get called in your new code?

In reading the database code, it seems that the headers do not addref the database, so they shouldn't hold the database open.

Comment 25

[David :Bienvenu]

(In reply to comment #24)
 
> In reading the database code, it seems that the headers do not addref the
> database, so they shouldn't hold the database open.

http://mxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgHdr.cpp#61

Comment 26

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 1.1

r+/sr+ carried over from last patch.

I'm not sure if I need the approval-seamonkey2 flag or not (I change some suite strings), but I'll request anyways in case I do.

Comment 27

[Mark Banner (:standard8)]

This is a pretty large patch and I'd be happier with some sort of risk assessment - How extensive are the tests? How likely are we to notice regressions? What sort of thing can regress?

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 402483 [details] [diff] [review]
Patch, version 1.1

Joshua has explained this to me on irc and its simpler than it looks. Hence a=Standard8.

Comment 29

[Joshua Cranmer [:jcranmer]]

Pushed as changeset 3897:c41b9cf09178.

Comment 30

[Robert Kaiser]

adding keyword so it falls off the radar.

Comment 31

[Francesco Lodolo [:flod]]

Is it possible to add localization notes for the new strings (newNewsgroupHeaders and newNewsgroupFilteringHeaders)?

Comment 32

[Joshua Cranmer [:jcranmer]]

(In reply to comment #31)
> Is it possible to add localization notes for the new strings
> (newNewsgroupHeaders and newNewsgroupFilteringHeaders)?

Done via changeset a8f2369df790.

Comment 34

[Alexander L. Slovesnik]

(In reply to comment #32)
> (In reply to comment #31)
> > Is it possible to add localization notes for the new strings
> > (newNewsgroupHeaders and newNewsgroupFilteringHeaders)?
> 
> Done via changeset a8f2369df790.
Looks like there is typo in:
newNewsgroupFilteringHeaders=Getting headers for filters: %1$S (%2$S/%3$S) on %4#S

s/%4#S/%4$S

Comment 35

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #34)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > Is it possible to add localization notes for the new strings
> > > (newNewsgroupHeaders and newNewsgroupFilteringHeaders)?
> > 
> > Done via changeset a8f2369df790.
> Looks like there is typo in:
> newNewsgroupFilteringHeaders=Getting headers for filters: %1$S (%2$S/%3$S) on
> %4#S
> 
> s/%4#S/%4$S

can't see it: http://hg.mozilla.org/comm-central/file/a8f2369df790/mail/locales/en-US/chrome/messenger/news.properties#l65

Comment 36

[Alexander L. Slovesnik]

(In reply to comment #35)
> can't see it:
> http://hg.mozilla.org/comm-central/file/a8f2369df790/mail/locales/en-US/chrome/messenger/news.properties#l65
It's in /suite copy - http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/mailnews/news.properties#65

Comment 37

[Alexander L. Slovesnik]

Attachment: Fix typo

Comment 38

[Robert Kaiser]

Comment on attachment 404000 [details] [diff] [review]
Fix typo

I found this indepentently and got r+a=Standard8 on #maildev for it, so I pushed it as http://hg.mozilla.org/comm-central/rev/b951cf498d4f - noting this attachment in the comment though.