77053 - Send failing when attaching above web page

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P2)

Description

[Sheela Ravindran]

Buildid:  2001041908

Open a compose window 
From the menu item select file|attach web page
try to send

Results in an wierd error and upon okaying the error displays Failed to Send 
message.

Comment 1

[Sheela Ravindran]

change qa contact

Suresh can you attach the stack trace.  thanks. Suresh crashed while attaching 
this page.  

Comment 2

[Suresh]

Here is that weird msg. "There was a problem including the file %.200s in the 
message. Would you like to continue
sending the message without this file?" 

On Canceling this dialog "sending of mag failed" pops up. On Ok'ing it crashes.

Stack trace: (from mozilla windows build from yesterday)
nsQueryInterface::operator()(const nsID & {...}, void * * 0x0012f684) line 32 + 
20 bytes
nsCOMPtr<nsIMsgSend>::assign_from_helper(const nsCOMPtr_helper & {...}, const 
nsID & {...}) line 970 + 18 bytes
nsCOMPtr<nsIMsgSend>::nsCOMPtr<nsIMsgSend>(const nsQueryInterface & {...}) line 
564
nsCOMPtr<nsIMsgSend>::Assert_NoQueryNeeded() line 499
nsCOMPtr<nsIMsgSend>::operator=(nsIMsgSend * 0x00000000) line 584
nsMsgAttachmentHandler::SetMimeDeliveryState(nsIMsgSend * 0x00000000) line 1130
nsMsgAttachmentHandler::UrlExit(unsigned int 2152398850, const unsigned short * 
0x00000000) line 1080
FetcherURLDoneCallback(unsigned int 2152398850, const char * 0x07fb53e0, const 
char * 0x00000000, int 0, const unsigned short * 0x00000000, void * 0x07fac2e4) 
line 440 + 16 bytes
nsURLFetcher::OnStopRequest(nsURLFetcher * const 0x07fadcb4, nsIRequest * 
0x07fad8b0, nsISupports * 0x00000000, unsigned int 2152398850) line 328 + 40 
bytes
nsURLFetcher::OnStateChange(nsURLFetcher * const 0x07fadcc0, nsIWebProgress * 
0x07fadc04, nsIRequest * 0x07fad8b0, int 65552, unsigned int 2152398850) line 
411
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x07fadc04, nsIRequest * 
0x07fad8b0, int 65552, unsigned int 2152398850) line 1325
nsDocLoaderImpl::doStopURLLoad(nsIRequest * 0x07fad8b0, unsigned int 2152398850) 
line 721
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x07fadbf4, nsIRequest * 
0x07fad8b0, nsISupports * 0x00000000, unsigned int 2152398850) line 585
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x07fadb80, nsIRequest * 
0x07fad8b0, nsISupports * 0x00000000, unsigned int 2152398850) line 491 + 44 
bytes
nsHTTPChannel::ResponseCompleted(nsIStreamListener * 0x00000000, unsigned int 
2152398850) line 2386
nsHTTPServerListener::OnStopRequest(nsHTTPServerListener * const 0x07fb7820, 
nsIRequest * 0x07fb7420, nsISupports * 0x07fad8b0, unsigned int 2152398850) line 
709
nsOnStopRequestEvent::HandleEvent() line 159
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x07fb79e4) line 64
PL_HandleEvent(PLEvent * 0x07fb79e4) line 588 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00e6acd0) line 518 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x03a80c22, unsigned int 49420, unsigned int 0, 
long 15117520) line 1069 + 9 bytes
USER32! 77e71820()
00e6acd0()

Comment 3

[Suresh]

fyi: I could duplicate this in today's debug build. 

Comment 4

[Suresh]

Ducarroz, I think the culprit is SetMimeDeliveryState(nsnull) in 
Attachmenthandler code (which btw was recently added to that file) :)

Comment 5

[varada]

Using today's release as well as debug build I see the send failing but I dont
see the crash and I think you will notice the same thing if you use the
following sites - www.tci.com, yahoo.org. All these pages use redirection to
another site and I think that is what is causing the msg compose some
consternation when it is assembling the attachments. 

Comment 6

[Jean-Francois Ducarroz]

Suresh, can you tell me which one of the SetMimeDeliveryState(nsnull)you think
is the culprit?

Comment 7

[Suresh]

Ducarroz, For this bug it fails in line 1079 (in nsMsgAttachmentHandler).  There
are few other places where SetMimeDeliveryState(nsnull) is called. 

The one thing I don't understand is why we are encountering this situation, i.e
why we are throwing up that weird dialog.  If re-direction is the reason (as
Varada mentioned) then I'm sure there will be tons of web pages with re-direction.

Comment 8

[scottputterman]

marking nsbeta1+.  BTW, I crashed when cancelling the send and then closing the
window.

Comment 9

[Jean-Francois Ducarroz]

Cancel??? Do you have the progress dialog turn on?

Comment 10

[Jean-Francois Ducarroz]

BTW, accepting and workibg on it...

Comment 11

[Sheela Ravindran]

No, Progress dialog was not turned on in my pref.js

Comment 12

[Jean-Francois Ducarroz]

We have two problem here:

1) URL fetcher somehow now failed with redirection, maybe a necko regression!

2) We crash when the user cancel the send operation. This because, when 
urlfetcher failed, it call back the attachmenthandler which call the send 
listener which tell compose to remove the reference on msgSend. Then the 
attachmenthandler remove its own reference on msgsend which cause msgsend to be 
delete which cause the attachmenthandler to goes away. And sure, when the 
attachment handler continue it's business, we crash! The fix is not let the 
listener breaking the msgsend reference, think we don't need to do anymore since  
the listener use a weakreference to the compose. it's a left over off the old 
code to break a circular reference between compose, sendlistener and msgsend.

Comment 13

[Jean-Francois Ducarroz]

Attachment: Proposed fix for the crash part of this bug

Comment 14

[Jean-Francois Ducarroz]

Attachment: New patch that fix both problems: the crash and the end failure

Comment 15

[Jean-Francois Ducarroz]

We weren't correctly supporting redirection. I had to add some code to detect a
redirection event and in this case avoid to abort the download when necko stop
the original request.

Comment 16

[varada]

reviewed and tested the patch.
r=varada

Comment 17

[David :Bienvenu]

two things - why is that method [nsocript]? It looks perfectly scriptable to me,
unless I'm missing something.

And, should you reset mRedirection to PR_FALSE after handling the redirection?
In case the user really does cancel later on.

Comment 18

[Jean-Francois Ducarroz]

I don't add any new method to the interface but rather remove it at I don't need
it and has it's very dangeroug to play with this attribute!

mRedirection is reseted when we receive a onStart message.

Comment 19

[David :Bienvenu]

sorry, I was talking about  [nocript] nsIMsgSend getMessageSend();

which I realize you didn't change, but seemed related, and I was just asking.
Better idl for this would be readonly attribute nsIMsgSend messageSend

sr=bienvenu

Comment 20

[Jean-Francois Ducarroz]

Fixed and checked in

Comment 21

[Sheela Ravindran]

buildid:  2001-05-07-06 
Unfortunately, this bug still exists on the above build.  Sorry reopening.

Comment 22

[Jean-Francois Ducarroz]

accepting (again :-( )

Comment 23

[Jean-Francois Ducarroz]

ok, the redirection handling has been rewrote by rpots on Appr 27. This cause my
redirection code detection stop working! I'll rewrite it on my side...

Comment 24

[Jean-Francois Ducarroz]

Attachment: patch v3

Comment 25

[Jean-Francois Ducarroz]

r=varada

Comment 26

[Scott MacGregor]

sr=mscott

Comment 27

[David :Bienvenu]

man, scott's always beating me to it! :-)

Comment 28

[Jean-Francois Ducarroz]

Fixed and checked in

Comment 29

[Sheela Ravindran]

verified able to attach web pages that have redirection. 
win98-2001051406
linux-2001051305
mac-2001051408