104398 - movemail + filters = crash

Status: RESOLVED FIXED

(MailNews Core :: Movemail, defect)

Description

[Hervé]

((((( first, please note that to create a filter on a movemail account, you've
got to :
- create a pop account
- declare some filter on it
- convert this pop account into a movemail account (exit, edit prefs.js, start)
- then any filter for the movemail account is hidden in the mail filters dialog
! you can only see filters for the other types of accounts (pop, imap) :-(
but that's another bug, and we've got this workaround, so let's move on... )))))


the crasher is when you get messages into the movemail account ("get msg" button)

how to reproduce :
- create this filter : when subject contains "foo" put the message in folder "A"
- send yourself 3 mails using any unix mailer, in this order : 
   1st with subject "whatever 1", 
   2nd with subject "foo"
   3rd with subject "whatever 2".
- when those mail are in your spool, get messages in mozilla : CRASH !
- restart... 
   you see the "whatever 1" message in your Inbox
   you see the "foo" message in the "A" folder (it's been correctly filtered) 
   but mozilla has not fetched the "whatever 2" message...
   your spool file is still there
   and Moz has left a .lock file in your spool directory !

i've reproduced it on two different linux machines with the 2001-10-11 build.

--
Hervé - http://mozillazine-fr.org/

Comment 1

[Ashley Bischoff (blog at handcoding.com)]

Hervé: Please try a talkback-enabled build:
http://ftp.mozilla.org/pub/mozilla/nightly/latest/mozilla-i686-pc-linux-gnu-sea.tar.gz

Then, if you get a crash, please post the Talkback ID here.

Comment 2

[Li Ming]

I have the same problem on the release of 0.9.5

Comment 3

[Hervé]

2 talbacks IDs, one for a test, one for confirmation :

TB36758892Y
TB36607016X


ps: movemail is seriously broken at this time ! the page
http://www.mozilla.org/mailnews/movemail/ should warn about it more strongly, i
think...

fyi, we've worked around this problem by setting pop3 servers on our linux boxes :\

Comment 4

[Ashley Bischoff (blog at handcoding.com)]

CC: [e-mail], for talkback retrieval, please.

(TB36758892Y, TB36607016X)

Crash, so: Severity -> Critical

Comment 5

[David :Bienvenu]

Here are the stack traces:

nsMovemailService::GetNewMail()
nsMovemailIncomingServer::GetNewMail()
nsMsgLocalMailFolder::GetNewMessages()

I'll look at the code - I don't see that filters are directly involved yet.

Comment 6

[Li Ming]

Hi, bienvenu,

When I don't use filter, everything will be fine. And the mail yesterday I sent
to you also showed that something related to the filter.

Hope you can figure it out. Good luck!

Ming.

Comment 7

[Li Ming]

Hi, bienvenu,

When I don't use filter, everything will be fine. And the mail yesterday I sent
to you also showed that something related to the filter.

Hope you can figure it out. Good luck!

Ming.

Comment 8

[David :Bienvenu]

Ming, your mail described a crash that is not showing up in the talkback logs
(I'm not sure they're the same crash). I checked in some bulletproofing fix last
night for the crash you were seeing (checking for a null filter list) - do you
crash with today's build?

Comment 9

[David :Bienvenu]

Attachment: bullet proofing

Comment 10

[(not reading, please use [e-mail] instead)]

Comment on attachment 54902 [details] [diff] [review]
bullet proofing

sr=sspitzer

Comment 11

[Adam D. Moss]

Comment on attachment 54902 [details] [diff] [review]
bullet proofing

r=[e-mail]

Comment 12

[David :Bienvenu]

OK, I've checked in the bulletproofing; please let me know if this still crashes
for you.

Comment 13

[Li Ming]

Hi,  Bienvenu,

Still crashes. But I debug the code today and maybe found something useful for
your further
hacking it out.

I found the direct reason for the crash is the line
delete m_newMailParser; (function nsMovemailService::GetNewMail)
in nsMovemailService.cpp
The following is the context which can help you locate that line.

                  // END
                   m_outFileStream->flush();    // try this.
                   m_newMailParser->OnStopRequest(nsnull, nsnull, NS_OK);
                   if (m_outFileStream->is_open())
                       m_outFileStream->close();                     delete
m_outFileStream;
                   m_outFileStream = 0;
                   delete m_newMailParser;
                   m_newMailParser = nsnull;


More deep analysis shows that after the function call OnStopRequest the member
variable
m_filterList and m_rootFolder contain null mRawPtr.
That explains why the program crash at delete m_newMailParser.

Now let's look at what OnStopRequest has done.
OnStopRequest-->DoneParsingFolder(aStatus)-->
PublishMsgHeader(nsnull)->ApplyFilters(&moved, msgWindow)
after ApplyFilters, m_filterList and m_rootFolder become null!

So maybe you can continue this investigation and find the bug soon.

By the way, temporary ugly fix would be commenting the line "delete m_newMailParse".

Ming.

Comment 14

[David :Bienvenu]

Attachment: proposed fix

Comment 15

[David :Bienvenu]

Ok, the fix is to use an nsCOMPtr so that the new mail msg parser is ref-counted
and cleaned up automatically. The problem was that the new mail parser was a
listener on the db, and in onstoprequest, the listener was removed, making the
ref-count go to 0 (since no one else ever had a reference to it). So I fixed
that, and also fixed a couple local variable names to conform to our variable
naming conventions (m_ is for member variables, so those two variables were
confusing the heck out of me). Navin, can I get a review? thx!

Comment 16

[Adam D. Moss]

I tried to review that, and failed.  :)

> nsCOMPtr <nsIMsgParseMailMsgState> iParseMailMsgState =
> NS_STATIC_CAST(nsIMsgParseMailMsgState*, newMailParser);

I don't quite follow this; I'm still not COMfident.  Am I right
in thinking that this holds a ref to newMailParser while it is
in scope?

If so, r=[e-mail] -- and re: the variable naming, copy-and-paste
hell I think.  Will try harder...

Comment 17

[Navin Gupta]

ya, I also don't see how mailParser is refcounted ?

Comment 18

[David :Bienvenu]

assigning it to the comptr does an addref.

Comment 19

[Navin Gupta]

ok, sorry i missed this line

+                    nsCOMPtr <nsIMsgParseMailMsgState> iParseMailMsgState = 
NS_STATIC_CAST(nsIMsgParseMailMsgState*, newMailParser);

r=naving

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 55128 [details] [diff] [review]
proposed fix

sr=sspitzer

my desire to switch to using the component manager won't work, as Init(), HandleLine() and other methods are not part of the interface.

Comment 21

[David :Bienvenu]

fix checked in.