144735 - Sending a signed e-mail often leads to crash. [@nsMsgMailSession::GetTopmostMsgWindow]

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Lapo Luchini]

1.open a new message
2.write it
3.enable digital signature
4.press send
5.either it is sent or Mozilla suddenly crashes and you are presented with
talktabk dialog

Unfortunately I just upgraded to 1.0rc2 and my 10 crashes of this king with
1.0rc1 are lost (well, they all were from '[e-mail]' and contained "sending
a signed message" as description).

Latest, with 1.0rc2 is: TB6284031Z

Comment 1

[Bob Clary [:bc] (inactive)]

signature: nsMsgMailSession::GetTopmostMsgWindow 3eea5b7a

Product ID  Gecko1.0
Build ID 2002051008
Trigger Time 2002-05-14 14:47:09
Platform Win32
Operating System Windows NT 5.1 build 2600
Module msgbase.dll
URL visited
User Comments sending a signed message
Trigger Reason Access violation
Source File Name d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgMailSession.cpp
Trigger Line No. 408
Stack Trace
nsMsgMailSession::GetTopmostMsgWindow
[d:\builds\seamonkey\mozilla\mailnews\base\src\nsMsgMailSession.cpp, line 408]
nsMsgComposeAndSend::GetNotificationCallbacks
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgSend.cpp, line 271]
nsMsgComposeAndSend::DeliverFileAsMail
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgSend.cpp, line 3245]
nsMsgComposeAndSend::DeliverMessage
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgSend.cpp, line 3106]
nsMsgComposeAndSend::GatherMimeAttachments
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgSend.cpp, line 1120]
nsMsgAttachmentHandler::UrlExit
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgAttachmentHandler.cpp,
line 1174]
FetcherURLDoneCallback
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsMsgAttachmentHandler.cpp,
line 476]
nsURLFetcher::OnStopRequest
[d:\builds\seamonkey\mozilla\mailnews\compose\src\nsURLFetcher.cpp, line 325]
nsDocumentOpenInfo::OnStopRequest
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 255]
nsFileChannel::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\protocol\file\src\nsFileChannel.cpp, line 522]
nsOnStopRequestEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsRequestObserverProxy.cpp, line 213]
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 597]
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 530]
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line
1078]
USER32.dll + 0x3c076 (0x77d4c076)
USER32.dll + 0x3c076 (0x77d4c076)
_except_handler3()
kernel32.dll + 0x3bb86 (0x77e7bb86) 

Comment 2

[timeless]

.

Comment 3

[nhottanscp]

nsbeta1

Comment 4

[nhottanscp]

I have not been able to reproduce this using 5/14 commercial branch.
But I noticed that some of the calls I use in the function may return NS_OK even
the output value is not valid. Probably I should also check if the returned
objects are null.

http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgMailSession.cpp#394

394         rv = windowEnum->GetNext(getter_AddRefs(windowSupports));
395         NS_ENSURE_SUCCESS(rv, rv);
396 
397         topMostWindow = do_QueryInterface(windowSupports, &rv);
398         NS_ENSURE_SUCCESS(rv, rv);
399 
400         rv = topMostWindow->GetDocument(getter_AddRefs(domDocument));
401         NS_ENSURE_SUCCESS(rv, rv);
402 
403         rv = domDocument->GetDocumentElement(getter_AddRefs(domElement));
404         NS_ENSURE_SUCCESS(rv, rv);
405 
406         rv = domElement->GetAttribute(NS_LITERAL_STRING("windowtype"),
windowType);
407         NS_ENSURE_SUCCESS(rv, rv);

Comment 5

[Jean-Francois Ducarroz]

I am not able to reproduce the problem too. But looks like
  rv = domDocument->GetDocumentElement(getter_AddRefs(domElement));
return a null domElement despite it doesn't return an error!

Reporter, can you tell use which other windows are open and in which order. That
would help use to reproduce the problem.

Comment 6

[Frank Tang]

crash bug. [adt1 rtm]

Comment 7

[Lapo Luchini]

It seems that with rc2 it is less frequent, or just the occurrence.
Usually I have a single browser windows with many tabs (10 or more) and a single
mailnews windows, and the compose mail windows of course.
I will try more signed message to try catching some more occurrences.

Comment 8

[Lapo Luchini]

Said, done.
I have another crash report: TB6284031Z

Same open windows (also described in the report itself).

More infos: I use a 2048bit cert, but the first crashes in 1.0rc1 were using the
old 1024bit cert if I remember correctly.

Comment 9

[nhottanscp]

TB6284031Z also crashes in the same place.
I still cannot reproduce but I believe that the extra error check will prevent
the crash.

Comment 10

[nhottanscp]

Attachment: Add ponter checks after the dom function calls.

Comment 11

[Jean-Francois Ducarroz]

Comment on attachment 83809 [details] [diff] [review]
Add ponter checks after the dom function calls.

r=ducarroz

Comment 12

[David :Bienvenu]

Comment on attachment 83809 [details] [diff] [review]
Add ponter checks after the dom function calls.

sr=bienvenu - my guess is that its the GetDocument call that's succeeding but
returning null - I have a vague memory of seeing that before. I know some of
the [e-mail] aren't going to like this fix, but I'd rather not
crash.

Comment 13

[esther]

I can't reproduce this either with 20020512pr1 build. Reporter can you verify
this works for you when fixed.

Comment 14

[nhottanscp]

checked in to the trunk

Reporter, please try tomorrow's trunk build.

Comment 15

[Lapo Luchini]

Sent some 5 mails with 2002051606, but too few to say "ok it is now tested",
more testing this evening.

Comment 16

[Lapo Luchini]

Got a new crash signing email with 2002051606 =(
Still waiting for talkback.netscape.com to accept the ticket...

Comment 17

[Lapo Luchini]

Finally... it's TB6467193W
Reopening this =(
Or maybe that snapshot was too old to have the patch?
Installing the new snapshot...

Comment 18

[nhottanscp]

This is strange, the same line number (TB6284031Z and TB6467193W) after adding
few lines.

Comment 19

[Lapo Luchini]

Definitely to be reopened, same error in today's snap (2002051906).
TB6481255Q

Comment 20

[nhottanscp]

Since I put a null check already, it could be the pointer is not null but invalid.
My original change was written for message viewing to identify the top window.
But this is called for message send.

The caller of topmostwidnow is 
nsMsgComposeAndSend::GetNotificationCallbacks

266 nsresult
nsMsgComposeAndSend::GetNotificationCallbacks(nsIInterfaceRequestor** aCallbacks)
267 {
268 // TODO: stop using mail3pane window!
269   nsCOMPtr<nsIMsgWindow> msgWindow;
270   nsCOMPtr<nsIMsgMailSession> mailSession(do_GetService(kMsgMailSessionCID));
271   mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow));
272   if (msgWindow) {
273     nsCOMPtr<nsIDocShell> docShell;
274     msgWindow->GetRootDocShell(getter_AddRefs(docShell));
275     nsCOMPtr<nsIInterfaceRequestor> ir(do_QueryInterface(docShell));
276     if (ir) {
277       *aCallbacks = ir;
278       NS_ADDREF(*aCallbacks);
279       return NS_OK;
280     }
281   }
282   return NS_ERROR_FAILURE;  
283 }

I want to know why the code needs topmost window in what situation.
cc to mscott
Scott, why does the code need topmost window? Is this anything to do with
progress dialog?

Comment 21

[nhottanscp]

>My original change was written for message viewing to identify the top window.
Actually, that was for reply/quote to find the original message window.

Comment 22

[nhottanscp]

Looking at the talkback data for TB6481255Q, it indicates the branch build is used.

Gecko1.0      mozilla.exe 1.0.0.0
Netscape Win32 (2002051909)
Trigger Time: 05/20/2002 14:27:54
Set Bug ID Delete This Incident
Back to List No Previous Incident Incident ID: 6481255


For trunk build, it is supposed to have this line instead of "Geko1.0 ...".
MozillaTrunk    Mozilla.exe 0.0.0.0

The patch has not been landed on 1.0.0 branch yet. Please try again using the
trunk build, thanks.

Comment 23

[scottputterman]

adding adt1.0.0+.  Please get drivers approval and then check into the 1.0 branch.

Comment 24

[Lapo Luchini]

TB6781047K TB6780851Z
I hope this only means that this fix didn't make into RC3 ^_^

Comment 25

[esther]

Lapo, your latest talkback reports you are not using the trunk builds.  This was
fixed on a trunk build and is a candidate for a branch build but has not landed
on the branch yet.  I can not verify this bug and resolve it as verified because
I don't have a reproducible case.  If you could take a trunk build (the nightly
builds are trunk builds) instead of a branch build and try this that would be
great.  

Comment 26

[Lapo Luchini]

I already verified it on trunk and it's OK, only I thought that it landed before
rc3, just it isn't.. no problem ^_^

Comment 27

[scottputterman]

changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 83809 [details] [diff] [review]
Add ponter checks after the dom function calls.

please check into the 1.0.1 branch ASAP. once landed remove the 
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 29

[nhottanscp]

checked in to 1.0.1 branch

Comment 30

[Lapo Luchini]

It's a pity it disdn't made into 1.0 code (TB7096797G)... it's the version the
majority of the users will use for a long time.
Well, I guess a "not checked for long" patch was worse than nothing for
1.0RC3->1.0  transition...

Comment 31

[esther]

I still can't verify this because I don't have a reproducible case.  Lapo,
thanks for all your help with this, but I can't tell by your comments if you
tried this on a branch build that includes the fix.  If so, could you comment in
the bug that you have verified it was fixed.  Thanks again, Esther

Comment 32

[esther]

Lapo emailed me and indicated that while using the branch builds last month
(June) the problem has not been seen.  The the fix went into the branch on 5-30.
 I will verify this now since the reporter is no longer seeing the problem.  We
will reopen if it should come back.