Ctrl+W and File|Close stop working after saving message, with "Error: gMsgCompose.getExternalSendListener is not a function"

VERIFIED FIXED

Status

MailNews Core
Composition
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: wsmwk, Assigned: Bienvenu)

Tracking

({regression})

Trunk
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Seen on version 3 alpha 1 (20070304) in safe mode.
On TB 2 version 2.0pre (20070305) ctrl-w and then save causes crash - Bug 372705 

reproducible: always

1. create blank message (don't need anything in address, body or subject)
2. ctrl-w
3. save, cancel or "don't save" in response to dialog
4. ctrl-w

actual results:
no dialog in response to ctrl-w of step 4.

"save" results in js error, but does save the draft
Error: gMsgCompose.getExternalSendListener is not a function
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1056

only way to get rid of compose window after initial ctrl-w is send the message or click X.
(Assignee)

Comment 1

11 years ago
but if you have a blank message, cntrl-w shouldn't even bring up a dialog. Do you have a sig file or something?

And after step 3, the window should close, so a subsequent ctrl-w shouldn't do anything...
(Reporter)

Comment 2

11 years ago
happens same in two imap accounts - one has no signature, the other does.
(Assignee)

Comment 3

11 years ago
I tried this with a nightly build from this morning, and it worked fine.

Wayne, do you have any extensions installed?
(Assignee)

Comment 4

11 years ago
confirming to get on the radar
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

11 years ago
updated by one day to latest nightly version 3 alpha 1 (20070305), safe mode startup, test profile, one extension in the test profile MR Tech local (plus talkback of course). and no js errors after startup

more interesting behavior - ctrl-s, then attemting ctrl-w shows same behavior, ctrl-w doesn't work
(Assignee)

Comment 6

11 years ago
Created attachment 257386 [details] [diff] [review]
proposed fix for crash
[Checkin: Comment 44 & 47]

Scott says he crashes with a null editor. No idea why I don't, but here's a patch.
Assignee: mscott → bienvenu
Status: NEW → ASSIGNED
Attachment #257386 - Flags: superreview?(mscott)

Updated

11 years ago
Attachment #257386 - Flags: superreview?(mscott) → superreview+
(Reporter)

Comment 7

11 years ago
file | save as (draft or template) also breaks ctrl-w 
(Reporter)

Comment 8

11 years ago
and file|close
Summary: ctrl-w stops working → ctrl-w and file|close stop working
(Reporter)

Comment 9

11 years ago
regressed between 20070224 and 20070228, so caused by bug 312275
Blocks: 312275
Summary: ctrl-w and file|close stop working → ctrl-w and file|close stop working after saving message with error gMsgCompose.getExternalSendListener is not a function
(Reporter)

Comment 10

11 years ago
still get the js error and other symptoms on trunk from this tinderbox build
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tinderbox-builds/tb-win32-tbox-trunk/
 thunderbird-3.0a1.en-US.win32.installer.exe    05-Mar-2007 18:48  6.7M 

didn't test 2.0 for the crasher

Comment 11

11 years ago
This also affects SM (see bug 372168).  I hit this anytime I try to close a message window (except for clicking the "x").

attachment 256449 [details] [diff] [review] removed getExternalSendListener() from the nsIMsgCompose interface (and removed the method implementation), but it's still being called from MsgComposeCommands.js

http://lxr.mozilla.org/seamonkey/search?string=getExternalSendListener

Updated

11 years ago
Blocks: 372168
(Assignee)

Comment 12

11 years ago
Andrew, that's true of the trunk, but not the 2.0 branch...

Comment 13

11 years ago
AFAICT from comments, there is a crash bug and a JS exception bug... perhaps they're not related? (and from the summary and comment 0, this is the JS exception bug although a crash patch is here).  I'm not seeing a crash on branch or trunk, and I don't see the exception on the branch.
I think comment 11 and comment 13 have the truth of it, so far.
(Reporter)

Comment 15

11 years ago
correct - I just wanted to ensure anyone reading the bug would know the crash fix didn't handle the js error. And I don't think anyone determined the origin of the crash regression.
(Assignee)

Comment 16

11 years ago
I do have a trunk patch for this - just need to test it. It's not related to the crash, and it was caused by  bug 312275
(Assignee)

Comment 17

11 years ago
Created attachment 257515 [details] [diff] [review]
(Bv1-TB) fix

the same fix will work for SM, I think.

I verified that with this patch, nsMsgCompose::OnSendNotPerformed is called.

The js assertion about sMsgComposeService still happens - that one baffles me.
Attachment #257515 - Flags: superreview?(mscott)

Updated

11 years ago
Attachment #257515 - Flags: superreview?(mscott) → superreview+
Summary: ctrl-w and file|close stop working after saving message with error gMsgCompose.getExternalSendListener is not a function → ctrl+w and file|close stop working after saving message with error gMsgCompose.getExternalSendListener is not a function
Created attachment 257602 [details] [diff] [review]
(Cv1-SM) <MsgComposeCommands.js>

Port TB patch, per comment 17.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070306 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Fixes both bug 372168 and bug 372216, which I'll mark as duplicates.
Attachment #257602 - Flags: superreview?(neil)
Attachment #257602 - Flags: review?(neil)
Duplicate of this bug: 372216
No longer blocks: 372168
Duplicate of this bug: 372168
(In reply to comment #17)
> The js assertion about sMsgComposeService still happens - that one baffles me.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a3pre) Gecko/20070306 SeaMonkey/1.5a] (nightly) (W2Ksp4)

I see this "new" (= after applying the JS patch) error with SM too.

1. Ctrl+M to open a first Compose window.
2. Ctrl+M to open a second Compose window.
3. Alt+F4 to close either of the window.
4. Alt+F4 to close the remaining window.
4r.
{{
Error: sMsgComposeService is not defined
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1085
}}

This latter bug may have been around for some time, and may not be related to the current bug !?
...
Oh well, this looks much like bug 160942.!.
Component: Message Compose Window → MailNews: Composition
Product: Thunderbird → Core

Comment 22

11 years ago
Comment on attachment 257602 [details] [diff] [review]
(Cv1-SM) <MsgComposeCommands.js>

As I mentioned in bug 312275 comment 17 this would be easier if nsIMsgCompose inherited from nsIMsgSendListener ...

Comment 23

11 years ago
Comment on attachment 257602 [details] [diff] [review]
(Cv1-SM) <MsgComposeCommands.js>

>         if (gMsgCompose)
>         {
>+          var externalListener = gMsgCompose.QueryInterface(Components.interfaces.nsIMsgSendListener);
>           if (externalListener)
>+            externalListener.onSendNotPerformed(null, Components.results.NS_ERROR_ABORT);
>         }
This isn't the correct way to test for an interface (although gMsgCompose always implements nsIMsgSendListener so it doesn't make any difference). Instead you should write
if (gMsgCompose instanceof Components.interfaces.nsIMsgSendListener)
  gMsgCompose.onSendNotPerformed(null, Components.results.NS_ERROR_ABORT);
Attachment #257602 - Flags: superreview?(neil)
Attachment #257602 - Flags: superreview-
Attachment #257602 - Flags: review?(neil)
Attachment #257602 - Flags: review+
(In reply to comment #22)
> (From update of attachment 257602 [details] [diff] [review])
> As I mentioned in bug 312275 comment 17 this would be easier if nsIMsgCompose
> inherited from nsIMsgSendListener ...

(I wouldn't know how to do that: I'll wait and see for David...)

(In reply to comment #23)
> Instead you should write
> if (gMsgCompose instanceof Components.interfaces.nsIMsgSendListener)
>   gMsgCompose.onSendNotPerformed(null, Components.results.NS_ERROR_ABORT);

David, do you agree for TB too !?

Comment 25

11 years ago
Instead of getting the externalSendListener from the gMsgCompose object and sending onSendNotPerfomed directly like:
var externalListener = gMsgCompose.getExternalSendListener();
if (externalListener)
{
    externalListener.onSendNotPerformed(null, Components.results.NS_ERROR_ABORT);
}

you can just call:
gMsgCompose.OnSendNotPerformed(messageId, Components.results.NS_ERROR_ABORT);

This will notify all registered nsIMsgSendListeners, exactly the same way you do in JavaScript, but then iterating over all listeners. The implementation of nsIMsgCompose has new methods for registering and removing nsIMsgSendListeners: AddMsgSendListener and RemoveMsgSendListener.

Do try to send the messageId as the first parameter of this call, since listeners may be interested in that information.

Oh, I see now that I'm saying the same as Neil, but then more verbose... sorry.

I think it's already implemented as part of bug 312275 and nsMsgCompose does implement nsIMsgSendListener. I can't see this reflected in the idl file though, so that might be the only thing that has to be changed? Maybe nsMsgCompose.h too, to delete the NS_DECL_NSIMSGCOMPOSE?

Comment 26

11 years ago
(In reply to comment #25)
>I think it's already implemented as part of bug 312275 and nsMsgCompose does
>implement nsIMsgSendListener. I can't see this reflected in the idl file
>though, so that might be the only thing that has to be changed? Maybe
>nsMsgCompose.h too, to delete the NS_DECL_NSIMSGCOMPOSE?
nsMsgCompose implements nsIMsgSendListener, but nsIMsgCompose does not extend nsIMsgSendListener, which is why we can't directly call onSendNotPerformed.

Comment 27

11 years ago
Yes. I think that's what I meant with "I can't see this reflected in the idl file". It has to state something like:

interface nsIMsgCompose : nsIMsgSendListener

instead of 

interface nsIMsgCompose : nsISupports

I presume the javascript objects are generated from the IDL and that makes calling it impossible, while nsMsgCompse does expose the correct methods.

Comment 28

11 years ago
(In reply to comment #27)
>I presume the javascript objects are generated from the IDL and that makes
>calling it impossible, while nsMsgCompose does expose the correct methods.
It's not impossible, you just have the extra step of getting the secondary IDL associated with the gMsgCompose object so that the extra methods are exposed.

Comment 29

11 years ago
Created attachment 257696 [details] [diff] [review]
patch for nsIMsgCompose and nsMsgCompose
[Checkin: Comment 32]

This is my attempt to fix the IDL and nsMsgCompose.h.
How does the javascript generation thingy work? Do I have to do something to make it work?

Comment 30

11 years ago
(In reply to comment #29)
>How does the javascript generation thingy work?
With this patch, it is automatic, and you can just call:
gMsgCompose.onSendNotPerformed(messageId, Components.results.NS_ERROR_ABORT);
[note lower ^ case in JS]

Comment 31

11 years ago
Comment on attachment 257696 [details] [diff] [review]
patch for nsIMsgCompose and nsMsgCompose
[Checkin: Comment 32]

I can confirm that this patch avoids a call to QueryInterface in the UI.
Attachment #257696 - Flags: review+

Updated

11 years ago
Attachment #257696 - Flags: superreview?(bienvenu)
(Assignee)

Updated

11 years ago
Attachment #257696 - Flags: superreview?(bienvenu) → superreview+

Comment 32

11 years ago
Comment on attachment 257696 [details] [diff] [review]
patch for nsIMsgCompose and nsMsgCompose
[Checkin: Comment 32]

I landed Matthijs' patch.  This bug is FIXED now, right?

Comment 33

11 years ago
Hmm, actually I still see the bug.

Comment 34

11 years ago
(In reply to comment #33)
>Hmm, actually I still see the bug.
I don't think suite has updated MsgComposeCommands.js to reflect that we now call onSendNotPerformed on gMsgCompose directly.

Comment 35

11 years ago
I'm actually sceptical about nsIMsgCompose extending nsIMsgSendListener. I think nsMsgCompose should fire events about it's state when appropriate to interested components. Other components should not tell nsMsgCompose to fire events.

In other words when a nsMsgCompose window is closed (or aborted?) it should fire the onSendNotPerformed event. nsMsgCompose knows best when to fire what event.
This would also prevent API misuse. 

On the other hand, when it's not broken, don't fix it...
Attachment #257696 - Attachment description: patch for nsIMsgCompose and nsMsgCompose → patch for nsIMsgCompose and nsMsgCompose [Checkin: Comment 32]
(Reporter)

Comment 36

11 years ago
ctrl+w still fails for me also and same js error. TB version 3 alpha 1 (20070326)
Created attachment 259729 [details] [diff] [review]
(Cv2-SM) <MsgComposeCommands.js>
[Checkin: See comment 39]

Cv1-SM, with comment 25 suggestion(s),
and one space nit.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070326 SeaMonkey/1.5a] (nightly) (W2Ksp4)

Matthijs, Neil:
As it is, I kept |null| because |messageId| is undefined at both lines...
Attachment #257602 - Attachment is obsolete: true
Attachment #259729 - Flags: superreview?(neil)
Attachment #259729 - Flags: review?(neil)

Comment 38

11 years ago
Comment on attachment 259729 [details] [diff] [review]
(Cv2-SM) <MsgComposeCommands.js>
[Checkin: See comment 39]

r+sr=me without the whitespace change.
Attachment #259729 - Flags: superreview?(neil)
Attachment #259729 - Flags: superreview+
Attachment #259729 - Flags: review?(neil)
Attachment #259729 - Flags: review+
Comment on attachment 259729 [details] [diff] [review]
(Cv2-SM) <MsgComposeCommands.js>
[Checkin: See comment 39]


Checkin: {
2007-03-27 03:13	neil%parkwaycc.co.uk 	mozilla/mailnews/compose/resources/content/MsgComposeCommands.js 	1.394
}
without the whitespace change.
Attachment #259729 - Attachment description: (Cv2-SM) <MsgComposeCommands.js> → (Cv2-SM) <MsgComposeCommands.js> [Checkin: See comment 39]
Attachment #257515 - Attachment description: fix → (Bv1-TB) fix
Created attachment 259782 [details] [diff] [review]
(Bv2-TB) <MsgComposeCommands.js>
[Checkin: Comment 42]

Bv1-TB, with comment 25 suggestion(s).
(== Port Cv2-SM patch.)

[Mozilla Thunderbird, version 3 alpha 1 (20070326)] (nightly) (W2Ksp4)
Attachment #257515 - Attachment is obsolete: true
Attachment #259782 - Flags: superreview?(mscott)
Attachment #259782 - Flags: review?(mscott)

Updated

11 years ago
Attachment #259782 - Flags: superreview?(mscott)
Attachment #259782 - Flags: superreview+
Attachment #259782 - Flags: review?(mscott)
Attachment #259782 - Flags: review+
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070327 SeaMonkey/1.5a] (nightly) (W2Ksp4)

V.Fixed, SM part.
Whiteboard: [checkin needed : Bv2-TB]
checked in Bv2-TB

Checking in mail/components/compose/content/MsgComposeCommands.js;
/cvsroot/mozilla/mail/components/compose/content/MsgComposeCommands.js,v  <--  MsgComposeCommands.js
new revision: 1.110; previous revision: 1.109
Whiteboard: [checkin needed : Bv2-TB]
(Reporter)

Comment 43

11 years ago
v fixed on trunk - ctrl+w closes correctly. No js error except the already known  
 Error: sMsgComposeService is not defined
 Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
 Line: 1055
Attachment #259782 - Attachment description: (Bv2-TB) <MsgComposeCommands.js> → (Bv2-TB) <MsgComposeCommands.js> [Checkin: Comment 42]
Comment on attachment 257386 [details] [diff] [review]
proposed fix for crash
[Checkin: Comment 44 & 47]


Checkin: {
1.503	bienvenu%nventure.com	2007-03-05 10:26
}

NB: This patch was for bug 372705, actually.
Attachment #257386 - Attachment description: proposed fix for crash → proposed fix for crash [Checkin: Comment 44]
[Mozilla Thunderbird, version 3 alpha 1 (20070330)] (nightly) (W2Ksp4)

V.Fixed.

(The other/"new" error is bug 160942.)
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Summary: ctrl+w and file|close stop working after saving message with error gMsgCompose.getExternalSendListener is not a function → Ctrl+W and File|Close stop working after saving message, with "Error: gMsgCompose.getExternalSendListener is not a function"
(In reply to comment #35)
> I'm actually sceptical about nsIMsgCompose extending nsIMsgSendListener.
> 
> On the other hand, when it's not broken, don't fix it...

You might want to open a followup bug for this !?
Status: RESOLVED → VERIFIED
Comment on attachment 257386 [details] [diff] [review]
proposed fix for crash
[Checkin: Comment 44 & 47]


Checkin: {
1.460.2.32	bienvenu%nventure.com	2007-03-05 10:25
}
Attachment #257386 - Attachment description: proposed fix for crash [Checkin: Comment 44] → proposed fix for crash [Checkin: Comment 44 & 47]

Comment 48

11 years ago
I'm using Thunderbird 2.0.0.0 (20070326) on WinXP sp2.  I receive the following error:

Error: sMsgComposeService is not defined
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1064

This might be fixed in the newer builds, but I'm reporting this because I couldn't find any bug report that mentioned line 1064 specifically.  That line contains:

if (sMsgComposeService.isCachedWindow(window)) {

To reproduce:

1) Click on the "Write" toolbar button.
2) Close the composition window by clicking the X at the top-right.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.