916505 - Exception... "'[JavaScript Error: "gMsgCompose is null" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 3159}]' when calling method: [nsIObserver::observe]" nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAIL

Status: RESOLVED DUPLICATE of bug 1250605

(Thunderbird :: Message Compose Window, defect)

Description

[ISHIKAWA, Chiaki]

During |make mozmill| test suite run of local DEBUG BUILD of TB
under valgrind, I found an error message as follows.


WARNING: NS_ENSURE_TRUE(nextNode) failed: file /REF-COMM-CENTRAL/comm-central/mozilla/editor/libeditor/html/nsHTMLEditor.cpp, line 940
Test Failure: No References Header in forwarded msg.
************************************************************
* Call to xpconnect wrapped JSObject poduced this error:  *
[Exception... "'[JavaScript Error: "gMsgCompose is null" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 3159}]' when calling method: [nsIObserver::observe]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: canQuitApplication :: line 42"  data: yes]
************************************************************
WARNING: NS_ENSURE_TRUE(ps) failed: file /REF-COMM-CENTRAL/comm-central/mozilla/editor/libeditor/html/nsHTMLEditor.cpp, line 2959


A full log is attached (after fixing the stacktrace with symbol dumps 
[ used /mozilla/tools/rb/fix-linux-stack.pl original-log-file  after 
  moving into binary directory.
  cd ${MOZ_OBJDIR}/mozilla/dist/bin
  ${MOZ_SRCDIR}/mozilla/tools/rb/fix-linux-stack.pl $1  
]
Below, a local patch is excerpted. 
I am contemplating changing the check of gMsgCompose.bodyModified
to (gMsgCompose && gMsgCompose.bodyModified).

If anyone has a suggestion for better fix, I will appreciate it.

TIA

PS: this bug is related to Bug 912151 since
the gloval variable gMsgCompose is concerned.
However, the place [the line number] where the error occurs, and
the situations where the bugs occur seem to be different.
- this one seems to be triggered when user aborts the composition?
- bug 912151 is triggered in a normal ordinary operation.
So I am filing this bug as a separate bug.

So far, in my |make mozmill| session logs, I only got this particular
message from this bug about a dozen times times while the message from
bug 912151 is about 600+ times.  Due to relatively low frequency, I
have overlooked this error.  Only now the message from bug 912151 is
gone in my local build of TB, I realize there is a different bug.,
(Also, obviously, there are unknown thread-race issues.)

# HG changeset patch
# Parent ea92e2e2985387f8f5199cb8b556b61c525c78d7
# User ISHIKAWA, Chiaki <[e-mail]>
gMsgCompose was undefined once during
a memcheck run.
- Timing issue caused by the internal thread-races of the main TB code?
- Timing issue of the test code itself?
  (Related to Bug 912151 )?

diff --git a/mail/components/compose/content/MsgComposeCommands.js b/mail/components/compose/content/MsgComposeCommands.js
--- a/mail/components/compose/content/MsgComposeCommands.js
+++ b/mail/components/compose/content/MsgComposeCommands.js
@@ -3151,16 +3151,18 @@ function ComposeCanClose()
     {
       gMsgCompose.abort();
       return true;
     }
     return false;
   }
 
   // Returns FALSE only if user cancels save action
+  // (In one |make mozmill| run, gMsgCompose was undefined here.)
+  // gMsgCompose.bodyModified -> (gMsgCompose && gMsgCompose.bodyModified)? 
   if (gContentChanged || gMsgCompose.bodyModified || gAutoSaveKickedIn)
   {
     // call window.focus, since we need to pop up a dialog
     // and therefore need to be visible (to prevent user confusion)
     window.focus();
     let result = Services.prompt
                          .confirmEx(window,
                                     getComposeBundle().getString("saveDlogTitle"),

Comment 1

[ISHIKAWA, Chiaki]

Sorry, the mention "the message from bug 912151 is about 600+ times." was incorrect.
 

It was from bug 915516.

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Fix for "gMsgCompose is undefined" error.

Here is the patch.
I ran |make mozmill| with this patch applied,
ad the error is no longer visible.
I think it has not introduced any ill-effect as far as I can tell.

Comment 3

[:aceman]

+  // (In one |make mozmill| run, gMsgCompose was undefined here.)
+  // gMsgCompose.bodyModified -> (gMsgCompose && gMsgCompose.bodyModified)?
I don't think the comment will be accepted like this.

Either gMsgCompose is OK to be null here and then no comment is needed (or just one that states so), or it is not OK and then we must find the cause.
There seem to be more uses of gMsgCompose above this line, albeit conditional so maybe they are not hit by luck. Have you looked over the code in that function whether it semantically is fine that gMsgCompose is not initialized (we are either in startup of the window, or close down)?

Comment 4

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #3)
> +  // (In one |make mozmill| run, gMsgCompose was undefined here.)
> +  // gMsgCompose.bodyModified -> (gMsgCompose && gMsgCompose.bodyModified)?
> I don't think the comment will be accepted like this.
> 
> Either gMsgCompose is OK to be null here and then no comment is needed (or
> just one that states so), or it is not OK and then we must find the cause.
> There seem to be more uses of gMsgCompose above this line, albeit
> conditional so maybe they are not hit by luck. Have you looked over the code
> in that function whether it semantically is fine that gMsgCompose is not
> initialized (we are either in startup of the window, or close down)?

Thank you, aceman, for your review and comment.

I admit that my patch was a bandage to suprress the seeming error
quicly without much in-depth analysis.

Here in my message let me answer your concern postivily (luckily!)
with a little bit of my analsis. 

The changed line in my patch is in the function, ComposeCanClose():

    // Check for changes to document and allow saving before closing
    // This is hooked up to the OS's window close widget (e.g., "X" for Windows)
    function ComposeCanClose()
    {
      // Do this early, so ldap sessions have a better chance to

[Big Question]

So how can gMsgCompose be null here?
Can we hit this code path due to ill-behaved mult-threading?

[Tentative Conclusion]

We can hit here if ComposeStartup() is interrupted early (by
clickng [x] mark at the upper-right corner (under X windows), for
example.  (Not sure how this happens in the test code, though. It
could be even related to the timing-issues of test programs
we are seeing elsewhere. Maybe the test program is causing the closing
of compose window EVEN BEFORE the main TB had the time to ready the
compose window at all!)

[Background]

Let us take a look at where gMsgCompose is set.

According to mozilla cross-reference tool, here is what I found.
http://mxr.mozilla.org/comm-central/ident?i=gMsgCompose&filter=

gMsgCompose  is declared/defined only in the following places:
(suite/ is for SeaMonkey)

Defined as a variable in:

    mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 55 -- var gMsgCompose; 
    mailnews/compose/test/unit/test_nsMsgCompose2.js (View Hg log or Hg annotations)
        line 9 -- var gMsgCompose = Components.classes[MsgComposeContractID] 
    mailnews/compose/test/unit/test_temporaryFilesRemoved.js (View Hg log or Hg annotations)
        line 11 -- var gMsgCompose; 
    suite/mailnews/compose/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 50 -- var gMsgCompose; 

     
It is set only in the following lines. I only picked up
(=, assignment) line:
(Codes in suite/ directory are omitted for the purpose of this memo.)

mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)

    line 112 -- gMsgCompose = null;

The above is part of the InitializeGlobalVariables():
    function InitializeGlobalVariables()
    {
      gMessenger = Components.classes["@mozilla.org/messenger;1"]
			     .createInstance(Components.interfaces.nsIMessenger);

      gMsgCompose = null;
      gWindowLocked = false;


    line 144 -- gMsgCompose = null;

    The above is part of ReleaseGlobalVariables()

    function ReleaseGlobalVariables()
    {
      gCurrentIdentity = null;
      gCharsetConvertManager = null;
      gMsgCompose = null;
      gMessenger = null;
      _gComposeBundle = null;
      MailServices.mailSession.RemoveMsgWindow(msgWindow);
      msgWindow = null;
    }


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

    The above line is part of ComposeStartup() function.

    function ComposeStartup(recycled, aParams)
    {
      var params = null; // New way to pass parameters to the compose window as a nsIMsgComposeParameters object
      var args = null;   // old way, parameters are passed as a string
      ...

ComposeSartup() is called in onReopen(), WizCallback(), and ComposeLoad().

Given the setting and unsetting of gMsgCompose, I can only think of
one particular scenario when ComposeCanClose() is inovked and gMsgCompose is
null; Here is the scenario.

a window for composition is created and eventually ComposeSartup() is invoked.
Note that, after ComposeStartup() is invoked until 
  gMsgCompose = MailServices.compose.initCompose(params, window, editorElement.docShell);
on line 1975 is executed, there are about 140+ lines of JavaScript
code that gets executed. 

Suppose the processing is interrupted while the execution is still
within this 140+ lines of code.  gMsgCompose is still null.

For example, when [X] on the upper-right corner of the window (under X
windows) is clicked during this "window of vulnerability",
ComposeCanClose() is invoked (cf.  The comment says: // This is hooked
up to the OS's window close widget (e.g., "X" for Windows)).

So, under this assumption, let me analyze what can happen inside
ComposeCanClose() when gMsgCompose is null, especially
with/without my patch.

Inside ComposeCanClose():

There is a reference to gMsgCompose in the first big if-statement.

  if (gSendOrSaveOperationInProgress)
  {
     ...
     ...
    if (result == 1)
    {
      gMsgCompose.abort();
      return true;
    }
    return false;
  }

Note the condition, gSendOrSaveOperationInProgress. This suggests that
Send / Save Operation for the composed message is in progress. If we
could send/save a composed material after all, ComposeStartup() would
have been already finished (correc me wrong here), so it is safe to
assume gMsgCompose is not null here. (Well, if it is null here, we
will get an error eventually by someone. We can wait for that report
in the long run although I doubut it would happen.)

(cf. gSendOrSaveoperationInProgress 
is set only in three places ignoring /suite/ files.)
These places suggest that the compose window must have been
initialized property already for a long time.
http://mxr.mozilla.org/comm-central/ident?i=gSendOrSaveOperationInProgress
mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)

    line 117 -- gSendOrSaveOperationInProgress = false;
    line 319 -- gSendOrSaveOperationInProgress = false;
    line 2560 -- gSendOrSaveOperationInProgress = true; )

Now comes the if-statement which was modified in my proposed patch.

  // Returns FALSE only if user cancels save action
  if (gContentChanged || gMsgCompose.bodyModified || gAutoSaveKickedIn)
  {

Obviously, as in my scenario, if ComposeStartup() has been
interrupted, gContentChanged may not have proper value set yet.  
(Hmm... we may need a lock or something that makes sure that
the entry to ComposeCanClose() can only proceeds when ComposeStartup()
is still executing.)

I will not go into the details here (see the comment at the end.), 
but it is highly likely that gContentChanged is also false.

If gContentChanged is false, the JavaScript interpreter tries to
evaluate

      gMsgCompose.bodyModified

and, under my proposed scenario, gMsgCompose can be null here.

So we must protect the reference to gMsgCompose.  Now, in hindsight,
if gMsgCompose is null here, we can probably skip the operation of the
whole if() {...} clause since after all gMsgCompose's being null
suggests that ComposeStartup() has not completed at all yet. Let me
think of a better fix then.

If gMsgCompose is null, we can return true as the value of
ComposeCanClose() [since after all, the intialization did not finish
completel even, and this is my previous patch would have done anyway
and I didn't notice serious errors, either.]
(This observation will be the basis of my next patch.)

Now, assuming gMsgCompose is not null, and 
gMsgCompose.bodyModified is false, we evaluate
gAutoSaveKickedIn.
If gAutoSaveKickedIn is true, I am sure that happens only after
ComposeStarup() has completed successfully and gMsgCompose is
well-defined. 

BTW, when gMsgCompose is null in the interrupted case, I think it is
highly likely that gAutoSaveKickedIn is false, too.  So it is likely
that with the second "if() { ...  }", the "{ ... }" portion was not
likely to be executed at all (during the interrupt scenario).

[Note: where gContentChanged is set.]

gContentChanged is defined/set only in the following places.
(I have omitted SeaMonkey references, i.e., under |suite| directory)

Defined as a variable in:

    mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 58 -- var gContentChanged; 
    suite/mailnews/compose/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 53 -- var gContentChanged; 

Referenced (in 4 files total) in:

    mail/components/compose/content/MsgComposeCommands.js (View Hg log or Hg annotations)
        line 114 -- gContentChanged = false; // in InitializeGlobalVariables
	     	 // it is set to false only a couple of lines below
		 // gMsgCompose. So it is likely that when
		 //  ComposeStartup() is called, it is still null.
        line 283 -- gContentChanged = true;  // in ComposeProcessDone
        line 2855 -- gContentChanged = true; // in onAddressColCommand
        line 2862 -- gContentChanged = true; // in onRecipientsInput
        line 3146 -- if (gContentChanged || gMsgCompose.bodyModified || gAutoSaveKickedIn)
	                                     // in ComposeCanClose()
        line 3227 -- gContentChanged = false;// in SetContentAndBodyAsUnmodified
        line 3399 -- gContentChanged = true; // in AddAttachments
        line 3538 -- gContentChanged = true; // in RemoveSelectedAttachment 
        line 3567 -- gContentChanged = true; // RenameSelectedAttachment
        line 4342 -- if (gMsgCompose.editor && (gContentChanged || gMsgCompose.bodyModified) 
	     	     			    // in function AutoSave()

       (Note: By the time AutoSave() is kicked in, I think it is a
       safe bet that ComposeStartup() completed long time ago and
       gMsgCompose is not null, and its use does not have to be
       protected for null case.)


    mail/components/compose/content/addressingWidgetOverlay.js (View Hg log or Hg annotations)
        line 1029 -- gContentChanged=true;  // in awRecipientInputCommand

cf. 
(I found a few interesting lines where (!gMsgCompose) is checked.
These lines return promptly when gMsgCompose is null.)

mail/extensions/smime/content/msgCompSMIMEOverlay.js (View Hg log or Hg annotations)
    line 30 -- if (!gMsgCompose || !gMsgCompose.compFields) 
    line 39 -- if (gSMFields || !gMsgCompose || !gMsgCompose.compFields)

These lines are part of onComposeClose() and onComposeReOpen() respectively.

    function onComposerClose()
    {
      gSMFields = null;
      setNoEncryptionUI();
      setNoSignatureUI();

      if (!gMsgCompose || !gMsgCompose.compFields)
	return;

      gMsgCompose.compFields.securityInfo = null;
    }

    function onComposerReOpen()
    {
      // Are we already set up ? Or are the required fields missing ?
      if (gSMFields || !gMsgCompose || !gMsgCompose.compFields)
	return;


mailnews/extensions/smime/content/msgCompSMIMEOverlay.js (View Hg log or Hg annotations)

    line 29 -- if (!gMsgCompose || !gMsgCompose.compFields)
    line 38 -- if (gSMFields || !gMsgCompose || !gMsgCompose.compFields)

It seems these are replicas of the lines above.
    function onComposerClose()
    {
      gSMFields = null;
      setNoEncryptionUI();
      setNoSignatureUI();

      if (!gMsgCompose || !gMsgCompose.compFields)
	return;

      gMsgCompose.compFields.securityInfo = null;
    }

    function onComposerReOpen()
    {
      // Are we already set up ? Or are the required fields missing ?
      if (gSMFields || !gMsgCompose || !gMsgCompose.compFields)
	return;

Anyway, I am uploading my updated patch based on the observation here.

TIA

Comment 5

[ISHIKAWA, Chiaki]

Attachment: An improved patch (Take Two)

Here is the improved patch.
- better comment,
- an early exit in when gMsgCompose is null, and
- a dump (a runtime assert-like message) when a certain condition is not met for future developers just in case.

TIA

Comment 6

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #4)
Thanks for the analysis. You can actually test your theory. Set some new global variable at the beginning of ComposeStartup() and set it to some other value just after initialising gMsgCompose there. Then on the problematic line in ComposeCanClose, dump out the value of the variable. You may then see the value in your mozmill log and will see if your theory is true.

Comment 7

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #6)
> (In reply to ISHIKAWA, Chiaki from comment #4)
> Thanks for the analysis. You can actually test your theory. Set some new
> global variable at the beginning of ComposeStartup() and set it to some
> other value just after initialising gMsgCompose there. Then on the
> problematic line in ComposeCanClose, dump out the value of the variable. You
> may then see the value in your mozmill log and will see if your theory is
> true.

OK, I did what you suggested and I think my analysis is correct (well,
not sure what is interrupting the ComposeStartup() yet, though.)

Here is the details.
(I am uploading the patch as the next attachment.)

Just as you suggested, I tried setting a global variable and
dump the value.
a global variable g_gazonk_foobar is set to 1234 upon initialization
and closing of compose window, and that is when gMsgCompose is set to null.
It gets set to 7890 when gMsgCompose is set to a compose window.

Now, during the run of |make mozmill|
the value of g_gazonk_foobar is printed as 7890 nineteen (19) times,
whereas, on a single occasion when gMsgCompose was still null
when ComposeCanClose() was called, the value of g_gazonk_foobar is
printed as 1234. That is, the proper setting of gMsgCompose was
skipped in this particular test, indeed. This is just I thought.

Here is an excerpt from the log.

TEST-START | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test_forward_as_attachments
	 ...
WARNING: g_gazonk_foobar = 7890
	 ...
WARNING: g_gazonk_foobar = 1234  <---  Smoking gun.
WARNING: gMsgCompose was null in ComposeCanClose(). <---
	 ...

Code of test_forward_as_attachments() is quoted in the following.

At this level, it is not entirely clear where the compose window
is created. But note that there is a long function name
forward_selected_messages_and_go_to_drafts_folder. 

function test_forward_as_attachments () {
  be_in_folder(folder);
  // original message header
  let oMsgHdr0 = select_click_row(0);
  let oMsgHdr1 = select_click_row(1);
  select_shift_click_row(0);

  forward_selected_messages_and_go_to_drafts_folder(open_compose_with_forward_as_attachments);

  // forwarded message header
  let fMsgHdr = select_click_row(0);

  assert_true(fMsgHdr.numReferences > 0, "No References Header in forwarded msg.");
  assert_true(fMsgHdr.numReferences > 1, "Only one References Header in forwarded msg.");
  assert_equals(fMsgHdr.getStringReference(1), oMsgHdr1.messageId,
    "The forwarded message should have References: = Message-Id: of the original msg#1");
  assert_equals(fMsgHdr.getStringReference(0), oMsgHdr0.messageId,
    "The forwarded message should have References: = Message-Id: of the original msg#0");

  // test for x-forwarded-message id and exercise the js mime representation as
  // well
  to_mime_message(fMsgHdr, null, function(aMsgHdr, aMimeMsg) {
    assert_equals(aMimeMsg.headers["x-forwarded-message-id"],
      "<"+oMsgHdr0.messageId+"> <"+oMsgHdr1.messageId+">");
    assert_equals(aMimeMsg.headers["references"],
      "<"+oMsgHdr0.messageId+"> <"+oMsgHdr1.messageId+">");
  });

  press_delete(mc);
}

code 2:
Here is what forward_selected_messages_and_go_to_drafts_folder() is like.

The lines marked with "*->*" at the beginning of the line must be the
ones that create a compose window, and then later cause the
simulated/software interrupt(!).

I think on a modern multi-core CPU/hardware/efficient library, the
code between the two parts can get executed so fast that the remote TB
as COM object may not be able to keep up with such quick inquiries.

    function forward_selected_messages_and_go_to_drafts_folder(f) {
      const kText = "Hey check out this megalol link";
*->*  // opening a new compose window
*->*  cwc = f(mc);
      cwc.type(cwc.eid("content-frame"), kText);

      let mailBody = get_compose_body(cwc);
      assert_previous_text(mailBody.firstChild, [kText]);

      plan_for_window_close(cwc);
      // mwc is modal window controller
      plan_for_modal_dialog("commonDialog", function click_save (mwc) {
	  //accept saving
	  mwc.window.document.documentElement.getButton('accept').doCommand();
	});

*->*  // quit -> do you want to save ?
*->*  cwc.window.goDoCommand('cmd_close');
      // wait for the modal dialog to return
      wait_for_modal_dialog();
      // actually quite the window
      wait_for_window_close();

      let draftsFolder = MailServices.accounts.localFoldersServer.rootFolder.getChildNamed("Drafts");
      be_in_folder(draftsFolder);
    }

It may be possible that by adding a delay before the
cwc.window.goDoCommand('cmd_close'), the error may not manifest
itself, but the important thing is to make TB robust and so I think
my patch (less the printing of g_gazonk_foobar) ought to be the
correct patch.).

TIA

Comment 8

[ISHIKAWA, Chiaki]

Attachment: Patch in progress (Work In Progress) with more sleep() calls, and one logical change.

This is the patch to test aceman's suggestion (proved right), and
additional delay in test programs that, as a total, eliminated
the "gMsgCompose is null" errors.
(Hmm, I may miss another patch. I will sort this out once the feedback is in.)

Comment 9

[ISHIKAWA, Chiaki]

Hmm, I am afraid that the patch incorporated the delay(s) to fix
nBox.getNotificationWithValue is not a function 
error (Bug 912151) by mistake.
(I am chasing too many errors at the same time, I suppose.) 
But the main change still stands.

TIA

Comment 10

[:aceman]

Comment on attachment 805992 [details] [diff] [review]
Patch in progress (Work In Progress) with more sleep() calls, and one logical change.

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

I don't think we can add so mamy sleeps into the tests. If those are really needed then there is some fundamental error to be fixed here.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +121,5 @@
>                           .createInstance(Components.interfaces.nsIMessenger);
>  
>    gMsgCompose = null;
> +  g_gazonk_foobar = 1234;
> +

Could you please think out a different value here?

@@ +159,5 @@
>    _gComposeBundle = null;
>    MailServices.mailSession.RemoveMsgWindow(msgWindow);
>    msgWindow = null;
> +
> +  g_gazonk_foobar = 1234;

And a different value here?

So that we get this flow:
InitializeGlobalVariables -> value1
ComposeStartup start -> value2 (1234)
ComposeStartup finish -> value3 (7890)
ReleaseGlobalVariables -> value4

Then we can see in which state the gMsgCompose is null happens.

In the current state with the output value of 1234, you do not really know, if the error happens before ComposeStartup is finished, or after ReleaseGlobalVariables is run.

Comment 11

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #10)
> Comment on attachment 805992 [details] [diff] [review]
> Patch in progress (Work In Progress) with more sleep() calls, and one
> logical change.
> 
> Review of attachment 805992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think we can add so mamy sleeps into the tests. If those are really
> needed then there is some fundamental error to be fixed here.
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +121,5 @@
> >                           .createInstance(Components.interfaces.nsIMessenger);
> >  
> >    gMsgCompose = null;
> > +  g_gazonk_foobar = 1234;
> > +
> 
> Could you please think out a different value here?
> 
> @@ +159,5 @@
> >    _gComposeBundle = null;
> >    MailServices.mailSession.RemoveMsgWindow(msgWindow);
> >    msgWindow = null;
> > +
> > +  g_gazonk_foobar = 1234;
> 
> And a different value here?
> 
> So that we get this flow:
> InitializeGlobalVariables -> value1
> ComposeStartup start -> value2 (1234)
> ComposeStartup finish -> value3 (7890)
> ReleaseGlobalVariables -> value4
> 
> Then we can see in which state the gMsgCompose is null happens.
> 
> In the current state with the output value of 1234, you do not really know,
> if the error happens before ComposeStartup is finished, or after
> ReleaseGlobalVariables is run.

You are right. Here is what happened.

I modified the values assigned to g_gazonk_foobar.

>So that we get this flow:
>InitializeGlobalVariables -> value1
>ComposeStartup start -> value2 (1234)
>ComposeStartup finish -> value3 (7890)
>ReleaseGlobalVariables -> value4

With the new modification in my local source, this now becomes 

InitializeGlobalVariables -> 1234
ComposeStartup start -> value2 5678
ComposeStartup finish -> value3 7890
ReleaseGlobalVariables -> value4 3456

Then I ran the |make mozmill| test, and found that g_gazonk_foobar has the value of
3456 when gMsgCompose was null.
All other valid cases printed 7890.

So this suggests that ComposeCanClose() is called in a very strange
context.

From the log of selectivly running composition/test-forward-headers.js
tests.

TEST-START | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | setupModule
TEST-PASS | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test-forward-headers.js::setupModule
TEST-START | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test_forward_inline
	   ...
WARNING: g_gazonk_foobar = 7890
	 ...
TEST-PASS | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test-forward-headers.js::test_forward_inline
TEST-START | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test_forward_as_attachments
	   ...
WARNING: g_gazonk_foobar = 7890
	 ...
TEST-PASS | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test-forward-headers.js::test_forward_as_attachments
TEST-START | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | teardownModule
TEST-PASS | /REF-COMM-CENTRAL/comm-central/mail/test/mozmill/composition/test-forward-headers.js | test-forward-headers.js::teardownModule
WARNING: g_gazonk_foobar = 3456
WARNING: gMsgCompose was null in ComposeCanClose().

---

By the way, here is an observation:
Can it be that the error happens when teardownModule is executed by |make mozmill|?

I must be carefull here since I am not entirely sure how
the following are intermixed:
- the output by dump() inside main TB code 
- the output from individual test programs of |make mozmill|
- the output from the whole |mozmill| test suite.
(The buffering of output routines, stderr vs stdout,and the manner |mozmill| test suite
may bundle the output from various subsystems with some magic :-)
may interfere with our straight-forward interpretation.)

But from the look of 
egrep -1 "(TEST-|gazonk_foobar|gMsgCompose)"
/FF-NEW/log261-mozmill-ref.txt
(the last filename is the logfile of 
make  SOLO_TEST=composition/test-forward-headers.js mozmill-one
)

which is summarized by removing uninteresting lines as above,
I have a suspicion the error may be printed during shutdown process.

But this may be a premature conjecture.  In a log where all the
targets are executed, it was not so obvious.  Maybe we need more
investigation in parallel with hardening the main TB code.

But one small progress.

TIA

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: A fix

The root cause is that ComposeCanClose() is invoked from messageComposeOfflineQuitObserver even if the compose window has been already closed.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 809044 [details] [diff] [review]
A fix

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

You could just check if(!msgWindow), no? Or possibly if (!msgWindow || msgWindow.closed)

Comment 14

[Magnus Melin [:mkmelin]]

hiro: can you make an updated patch?

Comment 15

[ISHIKAWA, Chiaki]

Hi, 

Sorry for my interruption from the side.
I found that, in my local DEBUG BUILD of TB, using |&&(!msgwindow)| check
near line 1458 as suggested in Comment 13 seems to work.
(I don't think it introduced a new bug or anything after running |make mozmill| several times.)

However, I would think Hiro's use of different variables with meaningful name may make
the source code more maintainable or understandable. 
Well, this is a subjective matter, and
there is probably a desire to reduce clutter in global name space. Sor YMMV.

Just my two cents worth.

TIA

Comment 16

[ISHIKAWA, Chiaki]

Sorry, actually the check should be |&& msgWindow|. Am I missing something here?
Hmm...

Comment 17

[:aceman]

Looks like this has been fixed in bug 1250605, see bug 1250605 comment 3, which specifically mentions this case.
The added check tests !gMsgCompose so see if composition is still up, and if no, it exits cleanly.

Comment 18

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #17)

Looks like this has been fixed in bug 1250605, see bug 1250605 comment 3,
which specifically mentions this case.
The added check tests !gMsgCompose so see if composition is still up, and if
no, it exits cleanly.

Thank you for the final follow-up of this bug report!