1250605 - Fix some "gMsgCompose is undefined" TypeErrors

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[aleth [:aleth]]

TypeError: gMsgCompose is null in MsgComposeCommands.js

Comment 1

[aleth [:aleth]]

Attachment: Fix some "gMsgCompose is undefined" TypeErrors

Comment 2

[Jorg K (CEST = GMT+2)]

How do your reproduce the problem? Are we hiding a deeper problem here?

Comment 3

[aleth [:aleth]]

This happens when you have used message compose and then quit with no open compose window, as the messageComposeOfflineQuitObserver fires and calls ComposeCanClose. In this circumstance the minimal changes I made make sense. 

I guess the observer isn't removed at the same time as gMsgCompose because it's possible that the message is still being sent despite the composer being closed, but I don't know that code flow well enough to know.

Comment 4

[Jorg K (CEST = GMT+2)]

I've been trying to reproduce the bug for a long time. I close the application by clicking on the (x) on the main window. That doesn't reproduce the bug.

I can however reproduce it by opening a compose window, closing it without doing anything and then closing the application from the File menu. I guess Apple people just type (Apple)Q.

May I suggest another patch:
Do not call ComposeCanClose() from messageComposeOfflineQuitObserver() if !gMsgCompose.
If no composition is in progress it makes no sense to ask if the composition can be closed. Alternatively, return from ComposeCanClose() straight away. I'm undecided, maybe the second option is best, like:
// Huh? Composition already gone away, nothing to test here.
if (!gMsgCompose)
  return.

Can you do my another favour. I've been playing around with this and closed both main window and composition window while a send was still in progress (sent 10 MB worth of pictures). This resulted in:
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 167: TypeError: gMsgCompose is null
The line number may be wrong due to my added debug. It's in updateEditableFields(). There we should have a:
if (!gMsgCompose) return.

Comment 5

[Jorg K (CEST = GMT+2)]

Sorry, return true in the first case.

Comment 6

[aleth [:aleth]]

Attachment: Fix some "gMsgCompose is undefined" TypeErrors

Comment 7

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #4)
> May I suggest another patch:
> Do not call ComposeCanClose() from messageComposeOfflineQuitObserver() if
> !gMsgCompose.
> If no composition is in progress it makes no sense to ask if the composition
> can be closed. Alternatively, return from ComposeCanClose() straight away.

Good idea.

The number of gMsgCompose checks in this file makes me suspect the code is badly structured in some way though. There is also no shutdown blocker, so I hope the mechanism that ensures sends complete during shutdown is robust enough...

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8722720 [details] [diff] [review]
Fix some "gMsgCompose is undefined" TypeErrors

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

This makes more sense, don't you think?

Comment 9

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #8)
> Comment on attachment 8722720 [details] [diff] [review]
> Fix some "gMsgCompose is undefined" TypeErrors
> 
> Review of attachment 8722720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This makes more sense, don't you think?

See comment 7 ;)

Comment 10

[Jorg K (CEST = GMT+2)]

Looks like wires crossed here a bit.

Let me remove the recycling in bug 777732 and we will have less complexity. The logic will be simpler then: Open a window, do stuff, close and destroy it and all that goes with it. No funny recycled/hidden stuff hanging around.

Comment 11

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #10)
> Looks like wires crossed here a bit.
> 
> Let me remove the recycling in bug 777732 and we will have less complexity.
> The logic will be simpler then: Open a window, do stuff, close and destroy
> it and all that goes with it. No funny recycled/hidden stuff hanging around.

Ah, I guess that explains it :-)

Comment 12

[Jorg K (CEST = GMT+2)]

Yes, what you saw was the observer of the recycled window being run. If you click on the (x) on the main window, the observer is removed before it triggers, not so via "Exit" from the menu. I added a lot of debug to see what happens ;-(

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/599fd18da6c144abcc9b7feec4ee30b92e9d7bfc
Bug 1250605 - Fix some "gMsgCompose is undefined" TypeErrors. r=jorgk

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8722720 [details] [diff] [review]
Fix some "gMsgCompose is undefined" TypeErrors

[Approval Request Comment]
Regression caused by (bug #): No regression, wrong from day one.
Risk to taking this patch (and alternatives if risky):
Not risky, just adverting JS runtime errors due to access via variable with is null.

Comment 15

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/b491a878367b
Landed with DONTBUILD since I want to try put the self-service at
https://secure.pub.build.mozilla.org/buildapi/self-serve

Comment 16

[Jorg K (CEST = GMT+2)]

s/try put/try out/

I visited https://secure.pub.build.mozilla.org/buildapi/self-serve/comm-aurora and requested a new nightly build for rev b491a878367b. See what happens tomorrow.

Comment 17

[Kent James (:rkent)]

Comment on attachment 8722720 [details] [diff] [review]
Fix some "gMsgCompose is undefined" TypeErrors

http://hg.mozilla.org/releases/comm-esr45/rev/dcc36613598f