Closed Bug 64476 Opened 24 years ago Closed 22 years ago

threads with unread returns false positives

Categories

(MailNews Core :: Database, defect, P2)

Product:
MailNews Core
Component:
Database
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.2alpha

People

(Reporter: sspitzer, Assigned: Bienvenu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [adt2 rtm] have working sync code; need code to sync only when needed)

Attachments

(5 files, 2 obsolete files)

Unthreaded Unread (14,786 bytes)
14.44 KB, image/gif
Details
Threaded Unread (85,120 bytes)
83.12 KB, image/gif
Details
proposed fix
3.78 KB, patch
cavin
: review+
sspitzer
: superreview+
Details | Diff | Splinter Review
make sure news has right read flag before adding to db
971 bytes, patch
sspitzer
: superreview+
Details | Diff | Splinter Review
A .msf file for n.p.m.general
162.67 KB, application/x-zip-compressed
Details 
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.
> 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?
Status: NEW → ASSIGNED
QA Contact: esther → stephend
no, i never did that exact command. I just read the new messages as they come in
on a daily or weekly basis.
I get false positives from threads with unread just with normal newsreading.
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.
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.
setting to 0.9, p1. 

while not a crasher, this false positive problem needs to be fixed.
Priority: -- → P1
Target Milestone: --- → mozilla0.9
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.
removing mozilla0.9 and nominating for nsbeta1. Seth, will this get fixed with
the rewrite?
Keywords: nsbeta1
Target Milestone: mozilla0.9 → ---
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
yes, the rewrite will almost certainly fix this.
marking nsbeta1+ and moving to mozilla0.9
Priority: P1 → P2
Whiteboard: [nsbeta1+]
Target Milestone: --- → mozilla0.9
Did this get fixed when the perf branch landed on 3/16?
Target Milestone: mozilla0.9 → mozilla0.9.1
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.
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).
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.
Just tried the latest nightly of the 0.9 branch, it seems that the bug is no
nonger there, although I am not sure.
moving to 0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
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.
Summary: threads with unread returns false positives if you ever did "Get Next n Messages" → threads with unread returns false positives
I believe that bug 59681 is a duplicate of this one.
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.
QA Contact: stephend → fenella
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.
Assignee: sspitzer → bienvenu
Status: ASSIGNED → NEW
moving to 0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
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?
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.
> 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)).

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.
Component: Mail Window Front End → Mail Database
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.
Keywords: mozilla0.9.2, patch, review
Whiteboard: [nsbeta1+] → [nsbeta1+] [potential fix in hand]
Whiteboard: [nsbeta1+] [potential fix in hand] → [nsbeta1+] potential fix in hand, needs r and sr
*** Bug 89247 has been marked as a duplicate of this bug. ***
Whiteboard: [nsbeta1+] potential fix in hand, needs r and sr → [nsbeta1+] potential fix in hand, needs reviews
Will this fix bug 62919 as well?
*** Bug 62919 has been marked as a duplicate of this bug. ***
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.
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 ;)
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.
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.
Status: NEW → ASSIGNED
> 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.
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.
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?
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 :-)
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?
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.
> 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.

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.
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.
r=naving
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.
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.
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.
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.
nice fix david. sr=mscott
r=sspitzer on bienvenu's fix.
*** Bug 91187 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla0.9.3 → mozilla0.9.4
> 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?




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.
Keywords: mozilla0.9.2 → mozilla0.9.4
Whiteboard: [nsbeta1+] potential fix in hand, needs reviews → [nsbeta1+] have working sync code; need code to sync only when needed
David, can you check this patch in, otherwise move this bug to .9.5 per the
triage meeting today? Thanks.
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).
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.

Theo, try the build that comes later today.  I think that will greatly reduce
the time to Mark All Read (according to bug 81859).
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.
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?
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.
pushing out to .9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
QA Contact: fenella → laurel
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 ?
<QA ignore>

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

</QA ignore>
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.
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.
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. 
moving to .9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
FWIW, i still see this with a brand new profile, 094 branch build 9/28/01 on
MacOSX. 
Of course you still see it - my patch (attachment 38926 [details] [diff] [review]) never made it in... :-(
Blocks: 104166
moving to 0.9.9
Target Milestone: mozilla0.9.6 → mozilla0.9.9
Keywords: nsbeta1+
Whiteboard: [nsbeta1+] have working sync code; need code to sync only when needed → have working sync code; need code to sync only when needed
*** Bug 121542 has been marked as a duplicate of this bug. ***
Blocks: 122274
Keywords: nsbeta1+ → nsbeta1-
Target Milestone: mozilla0.9.9 → mozilla1.2
No longer blocks: 122274
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?
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.
Blocks: advocacybugs
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?
adding self to cc list
*** Bug 115003 has been marked as a duplicate of this bug. ***
How does one apply attachment 38926 [details] [diff] [review] to 1.0-RC1 ?

The patch to mailnews/db/msgdb/src/nsNewsDatabase.cpp fails.
Attached patch proposed fix — — Splinter Review 
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.
Attachment #38926 - Attachment is obsolete: true
Attachment #42929 - Attachment is obsolete: true
Comment on attachment 84781 [details] [diff] [review]
proposed fix

r=cavin.
Attachment #84781 - Flags: review+
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 :-(
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.
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 on attachment 84781 [details] [diff] [review]
proposed fix

sr=sspitzer
Attachment #84781 - Flags: superreview+
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.


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.
WRT <Offtopic> in comment 86,  I see this same behaviour (bug 140613) despite
never having deleted a .msf file.
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.
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.
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.
*** Bug 147950 has been marked as a duplicate of this bug. ***
this patch makes sure that m_flags agrees with read set before adding to db.
Comment on attachment 85501 [details] [diff] [review]
make sure news has right read flag before adding to db

sr=sspitzer
Attachment #85501 - Flags: superreview+
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.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
*** Bug 59681 has been marked as a duplicate of this bug. ***
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.
Keywords: nsbeta1- → adt1.0.1, nsbeta1
Blocks: 74644
*** Bug 108906 has been marked as a duplicate of this bug. ***
This is a kind of reply to the following reply
-- Additional Comment #85 From bienvenu@netscape.com 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.
it's in trunk builds, not 1.01 
Before this gets approved for the branch, Laurel or someone who sees this, could
you verify this on the trunk?
Keywords: nsbeta1 → nsbeta1+
Whiteboard: have working sync code; need code to sync only when needed → [adt2 rtm] have working sync code; need code to sync only when needed
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.
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.
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?
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.
*** Bug 148602 has been marked as a duplicate of this bug. ***
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.
laurel, I only ever have this problem after a crash. Have you tried some of those?
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.
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? 
Bad counts relating to / caused by crashers, are covered by bug 101503
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.
adt1.0.1- let's get it in the next release
Keywords: adt1.0.1 → adt1.0.1-
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 "+"
The issue mentioned in comment #116 is reported as bug 158217
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.
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 :-(
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. 
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.
Status: RESOLVED → VERIFIED
QA Contact: laurel → esther
Product: MailNews → Core
Product: Core → MailNews Core
It seems there's a new bug with exactly same symptoms: #530059
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: