132641 - -kill needs to force closure of all open windows and exit Mozilla

Status: VERIFIED FIXED

(Core Graveyard :: Cmd-line Features, defect)

Description

[Mike Young]

bug 99940's original intent was morphed to this intent, and has been fixed back.
This bug is to house the new morphed intent.

Essentially, the installer group and others would like -kill (or another
commandline option) to completely shutdown mozilla. 

from Gregg Landskov
"This option is very important for the installer to key off to ensure that users
don't get stuck with apps open and so that all windows will be shut down
smoothly pre-install."

This bug needs to be examined for nsbeta nomination considering bug 99940 was
marked nsbeta1+ in the midst of morphing.

Comment 1

[Peter Trudelle]

GreggL, if this is the work you need for installer, please nominate for MachV.
Also, if bug 99940 (as originally reported) is not required for MachV, please
renominate by removing the plus.

Comment 2

[Mike Young]

adding dependencies from bug 99940 

Sorry for all the spam everyone, lets hope this is all worked out now.

Comment 3

[Gregg Landskov (gone)]

Yes, I would like to nominate for Mach V, adding nsbeta1 keyword.  My
understanding is that there are a few ways that this could be fixed.  I would
defer to dveditz, curt, and the Navigator team about which method is the best.

We could
a.) extend the functionality of the -kill command to shut down browser windows
in addtion to Turbo.
b.) create a new command line option that does the equivalent of Ctrl + Q, shut
down browser windows but not Turbo.
c.) have the installer attempt to close appropriate windows itself without using
command line.

Again, it is very important that have a fix for this. Preventing a denial of
install and losing the download is very bad.

Note:  http://bugzilla.mozilla.org/show_bug.cgi?id=110882 is the same issue.

Comment 4

[Peter Trudelle]

nsbeta1+/adt1 per Nav triage team.

Comment 5

[Aleksey Nogin]

See bug 134909 for a similar request for Linux, so a cross-platform solution
would be nice.

Comment 6

[Stephen P. Morse]

Attachment: -killAll to close all open windows

Comment 7

[Stephen P. Morse]

Attaching preliminary patch.  Not ready for review yet -- needs more testing.  
However if anyone wants to comment on the approach and tell me it's the wrong 
thing to do, speak up now.

Comment 8

[Stephen P. Morse]

Attachment: cleaner patch, ready for review

Comment 9

[Samir Gehani]

Comment on attachment 78104 [details] [diff] [review]
cleaner patch, ready for review

This patch is for windows only.  Do we want to support at least Linux and Mac
OS X for platform parity?

>+var ObserverService = Components.classes["@mozilla.org/observer-service;1"].getService();

Please convert ObserverService to gObserverService here at the declaration and
all callsites.

>+ObserverService = ObserverService.QueryInterface(Components.interfaces.nsIObserverService);
>+if (ObserverService) {
>+  ObserverService.addObserver(killAllObserver, "killAll", false);

Topics are usually of the format ``term1-term2'' rather than intercaps
``term1Term2''.  We should adhere to the convention and name the topic
``kill-all''.

Comment 10

[Samir Gehani]

Bill,
Does the ``quit-application'' topic take care of shutting down the app even when
it is in turbo mode?  That is, does the mozilla process die?

Comment 11

[Stephen P. Morse]

> This patch is for windows only.  Do we want to support at least Linux and
> Mac OS X for platform parity?

That's bug 134909.  And that bug is not nsbeta1.

Comment 12

[Stephen P. Morse]

Attachment: addressing issues in comment #9

Comment 13

[Stephen P. Morse]

Samir wrote:

> Bill,
> Does the ``quit-application'' topic take care of shutting down the app
> even when it is in turbo mode?  That is, does the mozilla process die?

It's my understanding that this is not what is needed.  The installation program 
already knows whether or not turbo mode is running, so it can issue a "mozilla 
-kill" prior to issuing the "mozilla -killAll".

Comment 14

[Daniel Veditz [:dveditz]]

Ideally the installer shouldn't have to call mozilla twice to shut it all the
way down. We want a "-diediedie-we-really-mean-it" command switch.

Comment 15

[Stephen P. Morse]

That's not what the customer asked for.  See comment #3, items b and c.

But I'll try to come up with an all-encompassing die command.

Comment 16

[Daniel Veditz [:dveditz]]

We're not doing the c) approach, and I'm pretty sure Gregg was assuming by b)
that we could pass a single command-line with -kill and the new switch.

If it's the case that we can do "mozilla -kill -killAll" then that's fine

Comment 17

[Stephen P. Morse]

Yes, mozilla -kill -killAll (or -killAll -kill) will work.  I just tried it.

But one little glitch -- you will get the pop-up box saying that you are exiting 
and leaving turbo mode active.  That's because I am doing the killAll before the 
kill.  So I'm about to attach another patch that does kill before killAll, 
thereby avoiding the popup.

Comment 18

[Stephen P. Morse]

Attachment: Process -kill before -killAll if both are present -- avoids alert popup.

Comment 19

[jag (Peter Annema)]

Comment on attachment 78233 [details] [diff] [review]
Process -kill before -killAll if both are present -- avoids alert popup.

>Index: globalOverlay.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/global/resources/content/globalOverlay.js,v
>retrieving revision 1.82
>diff -u -r1.82 globalOverlay.js
>--- globalOverlay.js	22 Oct 2001 11:39:02 -0000	1.82
>+++ globalOverlay.js	8 Apr 2002 21:03:43 -0000
>@@ -1,12 +1,27 @@
>+var killAllObserver = {
>+  observe: function(subject, topic, state) {
>+    if (topic == "kill-all") {
>+      try {
>+        goQuitApplication();
>+      } catch(ex) {
>+      }
>+    }
>+  }
>+}
>+
>+var gObserviceService = Components.classes["@mozilla.org/observer-service;1"].getService();
>+gObserviceService = gObserviceService.QueryInterface(Components.interfaces.nsIObserverService);
>+if (gObserviceService) {
>+  gObserviceService.addObserver(killAllObserver, "kill-all", false);

Since both |getService()| and |QueryInterface()| will throw an exception upon
failure, there is no need for the null check here.

sr=jag

Comment 20

[Stephen P. Morse]

Attachment: remove nullcheck (see comment 19)

Comment 21

[Stephen P. Morse]

Comment on attachment 78346 [details] [diff] [review]
remove nullcheck (see comment 19)

sr=jag (carried forward)

Comment 22

[Samir Gehani]

Comment on attachment 78346 [details] [diff] [review]
remove nullcheck (see comment 19)

Cool.  r=sgehani

Comment 23

[Bill Law]

What happens if the user refuses to close all their windows?  goQuitApplication
(I believe) will prompt the user for windows that have changes pending
(Composer, mail compose).  It would seem like there needs to be some mechanism
for propogating a user's "Cancel" back to the code that originated the request.
 nsIObserver can't do that, I don't think.

Note that regardless of whether we decide "-killAll" should deal with that
eventuality, we still have to deal with it in the bigger scheme of things.  See
bug 14807.  There, we definitely need to handle the case where the user cancels
shutdown and I don't think we want to be putting in place more than one
mechanism for accomplishing the same thing.

Bug 99848 has discussion of a solution that would work for that case, this one,
and bug 14807.

Comment 24

[Stephen P. Morse]

Posting a new patch that uses a separate component (modelled after the resetPref 
component) that gets invoked when a -killAll appears as a command-line argument.  

This component can be modifed to return different values depending on whether 
the user agrees to close all open windows or he refuses to close a window.  That 
should satisfy the objection that Bill Law raised.

It also does a complete shutdown (turbo as well as all windows) in response to a 
killAll so there is no need to issue a kill as well.  This should satisfy Dan 
Veditz' objection (although having -kill -killAll on one command was also 
acceptable to him).

Finally it solves the problem of having a new window open if the command is 
issued when there are no open windows.  That was the concern of bug 99940.

Comment 25

[Stephen P. Morse]

Attachment: Solution modelled on resetPrefs component

Comment 26

[Bill Law]

+    // Turn this on to get debugging messages.
+    debug: true,

This should be false, or removed if it isn't used at all.

+      var windowCount = 0;
+      var appShellService =
Components.classes["@mozilla.org/appshell/appShellService;1"]
+                           .getService(Components.interfaces.nsIAppShellService);
+      var nat = appShellService.nativeAppSupport;
+      if (nat) {
+        if (nat.isServerMode) {
+          windowCount = 1;
+          nat.isServerMode = false;
+        }
+      }

What's up with this windowCount twiddling here?  Can you add a comment
explaining that?

+      var appShell =
+        Components.classes['@mozilla.org/appshell/appShellService;1'].getService();
+      appShell = appShell.QueryInterface(Components.interfaces.nsIAppShellService);

This isn't necessary since the code above already sets appShellService to the
same service object.

+      var nativeAppSupport = null;
+      try {
+        nativeAppSupport = appShell.nativeAppSupport;
+      } catch ( ex ) {
+      }

Ditto this.  But the code above should be written the way it is here (with the
try/catch).  I now remember that ::GetNativeAppSupport will return
NS_ERROR_NULL_POINTER if there isn't one (and there isn't on some platforms).

+    *windowOpened = PR_TRUE;

I'm not sure that's a good idea.  This will execute if -resetPref is used and in
 that case we want the standard "Ensure1Window" logic to kick in.  Is this
necessary for some reason?  Doesn't NS_ERROR_ABORT cause us to bypass the window
opening code downstream?

Also, the test for NS_ERROR_ABORT seems unnecessary since the subsequent code
will do the same thing (correct?).

At this point I got totally lost trying to trace my way back through
LaunchApplicationWithArgs->HandleArbitraryStartup->DoCommandLines while keeping
track of the rv value and making sure it still does the right thing.

Work on the issues above and I'll try to figure this out later.  Is there some
way we can make it not so complicated (not make it *more* complicated, I should
say :-).

Comment 27

[Stephen P. Morse]

> What's up with this windowCount twiddling here?  Can you add a comment
> explaining that?

There is a comment at the point at which windowCount is used.  Namely:

      if (!windowCount) {
        // If we don't abort, we will hang on shutdown if no windows are open.
        // It's the appShell.quit that will cause the hang.  We could skip that
        // but then we will open a window and that would be undesirable
        throw Components.results.NS_ERROR_ABORT;
      }

I'm working on your other comments.

Comment 28

[Stephen P. Morse]

> +    *windowOpened = PR_TRUE;
>
> I'm not sure that's a good idea.  This will execute if -resetPref is used
> and in that case we want the standard "Ensure1Window" logic to kick in.  Is
> this necessary for some reason?  Doesn't NS_ERROR_ABORT cause us to bypass
> the window opening code downstream?

Yes, it's necessary.  The NS_ERROR_ABORT kicks in only when we found that no 
windows were open and turbo was not on.  In that case we had to do abort and 
bypass all the other processing in order to avoid a crash.  The case that I am 
dealing with here (NS_ERROR_NOT_AVAILABLE) is the more normal exit from the 
routine and has to be special-cased in order to avoid an extra window from 
opening.

But you are correct in that resetPrefs is going to come down this same path and 
it indeed needs to get a new window opened.  So my code was wrong.  Therefore I 
propose to fix it by triggering on a different error code, so that the exit from 
errorPrefs can still continue to work the way it used to, and the exit from 
killAll can get this special treatment.  Patch that I will post shortly will 
have a different error code.

Comment 29

[Stephen P. Morse]

Attachment: addresses bill law's issues (comment 26)

Comment 30

[Bill Law]

>There is a comment at the point at which windowCount is used.  Namely:
>
>      if (!windowCount) {
>        // If we don't abort, we will hang on shutdown if no windows are open.
>        // It's the appShell.quit that will cause the hang.  We could skip that
>        // but then we will open a window and that would be undesirable
>        throw Components.results.NS_ERROR_ABORT;
>     }

Perhaps it would help if the comment made sense, or the code (or better yet, 
both :-).  The moral of the story is that when the code makes no sense, I look 
to the comments to explain.  Your comment was almost enough to convince me the 
code was plausible, but now that you've gone and defended it, I was forced to 
dig deeper and see that the code was totally wacky.

First, I don't understand why the "window count" should be bumped just because 
we're in turbo mode.  *That* oddity would deserve its own comment, if it truly 
were the case.

Secondly, I don't see how windowCount counts the open windows.  It starts at 1 
if we're in turbo mode.  And it's incremented for each window.  But I don't see 
where there's any code that decrements it.  The comment says "are open" but 
maybe it should have been "were open?"  The bottom line is that you're *always* 
going down this path (since by definition we have to either be in turbo mode, 
or have windows open, or both).

Thirdly, this code is odd (especially in light of the rest):
+      if (!nativeAppSupport || !nativeAppSupport.isServerMode) {
This test is *always* true, so either we don't need any test, or, we should 
really be testing what you think needs testing, but doing it *properly*.

Fourthly (which sort of follows from the previous two points), I don't see why 
we need to do things differently depending on how many windows were (or are) 
open, or whether we're in turbo mode.  This code should be able to just do it's 
thing, then pass back some result that the calling code can use as a clue to 
not open a window.

Fifthly (and unrelated to the above), now that I think about it, we really 
should turn turbo mode back on if the user cancels the closing of a window.  
Maybe we could get away with not doing that, but I think that would be the 
right thing to do (and it's easy enough).

And lastly, a comment on new code in your updated patch:

+      if (nativeAppSupport) {
+        if (nativeAppSupport.isServerMode) {

I'd prefer
    if ( nativeAppSupport && nativeAppSupport.isServerMode ) {

More clever would be to initialize nativeAppSupport like this:
    var nativeAppSupport = { isServerMode: false; };

which would obviate the need for the null checks.

Comment 31

[Stephen P. Morse]

> Perhaps it would help if the comment made sense, or the code (or better yet,
> both :-).

The comment makes sense if you count turbo as a "virtual" window.  The point is, 
if there are no windows open and no turbo icon on the status bar, then issuing 
the -killAll results in a crash.  So the windowCount variable is an attempt to 
detect that situation and take preventative measures when it occurs.

So maybe windowCount is a bad name for it.  A better name might be 
anythingActive and treating it as a boolean (i.e., initializing it to false and 
setting it to true in the various cases) rather than bumping it as a count.  Or 
is there some other name you would prefer?

> Thirdly, this code is odd (especially in light of the rest):
> +      if (!nativeAppSupport || !nativeAppSupport.isServerMode) {
> This test is *always* true, so either we don't need any test, or, we should 
> really be testing what you think needs testing, but doing it *properly*.

Then we have a problem in globalOverlay.js line 37.  This code was copied 
directly from there, and is what is done when the user does file-quit.

Comment 32

[Bill Law]

>The comment makes sense if you count turbo as a "virtual" window.

In other words, it doesn't make sense :-).

>So maybe windowCount is a bad name for it.  A better name might be 
>anythingActive and treating it as a boolean (i.e., initializing it to false
>and setting it to true in the various cases) rather than bumping it as a 
>count.  Or is there some other name you would prefer?

Yes, I arrived at the same exact conclusion.  "wasMozillaAlreadyRunning" is 
more concrete, I think.

I said previously that windowCount was *always* non-zero.  But I was wrong.  
That is not the case iff Mozilla is not running and the user just strolls up 
and does "mozilla -killAll" out of the blue.  (That kind of detail would be a 
fine thing to note in a comment :-).

I'm really not trying to be difficult.  I didn't understand the code and the 
meager comments didn't help.  If *I* can't understand it, then heaven help 
anybody else.

Now that we've commented the code to my satisfaction, I can review it more 
thoroughly...

...so what happens in cases like "mozilla -turbo -killAll" or "mozilla -url 
http://www.foobar.com -killAll"?  Both those would (I think) result in 
windowCount/wasMozillaAlreadyRunning being set to true (which 
turns "wasMozillaAlreadyRunning" into a bit of a lie, unfortunately; I guess 
that means it should 
be "wasMozillaAlreadyRunningOrHaveWeAlreadyOpenedAWindow" :-).  Aside from 
that, do we want to throw NS_ERROR_ABORT in such cases (your code doesn't)?  If 
not, do we want to call appShell.quit() in those cases (your code does)?  Does 
your code work in those cases?

I don't know the answers to any of those questions off the top of my head, 
which makes me think that I don't understand what's going on sufficiently.  I 
don't like to have to just pray that the code will work.  If it just happens to 
work accidently, then that's sure as heck to break sometime in the future.

My other question involves the distinction between the cases where all the 
windows are closed by -killAll and the cases where they are not.  Your code 
does the same thing regardless, correct?  That doesn't make sense (and there 
are not comments to help me out, again :-).

*Why* does this code need to call appShell.quit() only sometimes?  Why does 
calling it sometimes result in a hang?

>> Thirdly, this code is odd (especially in light of the rest):
>> +      if (!nativeAppSupport || !nativeAppSupport.isServerMode) {
>>This test is *always* true, so either we don't need any test, or, we should 
>>really be testing what you think needs testing, but doing it *properly*.
>
>Then we have a problem in globalOverlay.js line 37.  This code was copied 
>directly from there, and is what is done when the user does file-quit.

Not quite.  *That* code doesn't follow *this* line of code (as it does in your 
patch):
+          nativeAppSupport.isServerMode = false;

I *think* your code will *always* call appShell.quit() if it gets that far.  It 
will get that far whenever wasMozillaAlreadyRunning is false.

It clarifies the code (IMHO) to rewrite that like this:
    if ( wasAlreadyRunning ) {
        // Need to exit appshell in this case.
        appShell.quit();
        // Bypass any downstream window-opening logic.
        throw NS_ERROR_NOT_AVAILABLE;
    } else {
        // This mozilla process hasn't advanced far
        // enough to need, or handle, appshell.quit().
        // This will also bypass window opening logic
        // further down in nsAppRunner.cpp.
        throw NS_ERROR_ABORT;
    }  

That sounds plausible because it might well be the case (I'm theorizing here, 
I'd have to go check the code) that we need to call appShell.quit() iff 
appShell.run() is active (or whatever it is that quit() is the complement of).

Theorizing further, I suspect that "mozilla -turbo -killAll" will crash and 
burn because in that case we'll call appShell.quit() but we won't (yet) have 
got to appShell.run().

Perhaps the whole issue of the appShell.quit() call can be dealt with at the 
points where DoCommandLine is called.  In the case of 
nsNativeAppSupportWin::HandleRequest, then it could detect specific return 
values and do nsIAppShellService::Quit if necessary.  nsAppRunner.cpp code that 
calls it would detect the return values and bail out before calling 
nsIAppShellService::Run.

One advantage to that strategy is that it makes it possible for other twisted 
command line handlers to also cause mozilla to shut down.

Don't forget these other issues:

1. Restoring turbo mode if the user cancels during window closing.
2. Installer package file mods to get this component into mozilla/commercial 
builds.

Comment 33

[Stephen P. Morse]

Posting patch that addresses nearly all of Bill's concerns.  The only 
outstanding problem is that it doesn't yet handle "mozilla -turbo -killAll" and, 
in fact, will crash if that command is issued when no other mozilla is running.

So this obviously needs a bit more work.  But I'm posting it now so that it 
doesn't get lost while I switch gears for a while and do my taxes.  Will resume 
work on this early next week.

Comment 34

[Stephen P. Morse]

Attachment: still need to work on "mozilla -turbo -killAll" case

Comment 35

[Stephen P. Morse]

Posting a separate patch to take care of the crash when -turbo and -killAll are 
issued on the same invocation of mozilla.  Problem is in the windowMediator so 
this patch adds some bulletproofing to that module.

Comment 36

[Stephen P. Morse]

Attachment: bulletproofing to prevent crash when -turbo -killAll appear together

Comment 37

[Bill Law]

The new component (.js file) needs to be added to the package lists at 
http://lxr.mozilla.org/seamonkey/source/xpinstall/packager/.

Comment 38

[Bill Law]

Steve, I can't seem to apply your patch.  All the Index: lines have no paths so 
it looks like I'd have to split it up into n files and apply them individually 
in various directories.  Can you just do a single "cvs diff" from the /mozilla 
directory?

Comment 39

[Stephen P. Morse]

Attachment: added packager files, included bulletproofing patch

Comment 40

[Stephen P. Morse]

Attachment: use full pathname in patch file

Comment 41

[Bill Law]

(this has nothing to do with the crash in the multi-profile case)

This hunk doesn't look right.  It seems as if we should always have to call 
NS_ShutdownXPCOM since we'll have always (at this point) succeeded in calling 
NS_InitXPCOM.

@@ -1768,8 +1783,10 @@
     NS_ASSERTION(NS_SUCCEEDED(rv), "DoOnShutdown failed");
   }
 
-  rv = NS_ShutdownXPCOM( NULL );
-  NS_ASSERTION(NS_SUCCEEDED(rv), "NS_ShutdownXPCOM failed");
+  if (mainResult != NS_ERROR_ABORT) {
+    rv = NS_ShutdownXPCOM( NULL );
+    NS_ASSERTION(NS_SUCCEEDED(rv), "NS_ShutdownXPCOM failed");
+  }
 
   return TranslateReturnValue(mainResult);
 }

Comment 42

[Stephen P. Morse]

Although it wasn't explicitly stated in comment 41, the wording implied that 
there was a crash when using mulitple profiles.  Inded there was as Bill Law 
discovered.

Attaching a new patch that doesn't have that crash.  Also it cleans up the code 
that Bill pointed out in comment 41.

Comment 43

[Stephen P. Morse]

Attachment: address issues in comment 41

Comment 44

[Stephen P. Morse]

Note that the assumption here is that the urgency (ADT1) is for windows only,  
based on comment 5, 9, and 11.  Can someone from the installer team verify that 
that is so.

Also can someone from the installer team try out the patch and make sure it does 
what you need.

Comment 45

[Bill Law]

(1) This stuff:
+      var windowManager =
+        Components.classes['@mozilla.org/rdf/datasource;1?name=window-
mediator']
+        .getService();
+      var windowManagerInterface =
+        windowManager.QueryInterface( Components.interfaces.nsIWindowMediator);
is really obsolete and deprecated style (not sure if it was ever really 
acceptable).  Just put Components.interfaces.nsIWindowsMediator as the argument 
to getService (and use windowManager for everything instead of 
windowManagerInterface).

(2) Two points on this file's changes:
Index: xpfe/bootstrap/nsAppRunner.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
retrieving revision 1.345
diff -u -r1.345 nsAppRunner.cpp
--- xpfe/bootstrap/nsAppRunner.cpp	2 Apr 2002 23:56:38 -0000	1.345
+++ xpfe/bootstrap/nsAppRunner.cpp	18 Apr 2002 04:25:34 -0000
@@ -546,6 +546,16 @@
 
   nsXPIDLCString chromeUrlForTask;
   rv = handler->GetChromeUrlForTask(getter_Copies(chromeUrlForTask));
+  if (rv == NS_ERROR_NOT_AVAILABLE) {
+    // GetChromeUrlForTask is telling us not to open a new window
+    *windowOpened = PR_TRUE;
+    return NS_OK;
+  }
+  if (rv == NS_ERROR_ABORT) {
+    // This happens if we get a -killAll and no windows are open.
+    // If we don't bail out here, then a window will open and that is 
undesirable
+    return rv;
+  }
   if (NS_FAILED(rv)) return rv;
 
a) We should comment the NS_ERROR_NOT_AVAILABLE case to say something to the 
effect that "Command line handlers return NS_ERROR_NOT_AVAILABLE to block 
opening of another window.  We need to fool the caller into thinking we opened 
a window by setting *windowOpened to true."  This is just too obscure it to 
chance that developers hacking on this in the future will know what it means.

b) The test for NS_ERROR_ABORT is not needed.  The following lines do the same 
exact thing regardless.  I made the same comment somewhere above, BTW.

Upon further reflection, and going down the aisle to talk to you and try it 
out :-), it seems we don't need two return values from the -killAll handlers 
chromeUrlForTask function.  Just one will do: NS_ERROR_NOT_AVAILABLE, which 
shall henceforth mean (in that context) that "the command line handler 
indicates that this should be a silent command and not cause any windows to 
open."

But to fully accomlish and support that, we need to tweak just a bit more code 
(and tweak some in your patch to work slightly differently).  But I won't say 
anything more since we've worked out the code and you'll be attaching it 
shortly.

Comment 46

[Stephen P. Morse]

About to attach patch that addresses almost all the issues Bill raised in 
comment 45, as well as those not written down but discussed between us.

The one issue that we discusses that I had to back off on was the following.  In 
nsAppRunner.cpp, after the return from LauncApplicationWindowWithArgs, my 
previous patch had a test for a specific error condition and a return if it 
occured.  Bill commented that we will always be returning an error from 
nsKillAll's getChromeURLFromTask which gets propogated up, so we should just do 
an unconditional return.

Problem is that when turbo is not running and we issue the command "mozilla 
-turbo", we never down as far as nsKillAll.  Instead we return earlier with an 
error code of NS_ERROR_FAILED.  And in that case doing the unconditional return 
will lead to a crash.

Therefore patch that I'm about to attach still has the test for the specific 
error condition before doing the return. 

Comment 47

[Stephen P. Morse]

Attachment: addresses all but one issue in comment 45

Comment 48

[Bill Law]

Index: xpfe/bootstrap/nsAppRunner.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
retrieving revision 1.345
diff -u -r1.345 nsAppRunner.cpp
--- xpfe/bootstrap/nsAppRunner.cpp	2 Apr 2002 23:56:38 -0000	1.345
+++ xpfe/bootstrap/nsAppRunner.cpp	19 Apr 2002 06:50:18 -0000
@@ -512,11 +512,6 @@
   else {
     rv = OpenWindow(chromeUrlForTask, width, height);
   }
-  
-  // If we get here without an error, then a window was opened OK.
-  if (NS_SUCCEEDED(rv)) {
-    *windowOpened = PR_TRUE;
-  }
 
   return rv;
 }
@@ -546,6 +541,13 @@
 
   nsXPIDLCString chromeUrlForTask;
   rv = handler->GetChromeUrlForTask(getter_Copies(chromeUrlForTask));
+  if (rv == NS_ERROR_NOT_AVAILABLE) {
+    // Command line handlers return NS_ERROR_NOT_AVAILABLE to block opening
+    // of another window.  We need to fool the caller into thinking we opened 
+    // a window by setting *windowOpened to true."
+    *windowOpened = PR_TRUE;
+    return NS_OK;
+  }

These two hunks are "reversed."  The first should remain and the second does 
not need to be added.   

Comment 49

[Bill Law]

Comment on attachment 79948 [details] [diff] [review]
addresses all but one issue in comment 45

r=law

With removal of the first two changes to nsAppRunner.cpp, as noted in my
previous comment in the bug.

Also, we only want this fix on Win32 so the changes to the Mac build script and
Linux and Mac xpinstall/packager files should be omitted.  We still need the
Makefile.in changes because gmake is used on Win32 now.

Comment 50

[Stephen P. Morse]

Attachment: addresses issues in comment #49

Comment 51

[Stephen P. Morse]

Comment on attachment 80475 [details] [diff] [review]
addresses issues in comment #49

r=law

Comment 52

[jag (Peter Annema)]

Comment on attachment 80475 [details] [diff] [review]
addresses issues in comment #49

sr=jag

Comment 53

[scottputterman]

please check this into the trunk and update the bug when this is tested. 
tpreston, can you take a look at this when it's landed?

Comment 54

[Stephen P. Morse]

It's checked in on the trunk.

Comment 55

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

Comment on attachment 80475 [details] [diff] [review]
addresses issues in comment #49

a=[e-mail] for branch checkin

Comment 56

[Terri Preston]

It's my understanding that by typing mozilla -kill should close all windows when
installing, is this correct?

Comment 57

[Stephen P. Morse]

-kill should exit turbo mode (and close the browser if no other window is open).  
If other windows are open, -kill will not close them.  But -killAll will.

ANd it has nothing to do with whether or not you are installing.  These commands 
should always do what I just said.

Comment 58

[Terri Preston]

Okay, with help from Steve I figured out how to test this, THANKS STEVE

I don't see any problems with the fix in trunk, I tested with turbo, without,
open mail, composed mail, open composer, composer document open, all looks great
to me!   So, I would say verified in trunk.  Sorry for the delay

Comment 59

[scottputterman]

adding adt1.0.0+.  Please check this into the branch as soon as possible and add
the fixed1.0.0 keyword.

Comment 60

[Stephen P. Morse]

Patch has been checked into the branch except for 
xpfe/bootloader/nsNativeAppSupportWin.cpp.  Seems to be some cvs problem on the 
branch prohibiting that check-in.  Leaf has been notified.

Comment 61

[Stephen P. Morse]

Leaf has cleared the cvs problem.  Final file has been checked into the branch.

Comment 62

[scottputterman]

Adding fixed1.0.0 keyword since Steve wrote that the final file has been checked
into the branch.

Comment 63

[Terri Preston]

Verified fixed on win XP branch 2002042906