57851 - Crash doing Send Later

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[scottputterman]

I composed a message to myself and did Send Later.  I get the following crash,
talkback id 19695499. Ninoschka saw the same crash when her Unsent Messages
folder was in a imported Eudora account. My Unsent Messages folder is in my
Local Folders.


0x172e9a6c
nsMsgAttachmentHandler::UrlExit
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgAttachmentHandler.cpp,
line 1016]
FetcherURLDoneCallback
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgAttachmentHandler.cpp,
line 445]
nsURLFetcher::OnStopRequest
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsURLFetcher.cpp, line 325]
nsDocumentOpenInfo::OnStopRequest
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 269]
nsFileChannel::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\protocol\file\src\nsFileChannel.cpp, line 649]
nsOnStopRequestEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 302]
nsStreamListenerEvent::HandlePLEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 106]
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 581]
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 517]
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line
1051]

Comment 1

[scottputterman]

nominating for rtm.

Comment 2

[scottputterman]

I forgot to mention that before crashing I was shown the dialog that says the
send was successful but copying to the Sent message folder failed.

Comment 3

[scottputterman]

It's crashing because in FetcherURLDoneCallback,

return ma->UrlExit(aStatus, aMsg);

ma has been destroyed already.

Now that I've crashed multiple times, here are the steps:

1. Open a Compose window and compose a message
2. Do Send Later
3. You will see a dialog that says the Send succeeded but coyping to the sent
folder failed. Hit OK to return to the compose window.
4. Crash.

Comment 4

[scottputterman]

I think we should look into this. Adding [rtm need info]

Comment 5

[Jean-Francois Ducarroz]

accepting...

Comment 6

[scottputterman]

I have an idea of what's going on. We can't find the Unsent Messages folder
because we are looking for

"nobody@local folders/Unsent Messages"

whereas we create the folders as

"nobody@local%20folders/Unsent%20Messages"

so we need to make sure the folder we look up for Unsent Messages is escaped.
This probably regressed when we changed the local folder code to use escaped uris'.

I haven't yet figured out why Drafts still work since we find the Drafts folder
the same way we find the Unsent Messages folder.

Comment 7

[scottputterman]

this is weird. My profile at home works even though the whole escaping issue is
still a problem. I was seeing it fail at work when trying to get a server but I
don't see how that's possible since getting a server always returns NS_OK and it
would only fail if it returned a failure code. I'll have to debug this some more
at work to figure out what's going on.

Comment 8

[scottputterman]

I'm still seeing different behavior at home vs work, but I was wrong about
something in my previous statement.

If the call to GetServer in LocateMessageFolder returns null, then it will
return failure. That's what I was seeing happen at work.  It couldn't find a
server for the folder. I don't know why I'm seeing different results now.

Comment 9

[Jean-Francois Ducarroz]

maybe is due to the fact than you are using a fast PC at home and a "slow" one
at work.

Comment 10

[Jean-Francois Ducarroz]

We have apparently two problem here:

1) GetServer failed for some reason
2) When GetServer failed, we crash later because a callback use a deleted object

Personally, I am not able to reproduce the first problem. But If I force the
return value from GetServer to -1 in LocalMessageFolder, I am able to reproduce
the second problem, the crash. It crash in nsMsgComposeAndSend::Fail because
this object has already been deleted. The callback comes from
nsMsgAttachmentHandler::UrlExit.

Rich, any idea? how can we avoid the crash? looks like we need to addref the
object somewhere!

Comment 11

[rhp (gone)]

Let me dig into this a bit on my machine.

- rhp

Comment 12

[rhp (gone)]

Ok, I think I have this one fixed. Its a problem where the failure routines 
were being called twice when the locate of the copy folder failed. That was 
wrong and removing those 2 lines has it working properly.

I will post the fix to this bug.

- rhp

Comment 13

[rhp (gone)]

Attachment: Fix for bug

Comment 14

[Jean-Francois Ducarroz]

I have some problem with you patch. Now if something went wrong while sending
attachment, the compose window doesn't come back anymore, therefore it's a data
loose!

I did the following test to simulate an error while sending attachment:

1) compose a message to yourself
2) attach a file
3) from your favorite OS file manager, rename or move the file you just have
attached
4) Send the message

sure, now, the function GatherAttachments won't be able to find the file and
therefore will abort and return an error. But in this case, the compose window
should come back, it's doesn't anymore with your patch.

BTW, I found another small error in the function nsMsgComposeAndSend::Fail. It's
possible that the failure_code we are trying to report to the user doesn't have
a string for it (it's the case with any generic errors like NS_ERROR_FAILURE or
NS_ERROR_UNEXPECTED). If we don't have a string for it, no alert is shown. The
fix is to do (like I did already else where):

if (NS_FAILED(nsMsgDisplayMessageByID(aPrompt, failure_code)))
   nsMsgDisplayMessageByID(aPrompt, NS_ERROR_SEND_FAILED);

Comment 15

[rhp (gone)]

Ok...I see that...this is kind of hairy and will be an ugly fix, but I'm on it.

- rhp

Comment 16

[rhp (gone)]

Ok, a new fix is in hand. I will attach :-)

- rhp

Comment 17

[rhp (gone)]

Attachment: Latest attachment

Comment 18

[scottputterman]

It turns out this may not be too big of an issue. The problem is occuring
because my Unsent Message folder has a uri of Local Folders but the account is
called Local Mail. This is because I have an old profile at work. Seth, did we
ever ship with a Local Mail profile?

I think escaping the uri would also solve the problem.

Comment 19

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

"Local Mail" became "Local Folders" around Jan 6.

PR1 was approx 4/5/99

so we are safe.  we should still escape the uri.

Comment 20

[scottputterman]

thanks Seth. I don't think we should worry about fixing the Send Later part of
this bug then. It's probably worthwhile to fix the crash given that we've shown
one way it could occur. 

Comment 21

[rhp (gone)]

So what should I do to get this into the tree...anyone want to mark it rtm+?

Comment 22

[scottputterman]

I've seen ducarroz review it, has anyone super reviewed it yet?

Comment 23

[Scott MacGregor]

So the fix is to just NOT null out mMsgSend? sr=mscott

Comment 24

[scottputterman]

marking rtm+ since mscott added the sr.

Comment 25

[selmer (gone)]

Are both attachments required for the fix?  I get the impression that the first 
patch fixes the crash and the second patch addresses something else.  MScott 
claims only the second patch is required to fix the crash.  Please let us know. 
 If the first patch is required, please get the sr= for that too.  Thanks.

Comment 26

[scottputterman]

It's the second patch. the one that says "latest".

Comment 27

[Phil Peterson]

This bug is in candidate limbo.  We will reconsider this fix once we have a 
candidate in hand, but we can't take this fix before then.

Comment 28

[selmer (gone)]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Comment 29

[rhp (gone)]

The fix is in! Both trees...no hands :-)

Comment 30

[Johnny Stenback (:jst)]

Verified on Win2k and Linux.

Comment 31

[Peter Trudelle]

I still crash after Send Later using the 10-31-14 MN6 candidate build on Win98.
I get the 'Send succeeded but copy to sent folder failed' message, and crash
when I cancel it.  I have a 'nobody on Local Mail' entry, but it has no folders
in it.

Comment 32

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

trudelle "nobody at Local Mail" indicates an old (pre PR1) profile.

putterman was seeing similar problems.

I think if you exit 6.0, and change the the hostname it should fix it for you.

change

user_pref("mail.server.server2.hostname", "Local Mail");
to
user_pref("mail.server.server2.hostname", "Local Folders");

can you try that?