82461 - Offline: Crash in this case sending msg with inaccessible web page

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P1)

Description

[laurel]

Using may23 commercial build

This is probably just an extended circumstance of bug 82460, but thought I'd log 
it separately since it crashes and involves certain other steps.

Steps:
1.  Go to mail window, login to (IMAP) account.
2.  Go offline, don't download.
3.  New Msg, type addressee, subject, body text. 
4.  File|Attach Web Page. Type a url (valid or bogus) in the text field, OK.  
URL appears in the attachment list.
5.  Select the url in the attachment list and press delete on your keyboard. 
Attachment entry is deleted.
6.  File|Attach Web Page. Type a (valid or bogus) url which is inacessible/not 
cached in the text field.  URL appears in attachment list.
7.  Click Send(Later).  Crashes.

Result: crash.

Comment 1

[David :Bienvenu]

this isn't offline per se - it's message | send later which is part of message
composition/sending. When you're offline, http urls just fail immediately. If
the server was down, you'd fail as well.

Comment 2

[laurel]

Attachment: talkback 30829289

Comment 3

[Jean-Francois Ducarroz]

Looks like the attachment handler get deleted but the url fetcher doesn't know about it.  I think I'll have finally to 
convert nsMsgAttachmentHandler to an true XPCOM object.

Comment 4

[scottputterman]

moving to 0.9.1.  It seems like this would be reasonably easy to hit for an
offline user.

Are steps 4-5 necessary or is just step 6 sufficient?

Comment 5

[laurel]

If you just delete the first attachment url then Send(Later) it saves to unsent 
fine, no crash, no hang.  You have to do step 6.

Comment 6

[Jean-Francois Ducarroz]

I have a fix

Comment 7

[Jean-Francois Ducarroz]

Attachment: proposed fix, v1

Comment 8

[Jean-Francois Ducarroz]

The fix is to fully initialize the url fetcher before we call OpenURI. This use to work because OpenURI was never 
calling back the listener synchroulsy. It's not true anymore.

This is a 100% safe fix.

Comment 9

[Jean-Francois Ducarroz]

I see this crash when replying to some message too, while being online!

Comment 10

[varada]

r=varada 
But it is suprising that it was working for this long if this is the case:)

Comment 11

[Jean-Francois Ducarroz]

As long OpenURI return before OnStartRequest or OnStopRequest are called, we are  
OK.

Comment 12

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

who else calls nsURLFetcher::Initialize()?

if no one else, you can remove the checks to see if fOut is non-null and open,
since you're already doing that at the start of FileURLRequest().  no need to do
it again.  then, you can fix the return type to be void.

if someone else calls nsURLFetcher::Initialize(), there might be other problems.

if fOut is null or if the stream isn't open, mOutStream, TagData and mCallBack
will not be initialized.  is that what you want?

Comment 13

[Jean-Francois Ducarroz]

There is a path in nsMsgAttachmentHandler::SnarfMsgAttachment where instead of
calling nsURLFetcher::FireURLRequest, we call messageService->DisplayMessage. In
this case, we have to call manually nsURLFetcher::Initialize.

Anyway, I can cleanup nsURLFetcher::FireURLRequest to address Seth concern. New
patch coming soon...

Comment 14

[Jean-Francois Ducarroz]

Attachment: proposed fix, v2

Comment 15

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

sr=sspitzer

Comment 16

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1

Comment 17

[Jean-Francois Ducarroz]

Fixed and checked in.

Comment 18

[Rodney Velasco]

adding lrg to cc.  please verify for gchan.

Comment 19

[lrg]

Verified in Windows with 2001060408 build
Verified in Linux with 2001060411 build
Verified in MacOS with 20010600408 build