Closed Bug 210731 Opened 21 years ago Closed 21 years ago

Setup should delete target installation dir on upgrade

Categories

(SeaMonkey :: Installer, defect)

x86
Windows 2000
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssu0262, Assigned: ssu0262)

References

Details

(Keywords: fixed1.4.1)

Attachments

(3 files, 5 obsolete files)

[I did a quick query in bugzilla and could not find this bug filed.  If there is
a bug already filed on this, please dupe this against it.]

There are lots of people running into crash at or browser not running correctly
at startup.  Almost all of these problems are caused by 3rd party components
(having been installed by the user) not being compatible in one way or another
with the latest browser version trying to be installed.

A solution is to have Setup detect such an upgrade (installing ontop of a
previous installation of the browser regardless of version) and prompt the user
that deleting the target dir before installation begins is highly recommended,
else problems will arise.

We need to not delete the following from the target dir:
  file: install_status.log
   dir: plugins
   dir: uninstall
Attached patch patch v0.1 (obsolete) — Splinter Review
Initial patch.	Strings needs to be cleaned up/reworded, but it works as is.

[note to self: need to apply the config.it part of the diff to the ns tree.]
Status: NEW → ASSIGNED
Just to be clear, only contents of DIR should be deleted, not the root
installation DIR. If my first install of Mozilla was on 1-1-1999, my dir should
show 1-1-1999 until I delete it myself.
right, since not everything is deleted, the root dir will still be around.  Even
if everything is deleted, it should still exist.
Blocks: 196902
the indent in the uninstall looks weird.
Attached patch patch v1.0 (obsolete) — Splinter Review
This patch will show the dialogs in the images attached above.
This patch (compared to the previous one) also fixes the uninstaller.  The
uninstaller will now also prompt the user in the case where there are still
files left after the uninstallation is done.
Attachment #126528 - Attachment is obsolete: true
Attachment #126581 - Flags: review?(dveditz)
The patch will also detect if the user had installed into a dir within the
%windir% folder.  If so, both the installer and uninstaller will inform the user
of this and not perform the cleanup/delete.
Blocks: 205277
- Deleting target installation dirs on upgrade is a tricky thing. For instance
under Linux this behavior is already implemented, rendering entire systems
unusable see bug 69153.

- According to the initial description this would also fix bug 200651.

- What if plugins are not forwards compatible? I don't know how likely that is
and therefore i do think that indeed the plugins folder should be spared.

- What if people select a wrong installation folder, eg. c:\program files\ or if
they for some reason store other stuff in their Mozilla folder as well?

- What's the target for this? If this still makes 1.4 and next version of
Netscape would be VERY convenient.
Doesn't seem to work on my Windows custom install.  No dialog and immediate crash.
Mark, can you elaborate on what you meant in your comment #10?  What's not
working?  This patch hasn't landed yet.

Patrick, regarding your comments:
> - What if plugins are not forwards compatible? I don't know how likely that
> is and therefore i do think that indeed the plugins folder should be
> spared.

the plugins format is a standard, common format.  future browser code should be
backwards compatible with older plugins AFAICT.

Cc'ing PeterL in case I'm wrong.

> - What if people select a wrong installation folder, eg. c:\program files\
> or if they for some reason store other stuff in their Mozilla folder as
> well?

I can place a check for [program files] and [common files] (exact match of course).
Oops.  Sean I thought this was a blocker for 1.4.  I apologize.
Message Cleanup On Upgrade:
A previous $ProductNameInternal$ installation has been found in the chosen 
destination folder.

It is highly recommended that this destination folder be cleaned up prior to 
installation, or instability of $ProductNameInternal$ may arise due to 
potential incompatibility of 3rd party files.

Allowing setup to cleanup this folder will not affect your profiles, 
preferences, or mail messages.  However, a reinstallation of any 3rd party 
files previously installed will be necessary.
--

I don't like:
arise due to potential incompatibility of 3rd party files.

I think you are either missing a 'the' before potential, or should 
change "potential incompatibility of" to "potentially incompatible".

Please make sure someone who cares about English reads and approves of the text 
that you choose.

I'm also not sure that you need the 'potential*' bit, i think you have enough 
fuzziness earlier (near "may") in your warning.
--

That said, is there a reason to delete more than:
bin\*.dll
bin\chrome\overlayinfo
bin\components\*.*

[where bin is the target install dir]
(overlayinfo is for kicks, mozilla should clobber it on its own if the jars 
have reasonable timestamps)

re comment 9: for a long time there was a adobe svg plugin which used unfrozen 
xpcom apis, it worked until we changed then, after which it led to many crashes.

wrt checking for program files/common files, i'd much rather a check for:

is special folder and a check for matches those strings (Program Files, Common 
Files). Note that just string matching will suck on any localized system. And 
just because someone has one program files doesn't mean they don't have more. I 
have at least 3, possibly more.

Here's my definition of special folder:
1. attrib +s
2. is listed in 
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\Shell 
Folders
3. is listed in 
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\User Shell 
Folders
4. HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths
5. is listed in HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion
Under anything named *Path, *Dir or *PathUnexpanded or *DirUnexpanded
6. is listed in %PATH%

Personally I'd also check:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Shared Tools
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Shared Tools Location

4 is a bit special
a. DocZilla's installer managed to mess up:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App 
Paths\D:\mozilla\DocZilla install version\mozilla.exe
so you should dig through keys looking for (default) or Path.

b. Path is like %PATH%, it's a list of directories, most things will only have 
one directory but some may have more so don't treat it as if it's the path to a 
single directory.

c. In the event things turn up in these places you can simply warn about the 
references.

d. If the installer happens to know that it's going to write to a given key, 
then it may ignore it, e.g.:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App 
Paths\Mozilla.exe
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App 
Paths\Netscp.exe
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App 
Paths\Netscp6.exe
Blocks: 195600
See Bug205277, Bug 211363, Bug 211484, Bug 211353, Bug 211458, and Bug 211602

Instead of deleting thes files, shouldn't the program alow them to be moved to
another directory?
Wanted for 1.4.1 and 1.5.  Dan, can you help us out with a review here?
Flags: blocking1.5a+
Flags: blocking1.4.x+
Comment on attachment 126581 [details] [diff] [review]
patch v1.0

Does this patch also do away with the annoying "This directory doesn't exist,
create it?" pop up? If you haven't changed that already it should be combined
with this patch since that dialog implies the opposite of this new dialog:
in that one re-using a directory is "normal", in this patch re-using a
directory gets you a big scary delete warning.

The following text comments apply to all variants of the config.ini template

>-UsageMsg Usage=Usage: %s [options]\n	[options] can be any of the following combination:
[.....]

Oy, that's horrible--a line over 1400 characters long, >1700 columns counting
expanded tabs. Lines this long may break some old tools--but more importantly
they're just plain hard for humans to deal with. Couldn't you create something
like your usual "UsageMsg Usage<N>" routines to concatenate multiple lines?
Plus you've had to write a text-\n to CRLF conversion routine that you wouldn't
otherwise need if you just stuck an EOL between separate config entries.

>+Message Cleanup On Upgrade=A previous $ProductNameInternal$ installation has
>+ been found in the chosen destination folder. [...]

This could be simply "...chosen destination" or "...chosen folder". In fact,
since the selecting UI says "Select a directory" and doesn't use the word
folder anywhere it should probably be "chosen directory" unless you're going
to change everything else to "folder".

>+It is highly recommended that this destination folder

"this directory"

>+ be cleaned up prior to installation, or instability of $ProductNameInternal$
>+ may arise due to potential incompatibility of 3rd party files.\n\nAllowing
>+ setup to cleanup this folder will not affect your profiles, preferences,
>+ or mail messages.  However, a reinstallation of any 3rd party files
>+ previously installed will be necessary.

I'm now frozen in fear. Do I risk pushing the button and deleting things?
Do I dare leave these nasty 3rd party files, whatever they are?  Why
did you mention my mail? Maybe I should _really_ worry!

My choices are "Delete" and "Ignore". Delete what? Ignore what? "Clean" (or
"Clean up") and "Skip" might be better, but it would be better yet to reword
this so the action we want users to take appears normal and non-scary.

  A previous $ProductNameInternal$ installation has been found in the
  chosen directory. Unrecognized 3rd party components will be removed
  from this directory to prevent version incompatibilities and will
  have to be re-installed.

	   Continue	    Skip

Something like that is a step in the right direction, I think. Would
it be possible to run the text by one of the help writers?

>+Cleanup On Upgrade Path Box String=Path to be cleaned up:

"directory" instead of "path" for consistency, but if you go with
my suggested warning text probably just "Destination directory:"

>+Message Cleanup On Upgrade Windir=Setup has detected that the previous
>+ installation of $ProductNameInternal$ was installed to a folder within
>+ your Windows folder.  Setup will not attempt to cleanup the previous
>+ installation of $ProductNameInternal$ due to the potential removal of
>+ critical system files.

Looking at the logic users won't see this message until after they
hit the "Delete" button (or "Continue" in my suggestion). It
would be better in this case not to offer the delete choice
since we know we won't honor it anyway. In fact if the destination
directory is in c:\windows (and perhaps expanded to include
other known silly places) we should warn the user when it's
chosen whether or not there's a previous installation.

   You have chosen to install into a directory used by
   the windows operating system which may interfere with
   the operation of $ProductNameInternal$, Windows, or
   both. We recommend you chose a different directory.

	    Back      Continue


>Index: packager/win_gre/uninstall.it
>===================================================================
>+MSG_INSTALLATION_PATH_WITHIN_WINDIR=Uninstall has detected that the installation path of $ProductNameInternal$ is installed to a folder within your Windows folder.  Uninstall will not attempt to delete this installation due to the potential removal of critical system files.

We should just skip this message and not offer to delete the directory
at all in this case.

>+MSG_DELETE_INSTALLATION_PATH=Not all files were uninstalled from the installation path:\n\n  %s\n\nDo you want to completely delete this folder?

    Files not installed with $ProductNameInternal$ remain in the installation
    directory %s

    Do you want to completely delete this directory anyway?

Avoid mixing folder/path/directory terminology -- pick one and stick with it.


>+Cleanup On Upgrade=TRUE

You couldn't call this option "Nuke From Orbit Upgrade"?
Just kidding ;-)

>Index: wizard/windows/setup/dialogs.c
>===================================================================
>+LRESULT CALLBACK DlgProcUpgrade(HWND hDlg, UINT msg, WPARAM wParam, LONG lParam)

>+      GetPrivateProfileString("Strings", "Message Cleanup On Upgrade", "",
>+          buf, sizeof(buf), szFileIniConfig);
>+      ReplacePrivateProfileStrCR(buf);
>+      SetDlgItemText(hDlg, IDC_MESSAGE0, buf);

If you take my multi-line suggestion from the Usage section you could
get rid of RplacePrivateProfileStrCR() but you'd have write a new
GetMultilineMessage("Strings", "Message Cleanup On Upgrade", etc)
function.

>+    case WM_COMMAND:
>+      switch(LOWORD(wParam))
>+      {
>+        case IDDELETE:
>+          /* If the installation path happens to be within the %windir%, then
>+           * show error message and continue without removing the previous
>+           * installation path. */
>+          if(IsPathWithinWindir(sgProduct.szPath))

As mentioned before, Windir should be checked before you show the delete dlg.

>+            /* Prompt user if deleting target path is okay. Only show
>+             * prompt if the setup is running in normal mode, else
>+             * assume user wants deletion */
>+            if(sgProduct.mode == NORMAL)
>+            {
>+              hDlgCurrent = InstantiateDialog(hWndMain, dwWizardState,
>+                  warningTitleString, DlgProcUpgrade);
>+              bDone = TRUE;
>+            }
>+            else
>+              sgProduct.doCleanupOnUpgrade = TRUE;

People running in !NORMAL mode don't get the benefit of the WINDIR check
and thus could end up deleting everything. I guess you're assuming that
no packager would be silly enough to make that the default, and if they
did they're smart enough to bury it down a few levels so this isn't a
problem. All right, I buy that.

>+        /* SiCNodeSetItemsSelected() is called from within DlgProcUpgrade().
>+         * If DlgProcUpgrade is not called (in the case of a !NORMAL install),
>+         * then we need to call it here. */

If the mode is NORMAL it appears there are two cases when
SiCNodeSetItemsSelected() gets skipped: if checkCleanupOnUpgrade
is false or no previous installation is found.

>Index: wizard/windows/setup/extra.c
>===================================================================
>+char *ExcludeRemoveList[] = {"plugins",
>+                             "uninstall",
>+                             "install_status.log",
>+                             ""};

This list belongs in the config file.

>+BOOL IsPathWithinWindir(char *aTargetPath)

>+    if(strstr(targetPath, windir))

Couldn't you use strncmp() for the length of windir? If
targetPath is shorter then fine, if longer it needs to
start with windir not just have windir somewhere within it.

I guess you could change it to if(strstr(targetPath, windir)==targetPath)
if you prefer.
Attachment #126581 - Flags: review?(dveditz) → review-
What's the status of the work on this bug?  Drivers would like to see this in
1.5alpha, but there doesn't seem to have been any progress for quite a few days.
working on new patch now.  stay tuned...
Filed bug 212244 to have ExcludeRemoveList moved to the config.ini file.  No
time to do it right now.
Attached patch patch v1.1 (obsolete) — Splinter Review
Changed the strings on the buttons:
  "&Delete" -> "&Continue" (default pushbutton now too)
  "&Ignore" -> "&Skip"
  "&Back"   -> no change

Removed the dialog prompting the user if they really want to create the
directory chosen.  It now just creates it.

Talked to dveditz and agreed to leave the "deleted installation dir is within
windir" prompt as is.

Changed Cleanup On Upgrade= strings.

>If the mode is NORMAL it appears there are two cases when
>SiCNodeSetItemsSelected() gets skipped: if checkCleanupOnUpgrade
>is false or no previous installation is found.

fixed.

>I guess you could change it to if(strstr(targetPath, windir)==targetPath)
>if you prefer.

changed.

>>+char *ExcludeRemoveList[] = {"plugins",
>>+				"uninstall",
>>+				"install_status.log",
>>+				""};
>This list belongs in the config file.

filed as bug 212244
Attachment #126581 - Attachment is obsolete: true
Comment on attachment 127411 [details] [diff] [review]
patch v1.1

r/sr=dveditz
Attachment #127411 - Flags: review+
Attachment #127411 - Flags: approval1.5a?
Attached patch patch v1.2 (obsolete) — Splinter Review
This patch (compared to v1.1) only adds a change that checks if the user
selected a directory within the windows dir.  Nothing else done to it.

I rather we take this patch over v1.1, if possible.  I've tested this change
too.
Attached patch diff between patch v1.1 and v1.2 (obsolete) — Splinter Review
diff to show the trivial change between patches v1.1 and v1.2.
Comment on attachment 127435 [details] [diff] [review]
patch v1.2

sr=jag
Attachment #127435 - Flags: superreview+
Comment on attachment 127435 [details] [diff] [review]
patch v1.2

moving r=dveditz to here
seeking a=

the patch is about moderate in the risk factor.

It does modify the dialog sequence logic because there's a new Cleanup dialog
now.  However, this has been tested thoroughly and the sequence is working
nomally.

The actual cleanup code is trivial because it just looks for and verifies that
it's not in the list of ExludeRemoveList, then simply calls existing functions
to do the deletion.

This has tremendous gains in that it will prevent users from upgrading to a
broken mozilla (usually crash on startup) due to 3rd party files beyond the
installer's control.
Attachment #127435 - Flags: review+
Attachment #127435 - Flags: approval1.5a?
Attachment #127435 - Flags: approval1.4.x?
Attachment #127411 - Flags: approval1.5a?
OT

One very small question that isn't related to the topic of this bug. What, if
any, differance is there between Approval1.5a and Blocking1.5a, and I see this
bug has one set to "?" and the other set to "+"?
Attachment #127435 - Flags: approval1.5a? → approval1.5a+
Attached patch patch v1.2.1Splinter Review
same patch as v1.2 except that it had some spelling errors fixed.  This is the
exact patch that was checked in to trunk.
Attachment #127411 - Attachment is obsolete: true
Attachment #127435 - Attachment is obsolete: true
Attachment #127436 - Attachment is obsolete: true
patch v1.2.1 checked in on the trunk only.  Leaving bug open until patch lands
on branch.
Blocks: 212599
Comment on attachment 127435 [details] [diff] [review]
patch v1.2

a=asa for checkin to the 1.4 branch (for this or v1.2.1, whichever is correct.)
Attachment #127435 - Flags: approval1.4.x? → approval1.4.x+
Please note in the checkin that this affects the translation of Windows install
files.

Thanks
patch checked in to the MOZILLA_1_4_BRANCH on the mozilla tree now.  In the ns
tree, only the files in xpinstall\packager\*.* were checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Keywords: fixed1.4
Resolution: --- → FIXED
Keywords: fixed1.4fixed1.4.1
*** Bug 200651 has been marked as a duplicate of this bug. ***
Blocks: 224532
No longer blocks: 195600
FYI: per comment 0: one additional file should've been added to the files that
don't need to be deleted from the target dir, and that's "mozilla.bmp".

Bug 248102 was opened for that.
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: