Closed Bug 505487 Opened 13 years ago Closed 13 years ago

Message filter rule incorrectly matching/firing

Categories

(MailNews Core :: Filters, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: mike, Assigned: Bienvenu)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2pre) Gecko/20090721 Shredder/3.0b4pre

I have a long-working message filter that started misbehaving with last night's nightly build (Jul 20/21).

The filter contains two match patterns: "Match any of the following":

Sender ends with slimdevices.com
From contains slimdevices.com

All messages since taking the nightly download started matching this rule (resulting in all my new messages being moved into another folder).

When I ran the Message Filter manually on my Inbox, *all* of my Inbox items were moved (i.e. the rule incorrectly matched everything).

Reproducible: Always

Actual Results:  
Filter incorrectly matching anything.

Expected Results:  
Filter should only match the given criteria.
Additional info.  When I edit the rule, make no changes, but click OK to leave dialog, I get an error dialog:

"This filter cannot be saved because some search terms are invalid in the current context."
Yet more info: it appears that Sender, while available as a choice, is no longer valid, and creating a rule with this criteria will cause the error dialog indicated in comment #1.

So, there are two bugs here:
1) Choosing Sender in a message filter causes an error
2) Message Filter criteria which contain problematic rules result in a TRUE outcome, causing the message rule to fire.  This should be changed such that problematic rules do not fire.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → Trunk
I seem to be getting something very similar to this w/ my filter(s).
hmm, I hope the filter enumerator changes I made to search didn't cause this somehow...
Component: General → Filters
OS: All → Windows XP
Product: Thunderbird → MailNews Core
QA Contact: general → filters
Hardware: All → x86
ah, the issue is the begins with/ends with (contains, doesn't contain works fine with sender).
Blocks: 499806
OS: Windows XP → All
Hardware: x86 → All
This popped up on me as well on MacOSX with last nights build. Screenshot show the "hidden?" SpamAssassinYes filter that is triggered from "Trust junk headers set by:" option in Account Settings. Second screenshot shows a filter that I use to auto-tag. Turned off "Trust junk headers" and edited my auto-tagging filter a few times (and made the filter screen bigger, which actually seemed to fix it for who knows what reason) so things are working fine for me right now.
I think the filter editor complaining about Sender is a regression from bug 495519, which is in b3, scarily. It looks to me like the FilterEditor code just reads off the end of the validity table for custom headers.
FYI: I created a new filter rule, w/o Sender (used From only), and the rule is firing incorrectly.
Scratch comment #9.
(In reply to comment #8)
> I think the filter editor complaining about Sender is a regression from bug
> 495519, which is in b3, scarily. It looks to me like the FilterEditor code just
> reads off the end of the validity table for custom headers.

The other alternative would be the filtered enumerator (as you suggested
before) from bug 499806. We did change the nsMsgSearchAttrib::OtherHeader value
so there could be something hard-coded somewhere.
I just did a local back out of bug 499806... there were various emails I got earlier that just wouldn't be marked as non-junk (they kept leaping back into junk folder regardless of what I did), backing out bug 499806 has resolved that, they stay in the inbox now.
I've just been looking at the filter editor issue because I'm trying to set up a filter like MrC did - that may very well be a separate issue, but it's the one I ran into first.

My from filter works fine...
Assignee: nobody → bienvenu
this fixes the filter editor regression, which, as I suspected, is separate from the filter execution error. I've reproduced that issue now, so I should be able to get a fix for that soon.

The bug fixed in this patch is that the validity tables treat all custom headers as "Other", so we need to reduce "value" to "Other" in order to use the validity tables. Otherwise, we're just reading off the end of the table, and getting random results.
Attachment #389789 - Flags: superreview?(bugzilla)
Attachment #389789 - Flags: review?(bugzilla)
Attached patch proposed fix (obsolete) — Splinter Review
ok, this fixes the filter execution error for me - the issue was limited to begins and ends with, for custom headers, I believe.

I'll go look at the test cases and see why this wasn't covered.
Attachment #389793 - Flags: superreview?(bugzilla)
Attachment #389793 - Flags: review?(bugzilla)
Comment on attachment 389793 [details] [diff] [review]
proposed fix

oh, the bufLength fix is unrelated, but it quiets an assertion I was seeing where we were calling SetLength(-1) on a CString when doing a body search.
Thanks for the fast work on this.  I'll check out the results with the next build that contains the fix.
prev body handling part of patch was wrong so I'm just removing it...
Attachment #389793 - Attachment is obsolete: true
Attachment #389811 - Flags: superreview?(bugzilla)
Attachment #389811 - Flags: review?(bugzilla)
Attachment #389793 - Flags: superreview?(bugzilla)
Attachment #389793 - Flags: review?(bugzilla)
Flags: in-testsuite?
Attachment #389789 - Flags: superreview?(bugzilla)
Attachment #389789 - Flags: superreview+
Attachment #389789 - Flags: review?(bugzilla)
Attachment #389789 - Flags: review+
Attachment #389811 - Flags: superreview?(bugzilla)
Attachment #389811 - Flags: superreview+
Attachment #389811 - Flags: review?(bugzilla)
Attachment #389811 - Flags: review+
I checked both of these patches in together:

http://hg.mozilla.org/comm-central/rev/1e5e809179cb

(in time for today's Thunderbird nightly).

Forgot to say, would be good to extend/create a unit test to cover this bug.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b4
Had to build it myself since the MacOSX nightly is slow in coming, however this FIXED the problem for me.

I will say that the "SpamAssassinYes" filter that is supposed to be hidden still shows up in my filter list, and when I go to Edit it, I still have the two blank "Subject" matches before the ***SPAM*** Subject match. I attached an image of what it looks like yesterday, still looks the same today. Looks VERY weird and confusing, but messages are not automatically going to spam folder any longer.
Blocks: 495519
Filters seem to be working correctly now.  Thanks!
this adds some basic search tests, including ones that would have caught this regression.  There are a lot of tests we could add to this harness, but this is a start...
Attachment #390068 - Flags: review?(bugzilla)
Blocks: 505698
Comment on attachment 390068 [details] [diff] [review]
some basic search tests

Thanks, increasing coverage bit by bit is certainly the best way to do this.
Attachment #390068 - Flags: review?(bugzilla) → review+
I think some of the view tests exercise a fair amount of the search code, but it's good to directly test the search criteria. This test adds some of the criteria that the view tests don't exercise.
Flags: in-testsuite? → in-testsuite+
Comment on attachment 390068 [details] [diff] [review]
some basic search tests


>+  { testString: "QAContact",
>+    testAttribute: OtherHeader,
>+    op: BeginsWith,
>+    count: 1},

Fails everywhere:
{
Finished search does 1 equal 0?
NEXT ERROR TEST-UNEXPECTED-FAIL | ../../mailnews/resources/searchTestUtils.js | 1 == 0 - See following stack:
[...]
JS frame :: ../../mailnews/resources/searchTestUtils.js :: anonymous :: line 66
}

Nit: While there, the 'Finished search does ...' line seems superfluous (now).
pushed file I forgot...
Still unable to edit the filter, the response being "This filter cannot be saved because some search terms are invalid in the current context," been fixed? I am in 17.0.2 and still getting the error.
You need to log in before you can comment on or make changes to this bug.