160942 - JS error exception closing new message windows: "sMsgComposeService is not defined"

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Adam Hauner]

(this bug was reported as bug 151715, but its part was duplicated to bug 141481)

Repro:
1. hit quickly several times Ctrl+M
2. after Compose windows appears, close them 

Actual:
after closing each window, JS error appear:

Error: gMsgCompose has no properties
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2102

2002073118/trunk/W2K

[with 2002061308/trunk/W2K error looks like:
Error: sMsgComposeService is not defined
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 1119]

Comment 1

[David G King]

Assuming I have the right MsgComposeCommands.js, line 2102 of
mozilla/mailnews/compose/resources/content/MsgComposeCommands.js is a dump
statement. That is, it looks like it's a leftover from someones debugging.

In fact, from further investigation, this file is full of still active dump
statements.

Taking.

Comment 2

[David G King]

Attachment: Patch for reported error

I have made no attempt to remove all the other dump statements.

Comment 3

[Scott MacGregor]

How can commenting out that dump statement fix the JS error? In the line right
after the dump we dereference gMsgCompose as well. 

Comment 4

[David G King]

I can no longer dup this bug, maybe due to one of the 3 fixes that went in since
I managed to dup it.

So my patch doesn't fix anything. Oh well, I tried.

However, is it good practice to leave "dump" statements in post v1 code?

Comment 5

[David G King]

Reporter - can you still dup this problem with a current build, like v1.1?

Comment 6

[Adam Hauner]

Yep, actually I'm getting:
Error: gMsgCompose has no properties
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2112

2002082608/trunk/W2K

Comment 7

[David G King]

Attachment: Modify to match other patches to file

Modified patch due to change in file.

Comment 8

[David :Bienvenu]

if you remove the dump, you'll still get an exception on the next statement
which also dereferences gMsgCompose

Comment 9

[David G King]

Hmmm, that would be true. Thus, this entire section of code isn't being executed
due to the Javascript error (making it redundant?):

// Returns FALSE only if user cancels save action
if (gContentChanged || gMsgCompose.bodyModified)
   {
     // call window.focus, since we need to pop up a dialog
     // and therefore need to be visible (to prevent user confusion)
     window.focus();
     if (gPromptService)
     {
       result = gPromptService.confirmEx(window,
                               sComposeMsgsBundle.getString("saveDlogTitle"),
                               sComposeMsgsBundle.getString("saveDlogMessage"),
                               (gPromptService.BUTTON_TITLE_SAVE *
gPromptService.BUTTON_POS_0) +
                               (gPromptService.BUTTON_TITLE_CANCEL *
gPromptService.BUTTON_POS_1) +
                               (gPromptService.BUTTON_TITLE_DONT_SAVE *
gPromptService.BUTTON_POS_2),
                               null, null, null,
                               null, {value:0});
       switch (result)
       {
         case 0: //Save
           if (LastToClose())
           NotifyQuitApplication();
           gCloseWindowAfterSave = true;
           SaveAsDraft();
           return false;
         case 1: //Cancel
           return false;
         case 2: //Don't Save
           break;
       }
     }
 
     SetContentAndBodyAsUnmodified();
   }

Comment 10

[David G King]

I finally got back to this bug.

As far as I can tell gMsgCompose.bodyModified isn't actually initialised until
after the dump and the following if statement. Hence, I suspect it's invalid for
it to be part of the if statement.

New patch coming to reflect this.

Comment 11

[David G King]

Attachment: New diff as per previous comment

This is a new patch to remove unused modifier.

Comment 12

[David :Bienvenu]

adding Jean-Francois to cc list for his input. Perhaps we could just check for a
null gMsgCompose as well.

Comment 13

[David G King]

I did some more digging (via lxr.mozilla.org), and gMsgCompose.bodyModified can
only be NULL or FALSE. So, when placed in an OR statement, it either has no
effect, or it breaks it.

I could check for a NULL value, but as nothing in Mozilla sets it to TRUE (or
anything but NULL or FALSE), I didn't see the point, hence why I removed it.

Comment 14

[Adam Hauner]

I'm actually get very similar JS error using 2003030408/trunk/W2K with this repro:

1. open browser
2. open Compose new message window
3. write and send message
4. Compose new message window disappear after message is sent, but browser 
   didn't get focus
5. hit Ctrl+W (for closing actual window)

Error: gMsgCompose has no properties
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2007

dgk: Does your patch address also this situation? Is your latest patch still fresh?

Comment 15

[David G King]

I'm not sure if my patch addresses that. As for it being fresh, it was when I
asked for a review, but that was a while ago, and no review was forthcoming.

Comment 16

[David G King]

Adam, my patch will fix your problem as it is the same as the original problem,
just the line numbers have changed.

I'll refresh my patch, and try for R and SR again.

Comment 17

[David G King]

Attachment: Updated so as to remain fresh

Comment 18

[[e-mail]]

Comment on attachment 116331 [details] [diff] [review]
Updated so as to remain fresh

IHMO this is the wrong way to do this; there are occasions where
gMsgCompose.bodyModified is true, we just nned to check more carefully.

Comment 19

[David G King]

Neil,

While gMsgCompose.bodyModified could possibly be true, the current
implementation within Mozilla refers to it in 3 places -

/mailnews/compose/resources/content/MsgComposeCommands.js, line 2007 --
dump("XXX changed? " + gContentChanged + "," + gMsgCompose.bodyModified + "\n");

/mailnews/compose/resources/content/MsgComposeCommands.js, line 2009 -- if
(gContentChanged || gMsgCompose.bodyModified)

/mailnews/compose/resources/content/MsgComposeCommands.js, line 2045 --
gMsgCompose.bodyModified = false; 


There is nowhere in Mozilla where it is set to true, or in fact initialised to
anything.

Comment 20

[David G King]

Comment on attachment 116331 [details] [diff] [review]
Updated so as to remain fresh

I thought the problem was with .bodyModified, now I know the problem is really
with gMsgCompose itself being NUL.

Comment 21

[Matthias Versen [:Matti]]

Comment on attachment 107907 [details] [diff] [review]
New diff as per previous comment

clearing obsolete review requests

Comment 22

[David G King]

Back again....

Looks like the gMsgCompose aspect of this bug has been fixed elsewhere, which
just leaves sMsgComposeService

Comment 23

[Wayne Mery (:wsmwk)]

(In reply to comment #22)
> Looks like the gMsgCompose aspect of this bug has been fixed elsewhere, which
> just leaves sMsgComposeService

yes still here in version 1.6a1 (20060228)

Error: uncaught exception: [Exception... "'[JavaScript Error: "sMsgComposeService is not defined" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 1058}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: anonymous :: line 155"  data: yes]

Comment 24

[Mike Cowperthwaite]

bug 241178 and bug 262241 seem to be dupes of this.
Bug 241178 comment 17-18 give easy steps-to-reproduce: open multiple compose windows at once, close them without sending: one error for each uncached window; or, you can turn off compose window caching.

An alternate way is to open a cached compose window using Send Link, then close it without sending

Comment 27

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> *** Bug 262241 has been marked as a duplicate of this bug. ***

Then,
OS: Windows 2000 -> All.

Comment 28

[[e-mail]]

As for the sMsgComposeService exception, this is a completely different bug:
>MsgComposeCloseWindow(true);
>
>// at this point, we might be caching this window.
>// in which case, we don't want to close it
>if (sMsgComposeService.isCachedWindow(window)) {
>  retVal = false;
>}

If we're not caching the window, then the call to MsgComposeCloseWindow closes (actually destroys) the window anyway, so we don't actually need to do this check, and we can simply always return false from DoCommandClose.

Comment 29

[Serge Gautherie (:sgautherie)]

While we're there,

<http://lxr.mozilla.org/mozilla/search?string=MsgComposeCloseWindow>
shows that this function is always (= twice) called with 'true'.
then
<http://lxr.mozilla.org/mozilla/search?string=function+CloseWindow>
shows that the parameter is ignored/absent anyway,
then
it seems the (unused) parameter (values) should be removed.

Comment 30

[[e-mail]]

Attachment: Suite fix [Checkin: Comment 31]

Comment 31

[[e-mail]]

Suite fix checked in.

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-TB) <MsgComposeCommands.js> [Checkin: Comment 33]

Port SM patch to TB.

[Mozilla Thunderbird, version 3 alpha 1 (20070402)] (nightly) (W2Ksp4)

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 260385 [details] [diff] [review]
(Cv1-TB) <MsgComposeCommands.js>
[Checkin: Comment 33]


Checkin: {
2007-04-03 10:10	bugzilla%standard8.demon.co.uk 	mozilla/mail/components/compose/content/MsgComposeCommands.js 	1.111
}

Comment 34

[Serge Gautherie (:sgautherie)]

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

V.Fixed, SM part.

Comment 35

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-SM) <MsgComposeCommands.js>

Remove useless |recycleIt| parameter: see comment 29.

Comment 36

[[e-mail]]

Comment on attachment 260528 [details] [diff] [review]
(Dv1-SM) <MsgComposeCommands.js>

>-    gMsgCompose.CloseWindow(recycleIt);
>+    gMsgCompose.CloseWindow();
You still need to pass something to CloseWindow...

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Dv2-SM) <MsgComposeCommands.js>

Dv1-SM, with comment 36 suggestion(s).

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

(In reply to comment #29)
> <http://lxr.mozilla.org/mozilla/search?string=function+CloseWindow>
> shows that the parameter is ignored/absent anyway,

I made a mistake there: I searched/checked the JS function, while it's the CPP function which is called...

Comment 38

[Serge Gautherie (:sgautherie)]

[Mozilla Thunderbird, version 3 alpha 1 (20070404)] (nightly) (W2Ksp4)

V.Fixed, TB part.

Comment 39

[[e-mail]]

Comment on attachment 260882 [details] [diff] [review]
(Dv2-SM) <MsgComposeCommands.js>

I discovered that there are people who call gMsgCompose.CloseWindow(false); and Mnyromyr suggests that they should be fixed to call MsgComposeCloseWindow. It might be a good idea to move the else window.close(); into MsgComposeCloseWindow too.

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Dv3-SM) <MsgComposeCommands.js> [Checkin: Comment 43]

Dv2-SM, with comment 39 suggestion(s).

Comment 41

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-TB) <MsgComposeCommands.js> [Checkin: Comment 44]

Port Dv3-SM patch to TB.

Comment 42

[Scott MacGregor]

Comment on attachment 260973 [details] [diff] [review]
(Ev1-TB) <MsgComposeCommands.js>
[Checkin: Comment 44]

nice.

Comment 43

[Serge Gautherie (:sgautherie)]

Comment on attachment 260920 [details] [diff] [review]
(Dv3-SM) <MsgComposeCommands.js>
[Checkin: Comment 43]


Checkin: {
2007-04-13 10:51	bugzilla%standard8.demon.co.uk 	mozilla/mailnews/compose/resources/content/MsgComposeCommands.js 	1.396
}

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 260973 [details] [diff] [review]
(Ev1-TB) <MsgComposeCommands.js>
[Checkin: Comment 44]


Checkin: {
2007-04-13 10:52	bugzilla%standard8.demon.co.uk 	mozilla/mail/components/compose/content/MsgComposeCommands.js 	1.112
}