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)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: schoessl, Assigned: ssu0262)
References
Details
(Keywords: dataloss, fixed1.4.1)
Attachments
(1 file, 1 obsolete file)
32.01 KB,
patch
|
asa
:
approval1.4.1+
|
Details | Diff | Splinter Review |
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.
Comment 1•21 years ago
|
||
Presumably this is an unanticipated side effect of bug 210731. Confirming, 2003071208 PC/WinXP
yes, definitely unanticipated side effect. perhaps I should keep a list of directories to remove instead of what not to remove.
Status: NEW → ASSIGNED
Comment 3•21 years ago
|
||
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)
Updated•21 years ago
|
Flags: blocking1.5b?
Comment 4•21 years ago
|
||
is this one going to be on track for 1.5 beta?
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.
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)
Comment 8•21 years ago
|
||
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 9•21 years ago
|
||
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+
Assignee | ||
Comment 10•21 years ago
|
||
Asa, yes, we do. I have held of checking of the other patch because of this patch.
Comment 11•21 years ago
|
||
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+
Assignee | ||
Comment 12•21 years ago
|
||
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
Assignee | ||
Comment 13•21 years ago
|
||
patch v1.1 checked in to trunk only
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Flags: blocking1.4.x? → blocking1.4.x+
Comment 14•21 years ago
|
||
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+
Assignee | ||
Comment 15•21 years ago
|
||
patch checked in on MOZILLA_1_4_BRANCH tag now. only config.it was patch on the ns tree.
Keywords: fixed1.4
Comment 17•21 years ago
|
||
Shouldn't that keyword be "fixed1.4.1"?
Assignee | ||
Comment 18•21 years ago
|
||
changing from fixed1.4 to fixed1.4.1
Keywords: fixed1.4 → fixed1.4.1
Comment 19•21 years ago
|
||
*** Bug 212585 has been marked as a duplicate of this bug. ***
Comment 20•21 years ago
|
||
*** Bug 208624 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•