Closed Bug 1250605 Opened 4 years ago Closed 4 years ago

Fix some "gMsgCompose is undefined" TypeErrors

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(thunderbird45 fixed, thunderbird46 fixed, thunderbird47 fixed)

RESOLVED FIXED
Thunderbird 47.0
Tracking Status
thunderbird45 --- fixed
thunderbird46 --- fixed
thunderbird47 --- fixed

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 1 obsolete file)

TypeError: gMsgCompose is null in MsgComposeCommands.js
Attachment #8722601 - Flags: review?(mozilla)
How do your reproduce the problem? Are we hiding a deeper problem here?
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.
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.
Sorry, return true in the first case.
Attachment #8722720 - Flags: review?(mozilla)
Attachment #8722601 - Attachment is obsolete: true
Attachment #8722601 - Flags: review?(mozilla)
(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 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?
Attachment #8722720 - Flags: review?(mozilla) → review+
(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 ;)
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.
(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 :-)
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 ;-(
https://hg.mozilla.org/comm-central/rev/599fd18da6c144abcc9b7feec4ee30b92e9d7bfc
Bug 1250605 - Fix some "gMsgCompose is undefined" TypeErrors. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 47.0
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.
Attachment #8722720 - Flags: approval-comm-beta?
Attachment #8722720 - Flags: approval-comm-aurora+
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 on attachment 8722720 [details] [diff] [review]
Fix some "gMsgCompose is undefined" TypeErrors

http://hg.mozilla.org/releases/comm-esr45/rev/dcc36613598f
Attachment #8722720 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #8722720 - Flags: approval-comm-beta+ → approval-comm-esr45+
Duplicate of this bug: 916505
You need to log in before you can comment on or make changes to this bug.