1751160 - New message indicator (biff) not working for newsgroups in folder pane if newsgroup folder not open

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Rachel Martin]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

There have been reports in de.comm.software.mozilla.mailnews that when a new message is received to a newsgroup, the newsgroup folder isn't displayed in blue and bold with a biff indicator (yellow icon) any more. Also, new messages don't appear to get the biff indicator in the thread pane/message list. Some users claim that it is still working, most claim that it's not working.

We've seen the newsgroup folder blue and bold with a biff indicator only once, so mostly it's not working.

The reports claim that the behaviour changed between 78 and 91.

Comment 1

[Rachel Martin]

Alice, if you have the time, could you see if you can find the regression. This might be quite tricky. There are some instructions on posting to a newsgroup in bug 1743264.

Comment 2

[Alice0775 White]

I tried with 78esr, but the newsgroup name changed to bold blue letters with(n), but the yellow icon is not displayed.
Is 78esr a good build? Please attach a screenshot of the expected result.

I could not see any yellow icon in folder pane in 78esr.

Comment 3

[Rachel Martin]

Attachment: biff-indicator-news.png

Thanks for looking into it. Well, the yellow icon is a secondary matter. If the folder goes blue in 78, that's already a plus, since it doesn't do so reliably in TB 91.

This is how it should look like: blue/bold and with icon. Just happened with TB 91 (!) after subscribing to alt.de.test on news.eternal-september.org where you can get a free account. Overall, news folders should behave like mail folders, then a new message arrives: blue/bold and there should be a yellow biff indicator.

Comment 4

[Rachel Martin]

Attachment: biff-indicator-news-2.png

Hmm, while the news folder was open when the new message arrived, we got this in TB 91. But it's important to get some visual indication even if the folder isn't open.

Comment 5

[Rachel Martin]

Attachment: biff-indicator-news-3.png

Here is a "bad" case. Newsgroup folder wasn't open when message arrived. Message shows biff indicator, but folder does not. It's possible that this behaviour changed further back.

Comment 6

[Alice0775 White]

Attachment: Tb91.5.0, works as expected

Tb91.5.0(20220106182030) Windows10 works as expected for me:
when new news message arrived, a yellow biff icon will appear above the globe icon of the news server name(If tree is collapsed) or newsgroup name(If tree is expanded), and the name will change to blue bold with number of messages. And the new message has a yellow biff icon and bold text in the thread pane.

Comment 7

[Franklin Schiftan]

Does this also happen when you are not in the NG that is open for which the new posting arrived?

Comment 8

[Alice0775 White]

Attachment: image.png

(In reply to Franklin Schiftan from comment #7)

Does this also happen when you are not in the NG that is open for which the new posting arrived?

Yes.
Even if the focus is placed in a folder(e.g. Local Folder) other than the newsgroups, the biff icon is displayed when the new message arrives. See attached.

Comment 9

[Rachel Martin]

Attachment: biff-indicator-news-4.png

Alice, thanks for trying. Here's a case where after starting TB no indication was given, neither on the folder nor on the messages themselves. The people in the newsgroup report that is seems to work on some NGs but not others, or sometimes and not other times. A real nice bug to track down :-(

Comment 10

[WaltS48 [:walts48]]

I'm testing with an AIOE account with 4 newsgroups along with mozilla.test on the Mozilla server with 91.5.0, and I don't see bold/blue when new messages arrive. The folders do become bold/white.

I also tested on Linux with the Fedora build of 91.4.0 with the same result.

Comment 11

[Rachel Martin]

The folders do become bold/white.

White?

Same behaviour in TB 52 on news.eternal-september.org: If the NG folder isn't open, no indication in the folder pane; works if NG folder open.

Comment 12

[WaltS48 [:walts48]]

(In reply to Rachel Martin from comment #11)

The folders do become bold/white.

White?

Same behaviour in TB 52 on news.eternal-september.org: If the NG folder isn't open, no indication in the folder pane; works if NG folder open.

Sorry, I do have a dark OS theme and use the Thunderbird System theme on both Linux and Windows.

I also see the count increase, but don't test other newsgroups other than mozilla.test when testing release candidates.

Same behavior in TB 52! I'm firing myself.

Comment 13

[Magnus Melin [:mkmelin]]

Since this is a very long time bug, now fixed on trunk (bug 1751270), I think at this point we won't be looking into a fix for 91.

Comment 14

[Rachel Martin]

Bug 1751270 fixes the issue for the thread pane biff in the JS module, this bug is about the NG folder biff/bold/blue in the folder pane. This bug is present on trunk, even using the JS module (and has been there forever, only more visible since TB 91 provides better biff indication). Basically the folder pane display is random (working "better" in slower debug builds), as the various attached images here show. This indicates some timing issue or a garbage-collected already closed newsgroup folder database which then can't receive these notifications any more:
C++ module:
https://searchfox.org/comm-central/rev/72709989df2bec210b253fba12bf6935f9cd42b0/mailnews/news/src/nsNNTPNewsgroupList.cpp#1034,1100
JS module:
https://searchfox.org/comm-central/rev/ce30946f9f46d13d98655dddcdd5d55ac48fffac/mailnews/news/src/NntpNewsGroup.jsm#274

Please reopen.

Comment 15

[Rachel Martin]

A fair amount of debugging was necessary to find the problem. Here's the fix for the C++ module, you can do the same in the JS module with one line this._db.AddListener(this._folder). Possibly there is a better solution.
https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1751160-news-folder-notify.patch

The problem is that this call
https://searchfox.org/comm-central/rev/78fa22677130ed6dba7b8848cb7a977cb34f3bce/mailnews/db/msgdb/src/nsMsgDatabase.cpp#2803
https://searchfox.org/comm-central/rev/78fa22677130ed6dba7b8848cb7a977cb34f3bce/mailnews/db/msgdb/src/nsMsgDatabase.cpp#876
calls nsMsgDBFolder::OnHdrAdded() which via CheckWithNewMessagesStatus(true) calls SetHasNewMessages(true) which causes the "highlight" colour of the NG folder in the folder pane. For some reason, likely some random garbage collection, the non-open NG folder is no longer in the listener list of the database, which appears peculiar.

Comment 16

[Rachel Martin]

The fix also closes bug 35892 and bug 195026.

Comment 17

[Ping Chen [:rnons] (away)]

Thanks for investigating, the listener seems to be explicitly removed here https://searchfox.org/comm-central/rev/746643c2bdd9e6123921aa1cf95dadd6cd69916a/mailnews/news/src/nsNewsFolder.cpp#258, introduced in bug 311774.

So instead of AddListener, I suggest just removing the RemoveListener, will send a patch later today.

Comment 18

[Rachel Martin]

Hmm, well, always hard to tell why certain things were coded the way they are. You noticed that there are three of these calls:
https://searchfox.org/comm-central/search?q=RemoveListener&path=nsNewsFolder.cpp&case=false&regexp=false
All these remove the listener and null out the database if the database wasn't already open for the folder.
It might make more sense to drop that GetDatabaseWithoutCache() function altogether and leave the database open. The garbage collection we were referring to is here in MsgDBCacheManager.jsm and should take care of it. It would make sense to check how other parts of the code (POP incorporation or "normal" filter execution) make sure that the listeners are in place so the unread counts are right. Also, we don't want to regress bug 311774.

Comment 19

[Ping Chen [:rnons] (away)]

Attachment: Bug 1751160 - Fix biff for news folder by using folder.msgDatabase directly. r=mkmelin

Because folder.getDatabaseWithoutCache removes db listener, biff state is note updated.

Comment 20

[Rachel Martin]

Not fixing the C++ part?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/30f4d3745f2e
Fix biff for news folder by using folder.msgDatabase directly. r=mkmelin

Comment 22

[Rachel Martin]

Following the JS module fix, this works as well in C++
https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1751160-news-folder-notify-alt.patch

Comment 23

[Ping Chen [:rnons] (away)]

(In reply to Rachel Martin from comment #20)

Not fixing the C++ part?

Since this is quite old, and nntp-js is already in beta, I don't want to touch the C++ code.

Comment 24

[Ping Chen [:rnons] (away)]

Attachment: Bug 1751160 follow-up - Wrap topmostMsgWindow in try..catch to fix nntp unit tests. r=mkmelin

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bf5bd6c5da1e
follow-up - Wrap topmostMsgWindow in try..catch to fix nntp unit tests. r=mkmelin

Comment 26

[Rachel Martin]

Any chance to close bug 35892 and bug 195026?

Comment 29

[Ulf Zibis]

Nice to see, that this bug was fixed.
But since TB 78 there is a similar mess with normal email messages, see: bug 1710669, bug 804214, bug 737880