Last Comment Bug 738907 - 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 te...
Status: RESOLVED FIXED
: regression
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: All All
: -- critical (vote)
: Thunderbird 14.0
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on:
Blocks: 389650
  Show dependency treegraph
 
Reported: 2012-03-24 03:46 PDT by Thomas D. (needinfo?me)
Modified: 2012-04-03 14:24 PDT (History)
9 users (show)
bugzilla2007: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed


Attachments
My debug log on Win 7 looks like we are crashing in the system (18.67 KB, text/plain)
2012-03-26 01:25 PDT, Ludovic Hirlimann [:Usul]
no flags Details
proposed fix (3.10 KB, patch)
2012-03-26 12:30 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
improve comment (3.26 KB, patch)
2012-03-27 08:03 PDT, David :Bienvenu
mkmelin+mozilla: review-
Details | Diff | Splinter Review
fix addressing comments (3.43 KB, patch)
2012-03-27 13:23 PDT, David :Bienvenu
mkmelin+mozilla: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Thomas D. (needinfo?me) 2012-03-24 03:46:13 PDT
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
Comment 1 Thomas D. (needinfo?me) 2012-03-24 03:57:40 PDT
(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.
Comment 2 Jb Piacentino 2012-03-24 04:05:04 PDT
Can't reproduce on nightly 23/03/2012 with windows 7.
Comment 3 Thomas D. (needinfo?me) 2012-03-24 05:27:11 PDT
I see this bug in safe mode, too (with Windows XP).
Comment 4 Magnus Melin 2012-03-24 05:37:49 PDT
For crashes it's really useful to get the crash report id. Do you have an id?
(Doesn't crash on linux either.)
Comment 5 Thomas D. (needinfo?me) 2012-03-24 06:13:39 PDT
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
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2012-03-24 06:20:29 PDT
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?
Comment 7 David :Bienvenu 2012-03-24 06:24:05 PDT
I haven't seen this either. So more information about how to reproduce would be helpful for our QA efforts.
Comment 8 Thomas D. (needinfo?me) 2012-03-24 07:09:07 PDT
(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.
Comment 9 Wayne Mery (:wsmwk, NI for questions) 2012-03-24 07:17:03 PDT
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)
Comment 10 David :Bienvenu 2012-03-24 07:20:17 PDT
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.
Comment 11 David :Bienvenu 2012-03-24 07:25:32 PDT
this sounds like an error in the code that keeps track of how many open windows there are...
Comment 12 Thomas D. (needinfo?me) 2012-03-24 07:47:02 PDT
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...
Comment 13 Thomas D. (needinfo?me) 2012-03-24 07:51:39 PDT
(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?
Comment 14 Thomas D. (needinfo?me) 2012-03-24 07:58:35 PDT
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.
Comment 15 David :Bienvenu 2012-03-24 10:05:11 PDT
Do you have any add-ons installed? I'm not seeing this with local drafts either.
Comment 16 David :Bienvenu 2012-03-24 11:10:11 PDT
(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.
Comment 17 Thomas D. (needinfo?me) 2012-03-24 13:25:53 PDT
(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.
Comment 18 Thomas D. (needinfo?me) 2012-03-24 13:39:53 PDT
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?
Comment 19 Wayne Mery (:wsmwk, NI for questions) 2012-03-24 13:44:45 PDT
suggest you identify the regression range
Comment 20 Thomas D. (needinfo?me) 2012-03-24 13:45:47 PDT
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.
Comment 21 rsx11m 2012-03-24 14:45:04 PDT
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).
Comment 22 rsx11m 2012-03-24 14:47:18 PDT
(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).
Comment 23 rsx11m 2012-03-24 15:29:19 PDT
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.
Comment 24 Joe Sabash [:JoeS1] 2012-03-24 17:46:29 PDT
FWIW
Fails also in:
Mozilla/5.0 (Windows NT 5.0; rv:12.0) Gecko/20120321 Thunderbird/12.0
Comment 25 Thomas D. (needinfo?me) 2012-03-25 01:11:31 PDT
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...
Comment 26 Thomas D. (needinfo?me) 2012-03-25 04:56:29 PDT
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?
Comment 27 Wayne Mery (:wsmwk, NI for questions) 2012-03-25 05:12:49 PDT
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" ?
Comment 28 Magnus Melin 2012-03-25 23:21:57 PDT
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)!
Comment 29 Ludovic Hirlimann [:Usul] 2012-03-26 00:52:17 PDT
Can anyone of the reproducing guy, Thomas, rsx11 or JoeS capture the stack trace when this happens using windbg ?
Comment 30 Ludovic Hirlimann [:Usul] 2012-03-26 01:02:38 PDT
Ok I'm reproducing , trying to get a stack trace. FWIW it's related to draft being saved into local folders.
Comment 31 Ludovic Hirlimann [:Usul] 2012-03-26 01:25:13 PDT
Created attachment 609242 [details]
My debug log on Win 7 looks like we are crashing in the system
Comment 32 Mark Banner (:standard8, afk until Dec) 2012-03-26 02:41:17 PDT
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.
Comment 33 Mark Banner (:standard8, afk until Dec) 2012-03-26 03:09:55 PDT
Oh also, trying a current build without the patch for bug 389650 would be useful.
Comment 34 rsx11m 2012-03-26 06:35:12 PDT
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.
Comment 35 David :Bienvenu 2012-03-26 07:42:13 PDT
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.
Comment 36 David :Bienvenu 2012-03-26 08:04:27 PDT
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.
Comment 37 David :Bienvenu 2012-03-26 09:07:14 PDT
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.
Comment 38 rsx11m 2012-03-26 10:39:53 PDT
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.
Comment 39 David :Bienvenu 2012-03-26 12:30:31 PDT
Created attachment 609433 [details] [diff] [review]
proposed fix

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.
Comment 40 Magnus Melin 2012-03-26 12:51:09 PDT
Indeed must be some recycling problem, setting mail.compose.max_recycled_windows 0 "fixes" it.
Comment 41 Thomas D. (needinfo?me) 2012-03-27 03:04:10 PDT
(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).
Comment 42 David :Bienvenu 2012-03-27 07:11:21 PDT
(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.
Comment 43 David :Bienvenu 2012-03-27 08:03:53 PDT
Created attachment 609711 [details] [diff] [review]
improve comment
Comment 44 Magnus Melin 2012-03-27 13:00:38 PDT
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.
Comment 45 David :Bienvenu 2012-03-27 13:23:19 PDT
Created attachment 609862 [details] [diff] [review]
fix addressing comments

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?
Comment 46 Thomas D. (needinfo?me) 2012-03-27 13:56:59 PDT
(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 47 Magnus Melin 2012-03-28 04:00:10 PDT
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.
Comment 48 David :Bienvenu 2012-03-28 07:03:33 PDT
(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.
Comment 49 David :Bienvenu 2012-03-28 07:24:11 PDT
http://hg.mozilla.org/comm-central/rev/5fda17fdf779
Comment 50 Mark Banner (:standard8, afk until Dec) 2012-04-03 14:24:07 PDT
Checked into branches:

http://hg.mozilla.org/releases/comm-aurora/rev/824e9a87e413
http://hg.mozilla.org/releases/comm-beta/rev/f23162a04301

Note You need to log in before you can comment on or make changes to this bug.