Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Newsgroup unread counts don't reflect filter actions

RESOLVED FIXED in Thunderbird 3.0rc1

Status

MailNews Core
Networking: NNTP
P2
normal
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: Gudmund Areskoug, Assigned: jcranmer)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {fixed-seamonkey2.0})

Trunk
Thunderbird 3.0rc1
fixed-seamonkey2.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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
QA Contact: front-end
(Assignee)

Comment 1

9 years ago
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.
Assignee: mscott → nobody
Whiteboard: closeme 2008-05-22
(Reporter)

Comment 2

9 years ago
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...
(Assignee)

Comment 3

9 years ago
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...
(Reporter)

Comment 4

9 years ago
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.
(Assignee)

Comment 5

9 years ago
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...
(Assignee)

Comment 6

9 years ago
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).
Blocks: 71728
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → Networking: News
Ever confirmed: true
OS: Windows 2000 → All
Product: Thunderbird → Core
QA Contact: front-end → networking.news
Hardware: PC → All
Summary: Newsgroup message filter "Delete the message" leaves non-sticky "unread" indication → Newsgroup unread counts don't reflect filter actions
Whiteboard: closeme 2008-05-22
Version: unspecified → Trunk
(Assignee)

Updated

9 years ago
Duplicate of this bug: 361374
(Assignee)

Updated

9 years ago
Duplicate of this bug: 34406
(Assignee)

Updated

9 years ago
Duplicate of this bug: 202341
(Assignee)

Updated

9 years ago
Depends on: 240209
(Assignee)

Comment 10

9 years ago
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.
Assignee: nobody → Pidgeot18
Blocks: 240209
Status: NEW → ASSIGNED
No longer depends on: 240209
(Assignee)

Comment 11

9 years ago
BTW, I want to set the milestone to 3.0rc1, but that's not possible until reorg pt. 2...
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
(Assignee)

Comment 12

9 years ago
Created attachment 336146 [details] [diff] [review]
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.
(Assignee)

Comment 13

9 years ago
Created attachment 337062 [details] [diff] [review]
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...
Attachment #336146 - Attachment is obsolete: true
(Assignee)

Comment 14

9 years ago
Created attachment 338168 [details] [diff] [review]
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.
Attachment #337062 - Attachment is obsolete: true
Attachment #338168 - Flags: superreview?(bienvenu)
Attachment #338168 - Flags: review?(bienvenu)

Comment 15

9 years ago
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?
(Assignee)

Updated

9 years ago
Attachment #338168 - Flags: superreview?(bienvenu)
Attachment #338168 - Flags: review?(bienvenu)
(Assignee)

Comment 16

9 years ago
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.
(Assignee)

Comment 17

9 years ago
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.
Depends on: 66150
(Assignee)

Updated

9 years ago
Blocks: 362500
Product: Core → MailNews Core
(Assignee)

Updated

8 years ago
Duplicate of this bug: 376804
(Assignee)

Comment 19

8 years ago
Created attachment 395058 [details] [diff] [review]
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.
Attachment #338168 - Attachment is obsolete: true
Attachment #395058 - Flags: superreview?(bienvenu)
Attachment #395058 - Flags: review?(bienvenu)

Updated

8 years ago
Attachment #395058 - Flags: review?(bienvenu) → review?(neil)

Comment 20

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

Updated

8 years ago
Attachment #395058 - Flags: review?(neil) → review+
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.

Updated

8 years ago
Attachment #395058 - Flags: superreview?(bienvenu) → superreview+

Comment 22

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

8 years ago
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?
(Assignee)

Comment 24

8 years ago
(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

8 years ago
(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
(Assignee)

Comment 26

8 years ago
Created attachment 402483 [details] [diff] [review]
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.
Attachment #395058 - Attachment is obsolete: true
Attachment #402483 - Flags: superreview+
Attachment #402483 - Flags: review+
Attachment #402483 - Flags: approval-thunderbird3?
Attachment #402483 - Flags: approval-seamonkey2.0?

Updated

8 years ago
Attachment #402483 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
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?
Whiteboard: [needs risk info]
Attachment #402483 - Flags: approval-thunderbird3? → approval-thunderbird3+
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.
Whiteboard: [needs risk info]
Target Milestone: mozilla1.9.1 → Thunderbird 3.0rc1
(Assignee)

Comment 29

8 years ago
Pushed as changeset 3897:c41b9cf09178.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 30

8 years ago
adding keyword so it falls off the radar.
Keywords: fixed-seamonkey2.0

Updated

8 years ago
Depends on: 518773
Is it possible to add localization notes for the new strings (newNewsgroupHeaders and newNewsgroupFilteringHeaders)?
(Assignee)

Comment 32

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

Done via changeset a8f2369df790.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 518773
(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

8 years ago
(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
(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
Created attachment 404000 [details] [diff] [review]
Fix typo
Attachment #404000 - Flags: review?(neil)

Comment 38

8 years ago
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.
Attachment #404000 - Flags: review?(neil)

Updated

8 years ago
Depends on: 539194
Depends on: 540288
(Assignee)

Updated

7 years ago
Blocks: 128845
(Assignee)

Updated

7 years ago
Blocks: 166342
You need to log in before you can comment on or make changes to this bug.