97136 - Unable to load filter on space characters

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Alexey Chernyak]

OS: Win2K
Build: 2001082608

Had similar problem in bug 77005
This time I am able to set the filter, but it is gone next time I start up Mozilla.
Basically I want to have a filter on 25 space characters in the Subject.
this is what corresponding part of my rules.dat looks like:

name="Spam"
enabled="yes"
type="1"
action="Move to folder"
actionValue="mailbox://[e-mail]/Trash"
condition="OR (subject,contains,                         )"
When I set filter, it holds until I restart Mozilla. But on load, it treats
these space characters as nothing.

Steps to reproduce:
1. Set a filter on 25 consecutive space characters as shown above.
2. Restart Mozilla.
3. Check the filter.

Comment 1

[Navin Gupta]

cc bienvenu, should we allow this or treat space as nothing.

Comment 2

[David :Bienvenu]

we should allow it, but that would require fixing our filing code.

Comment 3

[Navin Gupta]

when we load the value of the filter we eat the whitespace. But fixing 
that also wouldn't fix this problem. 

I tried sending messages with subject as  "        " but when I receive
it from the server there is no subject in the message. So either the 
server treats blank as null or smtp sends null. From the smtp log I 
could not make out much because it just says sending DATA. 

0[9511e0]: SMTP entering state: 21
0[9511e0]: SMTP entering state: 4
0[9511e0]: SMTP Send: MAIL FROM:<[e-mail]>^M
0[9511e0]: SMTP entering state: 0
0[9511e0]: SMTP Response: 250 Sender <[e-mail]> Ok^M
0[9511e0]: SMTP entering state: 6
0[9511e0]: SMTP Send: RCPT TO:<[e-mail]>^M
0[9511e0]: SMTP entering state: 0
0[9511e0]: SMTP Response: 250 Recipient <[e-mail]> Ok^M
0[9511e0]: SMTP entering state: 7
0[9511e0]: SMTP Send: DATA^M
0[9511e0]: SMTP entering state: 0
0[9511e0]: SMTP Response: 354 Ok Send data ending with <CRLF>.<CRLF>^M
0[9511e0]: SMTP entering state: 8

cc ducarroz for smtp issue. 

Comment 4

[Alexey Chernyak]

Subject: FREE CASH GRANTS, NEVER REPAY.....                         21688

This is an example of kind of subjects that I want to filter

Comment 5

[timeless]

unfortunately, nc4 suffers from this too.

Comment 6

[Navin Gupta]

ok, I have the fix for alexey's problem and it is not to eat the whitespace
when loading filters. 

Comment 7

[Navin Gupta]

Attachment: proposed fix

Comment 8

[Navin Gupta]

David, please review. 

Comment 9

[David :Bienvenu]

I don't like this approach, personally, because it might break some filters, if
anyone has edited filters by hand and put spaces in.

Comment 10

[Navin Gupta]

Well, if we have beginning/trailing whitespaces it will still work because 
when we match string we use PL_strcasestr. Also we do not expect user to set
up filter move subject "tes t" and expect a mail w/ subject  "test" to be 
filtered. 

Comment 11

[David :Bienvenu]

no, that's only for a contains match. If it's an exact match, the spaces would
matter if we didn't ignore them.

Comment 12

[Navin Gupta]

But isn't that the right thing to do. The user should be more careful while
setting up a filter for an exact match. Removing space is like manipulating
user input. 

Comment 13

[David :Bienvenu]

It would be the right thing to do, if we didn't let people edit their filter
files by hand. But as it is, your fix would break anyone who editing their
filters by hand and put extra spaces in, thinking they would be ignored. And I'm
confused why you think the following kind of filter would work the same after
your fix:

subject contains test

subject contains " test"

With your change, any whitespace the user might have put in by hand by editing
the filter file would be significant. For example, a msg with just the subject
"test" would have matched before, but not after your change, because "test" does
not contain " test". Does this make sense?

That being said, you're mainly breaking people who edit their filter files by
hand going forward, because anyone who had edited their filter file in the past
would have the format "fixed" any time the filter file was written out the next
time they used the filter ui editor. So as long as you understand and accept
that risk, r=bienvenu

Comment 14

[Navin Gupta]

point taken, but the user will have himself to blame. cc jglick for her 
input on how to deal with this issue. 

Comment 15

[David :Bienvenu]

well, the point is, this used to work, and we're making it so it won't work
anymore, so I think the user could blame us and not be totally unreasonable.

Comment 16

[Alexey Chernyak]

The filter I set through UI used to work, but not after I reloaded Mozilla.
I think this is more serious than something that stops working after user edits
it by hand. Either way, it's a one-off inconvenience. People who edited, will
notice it's broken, will understand why it's broken and will fix it and forget
about it.

Alternatively, I think there is a much better way of fixing this.
Do these files accept an escape character like '\'?
If they do, (they really should to be able to accept filters with '"' ")" and
"," characters in them), the fix to this problem would be simply inserting this
escape character in front of any leading spaces, like this:

condition="OR (subject,contains, \                         )"

In above example the leading space will be ignored, and all 25 spaces following
\ should be processed.

Comment 17

[Alexey Chernyak]

Attachment: another proposed fix

Comment 18

[Alexey Chernyak]

I'm not familiar with the code, so above patch is just a guess work. What I
tried to do there is to enclose the string into quotes when it is written down
into the file, just like in ')' case, in order to preserve leading spaces. I
also discovered that Mozilla swallows leading quotes as well, so I added it
there too.
This should not affect anyone who manually edited their rules.

Also I discovered a much more serious problem. If the string contains an odd
number of '"' then Mozilla corrupts this and all following rules on load until
it encounters another '"'. Basically it expects that the srings will contain
closed quotes, and is not ready to deal with it when there's only one '"' or an
unclosed quote.

Comment 19

[Håkan Waara]

The issue you mention sounds like a problem with the *.rules parser.

Can you file a new bug on that (and cc me, please)?

Comment 20

[David :Bienvenu]

Please don't file a bug. That's the way the parser works; you have to terminate
your quotes. It's part of the syntax.

Comment 21

[Håkan Waara]

If I have a quote in my filter-name, I assume our parser will do Bad Things.
Bienvenu, don't you consider that a bug?

Comment 22

[David :Bienvenu]

that's a separate issue. Quotes should either not be allowed in filter names, or
they should be escaped when written out.

Comment 23

[Alexey Chernyak]

filed bug 103658
bienvenu, they are escaped, for more details read bug 103658

so can I get feedback and maybe r=sr= on my patch here?

Comment 24

[David :Bienvenu]

for checking the first char, don't use pl_strnchr, just use m_value.string[0] ==

other than that, the patch seems OK, but it would need to work with whatever fix
is developed for bug 103658. 

I checked 4.x, and it has the same bug : it generates the same kind of escaping,
and can't read it back in correctly, so we succesfully ported that bug!

Comment 25

[Alexey Chernyak]

Attachment: fix

Comment 26

[Alexey Chernyak]

bienvenu, thanks, I've updated the patch to use [].

This patch modifies how rules are written down and doesn't change much because
it is already done on ')' char right now. The fix for bug 103658 will affect
either how the rules are read (which is unrelated) or some other modifications
for which the code for ')' and this patch might be removed, or it'll be modified
where this place won't be affected at all.

So it's better to check in the fix for this before any work on bug 103658 and
close this issue.

can I get r= and sr= please?

Comment 27

[timeless]

assigning to patch provider, perhaps sspitzer will r=...

Comment 28

[Navin Gupta]

we already have some code to take care of '"' in the next line. 
		if (PL_strchr(m_value.string, '"'))
		{

I didn't get why you have this in that if block. Can you explain ?
(m_value.string[0] == '"'))

Comment 29

[Alexey Chernyak]

All the code in the next line does, is escaping '"' characters.
What I am doing is enclosing the string in quotes so that the leading quote does
not get lost during read.
Basically the patch prevents leading spaces and leading '"' from being lost on load.

Comment 30

[Navin Gupta]

ok, looks ok, r=naving

Comment 31

[David :Bienvenu]

Comment on attachment 52586 [details] [diff] [review]
fix

sr=bienvenu, but please make sure you've tested this thoroughly.

Comment 32

[timeless]

Checking in mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp,v  <-- 
nsMsgSearchTerm.cpp
new revision: 1.76; previous revision: 1.75
done

Comment 33

[Alexey Chernyak]

verified fixed on 2001103003