movemail + filters = crash

RESOLVED FIXED

Status

MailNews Core
Movemail
--
critical
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: Hervé, Assigned: Bienvenu)

Tracking

({crash})

Trunk
x86
Linux
crash

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

1.01 KB, patch
Adam D. Moss
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
5.43 KB, patch
(not reading, please use seth@sspitzer.org instead)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
((((( 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/

Updated

16 years ago
QA Contact: esther → laurel
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.
Keywords: crash
Whiteboard: crash, stackwanted

Comment 2

16 years ago
I have the same problem on the release of 0.9.5
(Reporter)

Comment 3

16 years ago
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 :\
CC: stephend@netscape.com, for talkback retrieval, please.

(TB36758892Y, TB36607016X)

Crash, so: Severity -> Critical
Severity: normal → critical
Whiteboard: crash, stackwanted
(Assignee)

Comment 5

16 years ago
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

16 years ago
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

16 years ago
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.
(Assignee)

Comment 8

16 years ago
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 9

16 years ago
Created attachment 54902 [details] [diff] [review]
bullet proofing
Comment on attachment 54902 [details] [diff] [review]
bullet proofing

sr=sspitzer
Attachment #54902 - Flags: superreview+

Comment 11

16 years ago
Comment on attachment 54902 [details] [diff] [review]
bullet proofing

r=adam@gimp.org
Attachment #54902 - Flags: review+
(Assignee)

Comment 12

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

Comment 13

16 years ago
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.
(Assignee)

Comment 14

16 years ago
Created attachment 55128 [details] [diff] [review]
proposed fix
(Assignee)

Comment 15

16 years ago
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!
Assignee: adam → bienvenu

Comment 16

16 years ago
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=adam@gimp.org -- and re: the variable naming, copy-and-paste
hell I think.  Will try harder...



Comment 17

16 years ago
ya, I also don't see how mailParser is refcounted ?
(Assignee)

Comment 18

16 years ago
assigning it to the comptr does an addref.

Comment 19

16 years ago
ok, sorry i missed this line

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

r=naving
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.
Attachment #55128 - Flags: superreview+
(Assignee)

Comment 21

16 years ago
fix checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Updated

16 years ago
QA Contact: laurel → sspitzer
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.