141354 - adding new filter corrupts filter file

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Dan Wallach]

I've got 285 filters.  If I try adding more through the GUI, the resulting 
rules.dat file will be corrupt.  It's 100% reproducible.  I'm attaching two 
files.  "rules8.dat" is the correct pre-corruption file.  I then added one rule 
and the file size doubled.  This is attached as "rules.dat".  If you search 
for "Nettle" in the text, you'll find the first corruption, and then it gets 
worse from there.

"rules8.dat", FYI, was a file I created by hand from my Netscape 4.x rules file 
(mechanically rewriting the "//imap:" strings to the new URL style, but 
otherwise making no changes).  I had to do that because, by the time I realized 
the corruption had happened, the auto-imported file was long gone and this was 
all that I had left.  If I use my rules file like this without trying to edit 
it, everything works great.

Versions: I don't recall having any problems with this in earlier Mozilla 
builds (0.9.7 seemed pretty good for me), but the problem has been worse, 
lately.  I'm currently running 1.0RC1.  Start a fresh copy with "rules8.dat" as 
the rules, add one filter rule, and quit.  Same corruption, every time.

Comment 1

[Dan Wallach]

Attachment: original, correct rules file

Comment 2

[Dan Wallach]

Attachment: corrupt rules file after adding one new rule

Comment 3

[laurel]

Eek.  Your life must be pretty complicated if you need 285 filters.  My condolences.

Comment 4

[Dan Wallach]

I try to have one folder per person with who I regularly correspond.  Stretch
that out over near ten years of saved e-mail, then add in a ton of anti-spam
rules, and it adds up quick.

Comment 5

[Navin Gupta]

Attachment: proposed fix

The problem was customHeaders were not present in mailnews.customHeaders pref
but they were present in rules.dat. This was causing the corruption (atleast
for me).

The fix is to look for the header in the pref and if it is not there add it. 
The pref is later read when we try to edit an existing filter. 

david, can you review ? thx.

Comment 6

[Navin Gupta]

I think we should take this for beta because 4x rules.dat directly copied
to 6x will cause this problem if there are customHeaders in the filters. 

Comment 7

[Navin Gupta]

David, can you review the proposed fix, thx. I don't know if you saw my earlier
request. 

Comment 8

[David :Bienvenu]

doesn't this leak headersString?

+          *attrib += i; //we found custom header in the pref
+          return NS_OK;

If I understand the patch correctly (which is a long shot :-) ), you're adding
the header to the pref if you don't find it in the existing pref?

Comment 9

[Navin Gupta]

Attachment: proposed fix w/ freeing of headerString

I have added an assertion so that we know we have more than 50 hdrs in the
pref.
At present we allow a max of 50 hdrs and I have a bug about that.

Comment 10

[David :Bienvenu]

Comment on attachment 82072 [details] [diff] [review]
proposed fix w/  freeing of headerString

r=bienvenu - normally, if you add an assertion, you should also add the code
that deals with the situation, but as long as there's a separate bug about it,
it's OK.

Comment 11

[Scott MacGregor]

Comment on attachment 82072 [details] [diff] [review]
proposed fix w/  freeing of headerString

sr=mscott

Comment 12

[Navin Gupta]

fixed

Comment 13

[Jaime Rodriguez, Jr.]

Does size matter, or can one gte in this state with less than a few hundred filters?

Comment 14

[Dan Wallach]

Has this found its way into the nightly build yet?  If not, what do I have to
download to verify the fix?

Comment 15

[Navin Gupta]

Dan, you can download today's build and the fix should be in it.

size of filter doesn't matter, you can get into this state with few filters if all
of them happen to use custom headers. 

Comment 16

[Jaime Rodriguez, Jr.]

Let's get this one in after RC2. adt1.0.0- [adt2 RTM]

Comment 17

[laurel]

Using may03 trunk:
Did not see any corruption unless I launched with customheader pref having over
50  headers -- that caused a crash and messed up my custom header rules in
rules.dat. Saw the crash and a UI problem... will log new report(s) or comment
in the bug dealing with the 50 maximum -- bug 104085.

For the fix case and under 50 custom headers I saw no corruption:
1.  Tried with several different custom headers called in different rules
conditions in the rules.dat file when NO custom headers were defined in
prefs/ui. Added new filters, no problem.  Got mail, no problems. Exited and
returned and added filters, no problem.
2.  Tried same scenarios with several different custom headers called in
different rules condition in the rules.dat file when only a couple of the
headers were already defined in prefs/ui.

Comment 18

[laurel]

Also tried where some rules were AND, some OR with custom header and other
subj/sender as criteria -- all rules contained custom header.  Whether incoming
messages matched filters or not, all worked fine. Did not see corruption. Any
custom headers called out in rules that were not originally specified in
prefs.js were added, so that the ui, rules.dat and prefs.js custom header list
was in sync (or is that "NSync"?).

Marking verified for trunk.  Reporter: if you see differently, please reopen.

Comment 19

[Dan Wallach]

FYI, I finally got around to trying the latest nightly build, and the problem is
fixed.  It's not in 1.0RC2, but I guess this fix didn't find its way here.

Comment 20

[Navin Gupta]

We should take this now, mozilla1.0 has shipped.

>------- Additional Comment #16 From Jaime Rodriguez, Jr. 2002-05-03 14:00 -------

>Let's get this one in after RC2. adt1.0.0- [adt2 RTM]

Comment 21

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin to the 1.0 branch. pls check
this in asap, then add the "fixed1.0.1" keyword.

Comment 22

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 23

[Navin Gupta]

fixed on branch.

Comment 24

[laurel]

OK using june21 commercial branch: win98, linux rh6.2, mac OS 9.2