419143 - Ignore thread filter action doesn't

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

I have some newsgroup filters that detect a message posted by a certain 
individual and take the action "Kill Thread".  I find that about half of 
the threads in which this guy posts are NOT killed.  I turned on filter 
logging and found that the messages were being detected, and the log 
entry reports that the thread was killed, but it wasn't.  The messages 
are sitting right there, unread, and the thread is not killed.

I finally figured out why.  The Kill Thread action is a TOGGLE! 
The first time that action is executed it kills the thread, but the second
time that action is executed on another message in the same thread, it 
UNKILLS the thread.  

When used as a filter action, Kill Thread MUST NOT be a toggle.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

hmm.  I'm sure that "kill thread" is a toggle, but now I'm not so sure that
explains why the threads are not being killed, even when the filter logs 
says they are.  Maybe the kill thread mail filter action just doesn't work.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I may be mistaken, but I think this regression began when the new ability
to filter on any message header was added.

Comment 3

[Joshua Cranmer [:jcranmer]]

I'm getting WFM on my trunk build using the following filters on mozilla.test (news.mozilla.org):

Subject contains test -> Kill Thread
From contains Chris -> Kill Thread

Could you post the newsgroup and filters as a test case to reproduce? It may also be a factor of the news server, but I would doubt that unless it's involved with fishy XHDR/XOVER problems...

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The server is newsgroups.comcast.net also known as 
  netnews.comcast.net
  comcast.dca.giganews.com  (It's a giganews server)

When contacted (via SSL on port 563) the server identifies itself with
200 News.GigaNews.Com

I have had no luck with the kill thread filter action in any of the groups
on this server.  

The tests were all of the Subject or From contains string variety.  
An example:  in sci.crypt, if the Subject contains JSH or the From contains
JSH then kill thread.  That filter should typically catch the very first
message in a thread, (that is, in threads that match that criteria, typically
the first message in the thread matches) and most of the messages that follow it.  The observed result is that most messages in these threads (that arrive 
after the filter is set) remain unread, and the thread is not killed.

It may or may not be relevant that actions of "kill thread" and "mark read" 
do not stop the filtering of the message.  The Mailnews code goes on and 
continues to try to match the message against subsequent filters, and often
succeeds, despite the fact that the thread containing the message was killed.
I learned to add the additional action known as "stop filter execution", 
but that did not cure the problem of unkilled threads.

If you would like me to post my entire file of filters for that newsgroup,
I am willing to do so.

Comment 5

[Joshua Cranmer [:jcranmer]]

But JSH occasionally has some good gems, although he's nowhere near as interesting as he used to be. ;-) (No, before you ask, I do not think he's on to something with his surrogate factoring stuff)

Anyways, yes, I would like to see the entire filter file.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: sci.crypt.dat filter file

I make no claims regarding the merits of any sources filtered out by these
rules.  This is merely a test case.  

At the present, none of the rules in this file select "Kill Thread" because 
it has no effect (or, less effect than simply "Mark read").  The filters named 
JSH and jsteveh are some of the ones that I tried to use with Kill Thread.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Maybe the problem is broader than just the Kill Thread action.
Maybe the entire filtering subsystem is simply intermittent.  

I just read sci.crypt for today.  Despite the existence of a rule in the 
filter file that detects messages From [e-mail] and marks 
them read, a large number of new messages from that source now appear
unread in that group.  The filter log shows that they were detected and
were marked read.  But there they are, unread, in the message list pane.  

Comment 8

[Joshua Cranmer [:jcranmer]]

I've tried a few more times, but I've only managed to get one case where I came close to reproducing: the JSH threads stayed in the view after I marked them as ignored. However, when testing again to see if it was a view setting that was causing it, the problem failed to materialize.

To account for the possibility that it may be a newsserver, I requested someone else using a GigaNews-based server try it, and the results also seemed to indicate a WFM.

Other tests I would like to have you do to try to narrow down the problem:
1. Does this problem manifest only under certain View options?
2. Is this a newsserver-specific problem: do you get the same results when using news.aioe.org (Public, free news server that I use for most of my tests)?
3. Does this problem still manifest with my latest patch for bug 11054 included? I did a modification on how the Kill Thread action works to implement killing subthreads.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I also have this problem with newsgroups on the server news.grc.com 
(which is publicly and freely accessible, but requires 
For that server, I have several server-wide filters to kill threads that 
are started by certain individuals.  Those filters are all of the form:

(.) Match all of the following
   From    Contains         (substring of from address) 
   Subject Doesn't contain  Re:
Perform these actions:
   Ignore Thread
   Stop Filter Execution

The From strings I use include:
Retired
Smith
Keays

Effect: Many threads started by these individuals are not ignored.
For example a thread started on 3/21 by Retired in group grc.privacy
was not ignored.  

Comment 10

[Joshua Cranmer [:jcranmer]]

I just did some testing with a nightly, and I get the problem on the nightly, but definitely not with my 11054 applied.

Try building TB or SM with my 11054 patch and see if you still get the problem.

Comment 11

[Joshua Cranmer [:jcranmer]]

(other notes, sorry for spam)

Confirmed also on Linux as well, and probably on Mac. It may also be one of my other patches (bug 413260, bug 418551, bug 400331, and bug 413077 as well) that causes it. It is my learned opinion that bug 11054 fixes this problem.

Comment 12

[Rich Gray (:rbgray)]

I confirm this with nightly Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032701 SeaMonkey/2.0a1pre

sci.crypt via SuperNews (to be borged by GigaNews on 31 Mar.)

sci.crypt.dat:

version="8"
logging="yes"
name="kill JSH"
enabled="yes"
type="4"
action="Ignore thread"
condition="OR (subject,contains,JSH) OR (from,contains,JSH)"

Comment 13

[Joshua Cranmer [:jcranmer]]

(I am typing this from memory, so I may have a few things wrong here)

This is what I think is causing the problem. When implementing bug 16913, I had to add the header to the database before calling filters to get XHDR to work nicely without large amounts of auxiliary bookkeeping. However, kill thread and watch thread work by setting an illegal flag on the message header that is transferred to the thread upon addition to the thread/db. Since post-16913, the flag was set after adding to the database, so the thread did not receive this, which is causing the regression (note: there may be other filters that assume the header is to be added which were also regressed, I'll have to check).

In bug 11054, I decided I could save a bit by using one of the illegal flags. When coding, I saw that the flag was actually used to transfer the ignored property to the thread. I therefore changed the code for ignore (but not watch) to actually do the right thing. Since I never had a build that did not include bug 11054 as a patch, I couldn't reproduce your regression.

In short, bug 11054 accidentally fixed an accidental regression from bug 16913.

Comment 14

[Rich Gray (:rbgray)]

Confirming that bug 11054's attachment 311153 [details] [diff] [review] makes ignore thread work correctly in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008033022 SeaMonkey/2.0a1pre.  :)

Comment 15

[Joshua Cranmer [:jcranmer]]

Bug 11054's backend portion has been checked in, which appears to fix this bug per my tests and those of Rich Gray.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Did this get backed out? 
It surely isn't working in Gecko/2008052802

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Here's a test.
News server is news.grc.com port 119 
No "read new messages at startup"
No "Check for new messages every NN minutes"
server wide filter:
  Match all of the following
  From Contains paradoX@
  Subject doesn't contain Re:
  Perform these actions
  ignore thread
  mark as read
  stop filter
Also have this same filter in one of the newsgroups.
Then read newsgroups:
Expected results: any threads started by paradoX are ignored, and 
   no unread messages in any thread started by paradoX
Actual results: No threads are ignored.  Any thread started by paradoX
   has the first message marked read and no others.

100% reproducible on Windows

Comment 18

[Joshua Cranmer [:jcranmer]]

This regression was first caused by the fix for bug 16913 which I didn't see because bug 11054 fixed the regression by accident, BUT, it appears that backing out bug 16913 has reopened this regression, which I didn't see because bug 16913 is still applied in my builds...

So the cause of this regression is that, because the message's thread is being marked as ignored before it is threaded, it gets attached to a non-ignored thread.

The best fix for this case would be to have the database, when threading messages, to merge the thread flags together in some fashion...

Comment 19

[Joshua Cranmer [:jcranmer]]

Also, I think this is a tad more pressing than 3.0...

Comment 20

[Joshua Cranmer [:jcranmer]]

My original plan was to have the fix rolled into the 16913 re-submission, but between test cases, fakeserver upgrades, and the synergy of multiple changes, that patch was getting uncomfortably large, and was starting to make finding the error difficult.

It turns out that merely zipping up the flags together is insufficient because of some factor, most probably the fact that the fix ultimately needs to get a thread outside the database.

Thinking about it some more, it may make more sense to have the proto-headers carry along proto-flags themselves rather than relying on the db to create the thread.

But first, more testing.

Comment 21

[Joshua Cranmer [:jcranmer]]

Attachment: Patch

I myself am not 100% sure that this method is the best way, but the other way to handle this would involve a lot more work and a redefinition of how headers work before being added to the database, which I am pretty sure is unwarranted.

Now I see why people used the original hack of using MSG_FLAG_{IGNORED,WATCHED} on messages as a vehicle for the threads.

Comment 22

[David :Bienvenu]

> When implementing bug 16913, I had
> to add the header to the database before calling filters to get XHDR to work
> nicely without large amounts of auxiliary bookkeeping. 
You could simply have a sorted array of nsIMsgDBHdr's that haven't been added yet, which doesn't seem like a lot of bookkeeping. Once you've got the xhdr info for the hdr, you add it to the db and remove it from the array...

I'm a bit confused about the problem here - is it the case that some of the time the header has been added to the db, and hence to the thread, and some of the times not, when the filter fires? That would be a bit disturbing and prone to error. But if the header is in the db, and its thread, can't you just mark the thread ignored like we do normally? Are we adding the header to the db twice? That's a recipe for trouble...

Comment 23

[David :Bienvenu]

Comment on attachment 326690 [details] [diff] [review]
Patch

over irc, I learned from Joshua that the real issue is that kill/prune sub-threads is why the ignore/watch thread flags need to be passed in through a msg hdr property, since the sub-thread stuff co-opted the same flags. This means that we still have to add the msg hdr to the db/thread *after* the filter has run, but Joshua has a fix for that as well, as I understand it...

Comment 24

[Joshua Cranmer [:jcranmer]]

Checked in:
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.825; previous revision: 1.824
done
Checking in mailnews/db/msgdb/src/nsMsgThread.cpp;
/cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgThread.cpp,v  <--  nsMsgThread.cpp
new revision: 1.91; previous revision: 1.90
done
Checking in mailnews/news/src/nsNNTPNewsgroupList.cpp;
/cvsroot/mozilla/mailnews/news/src/nsNNTPNewsgroupList.cpp,v  <--  nsNNTPNewsgroupList.cpp
new revision: 1.145; previous revision: 1.144
done
Checking in mailnews/local/src/nsParseMailbox.cpp;
/cvsroot/mozilla/mailnews/local/src/nsParseMailbox.cpp,v  <--  nsParseMailbox.cpp
new revision: 1.306; previous revision: 1.305
done