915516 - gMsgCompose is null (was: items is undefined in cloudAttachmentLinkManager.js)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[ISHIKAWA, Chiaki]

+++ This bug was initially created as a clone of Bug #904719 +++

Error: items is undefined
Source file: chrome://messenger/content/messengercompose/cloudAttachmentLinkManager.js
Line: 108

I get this when composing a message and then closing it without saving or sending.

There is also another error, which I am not sure is related:
Error: gMsgCompose is null
Source file: chrome://messenger/content/messengercompose/cloudAttachmentLinkManager.js
Line: 32

The code causing this was there since the beginning of the file (http://hg.mozilla.org/comm-central/rev/42e362b0c7c6) but I don't know why the error was not produced at that time and only started to show recently.

---

I investigated why gMsgCompose is null.
The posts that follow will discuss the investigation and a possible fix.

Comment 1

[ISHIKAWA, Chiaki]

(The following is a copy of the comment posted to the original:
https://bugzilla.mozilla.org/show_bug.cgi?id=904719#c11)

---


I use thunderbird for work at the office and at home.
Before an annual technology exhibition, I receive about 1000 e-mails
in a month with large attachments (PDFs for proof-reading: mainly product
brochures, exhibition posters, booklets, etc.). This can 
break the 2GB and 4GB folder limit easily if I am not careful :-(


So I am desperately seeking "rock-solid e-mail client." (tm) :-)
That is why I am contributing bug fixes, etc.

Anyway, aceman, here is what I found about gMsgCompose:

gMsgCmpose seems to be set only in three places (aside from the test
routines).

https://mxr.mozilla.org/comm-central/ident?i=gMsgCompose&filter=

mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)
    line 112 -- gMsgCompose = null;
    line 144 -- gMsgCompose = null; 

    line 1975 -- gMsgCompose = MailServices.compose.initCompose(params, window, editorElement.docShell);

Line 112 is part of a function named InitializeGlobalVariables.

function InitializeGlobalVariables()
108 {
109   gMessenger = Components.classes["@mozilla.org/messenger;1"]
110                          .createInstance(Components.interfaces.nsIMessenger);
111 
112   gMsgCompose = null;
113   gWindowLocked = false;

Line 144 is part of a function named ReleaseGlobalVariables.

function ReleaseGlobalVariables()
141 {
142   gCurrentIdentity = null;
143   gCharsetConvertManager = null;
144   gMsgCompose = null;
145   gMessenger = null;
146   _gComposeBundle = null;
147   MailServices.mailSession.RemoveMsgWindow(msgWindow);
148   msgWindow = null;
149 }
150 

function 1975 is  part of a function named ComposeStartup


1973   // Get the <editor> element to startup an editor
1974   var editorElement = GetCurrentEditorElement();
1975   gMsgCompose = MailServices.compose.initCompose(params, window, editorElement.docShell);
1976 
1977   // Set the close listener.
1978   gMsgCompose.recyclingListener = gComposeRecyclingListener;
1979   gMsgCompose.addMsgSendListener(gSendListener);

Assuming that gMsgCompose is set up properly by line 1975, where
is ReleaseGlobalVariables() called to clear gMsgComose to null?


Referenced (in 2 files total) in:
(note the one under suite/mailnews is for SeaMonkey, I think.)

    mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 196 -- ReleaseGlobalVariables();
        line 2221 -- ReleaseGlobalVariables(); 
    suite/mailnews/compose/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 166 -- ReleaseGlobalVariables(); 

Line 196 is part of a function that is named onClose
171 var gComposeRecyclingListener = {
172   onClose: function() {
173     //Reset recipients and attachments
	...
194     SetContentAndBodyAsUnmodified();
195     updateEditableFields(true);
196     ReleaseGlobalVariables();  <--- here

Line 2221 is part of a function named ComposeUnload:

2199 function ComposeUnload()
2200 {
2201   UnloadCommandUpdateHandlers();
2202 
2203   // Stop gSpellChecker so personal dictionary is saved
2204   enableInlineSpellCheck(false);
2205 
2206   EditorCleanup();
2207 
2208   if (gMsgCompose)
2209     gMsgCompose.removeMsgSendListener(gSendListener);
2210 
2211   RemoveMessageComposeOfflineQuitObserver();
2212   gAttachmentNotifier.shutdown();
2213 
2214   if (gMsgCompose)
2215     gMsgCompose.UnregisterStateListener(stateListener);
2216   if (gAutoSaveTimeout)
2217     clearTimeout(gAutoSaveTimeout);
2218   if (msgWindow)
2219     msgWindow.closeWindow();
2220 
2221   ReleaseGlobalVariables();
2222 }


Considering that I see the following log when I close the compose
window after manually starting local build of thunderbird (comm-central)
let me check MsgComposeCommands.js line 234:

************************************************************

* Call to xpconnect wrapped JSObject produced this error:  *

[Exception... "'[JavaScript Error: "gMsgCompose is null" {file: "chrome://messenger/content/messengercompose/cloudAttachmentLinkManager.js" line: 32}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://messenger/content/messengercompose/MsgComposeCommands.js :: <TOP_LEVEL> :: line 234"  data: yes]

************************************************************


MsgComposeCommands.js:
	...

229     //Release the nsIMsgComposeParams object
230     if (window.arguments && window.arguments[0])
231       window.arguments[0] = null;
232     var event = document.createEvent('Events');
233     event.initEvent('compose-window-close', false, true);
234     document.getElementById("msgcomposeWindow").dispatchEvent(event);
235     if (gAutoSaveTimeout)
236       clearTimeout(gAutoSaveTimeout);
237   },


Now, wait a second!. This is still part of onClose() routine!
If for some reason, the sequence of statements starting from 232, 233,
and then ending 234 tries to access gMsgCompose, which seems likely,
there is a problem after all.
onClose() already calls, on line 196, ReleaseGlobalVariables() which
sets gMsgCompose to null. So gMsgComose is null when control reaches
line 234.

Possible solution:

Like the function CompooseUnload() does, maybe we should move
ReleaseGlobalVariables() to the end of onClose().

Or maybe move the processing of

    var event = document.createEvent('Events');
    event.initEvent('compose-window-close', false, true);
    document.getElementById("msgcomposeWindow").dispatchEvent(event);

before the call to ReleaseGlobalVariables().

What do people think?

Comment 2

[ISHIKAWA, Chiaki]

Based on comment
https://bugzilla.mozilla.org/show_bug.cgi?id=904719#c12
>Great detective work! If it works, moving ReleaseGlobalVariables() to the end sounds like good solution to me.
>(But we probably should handle the gMsgCompose problem in another bug, and let this one deal with the items issue.)

I moved ReleaseGlobalVariables() to the end of onClose(), and ran |make mozmill|.
It seems to work and I see no more "gMsgCompose is null" message.

Comment 3

[ISHIKAWA, Chiaki]

Attachment: Defer the clearing of gMsgCompose to null to the end of onClose() to fix "gMsgCompose is null" error on event-related usage.

This patch eliminates "gMsgCompose is null" error completely from |make mozmill| test suite run of TB (comm-central.)

TIA

Comment 4

[:aceman]

Thanks, I will test the patch by running TB manually, as I can reproduce it just by closing a compose window.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 803508 [details] [diff] [review]
Defer the clearing of gMsgCompose to null to the end of onClose() to fix "gMsgCompose is null" error on event-related usage.

Review of attachment 803508 [details] [diff] [review]:
-----------------------------------------------------------------

Thx for the patch! r=mkmelin with the below changes

::: mail/components/compose/content/MsgComposeCommands.js
@@ +192,5 @@
>      SetComposeWindowTitle();
>  
>      SetContentAndBodyAsUnmodified();
>      updateEditableFields(true);
> +    // ReleaseGlobalVariables(); moved to the end.

please remove this line

@@ +234,5 @@
>      document.getElementById("msgcomposeWindow").dispatchEvent(event);
>      if (gAutoSaveTimeout)
>        clearTimeout(gAutoSaveTimeout);
> +    ReleaseGlobalVariables(); 	// This must come at the end of onClose() 
> +                                // or bad things will happen.

this comment could be just // Make sure this is last.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: (Take Two) Defer the clearing of gMsgCompose to null to the end of onClose() to fix "gMsgCompose is null" error on event-related usage. (carrying over r=mkmelin)

Re-created patch.
TIA

Comment 7

[ISHIKAWA, Chiaki]

BTW, it has already received r+. It is not entirely clear how to move the status to a modified patch in bugzilla.

Comment 8

[:aceman]

You can add r=mkmelin to the commit line in the patch itself. Then in the comment to the new attachment in the bug say "carrying over r=mkmelin" and set review+ on the patch yourself. Yes, it will appear you set the r+. But the guy doing check-in will check if you have proper reviews (if needs be by looking at the flags of the obsoleted patches).

Comment 9

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #8)
> You can add r=mkmelin to the commit line in the patch itself. Then in the
> comment to the new attachment in the bug say "carrying over r=mkmelin" and
> set review+ on the patch yourself. Yes, it will appear you set the r+. But
> the guy doing check-in will check if you have proper reviews (if needs be by
> looking at the flags of the obsoleted patches).

I changed the comment to the patch (the commit line doesn't have the r=mkmelin). Hope this works.

Comment 10

[:aceman]

Comment on attachment 803670 [details] [diff] [review]
(Take Two)  Defer the clearing of gMsgCompose to null to the end of onClose() to fix "gMsgCompose is null" error on event-related usage. (carrying over r=mkmelin)

Review of attachment 803670 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this patch fixed it for me. Despite bug 904719 comment 2 , this patch alone does not fix bug 904719 (items is null) too. So both patches need to be checked in.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/e8b2fb3cfa65