1312172 - Changing between "match all of" and "match any of" fails for rules that are not visible. Then filter might fail.

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Wayne Mery (:wsmwk)]

Discussed with aceman on IRC.

steps
1. Have a filter whose list of rules exceeds the size of the window, resulting in scroll bar
2. change from Match All to Match Any
3. close filter
4. look at msgfilterules.dat

result
1. only those rules that were visible in the UI got changed to the OR condition. the rules which were not visible have AND condition
2. filter fails

perhaps a regression?

Comment 1

[:aceman]

I don't think it is a regression. It seems to me this is caused by the mechanism of "uninitialized rules" in the filter editor. Rules that are off-screen (scrolled away from view) are not initialized into the proper objects. There is an object holding the rules, but there is a strange mechanism where it only initialized (pulls in) rules into itself that are actually touched or made visible.
I'm not sure if that is a perf feature or what.

I'd like to ask Kent if the resulting "buggy filters" are actually evaluated literally, as they get stored.
After the bug happens, the msgfilterrules.dat file contains rule definitions like this:
condition="OR (subject,contains,123) OR (subject,contains,234) AND (subject,contains
,345)"

Are the rules evaluated in the backend as 2 ORs and 1 AND ? Could this be a source of some of the reports of people claiming their filters not working?

Comment 2

[Kent James (:rkent)]

I can't answer that question without spending time diving into the code, which you could also do. So I am going to decline the invitation for now.

Comment 3

[Wayne Mery (:wsmwk)]

I thought we previously encountered something when the number of items exceeded the size of the window - but I'm not finding such a bug report.

Comment 4

[:aceman]

Attachment: WIP patch

Wayne, can you try this?
Can you also determine whether the change of the bool operators make a difference? I.e. the broken mixed operators make the filter not hit on your message set, but the fixed operators (all OR or all AND) work?

Comment 5

[Wayne Mery (:wsmwk)]

Comment on attachment 8806494 [details] [diff] [review]
WIP patch

Thanks. All cases work great with the patch, including "ALL" which indeed fails without the patch.

Comment 6

[:aceman]

Comment on attachment 8806494 [details] [diff] [review]
WIP patch

Thanks.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8806494 [details] [diff] [review]
WIP patch

Review of attachment 8806494 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, the second hunk (cosmetic) don't apply though. r=mkmelin

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1312172.patch

Now it applies ;-)

Comment 9

[:aceman]

Comment on attachment 8826317 [details] [diff] [review]
1312172.patch

Thanks for finishing it.
I was just about to fix this, as I noticed the last line has now changed to searchTerms.replaceElementAt() in the file :)

Comment 10

[:aceman]

I suspect this bug may be the reason of some more bug reports claiming filters are not applied to messages that should be matched. Users toggling the "any of" and "all of" the rules in the filter in the past may have damaged the filter so that it now has mixed AND/OR operators in the msgFilterrules.dat file (see comment 1).

Once this bakes on trunk, we should consider it for uplifting to 52.

I CC our great bug triagers to be aware of this fix and method of diagnosing some of the bug reports (have the user check the AND/OR strings in the msgFilterrules.dat file). The proper format of a filter must always have either all operators being AND or all of them being OR.

Comment 11

[Jorg K (CEST = GMT+2)]

Would you like this landed?

Comment 12

[:aceman]

Ah yes, thanks.

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0d4ec1a4f47b813fae0f1ec5a35cb64b374cd5b6

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8826317 [details] [diff] [review]
1312172.patch

(In reply to :aceman from comment #10)
> Once this bakes on trunk, we should consider it for uplifting to 52.
I think "trunk baking" is over-estimated since most regression only get detected on ESR :-(
This change doesn't look like it would cause a whole lot of trouble, but one never knows.

Comment 15

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/8874f641f185755d8a6f283724ae9995cd7adfe8

Comment 16

[Jorg K (CEST = GMT+2)]

Beta (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/77704a98cda75e7059a69663b5991bcc3d61e05b