I guess Sean took an unsuccessful stab at this earlier but it is pretty important that we figure out how to do this now that we intend to serve up content during installation via the browser. Otherwise, the user could shut down all browsers in anticipation of clearing the way for the installation to complete seamlessly and then the installer would undo the user's effort by launching the browser to display content.

Yes, we actually really need to figure this one out to make the auto-run feature good. Sean - can you help Curt with this?

We don't absolutely have to have this for auto-run. We can auto-run the stub and eventually it will ask the user to shut down the browser. If they're not there to do it it'll wait. If we want to be nice we could warn the user early "We detected a browser running, you will be prompted to shut it down once all files have been downloaded" so that if they do wander off they might shut down on their own. The alternative is that the user wanders off to get a bite to eat and comes back to find we've shut down the browser and he's lost the long mail or editor document he's in the middle of editing, or lost the paid-download MP3 he was also downloading at the same time. How can we value such a marginal convenience over even one or two customers screwed in this way, and it won't be just one or two. I say WONTFIX this puppy, quick!

well, either way we implement this, by doing it automatically or showing a dialog that asks...the bottom line is that all browser windows/apps (including quicklaunch?) need to be shut down before install can occur. In both cases work needs to happen so that this can occur gracefully. I am fine with asking the user to shut down the browser themself, but we need good instruction so they don't cancel the download/install. I would like similar functionality like we have with the current 'Retry' button, but I would prefer that we only have an 'Ok' button and not a 'cancel' button. If the user wants to completely exit the install then the close 'X' in the upper right corner can allow them to exit.

Good points, and all covered in other bugs.

verified

I'm reopening this because the "Instruct the user how to shut down" seems to have some shortcomings that we'd like to avoid. Implementing this also impacts what the MessageBox should like that we're going to show the user. So the plan is to pop a box warning the user that we are about to close all the browser instances to complete the installation. The message box will have a single Okay button. When this button is clicked, we'll attempt to shut down the browser instances from the installer and proceed. Dan and I have been looking at the Windows API and it seems that sending a WM_CLOSE to all of the windows of class Mozilla Window should do what we want. If not, I'll have to talk with the client folks about whether we can get the -kill command-line option beefed up. In the meantime I'm making bug 119530 dependant upon this bug (and rewording its summary a bit).

Resetting milestone, only nsbeta1+ bugs can have a milestone on them, these are niminated, but not yet plussed.

Curt, isn't this fixed?

Nope. What we have done is postpone the point at which the user has to shut down the browser, but it is still up to the user to get the browser shut down before the installer will proceed. The objective of this bug is to get the installer smart about shutting the browser down itself. As Gregg points out above, this is very desirable. On the other hand, we are starting to run out of time and this bug is getting to be seriously at risk. So we should start talking about what our plan is if we aren't able to get this fix in, as is appearing to be likely. That probably will mean some changes to the ui (different verbage and a warning box if the user accidentally cancels out of the installer at this point).

nsbeta1- per Adt traige

Returning this to a + after talking with adt, but this depends on getting a fix for 99940 (or its equiavlent) from morse pretty quickly so I will make contact with him quickly do determine what the chances for that are. Without his fix we'll have to drop back to dealing with this by giving users instructions and change the ui to support that.

Yes we either need this one fixed, or the equivalent of this one, which has been created as: http://bugzilla.mozilla.org/show_bug.cgi?id=132641 My understanding is that there are a couple ways this can be solved. One is thru installer and trying to use windows commands. The other is thru a command line option in the browser that would gracefully shut down all 6.x apps. Again, this is important because we could be getting users into an unrecoverable state after download and before install.

You might want to change the target milestone...

The current Win Installers look for window classes Netscape6MessageWindow and MozillaWindowClass. The first is unique to N6, the second is shared between Mozilla and N6, and probably embedding apps like CompuServe or K-Meleon (it's definitely used by mfcembed and viewer, the ones I can test right now). Likewise the Mozilla checks are for the unique MozillaMessageWindow and the shared MozillaWindowClass. The result is that based on the first we send the -kill command to the right app if it's running, but if *any* Gecko based app is running we won't get past the second instance check. You can make a reasonable argument for requiring all Gecko-based apps to shut down, from profile sharing to pie-in-the-sky shared Mozilla Runtime Installations, but if that's what we're saying then we need to come up with a scheme that will work for all Gecko installations. That rules out any command-line based scheme, since we can't possibly know what command line args would be supported by arbitrary Gecko embeddors. Alternately we can restrict our caring to the specific app we're installing (N6 or Mozilla) and then work out a way to shut that one down. Side note: I noticed code in the installers to check for process name, which looks like it could be used to check for netscp6.exe vs mozilla.exe vs compuserve or whatever. Any reason we're not using it instead of window classes?

See also bug 84458

> Side note: I noticed code in the installers to check for process name, > which looks like it could be used to check for netscp6.exe vs mozilla.exe > vs compuserve or whatever. Any reason we're not using it instead of window > classes? It was only used to detect a very old version of psm.exe running. That version did not have a hidden window to be detected. However, using that code cause a crash with one person. I could not reproduce it, nor figure out exactly where in the code it was crashing. Since psm changed and was no longer standalone, it was no longer necessary to check for it running. You can try using that code to detect the running app name. Maybe you'll be able to reproduce the crash that one particular person kept running into and fix it :)

Curt, could you add a comment describing why this moved from [adt2] to [adt1]? The adt1 marking means high percentage of all users and high severity.

This is necassary to make the autoinstall experience reliable. As it stands users who click on the xpi version of the installer from their browser have to be instructed to shut down the browser ofter the installer files get downloaded so the installation can complete. There are cases when, if the user closes multiple browser windows instead of File|Exit'ing a process gets orphaned and the user would likely not know how to get the process out of memory. In this case the user is forced to exit the installation with multiple bad side effects: The user most likely will have to reboot to get out of the installer, will have to go find the page he/she was installing from, and will have to re-download the installer files again.

I've been told the fact that File|Exit ends quicklaunch is a bug, and may get fixed. The only approved way to end quicklaunch is to explicitly turn it off.

Need ETA on this very important bug, pls.

ETA is 2 days after 132641 works which this bug depends on.

I see that 132641 has an ETA, so I based my ETA on that (and gave myself an extra day becasue I think +2 may have been overly optimistic).

adding metabug blockage

Based on the ETA for 132641, this bug is going to be pushed past the code freeze. I can to some work to be prepared for 132641 to land, but even if it really lands on the 22nd, I'm guessing that my new ETA is pretty optimistic.

Here is the logic change which I think we'll have to make to take advantage of the functionality to be provided by bug 132641: Currently the functionality is pretty simple: 1) Check if browser is running. 2) if browser is running, 3) tell user to fix the problem. 4) go back to 1) 5) else move on We need to change this to: if (browser is running) show Shutdown Dialog warning user we are about to shut down the browser if user says "okay" shut down the browser if browser shuts down successfully move on else go back to the Shutdown Dialog endif else if the user says "quit" dialog warning about what a very bad thing it is to quit if user says "quit anyway" quit else go back to the Shutdown Dialog endif endif

*** Bug 138840 has been marked as a duplicate of this bug. ***

taking this. almost done.

Am I corrcect in saying that this can be finished now because the -kill, -killAll bug was checked in? http://bugzilla.mozilla.org/show_bug.cgi?id=132641 - -kill needs to force closure of all open windows and exit Mozilla

This patch attempts to fix two things: * Save downloaded files if the user cancels out of Setup at the check for browser running (after download has completed). It will also show the appropriate dialog at startup. * The check instance dialog still just has [OK]. * Installer will automatically shutdown browser for user if it's running (after the check instance dialog). The strings used for the check instance dialog are: ** for Stub (where files are downloaded): Download of Netscape XXX was successful. Netscape must be closed to proceed with installation. Click OK to exit automatically an d to begin installation. ** for Full (where files are already on the system): Netscape must be closed to proceed with installation. Click OK to exit automatically and to begin installation.

This patch attempts to fix two things: * Save downloaded files if the user cancels out of Setup at the check for browser running (after download has completed). It will also show the appropriate dialog at startup. * The check instance dialog still just has [OK]. * Installer will automatically shutdown browser for user if it's running (after the check instance dialog). The strings used for the check instance dialog are: ** for Stub (where files are downloaded): Download of Netscape XXX was successful. Netscape must be closed to proceed with installation. Click OK to exit automatically and to begin installation. ** for Full (where files are already on the system): Netscape must be closed to proceed with installation. Click OK to exit automatically and to begin installation.

Comment on attachment 81246 [details] [diff] [review] patch v1.0 (moz tree - above patch is for ns tree) r=syd

At the same time you were making this patch and adding ProductNameNoVersion, I was adding a patch ProductNameInternal. It happens that for Netscape both are "Netscape". Logically they are different. ProductNameInternal should be used for creating registry and installation paths whereas ProductNameNoVersion should be used where we want to display the name to the user but without version info. These two may not always have the same value. So it might be good to look over the changes which I just checked in to the branch and see if there are any instances where ProductNameNoVersion should be used. You are going to have some merging issues I think when you update to check in. On the other hand, since we know the values are correct for the current situation another option would be to open another bug to clean that up later. It looks to me like dwThreadID serves no purpose. In two different case it is given the return value from GetWindowThreadProcessId(), but I don't see that it is ever used. No harm done, I guess, but unnecassary? So, although we have been saying that this bug is dependant on getting a -killall parameter for the browser (bug 132641), it looks to me like the patch does what we want without actually using that functionality? ...and, have you tested to see that this give users the option of saving work if, for example, they have a Composer window? With those bits of clarification for my own edification, here is my... r=curt for both attachments, 81246 and 81242

Since I have the changes to add ProductNameNoVersion, I'll make sure the conflicts are resolved appropriately. you are correct, dwThreadId is not needed. It was used in my test app. I'll remove it before checking in. I have not tested this with a composer window open. I have a feeling that it might not ask, but we'l see (stay tuned). unfortunately, we can't really utilize -killAll because most of the browsers that we would encounter running will not have this feature yet, thus my function that attempts at shutting down the browser.

For the case of a compose window being open, it does ask if the user wants to: [Save] [Don't Save] [Cancel] The problem now is the since my shutdown code simply sends a WM_CLOSE window to all the windows from the browser process, the [Cancel] does not seem to work properly properly for me because windows have already been closed before hitting the compose window. However, the [Save] button seemed to have worked great. We'll need QA to do more extensive testing if this gets checked in to trunk.

The original code's retry loop is probably the way to go, except then we need to add the exit button to give folks a way out.

I don't mean the current text, I meant the general concept of show dialog/send WM_CLOSE/wait/test for running again.

Comment on attachment 81242 [details] [diff] [review] patch v1.0 >diff -u -3 -0 -r1.10 .cvsignore Wow--I'm a big fan of lots of context but -30 seems a wee bit (er, a *lot*) excessive for this one ;-) >Index: config.it >=================================================================== > [Check Instance1] > Class Name=MozillaWindowClass > Window Name= > ;*** LOCALIZE ME BABY *** >-Message=Download of $ProductName$ was successful. All instances of $ProductName$ must now be closed to proceed to installation. Select File|Exit from the browser window. Then click Ok to Install. >+Message=Download of $ProductName$ was successful. $ProductNameNoVersion$ must be closed to proceed with installation. Click OK to exit $ProductNameNoVersion$ automatically and to begin installation. >+;*** LOCALIZE ME BABY *** >+Message Full Installer=$ProductNameNoVersion$ must be closed to proceed with installation. Click OK to exit $ProductNameNoVersion$ automatically and to begin installation. > >+; This key indicates whether or not to close all the windows associated with >+; the process id of this app instance window found. >+Close All Process Windows=TRUE So this is going to catch all Gecko-based products, as the current one does. You mentioned that you only try to shut down windows associated with the target directory, but do you also limit the display of the warning message in that case? We don't want to shut down ActiveState Komodo or AOL. >+;*** LOCALIZE ME BABY *** >+Message Unfinished Install Xpi Restart=A previous session of Setup did not finish with installation of all the necessary files. Would you like to use the files previously downloaded, to avoid downloading them again? This one could probably use some doc polish, seems a bit repetitive. That's ok with me for now as long as we've got a bug or something to track improving it. But is this a choice we really want to offer users? If we need to offer a way to avoid using the previous files (and grudgingly I admit we probably do) I'd word it a lot stronger. "Interrupted installation detected, using previously downloaded files. OK/Cancel" (that, too, needs polish). whatever, file a bug on the UE issue. sr=dveditz assuming the UE bugs get filed.

>So this is going to catch all Gecko-based products, as the current one does. >You mentioned that you only try to shut down windows associated with the >target directory, but do you also limit the display of the warning message >in that case? We don't want to shut down ActiveState Komodo or AOL. Yep. This will detect all the embeded products that did not change their window's Class Name to something else. Most likely it will close them up too, but I haven't tested it.

Comment on attachment 81246 [details] [diff] [review] patch v1.0 (moz tree - above patch is for ns tree) Same kinds of issues with strings and such. >Index: wizard/windows/setup/dialogs.c >+ SetSetupState("unpacking xpcom"); >+ SetSetupState("installing xpis"); // clears and sets new setup state These magic values need to be #defines or constant strings. >+ DWORD dwRv = PUS_NONE; "pus" - yum. What's it stand for? >+ else if(lstrcmpi(szBuf, "unpacking xpcom") == 0) >+ dwRv = PUS_UNPACK_XPCOM; >+ else if(lstrcmpi(szBuf, "installing xpis") == 0) >+ dwRv = PUS_INSTALL_XPI; Ditto the #define comment above, since these are in different files but must be kept in sync. >+ SetSetupState("downloading"); A third magic value. -- the use of this function should be documented somewhere in comments so Curt or someone else in the future will know how to use and maintain it. >+BOOL CALLBACK EnumWindowsProc(HWND hwnd, LPARAM lParam) >+DWORD CloseAllWindowsOfWindowHandle(HWND hwndWindow) These two routines together look like they'll close all windows of whatever window classes you're looking for. There's no patch to change the installers to look only for <foo>MessageWindow and by looking for MozillaWindowClass you'll end up shutting down all Gecko-based products the user happens to be installing. Or did I miss a directory check somewhere, so that you only shut them down if the process is running in the target directory?

This patch addresses: 1) remove the MozillaWindowClass check due to conflicts with gecko-based products. 2) send WM_CLOSE to only visibe windows. this will allow the user to cancel out of compose and mail compose windows without crashing or hanging. It will still shutdown the browser as before. 3) fixed string issues Dan brought up.

This patch addresses: 1) remove the MozillaWindowClass check due to conflicts with gecko-based products. 2) send WM_CLOSE to only visibe windows. this will allow the user to cancel out of compose and mail compose windows without crashing or hanging. It will still shutdown the browser as before. 3) fixed string issues Dan brought up.

Comment on attachment 81643 [details] [diff] [review] patch v1.1 (ns tree) sr=dveditz

Comment on attachment 81644 [details] [diff] [review] patch v1.1 (moz tree) > [Check Instance0] > Class Name=MozillaMessageWindow Forgot to mention in the NS patch review. Is the equivalent window going to remain Netscape6MessageWindow, or will it change to something more generic like NetscapeMessageWindow, or even MachVMessageWindow? We'll be broken if it changes. > Extra Cmd Reg Key=Software\Microsoft\Windows\CurrentVersion\App Paths\$MainExeFile$ In the ns config $MainExeFile$ will be the renamed executable, while it most likely if the user is running -turbo they must have an old version and need to use an explicit netscp6.exe key. But what if they're upgrading from the beta to the final? Then they'd only have the newly named key. Dave is having similar problems with a plugin install that assume we're sticking with the Netscp6 key, and he was going to just create it all the time just in case. If he does we'll be covered here too, except we should change the Netscape version to an explicit netscp6.exe rather than let the build scripts replace $MainExeFile$ with the executable name flavor of the week. sr=dveditz for this one, too, with the above concerns which will have to be worked out before this can be checked in.

adding adt1.0.0-. Let's get this in for rtm.

Fixed problem with detecting the old and new netscape keys in the App Paths key. It will look for the old one first. If found, it will do its normal thing and stop. If not found, it will look for the next key and do the same logic. This is the only difference between this patch and the previous one.

Fixed problem with detecting the old and new netscape keys in the App Paths key. It will look for the old one first. If found, it will do its normal thing and stop. If not found, it will look for the next key and do the same logic. This is the only difference between this patch and the previous version.

renominating adt1.0.0 -- the current experience is that the users get an "OK" dialog that keeps coming up with no way to exit if they can't figure out how to shut-down all Gecko-based apps (yes, all) despite a message that implies we only look for the product we're installing. See bugscape 14085

Is there any way around the L10N issues of localizing the new string?

Thanks for the information and renomination Dan. Much Appreciated! adt1.0.0+ (on ADT's behalf) approval for checkin to the 1.0 branch, pending L10N approval, and s/s= for the latest patch. Pls check this in today, once you have recieved Drivers approval, then add the fixed1.0.0 keyword.

Comment on attachment 81723 [details] [diff] [review] patch v1.2 (moz tree - previous one is ns tree) r=curt

l10n approved. Please check this into the branch today. It's critical that this gets checked in today! thanks

Comment on attachment 81722 [details] [diff] [review] patch v1.2 sr=dveditz

Comment on attachment 81723 [details] [diff] [review] patch v1.2 (moz tree - previous one is ns tree) sr=dveditz

Comment on attachment 81723 [details] [diff] [review] patch v1.2 (moz tree - previous one is ns tree) a=roc+moz for branch checkin

oops. marking this fixed on trunk. will add fixed1.0.0 when checked into branch.

This needs to be landed on the branch (or at least the UI needs to land) today! thanks

verified on branch 2002050206 and trunk 2002050208

*** Bug 141961 has been marked as a duplicate of this bug. ***