Closed Bug 212599 Opened 21 years ago Closed 21 years ago

profile deleted when updating from 1.4b to 20030712 build (1.5b)

Categories

(SeaMonkey :: Installer, defect)

x86
Windows 2000
defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: schoessl, Assigned: ssu0262)

References

Details

(Keywords: dataloss, fixed1.4.1)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030712
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5b) Gecko/20030712

I kept my Mozilla profile not in my Windows profile directory (c:\documents and
settings\...) but within the Mozilla directory (..\Mozilla\Mozilla Users\Default
User\).
When updating from 1.4b to 1.5b (20030712 build) without previously deinstalling
the old version, the installer told me that it would remove all third-party
components. Obviously it did not recognise my \Mozilla\Mozilla Users folder als
valuable and simply deleted it. But it did not delete all contents of \Mozilla\
because some old installation log files in ..\Mozilla\uninstall are still there.


Reproducible: Didn't try

Steps to Reproduce:
1.
2.
3.



Expected Results:  
Mozilla installer should determine if there are any bookmark or email files
before deleting folders. Alternatively it could move the files into the recycle
bin or backup the files instead of simply deleting them.
Presumably this is an unanticipated side effect of bug 210731.

Confirming, 2003071208 PC/WinXP
Status: UNCONFIRMED → NEW
Depends on: 210731
Ever confirmed: true
Flags: blocking1.5a?
Keywords: dataloss
yes, definitely unanticipated side effect.

perhaps I should keep a list of directories to remove instead of what not to remove.
Status: NEW → ASSIGNED
A "list of directories to remove" is somewhat in the direction of the old code
which missed things. 

At the very least maybe we should go back to your original scarier warning that
says explicitly that we're deleting the directory. And drop the bit about "your
profiles will be safe" unless we start checking for profile files in the
directories we delete (e.g. check for 'prefs.js', 'bookmarks.html' and maybe
some standard mail file)
Flags: blocking1.5b?
is this one going to be on track for 1.5 beta?
Taking off the 1.5a blocker nomination.
Flags: blocking1.5a?
Blocks: 212244
Attached patch patch v1.0 (obsolete) — Splinter Review
This patch will fix the follwing bugs:
  bug 212599 - (this bug)
  bug 212244 - move ExcludeRemoveList to config.ini
  bug 213573 - Won't let me install in my preffered directory because it
	       contains the same letters as my windows directory

note to self: need to update ns' config.it file as well.
Blocks: 213573
Comment on attachment 128678 [details] [diff] [review]
patch v1.0

This patch fixes the following:

  * moves the ExcludeRemoveList[] to config.ini.

  * checks to see if a given dir to delete (on upgrade) within the
...\mozilla.org\mozilla dir contains user Profile(s).  If any subdir within the
dir being checked contains the file/dirs that indicate it's profile dir, then
that parent dir will not be deleted on upgrade.

  * added code to log to install_status.log when cleaning up orphaned GRE's on
upgrade.

  * fixed bug 213573 where it could mistake a none windows, user chosen
installation path for a windows path thus preventing the user from installing
to that path.
    This was a 2 line fix in IsPathWithinWindir().
Attachment #128678 - Flags: superreview?(jaggernaut)
Attachment #128678 - Flags: review?(dveditz)
Sean, will we want this cfor the 1.4.x branch since we took the install
directory cleanup fixes there as well?

Flags: blocking1.5b?
Flags: blocking1.5b+
Flags: blocking1.4.x?
Comment on attachment 128678 [details] [diff] [review]
patch v1.0

>Index: wizard/windows/setup/extra.c
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/wizard/windows/setup/extra.c,v
>retrieving revision 1.131
>diff -u -2 -0 -p -r1.131 extra.c
>--- wizard/windows/setup/extra.c	10 Jul 2003 23:42:03 -0000	1.131
>+++ wizard/windows/setup/extra.c	28 Jul 2003 03:52:28 -0000
>@@ -40,54 +40,54 @@
> HRESULT GetInstalledGreConfigIni(greInfo *aGre, char *aGreConfigIni, DWORD aGreConfigIniBufSize);
> HRESULT GetInfoFromInstalledGreConfigIni(greInfo *aGre);
> HRESULT DetermineGreComponentDestinationPath(char *aInPath, char *aOutPath, DWORD aOutPathBufSize);
>-BOOL    IsOkToRemoveFileOrDirname(char *aFileOrDirname);
>+BOOL IsOkToRemoveFileOrDirname(char *aFileOrDirname, char **aListToIgnore,
>+    int aListToIgnoreLength, char **aListProfileObjects, int aListProfileLength);

Inconsistent spacing.

>@@ -2292,178 +2292,231 @@ void GetInfoFromGreInstaller(char *aGreI
>   // Build the list of GRE's given greRegKey.  For example, if greRegKey is:
>   //
>   //   HKLM\Software\mozilla.org\GRE
>   //
>   // then build a list of the GRE IDs inside this key.
>   totalGreIDSubKeys = 0;
>   if(RegQueryInfoKey(greIDKeyHandle, NULL, NULL, NULL, &totalGreIDSubKeys, NULL, NULL, NULL, NULL, NULL, NULL, NULL) != ERROR_SUCCESS)
>+  {
>     rv = WIZ_ERROR_UNDEFINED;
>+    _snprintf(buf, sizeof(buf), "    Cleanup Orphaned GRE premature return (RegQueryInfoKey: all keys): %d\n", rv);
>+    buf[sizeof(buf) - 1] = '\0';
>+    UpdateInstallStatusLog(buf);
>+  }

Why not return here, but rv = instead? Looks like you may have to do some clean
up below? Is this still considered "premature return"?

> 
>   if((rv == WIZ_OK) && (greIDListToClean = NS_GlobalAlloc(sizeof(char *) * totalGreIDSubKeys)) == NULL)
>   {
>     RegCloseKey(greIDKeyHandle);
>+    _snprintf(buf, sizeof(buf), "    Cleanup Orphaned GRE premature return.  Failed to allocate memory for greIDListToClean: %d\n", WIZ_OUT_OF_MEMORY);
>+    buf[sizeof(buf) - 1] = '\0';
>+    UpdateInstallStatusLog(buf);
>     return(WIZ_OUT_OF_MEMORY);
>   }

But then you return here, so hmm...

>+      // if it's a directory, the we need to traverse it to see if there are

"then we" (typo)
Attachment #128678 - Flags: superreview?(jaggernaut) → superreview+
Asa, yes, we do.  I have held of checking of the other patch because of this patch.
Comment on attachment 128678 [details] [diff] [review]
patch v1.0

>+[Cleanup On Upgrade]
>+Cleanup=TRUE
>+ObjectToIgnore0=plugins
>+ObjectToIgnore1=uninstall
>+ObjectToIgnore2=install_status.log

Nice, thanks for incorporating this change in this patch. would it be worth
adding a couple profile-ish items here just in case a profile slips through the
other check? I'd nominate
  bookmarks.html
  prefs.js
  Mail
  ImapMail
  abook.mab
  cert7.db
  cert8.db
  key3.db
  user.js
  userChrome.css
  userContent.css

Maybe that just slows things down, though.

>+; List of files/dirs that is used to determine if a directory is a profile
>+; or not.  This is so the directory is not deleted as part of the Cleanup
>+; On Upgrade.
>+; All of the object files must be found in order for a dir to be assumed
>+; to be a Profile dir.
>+[Profile Dir Object List]
>+Object0=chrome
>+Object1=bookmarks.html
>+Object2=localstore.rdf

All, or any? Remove the chrome check -- that's in both the install and profile
directories so it's not very useful. If you change to an "any" check then you
could add a bunch of the ones I listed in the other category, if it remains an
"all" check I'd still add prefs.js [further on I see this is a recursive check,
so an "any" check would get tripped up by the default bookmark file.]

>Index: wizard/windows/setup/extra.c
>===================================================================
>   if(RegQueryInfoKey(greIDKeyHandle, NULL, NULL, NULL, &totalGreIDSubKeys, NULL, NULL, NULL, NULL, NULL, NULL, NULL) != ERROR_SUCCESS)
>+  {
>     rv = WIZ_ERROR_UNDEFINED;
>+    _snprintf(buf, sizeof(buf), "    Cleanup Orphaned GRE premature return (RegQueryInfoKey: all keys): %d\n", rv);
>+    buf[sizeof(buf) - 1] = '\0';
>+    UpdateInstallStatusLog(buf);
>+  }

I'm with jag, for consistency shouldn't you return from this block also? If you
don't you end up trying to allocate something size 0 which probably
successfully returns a pointer, but might be misused later depending on your
assumptions.

(just noticed, NS_GlobalAlloc calls GlobalAlloc() with the GMEM_ZEROINIT flag,
but then you call ZeroMemory() on the allocated chunk anyway. You could remove
one or the other)

better to just bail here. Also might want to see if the the successfully read
value is 0 and bail out in that case as well.

>         totalGREAppListSubKeys = 0;
>         if(RegQueryInfoKey(greAppListKeyHandle, NULL, NULL, NULL, &totalGREAppListSubKeys, NULL, NULL, NULL, NULL, NULL, NULL, NULL) != ERROR_SUCCESS)
>+        {
>           rv = WIZ_ERROR_UNDEFINED;
>+          _snprintf(buf, sizeof(buf), "    Cleanup Orphaned GRE premature return.  Failed to regquery for all keys in %s: %d\n", greAppListKeyPath, rv);
>+          buf[sizeof(buf) - 1] = '\0';
>+          UpdateInstallStatusLog(buf);
>+        }

Why don't you break here like the other cases? If it's on purpose please add a
comment explaining why. Looks to me like you'd corrupt memory or crash using
the unallocated greAppListToClean (allocation skipped because of the rv value).

>+/* Function: CleanupArrayList()
>+void CleanupArrayList(char **aList, int aListLength)

Could have used this in CleanupOrphanedGREs above

r=dveditz if you fix the missing breaks and swap out "chrome" for "prefs.js" in
the profile-file list
Attachment #128678 - Flags: review?(dveditz) → review+
Attached patch patch v1.1Splinter Review
updated patch with changes suggested by jag and dveditz.

however, I did not replace 'chrome' with 'prefs.js' per my conversation with
dveditz.  prefs.js does not exist on a newly created profile that has not been
used.
Attachment #128678 - Attachment is obsolete: true
patch v1.1 checked in to trunk only
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Flags: blocking1.4.x? → blocking1.4.x+
Comment on attachment 129269 [details] [diff] [review]
patch v1.1

a=asa (on behalf of drivers) for checkin to the 1.4 branch.
Attachment #129269 - Flags: approval1.4.x+
patch checked in on MOZILLA_1_4_BRANCH tag now.  only config.it was patch on the
ns tree.
Keywords: fixed1.4
V. 2003080704 trunk PC/WinXP
Status: RESOLVED → VERIFIED
Shouldn't that keyword be "fixed1.4.1"?
changing from fixed1.4 to fixed1.4.1
Keywords: fixed1.4fixed1.4.1
*** Bug 212585 has been marked as a duplicate of this bug. ***
*** Bug 208624 has been marked as a duplicate of this bug. ***
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: