Closed Bug 111855 Opened 23 years ago Closed 21 years ago

first group not read on news server with login

Categories

(MailNews Core :: Networking: NNTP, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: 3.14, Assigned: frank.schoenheit)

References

Details

Attachments

(2 files, 3 obsolete files)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.6) Gecko/20011120

The first group on a server with login is not read when you expand that server.

pi
Can you explain this a little more?  Are you referring to the updating of unread
counts?
Yes. If you have a server with login-authentification and you expand the group
list of that server the unread count of the first group in the list is not
updated (in most cases, sometimes it is updated, I cannot tell when). I.e., the
group will not be marked as unread even if there are new articles in it.

pi
Probably related: Also for this group it is not saved if I want the articles
threaded (I do) or sorted any other way.

pi
I can't reproduce this.  Are you sure you're not seeing bug 108650?

I tried with news://poisonivy.mcom.com/xrated (which is a poorly-named)
authenticated news server internal to use, running 

200 poisonivy.mcom.com Netscape-Collabra/3.5 33623 NNRP ready (posting ok).
It happens to me all the time and is different from bug 108650.

pi
*** Bug 128868 has been marked as a duplicate of this bug. ***
Confirming, because of dup bug 128868.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 102422 has been marked as a duplicate of this bug. ***
*** Bug 148447 has been marked as a duplicate of this bug. ***
*** Bug 156658 has been marked as a duplicate of this bug. ***
I created an NNTP log file. The first group on my list is
de.comm.infosystems.www.authoring.cgi. As you can see, upon sending the group
command we receive a 480. Once we get to 281 we skip to the next group, i.e.,
de.comm.infosystems.www.authoring.misc in my case.

pi
I can see this also on a server without login (news.t-online.de).
Daniel, can you please provide a log file for this, see this page for an
explanation, how to do it:
http://www.mozilla.org/quality/mailnews/mail-troubleshoot.html

pi
Pi, I made a log but cannot reproduce the behaviour. I don't know why, probably
the mistake was in my eyes and not inside mozilla. sorry.
I've made a log of the my NNTP-connection and it shows exactly the same
behaviour as pi's does:
Whenever you expand the news server (in the right tree view pane), the first
news group triggers the authentification dialog with the news server. But
afterwards, the group is not checked for new posts again.

(I can provide the log if necessary, but it's not different from pi's.)

Remark: my news server is the same as pi's.
Same problem under WinXP, 20030328.
*** Bug 195271 has been marked as a duplicate of this bug. ***
I've marked #195271 as a duplicate of this bug - the only difference is it
occurs under Windows XP. I think the "OS" field for this one needs to change to
"all", but I cannot change it as I'm not the original reporter. 
OS: Linux → All
Hardware: PC → All
Attached patch patch v0.9 (obsolete) — Splinter Review
quick shot for a patch. Works for me, and shouldn't have too much side effects
:), as far as I can see.
Anybody here willing to try this patch and tell if it worked? pi/Karsten?
I've applied it to my tree and it worked correctly AFACT.
Both the UI and my NNTP log support that. ;-)
Attachment #130075 - Flags: superreview?(sspitzer)
Attachment #130075 - Flags: review?(stephend)
Patch works for me. It should be in 1.5.

pi
Flags: blocking1.5?
Not going to block the release for this. If you get reviews in time please
request approval to land the patch.
Flags: blocking1.5? → blocking1.5-
Losing a group is a pain. Asking for blocking. The patch works.

pi
Flags: blocking1.6a?
Comment on attachment 130075 [details] [diff] [review]
patch v0.9

Clearing request for my r=?

I can't review C++, sorry.
Attachment #130075 - Flags: review?(stephend) → review?
Attachment #130075 - Flags: review? → review?(bienvenu)
Re this comment:

+    // note that this will restart reading *all* groups. Normally,
+    // we would only need to read the previous group, again. However,
+    // the groups are iterated using GetFirstGroupNeedingCounts, and
+    // it is only possible to completely restart this enumeration,
+    // but not to go one step back.

If the first group causes the authorization request, then isn't going back one
step usually equivalent to completely restarting the enumeration?

the patch seems OK - however, have you tried checking the "always request
authentiation when connecting to this server" setting on the news server
settings. It's been a few years, but I seem to remember that makes us try to
authenticate when connecting to the server, before getting the msg counts.
> If the first group causes the authorization request, then isn't going back one
> step usually equivalent to completely restarting the enumeration?

yes, in this case it is. But I don't have the technical background to judge the
probability of an authentication request for a group other than the first one.

E.g., consider the famous per-newsgroup vs. per-server authentification (we know
some people claimed it is a real-world feature :). If, for instance, you've
subscribed to a number of groups on a server which all require different
authentification, then the patch, in the worst case, could do O(n^2) unnecessary
GROUP requests. I thought I should at least point to this in a comment ...

In fact, now that I think about it, if the user decides to *not* store the auth.
information with the password manager, then s/he would be bothered to
authenticate for the first group again and again, and never advance, wouldn't
s/he? I suppose we need a v 0.9.1 ....

> however, have you tried checking the "always request authentiation when
> connecting to this server" setting

not yet (didn't know it :), will do ....
Frank, any update on this?
no, sorry, I currently don't have much time for Mozilla :( Don't expect anything
before next week, at the earliest.
If you're asking because of the 1.6a blocking flag: I suggest to remove it (but
won't do myself since I didn't add it - pi?), since I think the current patch
might have some bad consequences as outlined above. For the same reason I'm
going to remove the r/sr requests (sorry pi).
Attachment #130075 - Flags: superreview?(sspitzer)
Attachment #130075 - Flags: review?(bienvenu)
OK, I hope for a new patch soon. Works for me, though.

pi
Flags: blocking1.6a?
> Works for me, though
for me, too, but the problem is there could be rare scenarios where the user
would be left with the choice between a) an infinite loop b) something s/he
doesn't want for whatever reasons (remember passwords) c) not updating the
newsgroups
Attached patch patch v0.9.1 (obsolete) — Splinter Review
This patch does not restart readin all groups, but skips the groups for which
the numbers were already obtained.
Karsten, pi: would you mind trying this? :)
Attachment #130075 - Attachment is obsolete: true
Comment on attachment 134118 [details] [diff] [review]
patch v0.9.1

David, I'd appreciate if you'd find some minutes to have a look at this.
Thanks.
Attachment #134118 - Flags: review?(bienvenu)
Frank, the approach in the patch seems fine. 

a few comments:

nsNNTPProtocol::AdvanceGroupNeedingCount()

I think this should probably be called something like this:

nsNNTPProtocol::GetNextGroupNeedingCounts(nsISupports **nextGroup)

this var name m_newsRCListSkip could be more descriptive: - something like:

m_RCIndexToResumeAfterAuthRequest
Attachment #134118 - Attachment is obsolete: true
Attachment #134118 - Flags: review?(bienvenu)
Comment on attachment 134348 [details] [diff] [review]
patch v0.9.2 - addressing David's comments

David, thanks for the comments. I addressed them both in this next version ...
Attachment #134348 - Flags: review?(bienvenu)
Comment on attachment 134348 [details] [diff] [review]
patch v0.9.2 - addressing David's comments

some nits - 

+    *_nextGroup = nsnull;
+	nsresult rv = m_nntpServer->GetFirstGroupNeedingCounts( _nextGroup );

you don't need to assign null to _nextGroup. Also, it's not the normal style to
have arg names with leading underscores.  pNextGroup is better.

and an other nit:
I suggested this var name
m_RCIndexToResumeAfterAuthRequest

partly so that it's clear to the reader that the var is an Index...
Attached patch patch v0.9.3Splinter Review
> Also, it's not the normal style to have arg names with leading underscores.
> pNextGroup is better.

I admit I tried mixing some foreign style (which I am used to) into the code,
maybe because I didn't see a clear line in Mozilla code of how e.g. parameters
are named ... Anyway, shame on me :).

> I suggested this var name
> m_RCIndexToResumeAfterAuthRequest

I liked the "Skip" in the old name, but I surely can get over this :)
Attachment #134348 - Attachment is obsolete: true
Comment on attachment 134401 [details] [diff] [review]
patch v0.9.3

thx, Frank, looks good.

One nit - you don't need these lines. The only caller passes in valid pointers.
This isn't an idl method that can be called from anyone or anywhere, so you
don't have to check for a null pointer (it's a code bloat vs. safety issue -
but we know this routine is only called from one place)
+    NS_ENSURE_ARG_POINTER( pNextGroup );
+    NS_ENSURE_ARG_POINTER( returnStatus );

also, there look like there might be tabs in here. If so, I can clean them up
when I check in (if you want me to check in - can't remember if you have
checkin privileges.
Attachment #134401 - Flags: review+
Comment on attachment 134401 [details] [diff] [review]
patch v0.9.3

Thanks David!

> One nit - you don't need these lines ...

Well, I'm usually trying to be as paranoid as possible, simply because I've
been bitten too often by things which "can never fail".
But I also see your point  in the code bloat argument.

And yes, I'm going to ask you for checkin once there's a sr, and yes, please
remove the tabs then (sorry, I didn't bother removing them from the code which
I just moved).
Attachment #134401 - Flags: superreview?(sspitzer)
-> frank
Assignee: sspitzer → frank.schoenheit
fix checked in, sr=mscott.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
thx, Frank
Comment on attachment 134401 [details] [diff] [review]
patch v0.9.3

thanks for the checkin ...
Attachment #134401 - Flags: superreview?(sspitzer)
Attachment #134348 - Flags: review?(bienvenu)
Works, thanks.

pi
Status: RESOLVED → VERIFIED
*** Bug 207869 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: