64476 - threads with unread returns false positives

Status: VERIFIED FIXED

(MailNews Core :: Database, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

pink, I think I figured out what is going on.

I got 100 messages from n.p.m.mac, marked all read but the last two, switched to
threads with unread mode, and it worked fine.  no false positives.

then I did "Edit | Get Next <n> Messages" (<n> for me was 1000?) and got another
1000 messages, and marked all but the last two as read.

then I got tons of false positives.  we must think those messages are unread,
even though they are weren't.

did you ever do a "Get Next <n> Messages"?  I have a feeling if I try n.p.m.mac
on a new profile and download all the messages at once, threads with unread will
work correctly.

I'll go check that out.

Comment 1

[(not reading, please use [e-mail] instead)]

> I have a feeling if I try n.p.m.mac on a new profile and download all the 
> messages at once, threads with unread will work correctly.
> I'll go check that out.

I was right!  pink, I can reproduce this bug if I do "Get Next <x> News
Messages", did you ever do that?

Comment 2

[Mike Pinkerton (not reading bugmail)]

no, i never did that exact command. I just read the new messages as they come in
on a daily or weekly basis.

Comment 3

[[e-mail]]

I get false positives from threads with unread just with normal newsreading.

Comment 4

[[e-mail]]

Attachment: Unthreaded Unread (14,786 bytes)

Comment 5

[[e-mail]]

Attachment: Threaded Unread (85,120 bytes)

Comment 6

[Greg Miller]

I tend to read newsgroups a lot for a while, then stop for a few months. As a
result, I have lots of old messages left over from around April of 2000, some
more from november and december, and some current messages.

All or nearly all of the threads from last year show false positives. Many of
those were downloaded by clicking on the newsgroup name and allowing the
newsreader to download 500 messages.

However, I also get a number of temporary false positives on threads I've marked
read with "Mark All Read" or through normal reading recently. Typically, these
go away the next time I open the newsgroup. The false positives in the first
group don't do that.

Comment 7

[Peter Janes]

I see the same as gmiller and neil; I've never done a 'get next n messages', and
I deleted my news files at the beginning of the year, but I use 'mark all read'
regularly.

Comment 8

[(not reading, please use [e-mail] instead)]

setting to 0.9, p1. 

while not a crasher, this false positive problem needs to be fixed.

Comment 9

[Jacek Piskozub]

Some of the "false positives" in threaded newsgroups could be the deeply nested
messages that are not found by the "n" command (bug 70121). That is they could
be real but unreachable by "next message", and therefore looking like false
positives.

Comment 10

[scottputterman]

removing mozilla0.9 and nominating for nsbeta1. Seth, will this get fixed with
the rewrite?

Comment 11

[[e-mail] (gone - use [e-mail] instead)]

This might not be valid when the performance trunk and the branch merge a bit
later on.  The whole front end of the mail window is getting re-written,
including the threading model.  See
http://www.mozilla.org/mailnews/performance/speed.html

Comment 12

[David :Bienvenu]

yes, the rewrite will almost certainly fix this.

Comment 13

[scottputterman]

marking nsbeta1+ and moving to mozilla0.9

Comment 14

[scottputterman]

Did this get fixed when the perf branch landed on 3/16?

Comment 15

[[e-mail] (gone - use [e-mail] instead)]

Seth and I discussed this, most likely it's still there (I'll try it out soon)
due to the way we check the news rc file.

Comment 16

[Aleksey Nogin]

It seems I have found a new way of creating "false positives". Very often if I
click the "green button" column in the subjects pane, nothing will happen (in
particular, Mozilla would nt start showing the message as unread), but it some
indirect evidence (see, for example, bug #70121) shows that this message becomes
"false positive" (e.g it is displayed as read, but many parts of Mozilla think
it's not). Clicking in the "green button" column or reading the message clear
the "false positive" status (again, as observed by some indirect means).

Comment 17

[[e-mail]]

Another oddity that may be related:

I accidentally marked a message as unread in an otherwise read newsgroup.

Mark > All Read failed to mark the message as read!

N.B. The build I was using is a week old.

Comment 18

[Aleksey Nogin]

Just tried the latest nightly of the 0.9 branch, it seems that the bug is no
nonger there, although I am not sure.

Comment 19

[scottputterman]

moving to 0.9.2

Comment 20

[Aleksey Nogin]

I am still getting false positives with recent builds. On scenario - I read
several messages and then Mozilla crashes after it have updated the newsrc file
and before it has updated the .msf file. After I restart Mozilla, it will use
the new newsrc for figuring if a message is unread, but the old .msf file for
figuring how many unread messages a thread has!

I believe that the solution would be for Mozilla to never assume that newsrc and
.msf agree and the first time it reads an .msf, Mozilla should update it to
match the newsrc information.

Comment 21

[Aleksey Nogin]

I believe that bug 59681 is a duplicate of this one.

Comment 22

[Daniel Bratell]

I belive that would be a good fix. Several similar bugs are caused by the
database (msf?) and the newsrc disagreeing. There dosen't seem to be any syncing
between the two at all.

Comment 23

[David :Bienvenu]

actually, there is code to try to sync up the newsrc and .msf files, but it
doesn't seem to be working. It's the code that's called when we build up the
view, actually. I can try to look into it.

Comment 24

[scottputterman]

moving to 0.9.3

Comment 25

[Aleksey Nogin]

This seems to be working in recent builds, but it works the wrong way - Mozilla
seems to be choosing to use .msf info over the newsrc info. However, Mozilla
saves newsrc much more often that .msf, so the newsrc info is the one that
should be used when the two disagree. So there are two potential choices:
(A) Change the sync code to update .msf base on newsrc, not the other way around.
(B) Keep the current sync code, but save .msf to disk more often.
What do you think?

Comment 26

[David :Bienvenu]

we should always prefer the .newsrc over the .msf because .newsrc in theory
could be written out by other apps (or at least other netscape/mozilla
variants). However,  I'm pretty sure .msf's are written out more than .newsrc's,
or at least they should be because .msf's have an incremental save, and
.newsrc's don't.

Comment 27

[Aleksey Nogin]

> actually, there is code to try to sync up the newsrc and .msf files, but it
> doesn't seem to be working
Where is this code located?

Crossposted with bug #59681:

I was looking at what is happening in nsMsgNewsFolder and I observed the following:
1) nsMsgNewsFolder::UpdateSummaryFromNNTPInfo would set mNumUnreadMessages and
mNumTotalMessages to correct values.
2) nsMsgNewsFolder::UpdateSummaryTotals would call ReadDBFolderInfo that would
reset both of them to stale values (note that _both_ unread and total counters
would be wrong).

On biff only (1) would happen, so the values would be correct (untill I try to
read the group, which will cause (1) and (2)).

Comment 28

[Aleksey Nogin]

I am starting to get an idea where things get unsynchronized. IMHO the problem
lies in nsNewsDatabase implementation. Namely, it keeps a field m_readSet (of type 
nsMsgKeySet *) and uses it to keep the information on whether a message is read
or not. However the global and thread read/unread counts are handled by an
underlying 
nsMsgDatabase which has its own idea of read/unread flags.

Although one can try to synchronize neswrc (which is used to produce m_readSet
with .msf (that is used by nsMsgDatabase) outside of nsNewsDatabase, IMHO the
best way to ensure that things never get unsynched is to keep them synched in
the nsNewsDatabase itself.

Because of that I beleieve that the best place for sync code would be in
nsNewsDatabase::SetReadSetStr - whenever some "outsider" changes the ReadSet we
should immediatelly propagate all changes to the underlying nsMsgDatabase.

P.S. Currently, nsNewsDatabase::SetReadSetStr does not have any synch code in it.

Comment 29

[Aleksey Nogin]

I wrote some synchronization code (see attachment 38880 [details] [diff] [review] to bug 59681). I still
need to add some code to synchronize thread counters (right now it synchronizes
everything else, but not them), but I am pretty confident that I will have that
code in a few days.

Comment 30

[Aleksey Nogin]

Attachment: Synch code that fixes this bug as well as bug 59681

Comment 31

[David :Bienvenu]

*** Bug 89247 has been marked as a duplicate of this bug. ***

Comment 32

[[e-mail]]

Will this fix bug 62919 as well?

Comment 33

[Aleksey Nogin]

*** Bug 62919 has been marked as a duplicate of this bug. ***

Comment 34

[David :Bienvenu]

I don't think all this sync code is needed - I believe there's a much simpler
fix that won't iterate over the whole DB the way this patch does. The way this
used to work is that when we built up the view, we dealt with inconsistencies
between the .msf file and the news.rc file - I would much prefer to figure out
why that code isn't working than add all this new code. I'll try to get to that
when I get over my illness.

Comment 35

[Theo Jakobus]

M0.9.2 on Alpha PWS500au OpenVMS V7.2-1

The news group comp.os.vms has threads which are deeply nested. After selecting
the news group with about 200 unread messages it takes about 30s til the
messages are shown. Mark all "read" by CTRL+SHIFT+c takes about 1min to finish
if I've 150 unread messages.
For my 64bit processor running at 500MHz a bit to slow ;)

Comment 36

[Aleksey Nogin]

bienvenu, where is that code located? I wrote the new code because I could not
find any existing sync code. My code does exactly what you've said - it
reconsiles the differences between newsrc and the db.

Comment 37

[David :Bienvenu]

not sure how that last comment about marking all read is relevant to this bug -
you probably meant to add it to the bug about mark all read being slow.

I looked into this some bug some more - there are some serious problems here -
we're not writing out the newsrc at shutdown, only on the 5 minute timer. 

The other problem is that we're not trusting the newsrc over the db, and we're
not correctly syncing the .msf file and the .newsrc files in the view creation
code, which I'll fix. Look for this comment:         // make sure DB agrees with
newsrc, if we're news.

Comment 38

[Aleksey Nogin]

> we're not writing out the newsrc at shutdown, only on the 5 minute timer.
>
I would consider that a separate problem (although it does make this bug appear
more often).

> we're not trusting the newsrc over the db
>
Actually, the problem happens exactly because we (correctly) do trust newsrc
over db in determining whether *a particular message* is read or not - take a
look at nsNewsDatabase - the main purpose of this class is exactly that - using
newsrc for read/unread flag *on particular messages*. Unfortunatelly, for all
other things (unread counts for groups and threads), we do trust the db and that
is where the things get unsynched. Please take another look at my comment from
2001-06-17 16:57

> not correctly syncing the .msf file and the .newsrc files in the view creation
> code
>
That code is absolutely irrelevant, it tries to reconsile that idea of
read/unread flag on *a particular message* which is unnecessary since we are
already using the newsrc for those flags. Take a look at GetNumUnreadChildren
*above* that code, that call will read the number from the db and that number
minght be wrong unless we synced the thread counts beforehand.

In short, after looking at the code you've pointed out, I believe that that code
is irrelevant (it also might be useless, but I am not sure) and my sync code is
still needed. 

Also, to reiterate, the read/unread flags on particular messages are already
correctly based on newsrc, but all the counts are based on DB.

Comment 39

[David :Bienvenu]

well, you're wrong, I'm afraid. When displaying in the view whether messages are
read or unread, we use the flag in the db, not the newsrc file. A simple test
might convince you of this. Look at the timestamp for the .newsrc file, then,
open a newsgroup, mark a message read, open a different group and go back to the
first group (this forces a commit of the .msf file - another problem is that
we're not commiting the .msf file for newsgroups very often). Then, kill the
app, restart and look at the group again. The message will be displayed as
marked read, but the newsrc file's timestamp will not have changed. I think this
should be sufficient evidence to convince you that we're using the .msf file for
displaying whether a message is read or unread. If not, you'll just have to step
through the code like I did.

The view code is not irrelevant - the .msf file and the .newsrc file should be
in sync, and marking a message read here will do that, when I fix the code to do
the right thing.

Penultimately, I'm going to add code that detects negative unread counts for a
thread and sets the unread count to 0.

Finally, I need to figure out why the counts are getting unread.

P.S. please see my comment from 2001-06-14 12:44  about why we should use the
newsrc file. If we followed your logic, we'd just get rid of the .newsrc file
completely. That's a possibility, of course.

Comment 40

[Aleksey Nogin]

Do the opposite test - quit app, change newsrc by hands and restart. You'll see
that the read/unread marks will agree with newsrc.

The code is irrelevant because it changes the read/unread flag, but the flags
are OK, it's the counts that are wrong. My code recounts the messages and
updates the counts thus fixing *both* this bug and bug 59681.

If you still think that your sync code is sufficient please unswer the follwing
questions:
1) How can that code fix the DB unread in group/in thread *counters* after the
newsrc is updated by external app?
2) How can the code compensate in case the GetNumUnreadChildren returnd non-zero
number when the newsrc-derived number would have been 0 and the thread should
have been skipped althogether?
3) How can the thread-view based sync code fix the more global "unread in the
group" counter and does not this suggest that it is the wrong place for the sync
code?

Comment 41

[Aleksey Nogin]

Ah, seems we were both wrong about precedence - my tests show that *both* are
consulted and if *at least one* of db, newsrc thinks messages is read, the
message will be shown as read. Weird.

However, my patch takes care on this problem too since it makes sure
*everything* in the db (both counts and read/unread flags) agree with newsrc, so
this is just another reason to go with my patch :-)

Comment 42

[Aleksey Nogin]

To continue the list of things that are unclear to me:
4) The current code only touches *thread roots*, how is it supposed to help sync
the flags on non-root messages?
5) When unreadOnly is set, the current code will only look at threads that have
unread messages (according to DB!), what about threads that have unread messages
only according to newsrc?
6) Shouldn't the sync code be located in news-specific class to avoid
unnecessary overhead for non-news db's?

Comment 43

[David :Bienvenu]

those are good questions. There's also code that gets called when we list the
messages in a thread for building up the view. That code would also be changed
(or work indirectly if I can fix this by changing an underlying method).

For unread only views, that's a potential problem, except that in the real
world, what's happening is that we're losing the fact that messages have been
read, not that they've been marked unread. So we may get false positives, but
once they're fixed, the problem will go away.

The sync code I'm proposing is pretty much a no-op for non news db's.

My only real problem with your patch is that we will get a performance hit
trying to fix db's every time you open a news folder, whether or not there's any
fixing required. And that hit will be non-trivial for large newsgroups. I don't
want to make it slower, I want to make it faster. The primary advantage of doing
it where I'm proposing doing it is that we've already got the thread object and
the msg hdr object in memory, ready to be fixed, because we created it to create
the view. Your proposal will cause us to instantiate all the msg hdrs and all
the thread objects twice, once for the view (which won't create all the msg hdrs
if you're in threaded view anyway), and once for doing the sync. Now, if we
could figure out a way to only call your sync code when it was needed, then I'd
be all for it. That is quite possible to do going forward with the following
technique - in the .msf file, write out the db's copy of the current news.rc
line for the newsgroup when we commit the db. Then, when we start up, compare
the db's news.rc line against the news.rc line in the actual news.rc file, and
if they differ, run your sync code.

However, I do need to fix the view code to pay attention to the news.rc file and
not the db, and fix it so that we are writing out the newsrc file when we close
the app, and a few other things. These are probably orthogonal to your patch.
Also, I'm still confused how I'm getting negative thread counts, and I'm going
to look into that as well.

Comment 44

[Aleksey Nogin]

> Also, I'm still confused how I'm getting negative thread counts
>
This is something I believe I can answer. If a database thinks a message is
read, but nsNewsDatabase thinks it is unread (and this can happen if db/newsrc
get unsynched, although I am not 100% sure how to reproduce this), then
nsMsgDatabase::MarkHdrRead will think that the message was unread (since
IsHeaderRead would be provided by nsNewsDatabase and not nsMsgDatabase) and
would call MarkChildRead that would decrement thread unread count. However since
DB has already considered this message unread, the thread unread count did not
include this message, so this make the thread unread count lower then the
correct count.

If you repeat the above operation, you can easily get a negative count.

Comment 45

[David :Bienvenu]

d'uh, of course. I was confused because the assertions I put in to detect this
case weren't getting hit when I reproduced the problem. But it turned out some
goober had disabled assertions in the tree, so I wasn't seeing them. So fixing
this problem (threads with unread returning false positives) is trivial, I'll
attach a patch in a minute.

Comment 46

[David :Bienvenu]

Attachment: proposed fix to repair negative unread thread counts

Comment 47

[David :Bienvenu]

Seth and Navin, if you could r/sr this, I'd appreciate it. This just
prevents/repairs negative unread thead counts, which is the right thing to do in
general. There's still a lot more work to do for the general problem of keeping
the newsrc and .msf file in sync, but this will stop this annoying part of the
problem.

Comment 48

[Navin Gupta]

r=naving

Comment 49

[Aleksey Nogin]

Why is this patch necessary? If we add a code that would sync newrc and DB, the
DB unread msg counts should always be correct, so they'll never be negative. 

In short, IMHO this only fixes the symptoms without fixing the problem (the
counter being different from the true count). Also, IMHO the negative counts is
not the most annoying thing; IMHO the most annoying thing is the positive counts
in threads that do not really have unread messages.

Comment 50

[David :Bienvenu]

it's neccesary to fix threads with negative counts, which also show up as
threads with unread because the negative counts are treated as very large
positive numbers. It's only part of the fix, as I mentioned when I described the
patch.

Comment 51

[Aleksey Nogin]

Still, we need to make sure that the counter always has the correct count. If
that wuill be fixed, the counter will always be non-negative, so this "quick
fix" would be unnecessary.

Why fix one particular symptom of a problem when we can [try to] fix the problem
itself and get rid of all symptoms althogether.

Comment 52

[David :Bienvenu]

my fix is trivial, and a reasonable thing to do in any case (the count should
never be negative, so don't allow it to be negative). Your fix needs more work,
which I'm willing to do, but I want to get this in first.

Comment 53

[Scott MacGregor]

nice fix david. sr=mscott

Comment 54

[(not reading, please use [e-mail] instead)]

r=sspitzer on bienvenu's fix.

Comment 55

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 91187 has been marked as a duplicate of this bug. ***

Comment 56

[Aleksey Nogin]

> My only real problem with your patch is that we will get a performance hit
> trying to fix db's every time you open a news folder, whether or not there's any
> fixing required
...
> That is quite possible to do going forward with the following
> technique - in the .msf file, write out the db's copy of the current news.rc
> line for the newsgroup when we commit the db. Then, when we start up, compare
> the db's news.rc line against the news.rc line in the actual news.rc file, and
> if they differ, run your sync code.

How about this partial solution:

When things go wrong, we can be almost sure that the global #unread count for
the whole group would be different in newsrc and in db, what if we use that
(unread counts do not agree) as a trigger for my "total sync" code?

Comment 57

[David :Bienvenu]

I still think it's much better to check the actual newsrc lines - it's not any
harder to compare strings vs. ints, and it will always be accurate. Also, I'm
not 100% sure that the unread count in the db isn't just for the headers we've
downloaded, vs all the headers in the group, so that comparison might not work
at all.

Comment 58

[Scott MacGregor]

David, can you check this patch in, otherwise move this bug to .9.5 per the
triage meeting today? Thanks.

Comment 59

[Aleksey Nogin]

Would you consider letting my patch (attachment 38926 [details] [diff] [review]) in as is and than add the
code that would trigger it only when necessary later? IMHO, this bug is truly
annoying and the slowdown we'll get with this code (my experience shows that
it's barely noticable, even on slow machine and NG with thousands of messages)
is better that current situation. Thanks!

P.S. I am running Mozilla with this patch for quite a while, so far I didn't
notice any adverce effects other then a very slight delay (about a second or
less) when opening large newsgroups on a relatively slow machine (PPro-200).

Comment 60

[Theo Jakobus]

Besides the "Unread" error there is a performance problem with the DB.
Showing my inbox after selecting takes around 45sec. I've about 10 old messages
and about 50 new mails without attachments.
CTRL+SHIFT+C on a newsgroup with about 200 unread messages takes about 30 sec
for marking all read.

Comment 61

[Håkan Waara]

Theo, try the build that comes later today.  I think that will greatly reduce
the time to Mark All Read (according to bug 81859).

Comment 62

[David :Bienvenu]

yes, try today's build - and it's not a performance problem with the db, it's a
performance problem with updates to the old tree control, which has been removed
from the fodler pane.

Comment 63

[Aleksey Nogin]

I have an alternative idea - when we open folder and build view, we call
IsHeaderRead on each of the headers. What if we add a check to IsHeaderRead that
would compare newsrc idea of the read flag with the DB's one and then call my
sync code if they disagree?

Pros:
- We only call sync code when necessary, so no overhead there.
- The overhead we add is only a call to nsMsgDatabase::IsHeaderRead (with the
header that we already have).

Cons:
- We keep calling nsMsgDatabase::IsHeaderRead even when there is no need to.
- If for some reason IsHeaderRead was not called on some of the headers, we may
not realize that sync is necessary.

What do you think?

Comment 64

[David :Bienvenu]

good idea, but it won't help much - I already checked in code to fix up
db<->newsrc values for particular messages, if they're in the view, but as you
say, it won't help if messages aren't in view, which they won't be if the thread
is collapsed. When I get a chance, I'll try to code up the comparing of the
newsrc file line with a newsrc line in the folder db.

Comment 65

[Scott MacGregor]

pushing out to .9.5

Comment 66

[Theo Jakobus]

Besides the unread bug whose patch is moved to 0.9.5 I reported that the file
INBOX. is growing. The opening of the inbox takes now about half a minute, I get
about 40 new e-mails per day most of them are deleted. The INBOX. file has now
130000 blocks. The deleted e-mails are still in the file INBOX. Is this bug
solved in version 0.9.4 ?

Comment 67

[Nathan Bidwell]

<QA ignore>

Theo,
Try right clicking your inbox and selecting "Compact This Folder".  That should
reduce the size of your INBOX file.

</QA ignore>

Comment 68

[Theo Jakobus]

Nathan,
First I read your e-mail next I moved the e-mail in folder mozilla. After
compressing the inbox the size decreased from 132000 to 1600 :)
Next I wanted to read the moved e-mail in folder mozilla. Selecting the subject
line poped up a window with the message: Unknown Error 80470002.

Comment 69

[David :Bienvenu]

Theo, what build are you using? You should update to a newer build, delete the
.msf file for your inbox and let it get regenerated.

Comment 70

[Theo Jakobus]

I deleted the file INBOX.MSF and restarted MOZILLA, the moved e-mail is gone for
ever.

I'm using version 0.9.3, Build ID: 2001080116. My system is an Alpha under
OpenVMS so no nightly builds are available. 

Comment 71

[David :Bienvenu]

moving to .9.6

Comment 72

[Mike Pinkerton (not reading bugmail)]

FWIW, i still see this with a brand new profile, 094 branch build 9/28/01 on
MacOSX. 

Comment 73

[Aleksey Nogin]

Of course you still see it - my patch (attachment 38926 [details] [diff] [review]) never made it in... :-(

Comment 74

[scottputterman]

moving to 0.9.9

Comment 75

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 121542 has been marked as a duplicate of this bug. ***

Comment 76

[[e-mail]]

What's really annoying is when you crash Mozilla while reading a newsgroup, the
.newsrc is up-to-date but the .msf isn't, so when you look in the folder pane it
says you have the right unread count but when you select the group you see the
wrong count :-( So could you at least rebuild the msf when the counts don't match?

Comment 77

[Aleksey Nogin]

Neil, my code (attachment 38926 [details] [diff] [review]) does exactly that, but we still need some fast
way to find out that things do not match - just going and counting everything is
too slow.

Comment 78

[Bernd Laengerich]

Selecting a news group with settings "View/Messages/Threads with unread" results
in either a threaded or an unthreaded view (equal to "Unread").
Is this related to the false positives unread?

Comment 79

[Ian Pottinger]

adding self to cc list

Comment 80

[Vadim Berezniker]

*** Bug 115003 has been marked as a duplicate of this bug. ***

Comment 81

[Mogens Kjær]

How does one apply attachment 38926 [details] [diff] [review] to 1.0-RC1 ?

The patch to mailnews/db/msgdb/src/nsNewsDatabase.cpp fails.

Comment 82

[David :Bienvenu]

Attachment: proposed fix

I took the existing sync code and cleaned it up, and then I just call it when
the db and the newsrc file seem out of sync. It seems to work fine but I'll run
with it for a while.

Comment 83

[Cavin Song]

Comment on attachment 84781 [details] [diff] [review]
proposed fix

r=cavin.

Comment 84

[Aleksey Nogin]

Two comments:

1) The "Update FolderInfo Counters" should be probably put inside the
if NS_SUCCEEDED(rv) { ... }
so that message counters are *not* updated when the while loop was not
succesfully completed (which would mean the the new counters are incomplete).

2) What about counting messages in every thread (as my patch did)? Otherwise
this patch will fix bug 59681, but not this one :-(

Comment 85

[David :Bienvenu]

Aleksey, I'll fix comment 1, thx.

Re comment 2, this original bug is actually already fixed (see bug 64480) - Mark
all read after get next NNN news messages was not resetting the lowwater mark,
and thus getting the newsrc read state out of sync with the db hdr state. 

I don't know of any reason to update the thread counts. There's no known bug
that makes the total msg counts wrong, and the unread counts will get updated
correctly with my patch because I call MarkHdrRead, which goes through the
thread object to mark the hdr read/unread, and updates the thread unread count
as appropriate. Why specifically do you feel more needs to be done? Can you give
me a scenario that causes the total or unread thread counts are wrong *inside
the db*, independent of the .newsrc file, after this code is run? If so, we
should definitely fix that code.

Comment 86

[Aleksey Nogin]

Well, one obvious scenario is that any db created with Mozilla before this patch
may have wrong thread counts.

Another scenario that need to be tested (it might be a recent regression and a
separate bug, I am not sure what goes on here):
0) quot mozilla
1) delete the .msf db, but keep the newsrc
2) Start Mozilla

Currently (2002051816 trunk, w/o your or mine patch) what happens is that the db
unread count would be 0 even when the newsrc one is not. The thread counts are
correct, though. I am wondering whether the sync code migth "overcorrect"
something here and break the thread counts. May be not...

<Offtopic>
BTW, another weird thing that happens in the above scenario is that the
threading (as least the by-subject one, not sure about the "References:" one)
breaks in a funny way - the new messages (e.g the ones arriving after .msf was
re-created) will never be put in the same thread as the old ones. In other words
each active thread will be split in two - the "before" one and the "after" one.
This is definitely a somewhat recent regression - do you know if it is reported yet?
</Offtopic>

Comment 87

[(not reading, please use [e-mail] instead)]

Comment on attachment 84781 [details] [diff] [review]
proposed fix

sr=sspitzer

Comment 88

[David :Bienvenu]

that scenario is handled by my patch, for the reason I described before. Let me
try summarizing again, from the top.

There are two ways we keep track of read state - in the .msf file, via the
header flags, which are kept in sync with the thread unread counts, and in the
.newsrc file, which just knows which messages are read. When a hdr is marked
read in the db, the unread count for the thread is decremented, and when a new,
unread header arrives, the unread count is incremented. The db is committed as a
whole, so these counts should always be consistent with the msg hdr flags. 

The msg hdr flags are used for mail messages exclusively to keep track of
readness. However, for compatibility reasons, for news, we use the .newsrc state
to display readness for a message. Thus, it's easy to see that if the .newsrc
state doesn't correspond to the msg hdr flags, the thread unread count will also
seem wrong based on what the user sees. However, internally, the hdr flags and
thread counts are consistent; it's the newsrc state that's out of whack with the
other two pieces of state.

So, imagine a case where there's a thread with a single message that's unread
read in the db, but the .newsrc file thinks it's read. The user will see an
unread count on the thread of 1, but the message will appear as read, because
its read in the .newsrc file. What the sync code will do is mark the message as
read in the db, to sync with the .newsrc file. When it marks it read in the db,
it will decrement the unread count for the thread.  Thus, it will end up with an
unread thread count of 0, and the message will still appear read. Thus, the sync
code actually fixes the thread unread count automatically. It also fixes the
total unread count for the newsgroup as well, so technically that code isn't
needed either, but it's harmless performance-wise. Look at
nsMsgDatabase::MarkHdrRead, and see how it gets the thread object, and calls
nsMsgThread::MarkChildRead, which, you will see, updates the thread unread
count. Oy, sorry to be so long-winded, but hope that helps.

Re the subject threading question, that's very odd. I don't know if it's
reported; I'll try it out.

Comment 89

[David :Bienvenu]

I am seeing a problem with deleting the .msf file and loading the newsgroup - it
seems that if the news hdr gets added is already marked read in the .newsrc
file, we have some sort of problem with the thread unread counts. It might be
related to the header retention setting I have, which is to only keep headers
from the last 30 days. I'm looking into it. I did not see the subject threading
problem.

Comment 90

[Ian Pottinger]

WRT <Offtopic> in comment 86,  I see this same behaviour (bug 140613) despite
never having deleted a .msf file.

Comment 91

[Ian Pottinger]

I beleave that this bug an bug 140613 are related.

After reading through this bug, I was able to find a set of steps that will
reproduce bug 140613. (<Offtopic> in comment 86)  It seems that if mozilla is
exited and restarted *after marking all the messages in a thread as read* then
any new messages will be placed in a new thread instead of being appended to the
original.

Comment 92

[Aleksey Nogin]

BTW, now that we have the SyncWithReadSet function, would it be a good idea to
have it triggered whenever some inconsistency is detected - for example, when
some unread count (thread or total) drops below zero? IMHO it won't make any
perf imact (since these thing are not supposed to happen anyway) and will help
to take care of the "things got out of sync before this patch went in" scenarios.

Comment 93

[David :Bienvenu]

Aleksey, that's a possibility, though to fix the thread counts, we'd need to
resurrect the thread counting code, which I think I'm going to do anyway. I'd
like to find the cause of some more of these problems first.

Comment 94

[David :Bienvenu]

*** Bug 147950 has been marked as a duplicate of this bug. ***

Comment 95

[David :Bienvenu]

Attachment: make sure news has right read flag before adding to db

this patch makes sure that m_flags agrees with read set before adding to db.

Comment 96

[(not reading, please use [e-mail] instead)]

Comment on attachment 85501 [details] [diff] [review]
make sure news has right read flag before adding to db

sr=sspitzer

Comment 97

[David :Bienvenu]

last two patches have been checked in. Let's see if this clears up the problems.
I'm still not sure if we need to update the thread counts or not, but we can add
that later.

Comment 98

[Aleksey Nogin]

*** Bug 59681 has been marked as a duplicate of this bug. ***

Comment 99

[Daniel Veditz [:dveditz]]

Nominating. This is a long-complained about problem and if this simple, safe
patch fixes the problem on the trunk it should be risk-free to apply to the
1.0.1 branch.

Comment 100

[David :Bienvenu]

*** Bug 108906 has been marked as a duplicate of this bug. ***

Comment 101

[J.O. Aho]

This is a kind of reply to the following reply
-- Additional Comment #85 From [e-mail] 2002-05-23 10:43 --

When is your patched supposed to appear? 1.0.1 or?
For as it's today, I have quite many threads that says that there are 1 or more
unread posts when there aren't any unread at all. The only way I know at the
moment to get rid of this is to wait untill all posts in that thread has expired.

If it happens to be that your patch is already applied, in that case it don't work.

Comment 102

[David :Bienvenu]

it's in trunk builds, not 1.01 

Comment 103

[scottputterman]

Before this gets approved for the branch, Laurel or someone who sees this, could
you verify this on the trunk?

Comment 104

[David :Bienvenu]

this should definitely be baked and verified on the trunk. It's not a trivial
fix, and I didn't get it right the first time, which is a definite warning sign.

Comment 105

[David :Bienvenu]

whoops, that comment was meant for another bug - this one is a relatively safe
fix. It might not fix all problems, but it will help for the scenario where the
.msf file is committed but the .newsrc file isn't written out, or vice versa.

Comment 106

[laurel]

Using may31 trunk, win98:
I checked a branch build and found one newsgroup which was showing read threads
in the Threads with Unread view.  In today's trunk it still shows the same
results in the view.  David, should we have to remove .msf or do anything to
reset scenario before looking for verification of the fix?

Comment 107

[David :Bienvenu]

unless and until I add back the code to resync the thread counts, you would need
to delete your .msf file. This code only gets invoked when we think the .newsrc
and .msf files have gotten out of sync, and I only recently added code to figure
that out. So, in other words, this code basically prevents them getting out of
sync going forward.

Comment 108

[Jonas Jørgensen]

*** Bug 148602 has been marked as a duplicate of this bug. ***

Comment 109

[laurel]

So far I have not been able to turn up any false positive situations in the
trunk. I set up a new profile with about 10 newsgroups -- since I'm not at
liberty to just stay on the trunk builds, I put a specific profile to isolate
where I know I haven't used the branch to read news (and potentially get the
out-of-sync situation without the fix and have to reset for trunk testing).  I
left the "ask me when downloading over N messages" on and set it fairly low
(100). With each group set to Threads with Unread view,  I got the 100 messages
and on some of the newsgroups checked the "mark remaining headers as read"
option. I read through the newsgroups, marking some messages read, some Threads
as read and a couple groups marked All Read or marked group read. On every group
I did a Get Next 100 Messages.  Exited and returned.  Repeated the general
scenario described above. Did this many times and have not seen a false unread
thread.

I don't know how long we need to bake this before marking verified on the trunk
to get to the branch checking phase, but so far so good from what I can see. 
Maybe some other folks out there can chime in with comments.

Comment 110

[[e-mail]]

laurel, I only ever have this problem after a crash. Have you tried some of those?

Comment 111

[Cosmn Negulescu]

I can second that. From what I've seen so far those false positives almost
always ocur after a crash. Happened to me last night too.

Comment 112

[laurel]

Well, I just spent some time on the june4 trunk build where I repeated the
general scenario(s) in comment #109, but ended the task at various points (since
I'm not crashing, thought ending the task would be similar) then came back to
get more messages, often getting N next older messages.  I never saw any false
positives throughout this exercise, although I did see some other display
weirdnesses (new message inserted into unrelated thread, blank entries/icons
only after reading a thread then getting N next messages). Said weirdnesses went
away after selecting another group and returning to the problem view.  Could not
reproduce.

Comments, David?

Regard comment #111, Cosmn Negulescu: So, when "it" happened to you last night,
was that on a current trunk build with the fix? 

Comment 113

[[e-mail] (gone - use [e-mail] instead)]

Bad counts relating to / caused by crashers, are covered by bug 101503

Comment 114

[Aleksey Nogin]

Stephen, bug 101503 is likely just a dup of this one (and it was marked fixed
after the patch from this one went in).

Laurel, I am not sure what exactly you mean by "ending the task", but I would
imagine that it would be insufficient. The way to reproduce this bug used to be
to crash after the newsrc was updated the disk while the msf is still outdated
(or the other way around) and then (after the crash) restart Mozilla and observe
what happens if Mozilla is started when the newsrc and  msf do not match.

Another way to reproduce is to quit Mozilla and then to mess with newsrc
manually, then restart Mozilla again. But I am not sure whether this will test
all the possibilities - it might very well be that the "get next N" creates a
whole separate set of nasty scenarious.

Comment 115

[Syd Logan]

adt1.0.1- let's get it in the next release

Comment 116

[Beverly Howard]

Description above is not clear

Just installed win32 release 1.1 and observed

New unread threads with a single message in the thread (viewing "threads with
unread") shows a "+" when sorted by thread

When "+" is clicked, "-" appears but no additional messages displayed.

When "view all messages" is subsequently selected, behaviour is normal and
(read) message shows a single message with no "+"

Comment 117

[laurel]

The issue mentioned in comment #116 is reported as bug 158217

Comment 118

[David Tenser [:djst]]

Attachment: A .msf file for n.p.m.general

This bug is still there in the latest nighly build (2002090804, WinXP).

I've attached a msf file for the n.p.m.general newsgroup. I assume most of you
guys are subscribing to it. Close Mozilla and rename your original
"netscape.public.mozilla.general.msf" file and place this one there instead.
Then restart Mozilla.. Select View->Messages->Threads with Unread. Hopefully,
you'll see that there are four threads that are visible, but when expanding
them, there are no unread messages.

Comment 119

[[e-mail]]

I've just seen this again: Since I switched ISPs I subscribed to my favourite
groups and used Mark All Read to catch up. The next day there were threads with
false positives :-(

Comment 120

[esther]

I don't see the problem with trunk builds 20030303 on winxp, mac and linux, but
then I didn't see the problem before so neil and david if you still see could
you try with a current build and/or give the build date you see the problem on.
 Then reopen this bug. Thanks. 

Comment 121

[esther]

Using trunk build 20030415 on winxp, mac osx and linux I tested 
Mark Newsgroup Read -exiting and relaunching
Manually marked messages as read -exiting and relaunching 
Get Next 500 msgs - exiting and relaunching
and could not reproduce a false postive in threads.  No comment from previous
reporters of the problem with current builds so I will verify this now.  
If reopened, please give build date and specific steps.  Thanks.

Comment 122

[Wojtus]

It seems there's a new bug with exactly same symptoms: #530059