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.

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.

adding dependencies from bug 99940 Sorry for all the spam everyone, lets hope this is all worked out now.

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.

nsbeta1+/adt1 per Nav triage team.

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

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 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''.

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?

> 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.

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".

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.

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.

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

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 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 on attachment 78346 [details] [diff] [review] remove nullcheck (see comment 19) sr=jag (carried forward)

Comment on attachment 78346 [details] [diff] [review] remove nullcheck (see comment 19) Cool. r=sgehani

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.

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.

+ // 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 :-).

> 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.

> + *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.

>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.

> 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.

>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.

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.

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.

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

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?

(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); }

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.

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.

(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.

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.

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 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 on attachment 80475 [details] [diff] [review] addresses issues in comment #49 r=law

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

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?

It's checked in on the trunk.

Comment on attachment 80475 [details] [diff] [review] addresses issues in comment #49 a=rjesup@wgate.com for branch checkin

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

-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.

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

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

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.

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

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

Verified fixed on win XP branch 2002042906