193612 - filters are not thread-safe

Status: NEW

(MailNews Core :: Filters, defect)

Description

[Chris Pickett]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1; MultiZilla v1.1.32 final) Gecko/20021130
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1; MultiZilla v1.1.32 final) Gecko/20021130

I was running a filter on my Sent Items folder and then half-way through I
realized the filter said "To contains X" OR "Sender contains Y", and I stupidly
changed the filter to "To contains X" AND "Sender contains Y" in the middle of
the move operation.  This resulted in about half of the messages in my Sent
Items folder being lost forever (about 50).  

Fix this by not allowing users to add or modify filters (or at least apply the
changes) while filters are running.  It would also be handy if filters could be
stopped midway through applying them.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Chris Pickett]

I was able to reproduce this in 1.3b, but have stopped using that version. 
Although this bug is pretty unlikely to happen to someone, it's very horrible if
it does so if you have time please fix it for 1.3 by just putting some locks
around the filtering code.

Comment 2

[David :Bienvenu]

what did the filter that you were applying do as its filter action? Delete
messages? move messages?

Comment 3

[Chris Pickett]

the filter was moving messages, not deleting them.

i just tried to reproduce in 1.5 for a couple hours but could not.  i got
confused a little too, because some folders had the .msf file rebuilt in the
process and this destroyed the message counts, at which point i gave up.

however ... i think that while filters are running, the user should only be able
to hit "stop" in the filters dialog, and not be able to create / edit / delete
any filters.

also, i noticed that if i start filters from the tools menu, and then open the
filters dialog while they are running, the "stop" button is not visible, and i
can also create / edit / delete filters while they are running.

if you fix that (make "stop" the only thing possible while filters are running),
then there's no way a user can goof things up.

do you want me to file a new bug for that?  or just change the summary of this one?

Comment 4

[David :Bienvenu]

well, I'm not convinced that editing the filter was what caused your problem -
once you start a filter running, we never look at the original filter again.
I.e., for filter "after the fact", we translate the filter into a search, and
run the search. You can change the filter, but we won't notice it until the next
time you run the filter. Actually, that's true for imap, but probably not for
local mail. The simpler fix would be just to make sure the filter after the fact
code is using a copy of the search terms, and not the original search terms.

Comment 5

[Howard Chu]

(In reply to comment #4)
> well, I'm not convinced that editing the filter was what caused your problem -
> once you start a filter running, we never look at the original filter again.
> I.e., for filter "after the fact", we translate the filter into a search, and
> run the search. You can change the filter, but we won't notice it until the next
> time you run the filter. Actually, that's true for imap, but probably not for
> local mail. The simpler fix would be just to make sure the filter after the fact
> code is using a copy of the search terms, and not the original search terms.

Hm, this behavior changes with bug #295088. We probably ought to disable the
New/Edit/Delete/Move Up/Move Down buttons when Run Now has been clicked, until
the running filter has completed.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to comment #5)
> (In reply to comment #4)
> > well, I'm not convinced that editing the filter was what caused your problem -
> > once you start a filter running, we never look at the original filter again.
> > I.e., for filter "after the fact", we translate the filter into a search, and
> > run the search. You can change the filter, but we won't notice it until the next
> > time you run the filter. Actually, that's true for imap, but probably not for
> > local mail. The simpler fix would be just to make sure the filter after the fact
> > code is using a copy of the search terms, and not the original search terms.
> 
> Hm, this behavior changes with bug #295088. We probably ought to disable the
> New/Edit/Delete/Move Up/Move Down buttons when Run Now has been clicked, until
> the running filter has completed.

Should the above be a new bug, and this one closed incomplete?

Comment 7

[Wayne Mery (:wsmwk)]

Chris write: "I stopped using filters ages ago and I don't have a good way to test it now.  Playing around with dataloss takes some precaution.  I notice that a previous comment says I couldn't reproduce it in 1.5.

Anyway, Howard Chu's suggestion seems reasonable."

Comment 8

[Wayne Mery (:wsmwk)]

Kent, what do you think of this?

Comment 9

[Kent James (:rkent)]

I don't see a clear description of the problem this bug is trying to solve. I'm not sure how the current filter acts when you change it in the middle of an operation - but there is no evidence presented that the current behaviour is unacceptable. I would NOT recommend disabling changes unless there is a clear problem that that is trying to solve.

I recommend INCOMPLETE.

Comment 10

[Howard Chu]

(In reply to comment #9)
> I don't see a clear description of the problem this bug is trying to solve. I'm
> not sure how the current filter acts when you change it in the middle of an
> operation - but there is no evidence presented that the current behaviour is
> unacceptable. I would NOT recommend disabling changes unless there is a clear
> problem that that is trying to solve.
> 
> I recommend INCOMPLETE.

As I mentioned in comment #5, due to the optimizations I wrote for bug #295088, the filter is only constructed once and evaluated in-place. If you edit the filter and delete terms from it while the filter is executing, you'll cause a crash.

Comment 11

[timeless]

the current summary is perfectly clear.

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to Howard Chu from comment #10)
> As I mentioned in comment #5, due to the optimizations I wrote for bug
> #295088, the filter is only constructed once and evaluated in-place. If you
> edit the filter and delete terms from it while the filter is executing,
> you'll cause a crash.

/me wonders if we have such a crash bug on file