117791 - crash when using insert symbol/character.

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Stephane Saux]

build ID 2002010103.
To reproduce:
Compose email.
insert->symbol/character. This opens a new utility window.
Send email.
compose new email. Attempt to use the previously opened insert char/symbol window.
Crash.
The insert window is probably tied to the mail compose window that opened it.
Sending the email leaves the pointer dangling.

Comment 1

[Stephane Saux]

See TB1155657Y
This may be an editor bug.

Comment 2

[Stephane Saux]

Actually I tried the editor, and closing an editor window that opened an insert
char/symbol window causes the latter to close.

Comment 3

[Sheela Ravindran]

Here is the stack trace:  

nsEditor::BeginUpdateViewBatch
[d:\builds\seamonkey\mozilla\editor\libeditor\base\nsEditor.cpp, line 4338] 
nsEditor::BeginPlaceHolderTransaction
[d:\builds\seamonkey\mozilla\editor\libeditor\base\nsEditor.cpp, line 691] 
nsAutoPlaceHolderBatch::nsAutoPlaceHolderBatch [..\base\nsEditorUtils.h] 
nsHTMLEditor::InsertHTMLWithCharsetAndContext
[d:\builds\seamonkey\mozilla\editor\libeditor\html\nsHTMLDataTransfer.cpp, line
304] 
nsHTMLEditor::InsertHTMLWithCharset
[d:\builds\seamonkey\mozilla\editor\libeditor\html\nsHTMLDataTransfer.cpp, line
275] 
nsHTMLEditor::InsertHTML
[d:\builds\seamonkey\mozilla\editor\libeditor\html\nsHTMLDataTransfer.cpp, line
263] 
nsEditorShell::InsertSource
[d:\builds\seamonkey\mozilla\editor\composer\src\nsEditorShell.cpp, line 2317] 
XPTC_InvokeByIndex
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp,
line 106] 
XPCWrappedNative::CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2011] 
XPC_WN_CallMethod
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp,
line 1267] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 834] 
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2799] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 850] 
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 925] 
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3407] 
nsJSContext::CallEventHandler
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 1014] 
GlobalWindowImpl::RunTimeout
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 3844] 
GlobalWindowImpl::TimerCallback
[d:\builds\seamonkey\mozilla\dom\src\base\nsGlobalWindow.cpp, line 4152] 
nsTimerImpl::Process [d:\builds\seamonkey\mozilla\xpcom\threads\nsTimerImpl.cpp,
line 242] 
handleMyEvent [d:\builds\seamonkey\mozilla\xpcom\threads\nsTimerImpl.cpp, line 279] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 591] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 524] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line
1072] 
nsAppShellService::Run
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 303] 
netscp6.exe + 0x167d (0x0040167d) 
netscp6.exe + 0x121c (0x0040121c) 
netscp6.exe + 0x3221 (0x00403221) 
KERNEL32.DLL + 0x17d08 (0x77e97d08) 

Comment 4

[Jean-Francois Ducarroz]

reassign to editor

Comment 5

[Kathleen :Brade]

-->cmanske for investigation

I bet we don't do the right "magic" for reassigning the window for insert char
dialog (as we do for Composer windows)

Comment 6

[Charles Manske]

Yep, we aren't doing the proper monitoring of the Insert Char dialog when in
Mail Composer.

Comment 7

[Charles Manske]

Attachment: Safest fix

This is the most conservative fix: Always close the Insert Character dialog
whenever we attempt to close the message Composer window. This will still close

the dialog even if window doesn't close, like when user clicks "Cancel" button
when prompted to save message.
More complicated fixes would be
1. Close the dialog only if result of ComposeCanClose is actually "true", thus 

window is closing.2. Don't always close dialog, but shift it to another editor
window, like we 
do in main page Composer. We would have to add an OnFocus() handler in mail
composer to do what we do in EditorOnFocus() in editor.js.
Any opinions on these options?

Comment 8

[Kathleen :Brade]

I'm ok with the patch submitted above (r=brade) but you'll need module owner
permission to check it in.  JFD or someone else may prefer a different strategy
(which would be fine too).

Comment 9

[Jean-Francois Ducarroz]

WHat would append in the case of the user send the message: The msg compose
backend will close the window without calling the JS function ComposeCanClose!
Also, I am worry in the case we cache the compose window instead of destroying
in (when reclycling the msg compose window), we should close the insert char
window as well in that case. Maybe a better solution would be for editor to have
a "cleanup" function (in C++ or JS) I can call in various point in msg compose.

Comment 10

[Charles Manske]

Supplying a general EditorCleanup() method sounds like a good idea.

Comment 11

[Charles Manske]

Attachment: Updated patch

With this patch, the InsertCharacter dialog will be closed only if
ComposeCanClose() is returning "true" and the messenger window will be closed.
I'll let Ducarroz add a patch to include calling EditorCleanup() at other
places in the messenger code, as he suggested.

Comment 12

[Jean-Francois Ducarroz]

Attachment: News patch for MsgComposeCommands.js

This patch is only for the mailnews side of the fix. I don't think we need to
apply the modification cmanske proposed for MsgComposeCommands.js in the
previous patch.

Comment 13

[Charles Manske]

Attachment: Updated patch

This patch contains exactly the changes as in patch 67440, plus:
1. It turns out it is important to include the 'onfocus' handler exactly as
done 
in Web Composer else when you change focus to/from a Message Compose window,
the
Insert Character dialog will loose it's associated composer. This is done
by simply setting onfocus="EditorOnFocus()" in messengercompose.xul
2. EditorCleanup() simply calls SwitchInsertCharToAnotherEditorOrClose()
exactly
as is done in Web Composer so the behavior is the same. When a mail window is 
closed, the Insert Character dialog doesn't need to be closed if there's
another
Composer to take ownership.

Comment 14

[Charles Manske]

Comment on attachment 67440 [details] [diff] [review]
News patch for MsgComposeCommands.js

r=cmanske

Comment 15

[Jean-Francois Ducarroz]

Comment on attachment 67528 [details] [diff] [review]
Updated patch

R=ducarroz for the mailnews part of it.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 67528 [details] [diff] [review]
Updated patch

sr=dveditz

Comment 17

[Kathleen :Brade]

r=brade

Comment 18

[Charles Manske]

checked in

Comment 19

[sujay]

Stephane,  does this work for you now?

Comment 20

[Stephane Saux]

yes. It works.
If I have one complaint is that you can "lose" the insert window:

Open a compose window, open the insert char dialog, use it.

Close compose window. The insert dialog stays. Bury it behind other windows.

Open new compose window.  Do insert->char symb.

There's no feedback. The window is still buried. It would be best if the focus
was given to the insert window.

Comment 21

[sujay]

marking verified.

Stephane, please file a bug for the new issue you brought up...thanks.