Closed Bug 738907 Opened 12 years ago Closed 12 years ago

Using red [x] button to close & save draft composition window unexpectedly terminates TB application

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
critical

Tracking

(thunderbird12+ fixed, thunderbird13 fixed)

RESOLVED FIXED
Thunderbird 14.0
Tracking Status
thunderbird12 + fixed
thunderbird13 --- fixed

People

(Reporter: thomas8, Assigned: Bienvenu)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

TB Trunk 14.0a1 2012-03-23 on win xp

STR

1 click Write for a new msg
2 attach file (smaller or bigger than bigfiles threshold)
3 close msg window using red [x] button (important!)
4 from save message dialogue, click Save

Actual result

- TB main application is unexpectedly closed (reproducable every time)
- draft is correctly saved
- bug does not occur when closing msg window with File > Close or Ctrl+W
- bug does not occur in TB11, so this is a recent regression

Expected result

- TB shouldn't die from closing a draft, just closing the draft would be fine
- improve our system of QA to prevent such major bugs in basic functionality from being introduced unnoticed
Summary: Bad failure: Saving drafts with attachments closes TB application → Bad failure: Using red [x] button to close & save draft with attachments shuts down TB application
(In reply to Thomas D. from comment #0)
> STR
> 1 click Write for a new msg
> 2 attach file (smaller or bigger than bigfiles threshold)

Worse, this actually happens for *any* draft with or without attachments, so for STR, step 2, just write "hello world" into subject or body so that you get the save message dialogue, then click Save.

I believe we must have a serious problem in our QA system.
Summary: Bad failure: Using red [x] button to close & save draft with attachments shuts down TB application → Bad failure: Using red [x] button to close & save draft composition shuts down TB application
Can't reproduce on nightly 23/03/2012 with windows 7.
I see this bug in safe mode, too (with Windows XP).
For crashes it's really useful to get the crash report id. Do you have an id?
(Doesn't crash on linux either.)
Keywords: crash
If you mean about:crashes from "Help > Troubleshooting Information", then there's nothing, unfortunately (only two crash report ids from 2010).

I get this error after startup:

Timestamp: 24.03.2012 13:58:22
Error: Attempting to set a null username to an imIncomingServer
Source File: resource:///components/imIncomingServer.js
Line: 180

After following the STR of this bug, I get two errors (and TB doesn't shut down, because of the error console win):

Timestamp: 24.03.2012 13:58:57
Error: footer is null
Source File: resource:///modules/LightweightThemeConsumer.jsm
Line: 92

Timestamp: 24.03.2012 13:59:07
Error: gMsgCompose is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 2844
I can't produce. But I didnot try safe mode

> I believe we must have a serious problem in our QA system.

does same thing happen if you use ctrl+w or file | close?
I haven't seen this either. So more information about how to reproduce would be helpful for our QA efforts.
(In reply to David :Bienvenu from comment #7)
> I haven't seen this either.

David, Wayne, pls specify on which OS and which OS version you cannot reproduce.
So far, this bug is reported against Windows XP only.

(In reply to Wayne Mery (:wsmwk) from comment #6)
> does same thing happen if you use ctrl+w or file | close?

As stated in comment 0, no: This bug only occurs when clicking on red [x] to close the msg composition window.
> 3 close msg window using red [x] button (important!)
> - bug does not occur when closing msg window with File > Close or Ctrl+W

(In reply to David :Bienvenu from comment #7)
> So more information about how to reproduce would be helpful for our QA efforts.

David, did you see my comment 5 which includes *errors from error console*, some of which are clearly related to this bug?
(Unfortunately, again per my comment 5, there are no crash reports.)

To my best knowledge, all available information for reproducing is available on this bug, and I can reproduce every time on windows XP with or without safe mode.
It's important to pay attention to the details. If you don't follow STR exactly, you will not see this bug.

Fwiw, here's the all-in-one summary:

STR

0.1) Ensure your OS is Windows XP

0.2) Ensure you do *not* have Error Console open (as long as Error console is open, TB will not shut down on errors so you will not immediately see the effect of this bug; but you can still confirm the effect because as soon as you close error console after the error occured, TB will shut down).

1) use mouse to click on "Write" button (which will open a new msg composition window)

2) mouse click to focus message body, type "hello world" as message text (we need something to trigger the "Save Message" dialogue lateron)

3) use mouse(!) to click on red [x] button (upper right corner of composition window) to close the draft (which will trigger "Save message" dialogue)
(DO NOT use Ctrl+W or File > Close as they do not trigger this bug.)

4) From "Save Message" dialogue, click "Save" (!)
(DO NOT choose "Don't Save", as you will not see this bug if you do.)

Actual result

- composition window is closed
- draft is correctly saved
- unfortunately, TB main window also disappears silently (this bug)

If you repeat STR with "Error console" open, TB will *not immediately* shut down; instead you will see some errors as reported in comment 5. When you close error console, TB main window will vanish (this bug).

The errors described in comment 5 should be examined to narrow down the cause of this bug.
vista. i don't ahve XP any more.

> (Unfortunately, again per my comment 5, there are no crash reports.)
prob not a crash - apparently it just terminates on an improper code path

(Fails to see how this is worse than any of our other "crashes" or that this is a colossal QA failure)
windows 7, but I kinda doubt it's specific to xp . I suspect it more has to do with your profile, or something specific about your steps/add-ons/profile, etc. The fact that gMsgCompose is null is probably relevant. the footer warning is not.
this sounds like an error in the code that keeps track of how many open windows there are...
I hasten to add that this bug occured for a draft on a *POP3 account* (didn't test for IMAP).

(In reply to Wayne Mery (:wsmwk) from comment #9)
> prob not a crash - apparently it just terminates on an improper code path

I think so too

> (Fails to see how this is worse than any of our other "crashes" or that this
> is a colossal QA failure)

Given the more limited nature of this bug, I'm sorry that my criticism of QA systems was probably too harsh.

Take it as a sign of childlike trust in TB: There are still times when I'm naively hoping to do a couple of routine things in TB without running into old and new bugs and missing basic features. Alas, it's dreams...
Summary: Bad failure: Using red [x] button to close & save draft composition shuts down TB application → Win XP, POP3: Using red [x] button to close & save draft composition window shuts down TB application
(In reply to David :Bienvenu from comment #11)
> this sounds like an error in the code that keeps track of how many open
> windows there are...

David, any idea why this occurs only when using the red [x] for closing the window, but *not* when using File > Close or Ctrl+W? Are these methods not using the same command?
This might help:

STR as above, but using File > Close instead of red [x] button, creates the following error:

Timestamp: 24.03.2012 15:52:33
Error: An error occurred executing the cmd_close command: [Exception... "'[JavaScript Error: "gMsgCompose is null" {file: "chrome://messenger/content/messengercompose/MsgComposeCommands.js" line: 2844}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goDoCommand :: line 96"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 100

So the same error (MsgComposeCommands.js, line 2844) also occurs for other methods of closing, but TB survives.
Summary: Win XP, POP3: Using red [x] button to close & save draft composition window shuts down TB application → Using red [x] button to close & save draft composition window shuts down TB application (seen on Win XP, POP3)
Do you have any add-ons installed? I'm not seeing this with local drafts either.
(In reply to Thomas D. from comment #13)
> David, any idea why this occurs only when using the red [x] for closing the
> window, but *not* when using File > Close or Ctrl+W? Are these methods not
> using the same command?
they start off in different places in the window manager and toolkit, but they end up in the same place in our code. So no, I really don't know. I assume an exception is getting thrown in our code and perhaps the toolkit code doesn't handle that exception correctly. But I don't know why gMsgCompose got set to null.
(In reply to David :Bienvenu from comment #15)
> Do you have any add-ons installed? I'm not seeing this with local drafts
> either.

Well, it also happens in safe mode, and I only have very few addons for Daily:
- Jim's attachment options 1.0
- Jim's attachment tree 1.0b1
- German dictionary 2.02
- Test Pilot for TB 1.3.7

Furthermore, I also see this bug on *Win7* Starter 32 Bit:
- fresh install of tb14.0a1 2012-03-24
- new profile, single pop3 account
- safe mode (no addons except Test pilot)
- all *plugins* manually disabled (there are a lot of *Plugins* listed)

I initially made one mistake when installing and started daily with old profile once, but then used daily's profile manager to create a new profile, then started with that new profile on Win7 as described above. I *do* see the same bug there.
Both on WinXP and Win7, I am starting TB Daily with a specific non-default profile like this:
C:\Program Files\Daily\thunderbird.exe -p MyDailyProfile -no-remote
I don't have Daily set to be my default mail handler.

Any other information that might be of interest?
suggest you identify the regression range
More information about conditions of bug:

I use Avira Antivir free personal version as AV scanner, it'll probably check the file before we write the draft to hard disk.
I can reproduce with a recent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120319 Thunderbird/14.0a1 build on Windows 7, IMAP account which is storing drafts in Local Folders. Ctrl+W followed by "Save" works fine, using instead [x]+"Save" unexpectedly terminates the application (no crash report).
(In reply to Thomas D. from comment #0 and comment #20)
> 2 attach file (smaller or bigger than bigfiles threshold)
> I use Avira Antivir free personal version as AV scanner

Neither applies to my set up, this was a simple save-as-draft action after typing some text (using plain-text editor, that is).
This happens with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120312 Thunderbird/13.0a1 already, thus apparently not related to recent BigFiles or InstantMessaging landings. Observed in both plain-text/HTML modes, no add-ons.
FWIW
Fails also in:
Mozilla/5.0 (Windows NT 5.0; rv:12.0) Gecko/20120321 Thunderbird/12.0
rsx11m, Joe, thanks! So I'm no longer the lonely only one to see this. Several versions of TB are affected, and it has been seen on both WinXP and Win7.
I wonder why some others did *not* see this on Win7, but anyway...
Summary: Using red [x] button to close & save draft composition window shuts down TB application (seen on Win XP, POP3) → Using red [x] button to close & save draft composition window unexpectedly terminates TB application
Summary: Using red [x] button to close & save draft composition window unexpectedly terminates TB application → Using red [x] button to close & save draft composition window unexpectedly terminates TB application (on Windows)
As a penance for my little QA clamor, I installed 11 builds to identify the *REGRESSION RANGE* - here it is:

Daily12 2011-12-30 does *not* show this bug on Windows XP:
Mozilla/5.0 (Windows NT 5.1; rv:12.0a1) Gecko/20111230 Thunderbird/12.0a1

Daily12 2011-12-31 *does* show this bug on Windows XP:
Mozilla/5.0 (Windows NT 5.1; rv:12.0a1) Gecko/20111231 Thunderbird/12.0a1

Can somebody provide some details (appropriate queries, urls etc.) how to proceed from here to identify the bugs that landed in that one-day regression window?
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-12-30%2004:00:00&enddate=2012-12-31%2008:00:00
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2011-12-30%2004:00:00&enddate=2011-12-31%2008:00:00

always nice when not much has checked in during a regression range :) although f9d611e3d0a5Mark Banner — Bug 695256 - Switch from PRBool to bool and replace is a huge changeset.  

Is failure in Magnus' http://hg.mozilla.org/comm-central/rev/6846848bd7d0 Bug 389650 - "Unable to save your message as a draft" ?
If i'd had to guess, yes, that may have exposed something.
I was able to reproduce this, but it doesn't seem to be a real crash. Strangely enough, if you have the error console open, thunderbird stays open as long as that's open (and then closes)!
Keywords: crash
Can anyone of the reproducing guy, Thomas, rsx11 or JoeS capture the stack trace when this happens using windbg ?
Ok I'm reproducing , trying to get a stack trace. FWIW it's related to draft being saved into local folders.
The debug log doesn't help much. I think the best thing to do is to try reproducing this in a debug build and examine what is happening there.
Oh also, trying a current build without the patch for bug 389650 would be useful.
Changeset 6846848bd7d0 does no longer apply to trunk, but I've quickly hacked a 12.0b2 build to revert MsgComposeCommands.js only.

Without backing out that patch, 12.0b2 closes as previously described. After reversing the MsgComposeCommands.js changes, no more closing on [x]+"Save".

Thus, it looks indeed like bug 389650 has caused the issue.
Blocks: 389650
I did recreate this in a debug build (my drafts were getting saved to imap folders before). Indeed, the toolkit code does think we are closing the last open window, and exiting the app. It might be that somehow there are two notifications getting sent about the compose window getting closed.
I suspect this has to do with some mis-handling of the compose window caching notifications. By default, we leave a compose window open, but hidden, when the compose window is closed, so that opening a new compose window is fast after the first open. So we manually send a xul-window-destroyed notification, but prevent the xul window from actually getting destroyed. I suspect this path through the code is now destroying the xul window and sending the extra notification, resulting in the toolkit app code getting confused about how many windows are open.
adding a null check at the end of GenericSendMessage to protect against the gMsgCompose exception seems to fix this. Adding a null check for gMsgCompose at the top of GenericSendMessage doesn't fix this, so somehow gMsgCompose is getting nulled out inside the call to GenericSendMessage. Also, I don't know why the exception is causing an extra close notification.
I don't know if it's of any significance to the problem, but I can also reproduce on Linux with drafts stored in Local Folders, exactly the same behavior as on Windows.
OS: Windows XP → All
Hardware: x86 → All
Summary: Using red [x] button to close & save draft composition window unexpectedly terminates TB application (on Windows) → Using red [x] button to close & save draft composition window unexpectedly terminates TB application
Attached patch proposed fix (obsolete) — Splinter Review
this catches the exception in the call to GenericSendMsg, and prevents the exception from happening.

It would be great to have a mozmill test for this, but if it requires actually clicking the red x, that would be challenging.
Assignee: nobody → dbienvenu
Attachment #609433 - Flags: review?(mkmelin+mozilla)
Indeed must be some recycling problem, setting mail.compose.max_recycled_windows 0 "fixes" it.
(In reply to David :Bienvenu from comment #39)
> Created attachment 609433 [details] [diff] [review]
> proposed fix
> this catches the exception in the call to GenericSendMsg, and prevents the
> exception from happening.

+          // Need to catch exception because this call can cause the window
+          // to close and we need to be sure we return false.

Perhaps this should read:

+          // Need to catch exception because this call can cause the *application*
+          // to *terminate* and we need to be sure we return false.

And since it seems we don't fully understand this, shouldn't there be a reference to this bug in the comment?

> It would be great to have a mozmill test for this, but if it requires
> actually clicking the red x, that would be challenging.

You could send Alt+F4 on windows (which will currently trigger this bug).
(In reply to Thomas D. from comment #41)
> (In reply to David :Bienvenu from comment #39)
> > Created attachment 609433 [details] [diff] [review]
> > proposed fix
> > this catches the exception in the call to GenericSendMsg, and prevents the
> > exception from happening.
> 
> +          // Need to catch exception because this call can cause the window
> +          // to close and we need to be sure we return false.
> 
> Perhaps this should read:
> 
> +          // Need to catch exception because this call can cause the
> *application*
> +          // to *terminate* and we need to be sure we return false.
Because that would be not quite correct and *less* informative as to the issue. But I can extend the comment to explain why returning false and closing the window at the same time can be bad.

> 
> And since it seems we don't fully understand this, shouldn't there be a
> reference to this bug in the comment?

checkin comments refer to the bug #, so the history is easily discoverable, and I don't like sprinkling bug #s in comments throughout the code.

> 
> > It would be great to have a mozmill test for this, but if it requires
> > actually clicking the red x, that would be challenging.
> 
> You could send Alt+F4 on windows (which will currently trigger this bug).

Ah, thx, I didn't see that in the bug. Odd that Alt f4 works differently than cntrl W.
Flags: in-testsuite?
Attached patch improve comment (obsolete) — Splinter Review
Attachment #609433 - Attachment is obsolete: true
Attachment #609433 - Flags: review?(mkmelin+mozilla)
Attachment #609711 - Flags: review?(mkmelin+mozilla)
Comment on attachment 609711 [details] [diff] [review]
improve comment

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

Sure would be nice to get the root cause for this. 
The patch does fix the bug for me.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +2844,5 @@
>      enableEditableFields();
>      updateComposeItems();
>    }
> +  if (originalCharset != saveMsgCompose.compFields.characterSet)
> +    SetDocumentCharacterSet(saveMsgCompose.compFields.characterSet);

This is a no-op, as SetDocumentCharacterSet needs gMsgCompose :/

@@ +3387,5 @@
> +          // We catch the exception because this call can cause the window
> +          // to close. If we close the window ourselves and don't catch the
> +          //  exception, the toolkit code that keeps track of the open windows
> +          // gets off by one and the app can close unexpectedly on windows.
> +          try {

Nit: extra space before exception.
Also, this is not just windows, it's seen (at least) on linux too as well.
Attachment #609711 - Flags: review?(mkmelin+mozilla) → review-
ok, so it doesn't matter if gMsgCompose is null when closing after a save, since we don't need to update the window title, so I can just check for null gMsgCompose

I've tried to enhance the comment to better explain what's going on. Does that help explain it better?
Attachment #609711 - Attachment is obsolete: true
Attachment #609862 - Flags: review?(mkmelin+mozilla)
(In reply to David :Bienvenu from comment #45)
> Created attachment 609862 [details] [diff] [review]
> fix addressing comments
> I've tried to enhance the comment to better explain what's going on. Does
> that help explain it better?

As far as I'm concerned - yes, very much so. Thank you, David, for the quick patch, too!
Comment on attachment 609862 [details] [diff] [review]
fix addressing comments

Thx, looks good! r=mkmelin

I do still wonder though why gMsgCompose goes away during gMsgCompose.SendMsg for pop, but not imap. It's not exactly common behavior for js variables to null out from the c++ side, and in this case apparently a gMsgCompose function even nulling out its own object.
Attachment #609862 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #47)
> Comment on attachment 609862 [details] [diff] [review]
> I do still wonder though why gMsgCompose goes away during
> gMsgCompose.SendMsg for pop, but not imap.

The save is synchronous for local messages, and gComposeRecyclingListener's onClose method calls ReleaseGlobalVariables, which nulls out gMsgCompose.
http://hg.mozilla.org/comm-central/rev/5fda17fdf779
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Attachment #609862 - Flags: approval-comm-beta?
Attachment #609862 - Flags: approval-comm-aurora?
Attachment #609862 - Flags: approval-comm-beta?
Attachment #609862 - Flags: approval-comm-beta+
Attachment #609862 - Flags: approval-comm-aurora?
Attachment #609862 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: