111855 - first group not read on news server with login

Status: VERIFIED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Boris 'pi' Piwinger]

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

Comment 1

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

Can you explain this a little more?  Are you referring to the updating of unread
counts?

Comment 2

[Boris 'pi' Piwinger]

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

Comment 3

[Boris 'pi' Piwinger]

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

pi

Comment 4

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

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).

Comment 5

[Boris 'pi' Piwinger]

It happens to me all the time and is different from bug 108650.

pi

Comment 6

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

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

Comment 7

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

Confirming, because of dup bug 128868.

Comment 8

[Boris 'pi' Piwinger]

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

Comment 9

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

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

Comment 10

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

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

Comment 11

[Boris 'pi' Piwinger]

Attachment: NNTP log of connection with login

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

Comment 12

[Daniel Küstner]

I can see this also on a server without login (news.t-online.de).

Comment 13

[Boris 'pi' Piwinger]

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

Comment 14

[Daniel Küstner]

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.

Comment 15

[Karsten Düsterloh]

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.

Comment 16

[Sven Grull]

Same problem under WinXP, 20030328.

Comment 17

[Tom Cumming]

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

Comment 18

[Tom Cumming]

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. 

Comment 19

[Frank Schönheit]

Attachment: patch v0.9

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?

Comment 20

[Karsten Düsterloh]

I've applied it to my tree and it worked correctly AFACT.
Both the UI and my NNTP log support that. ;-)

Comment 21

[Boris 'pi' Piwinger]

Patch works for me. It should be in 1.5.

pi

Comment 22

[Asa Dotzler [:asa]]

Not going to block the release for this. If you get reviews in time please
request approval to land the patch.

Comment 23

[Boris 'pi' Piwinger]

Losing a group is a pain. Asking for blocking. The patch works.

pi

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

Comment on attachment 130075 [details] [diff] [review]
patch v0.9

Clearing request for my r=?

I can't review C++, sorry.

Comment 25

[David :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.

Comment 26

[Frank Schönheit]

> 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 ....

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Frank, any update on this?

Comment 28

[Frank Schönheit]

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).

Comment 29

[Boris 'pi' Piwinger]

OK, I hope for a new patch soon. Works for me, though.

pi

Comment 30

[Frank Schönheit]

> 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

Comment 31

[Frank Schönheit]

Attachment: patch v0.9.1

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? :)

Comment 32

[Frank Schönheit]

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.

Comment 33

[David :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

Comment 34

[Frank Schönheit]

Attachment: patch v0.9.2 - addressing David's comments

Comment 35

[Frank Schönheit]

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 ...

Comment 36

[David :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...

Comment 37

[Frank Schönheit]

Attachment: patch v0.9.3

> 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 :)

Comment 38

[David :Bienvenu]

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.

Comment 39

[Frank Schönheit]

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).

Comment 40

[David :Bienvenu]

-> frank

Comment 41

[David :Bienvenu]

fix checked in, sr=mscott.

Comment 42

[David :Bienvenu]

thx, Frank

Comment 43

[Frank Schönheit]

Comment on attachment 134401 [details] [diff] [review]
patch v0.9.3

thanks for the checkin ...

Comment 44

[Boris 'pi' Piwinger]

Works, thanks.

pi

Comment 45

[Asa Dotzler [:asa]]

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