136008 - Closing msg compose window last does not shut down Mozilla

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[John Keiser (jkeiser)]

When you close the msg compose window last Mozilla does not shut down.

STEPS:
1. Open browser.
2. Enter URL mailto:[e-mail]?body=ab
3. Close browser.
4. Close msg compose window.

EXPECTED RESULTS: Browser shuts down.

ACTUAL RESULTS: Browser does not shut down.

I observed this in a recent CVS debug build (this morning or last night) as well
as in Linux 2002040606.  jrgm observed on Win2K and Linux, tip optimized builds
from around 4/6/2002 9:00PM PST.

Comment 1

[Antonio.Xu]

I think this bug due to cacheWindows,Because compose have cachewindow,when 
mozilla close compose,it is only close one cachewindow,rather than compose quit.

Comment 2

[Antonio.Xu]

Attachment: This is a new patch for this bug,i'm sure it can fix this bug,please rv my patch.

This patch can fix this bug,so please rv my patch,i has tested my fix.that is
ok.i hope somebody can rv my patch.Thank you

Comment 3

[David :Bienvenu]

you don't need to count all the windows to see if at more than one is open - you
just need to see if there's more than one. Cc'ing JF, since I would have thought
this bug would have come up before.

Comment 4

[Jean-Francois Ducarroz]

I was sure we took care of that issue but apparently no!!! Your patch is not
good enough for the following reasons:

1) It wont works when the compose window is the last one and you press the send
button, we use a diferrent path for closing the window

2) we should not do that on Mac as we never quit the app when the last window
closed.

3) I would rather see that code direclty implemented in C++ in nsMsgCompose.cpp
and should be called from nsMsgCompose::CloseWindow(). That will BTW take care
of issue 1 and an ifdef XP_MAC will take care of issue 2

Comment 5

[Antonio.Xu]

I could add this logic in nsMsgCompose.cpp,then add some code avoid MAC when
build mozilla,add i will change my fix according to your advice.

Comment 6

[Antonio.Xu]

Attachment: According to everybody's advice,i make anew patch for this bug,please rv my patch.

i had move my logic into nsMsgCompose.cpp,and resolve issues of Ducarroz and
Bienvenu,so could rv my patch.Thank you!

Comment 7

[Jean-Francois Ducarroz]

This is a requirement for MAPI, the app need to quit when we close the last
window! Nominating nsbeta1

Comment 8

[Jean-Francois Ducarroz]

This is a compose issue, reassign to myself

Comment 9

[Jean-Francois Ducarroz]

accepting and moving to 1.0

Comment 10

[Jean-Francois Ducarroz]

Comment on attachment 78865 [details] [diff] [review]
According to everybody's advice,i make anew patch for this bug,please rv my patch.

 the function IsLastWindow is too confusing! why not doing something much
simpler like:


PRBool nsMsgCompose::IsLastWindow()
{
  nsresult rv;
  nsCOMPtr<nsIWindowMediator> windowMediator =
	       do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
  if (NS_SUCCEEDED(rv))
  {
    nsCOMPtr<nsISimpleEnumerator> windowEnumerator;
    rv = windowMediator->GetEnumerator(nsnull,
getter_AddRefs(windowEnumerator));
    if (NS_SUCCEEDED(rv))
    {
      nsCOMPtr<nsISupports> isupports;

      if (NS_SUCCEEDED(windowEnumerator->GetNext(getter_AddRefs(isupports)))
	if (NS_SUCCEEDED(windowEnumerator->GetNext(getter_AddRefs(isupports)))
	  return PR_FALSE;
    }
  }

  return PR_TRUE;
}

Also, don't forget to put the function implementation AND definition between
#if !defined(XP_MAC) as it's not needed on Mac!

Comment 11

[Antonio.Xu]

Attachment: I had changed my code due to Bucarroz's advice ,Please rv my patch.

i have changed my patch for this bug,i make it simpler than before,so please rv
my new patch.Thank you.

Comment 12

[Jean-Francois Ducarroz]

Comment on attachment 79219 [details] [diff] [review]
I had changed my code due to Bucarroz's advice ,Please rv my patch.

good job. R=ducarroz

Comment 13

[David :Bienvenu]

Comment on attachment 79219 [details] [diff] [review]
I had changed my code due to Bucarroz's advice ,Please rv my patch.

sr=bienvenu - I'm not sure that we want IsLastWindow to err on the side of
returning TRUE in all the error cases, but it shouldn't cause horrible problems
if we do since it will just mean we won't cache the compose window.

Comment 14

[scottputterman]

Please check this into the trunk and get some testing done on it.  When that's
happened, update the bug.  I'm nominating for adt1.0.0.  Why isn't this needed
on the Mac?

Does this mean that when performing a MAPI send operation that our app never
shuts down?

Comment 15

[David :Bienvenu]

On the mac, closing the last compose window shouldn't shut down the app, since
mac apps stay running even when you close down the last window, instead of
shutting down.

Comment 16

[Antonio.Xu]

Thank's your help.I'm glad to fix this bug.
[e-mail]

Comment 17

[Jean-Francois Ducarroz]

Fixed in the trunk. Thank you [e-mail] for fixing that problem.

Comment 18

[esther]

From reading this bug it looks like:
1.)Mac doesn't need to be tested is that correct?
2.)Testing should include testing around MAPI on windows, is that correct?
3.) Testing the orgiginal scenario on windows, with turbo on - what is expected?
Right now I see a netscp6.exe in the task menu, performing the original test
with turbo on after closing I see (2) netscp6.exe tasks.  If I end one of them
the turbo icon goes away but I still see (1) netscp6.exe running but I'm not
sure if turbo is still on.  Will this fix exit from Turbo too?

Comment 19

[esther]

I took the 4-16 trunk builds for linux  and windows and tested this specific
scenario:
1.) Linux is fixed  - I also tested actually sending the msg it worked too.
2.) Windows is not fixed:
Turbo = off or on
a.) exact scenario = netscp6.exe still running in tasks menu - relaunch app
bypasses Profile Manager.
b.) browser open, mail 3-pane open, compose open, close browser, close 3-pane
mail close composer = netscp6.exe still running in task menu. relaunch app
bypasses Profile Manager.
Note: for windows this happens with the 4-16 trunk and 4-13 branch, it basically
the same behavior as before this fix.

Comment 20

[esther]

reopening so comments can be made to my results of testing on windows.

Comment 21

[scottputterman]

removing adt1.0.0 until there's a new fix.

Comment 22

[Jean-Francois Ducarroz]

Ester, are you sure you test a Windows trunk build and not a branch one? I've
just verified with my trunk debug build pulled this morning at 7:45 am and it
works fine!

Comment 23

[esther]

yes, I took from sweetlou the 2002-04-16-06trunk at 7:26am.  I used an existing
profile, did I need to create a new one?

Comment 24

[Jean-Francois Ducarroz]

Don't need to create a new profile. I am downloading this build as well and will
test it...

Comment 25

[Jean-Francois Ducarroz]

I downloaded the official build (took me 40 minutes), tested it and...it doesn't
work. But guess what? my checkin went in the tree at 6:51am and build
2002-04-16-06trunk like it's name say, it's from 6am, therefore it has 5/6
changes that it has missed my checkin...

Please try again tomorrow or try build 2002-04-16-08-WIN-GMAKE

Closing again as fixed and adding back adt1.0.0 nomination

Comment 26

[esther]

Jean-Francois, in verifying this the original scenario is fixed on windows using
the 4-17 late build. However in testing around this,  I am running into the new
bug in the trunk Crash in Compose (138138), but only when I have opened the
Compose window and sent while the 3-pane window is opened.  
If I compose and send from a mailto link within the browser (mail window never
opened) I don't crash.
If I launch -mail, log in, open a compose window, xclose the 3-pane mail window
and finish composing and sending I don't crash like reported in bug 138138.
I'm going to wait until bug 138138 is fixed before I finish verifying this one.
 I also noticed someone logged another crasher with the 4-17-10 build with a
scenario just like this one (bug 138121) where it crashes when closing a mailto
link compose window. They used the early 4-17 build I used the last 4-17-18
build and do not see this crash.  Another reason to wait to verify this bug on
the trunk.

Comment 27

[esther]

Using trunk build 2002041818 all is well with mailto link xclose and the other
tests I tried in comment #26.  Verified

Comment 28

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checking into the 1.0 branch. Pls check
this in today, then add the fixed1.0.0 keyword.

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 79219 [details] [diff] [review]
I had changed my code due to Bucarroz's advice ,Please rv my patch.

a=[e-mail] for branch checkin

Comment 30

[Jean-Francois Ducarroz]

Fix checked in the branch

Comment 31

[esther]

Using windows branch build 20020423 on winxp this is working OK. 
I tested the original scenario with turbo on, Turbo launch still worked OK.
I tested the original scenario with turbo off, netcp6.exe was not left running
I tested the original scenario, then did a send page (used Simple MAPI) from
word.  Simple MAPI worked as expected.
On linux netscp6.exe shuts down now.  
Still testing one more thing, will update when done

Comment 32

[esther]

Verified on branch changing fixed1.0.0 to verified1.0.0