Closed
Bug 245135
Opened 20 years ago
Closed 19 years ago
The using profile can be deleted after renaming
Categories
(SeaMonkey :: Startup & Profiles, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: eagle.lu, Unassigned)
References
Details
Attachments
(1 file, 8 obsolete files)
1.77 KB,
patch
|
ccarlen
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
repro: 1.use menu "tools/switch profile" to open profile window 2.click on Manager Profile, open manager profile window 3.now that the using profile can't be deleted 4.select the using profile,and click on rename profile 5.change the name of the using profile to a different one 6.Then you can delete the using profile which is expected can't
Comment on attachment 149667 [details] [diff] [review] patch v0 Can you give r? Thanks
Attachment #149667 -
Flags: review?(ccarlen)
Comment 3•20 years ago
|
||
Setting the current profile to the new name in the frontend isn't the way to go because setting the current profile is a very expensive operation. If the current profile is named "A" and "A" is renamed to "B" profile.currentProfile should now return "B" If that is not happening, something is wrong in the backend and should be fixed there. See bug 225264 - this may be related or fixed by that patch.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 4•19 years ago
|
||
my user profile was deleted from Mozilla profile manager when I restarted my
computer.
> rename user profile, everything works ok!
> restart computer.
> the user profile you just renamed is deleted
Attachment #149667 -
Attachment is obsolete: true
Attachment #176562 -
Flags: review?(ccarlen)
Comment 6•19 years ago
|
||
Isn't this DUP of Bug 113203, which is written in "Known Issues/Profiles" section of Mozilla's "Release Notes" of almsot all realeses?
Comment 7•19 years ago
|
||
Comment on attachment 176562 [details] [diff] [review] patch v1 Looks good - setting it in gProfileDataAccess is quick and cheap, and will make GetCurrentProfile return the expected result.
Attachment #176562 -
Flags: review?(ccarlen) → review+
Attachment #176562 -
Flags: superreview?(neil.parkwaycc.co.uk)
Comment 8•19 years ago
|
||
Comment on attachment 176562 [details] [diff] [review] patch v1 This only prevents you from deleting the profile you just renamed. Nor does it update mCurrentProfileName (ForgetCurrentProfile inconveniently clears it).
Attachment #176562 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Attachment #176562 -
Attachment is obsolete: true
this patch contains the patch https://bugzilla.mozilla.org/attachment.cgi?id=143219&action=edit
Attachment #176722 -
Flags: review?(ccarlen)
Attachment #176722 -
Attachment is obsolete: true
Attachment #176722 -
Flags: review?(ccarlen) → review-
Reporter | ||
Comment 10•19 years ago
|
||
I made following changes: 1. Remove ForgetCurrentProfile(), which should not be called in RenameProfile() 2. Update some data fields if the renamed profile is current used profile This patch uses some codes posted by carlen at the bug 225264. I have tested the patch in my environment and it fixed both 225264 and 245135.
Attachment #176856 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176856 -
Flags: review?(ccarlen)
Comment 11•19 years ago
|
||
Comment on attachment 176856 [details] [diff] [review] a new proposal I'd like to think that the two blocks could be merged together: >- >- rv = ForgetCurrentProfile(); >- if (NS_FAILED(rv)) return rv; >+ >+ nsXPIDLString currProfileName; >+ gProfileDataAccess->GetCurrentProfile(getter_Copies(currProfileName)); >+ if (currProfileName.Equals(oldName)) >+ gProfileDataAccess->SetCurrentProfile(newName); >+ mCurrentProfileName.Assign(newName); >+ mCurrentProfileAvailable = PR_TRUE; Sorry, I forgot to copy the { and }s. Should we be checking the gProfileDataAccess profile name or mCurrentProfileName? Also, I'm not sure that we should be setting mCurrentProfileAvailable, although I suppose it will only be set if mCurrentProfileName is set.
Comment 12•19 years ago
|
||
Sorry, I made that comment before reading your email. In that case, I would like your comments on two ideas I have: The first idea is to update the current profile name (using the single block) between CopyRegKey and DeleteProfile. The second idea is to fix things up if gProfileDataAccess->mForgotProfileCalled is set by DeleteProfile when renaming the current profile.
Reporter | ||
Comment 13•19 years ago
|
||
(In reply to comment #12) > Sorry, I made that comment before reading your email. In that case, I would like > your comments on two ideas I have: > > The first idea is to update the current profile name (using the single block) > between CopyRegKey and DeleteProfile. > I don't think we can update the current profile name between CopyRegKey() and DeleteProfile(). Because DeleteProfile() calles ForgetCurrentProfile(), which will set current profile to empty. > The second idea is to fix things up if gProfileDataAccess->mForgotProfileCalled > is set by DeleteProfile when renaming the current profile. I have checked the code base, there is only one place where mForgotProfileCalled is accessed: nsProfileAccess::GetCurrentProfile(PRUnichar **profileName) { ... if (!mCurrentProfile.IsEmpty() || mForgetProfileCalled) { *profileName = ToNewUnicode(mCurrentProfile); } ... } I'm not sure if the mForgetProfileCalled is really needed. In my patch I set mCurrentProfile to the renamed profile, so nsProfileAccess::GetCurrentProfile() will return the correct profile name. For the sake of safty, I will modify the patch to update mForgetProfileCalled.
Reporter | ||
Comment 14•19 years ago
|
||
Attachment #176980 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176980 -
Flags: review?(ccarlen)
Comment 15•19 years ago
|
||
(In reply to comment #13) >I don't think we can update the current profile name between CopyRegKey() and >DeleteProfile(). Because DeleteProfile() calles ForgetCurrentProfile(), which >will set current profile to empty. But if we've updated the current profile name DeleteProfile is no longer deleting the current profile.
Reporter | ||
Comment 16•19 years ago
|
||
(In reply to comment #15) > (In reply to comment #13) > >I don't think we can update the current profile name between CopyRegKey() and > >DeleteProfile(). Because DeleteProfile() calles ForgetCurrentProfile(), which > >will set current profile to empty. > But if we've updated the current profile name DeleteProfile is no longer > deleting the current profile. No the calling sequence is: CopyRegKey() DeleteProfile() set the current profile name to renamed profile CopyRegKey() doesn't set the current profile to the new renamed profile so DeleteProfile() still works on the old named profile.
Comment 17•19 years ago
|
||
(In reply to comment #16) > No the calling sequence is: > CopyRegKey() > DeleteProfile() > set the current profile name to renamed profile So changing the sequence won't work? Maybe that's a bad idea.
Reporter | ||
Comment 18•19 years ago
|
||
(In reply to comment #17) > (In reply to comment #16) > > No the calling sequence is: > > CopyRegKey() > > DeleteProfile() > > set the current profile name to renamed profile > So changing the sequence won't work? Maybe that's a bad idea. I think the sequence is correct. Why should we change the sequence?
Comment 19•19 years ago
|
||
When starting Mozilla GetCurrentProfile returns the first profile in the list, although mCurrentProfileAvailable is still false. Maybe writing PRBool renamedIsCurrent = mCurrentProfileName.Equals(oldName); will fix this.
Comment 20•19 years ago
|
||
(In reply to comment #19) > When starting Mozilla GetCurrentProfile returns the first profile in the list, > although mCurrentProfileAvailable is still false. Maybe writing PRBool > renamedIsCurrent = mCurrentProfileName.Equals(oldName); will fix this. Yep. GetCurrentProfile is badly named. It's not really the "currently set", or active profile, but a the last profile that was active - persisted in the registry. mCurrentProfileName is the name of the current profile, as set by SetCurrentProfile.
Reporter | ||
Comment 21•19 years ago
|
||
I made following modifications in this patch: 1. Move the codes of updating current profile to the place just after DeleteProfile() but before the error checking as Neil suggested. This is to make sure that nsProfile points to the correct profile even deleting is failed 2. Modify the implementation of nsProfile::GetCurrentProfile() as the following: if mCurrentProfileName points to a proifle i.e. it is not empty, just use this value else use the value gotten by gProfileDataAccess->GetCurrentProfile() as the current profile while set mCurrentProfileName to this value. I don't think the nsProfile::GetCurrentProfile() is badly named, because it implements the nsIProfile::currentProfile interface, which is supposed to return current profile name. nsProfile::GetCurrentProfile() is just implemented somewhat incorrectly. 3. Change the gProfileDataAccess->GetCurrentProfile(getter_Copies(currProfileName)); if (currProfileName.Equals(oldName)) renamedIsCurrent = PR_TRUE; to if (mCurrentProfileName.Equals(oldName)) renamedIsCurrent = PR_TRUE; as Neil suggested
Attachment #177121 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177121 -
Flags: review?(ccarlen)
Comment 22•19 years ago
|
||
Comment on attachment 177121 [details] [diff] [review] a new patch >diff -u -r1.310 nsProfile.cpp -pu is nicer ;-) >- gProfileDataAccess->GetCurrentProfile(profileName); I don't think we should be changing this (maybe I misunderstood your reason). >+ // If the profile being renamed is the current profile, >+ // change the current profile name. >+ PRBool renamedIsCurrent = PR_FALSE; >+ if (mCurrentProfileName.Equals(oldName)) >+ renamedIsCurrent = PR_TRUE; You can initialize this in one statement: PRBool renamedIsCurrent = mCurrentProfileName.Equals(oldName); Also I don't think the comment is quite accurate because the code that changes the name is in the next change below. Maybe you should move the comment there instead.
Reporter | ||
Comment 23•19 years ago
|
||
Because the nsProfile::GetCurrentProfile() returns current profile name and mCurrentProfileName contains this value, I think we should use it instead of gProfileDataAccess->GetCurrentProfile() if mCurrentProfileName is valid. If mCurrentProfileName is empty, we should set it to a correct profile name. "else" part does this.
Attachment #177129 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177129 -
Flags: review?(ccarlen)
Reporter | ||
Comment 24•19 years ago
|
||
mCurrentProfileName should be set in nsProfile::GetCurrentProfile(), if it is empty. Because when mozilla is startup with -P, mCurrentProfileName is empty, so + PRBool renamedIsCurrent = mCurrentProfileName.Equals(oldName); will have problem.
Comment 25•19 years ago
|
||
(In reply to comment #24) >mCurrentProfileName should be set in nsProfile::GetCurrentProfile(), if it is >empty. Because when mozilla is startup with -P, mCurrentProfileName is empty, so > >+ PRBool renamedIsCurrent = mCurrentProfileName.Equals(oldName); > >will have problem. Sorry, I don't see the problem here. When mozilla is started with -P then no profile is current so that renaming a profile shouldn't change anything.
Reporter | ||
Comment 26•19 years ago
|
||
Yes you are right. But I still think mCurrentProfileName should be checked first in nsProfile::GetCurrentProfile(), becuase this method returns the current profile name and mCurrentProfileName contains this value.
Comment 27•19 years ago
|
||
Comment on attachment 177129 [details] [diff] [review] patch >- gProfileDataAccess->GetCurrentProfile(profileName); >+ >+ if (!mCurrentProfileName.IsEmpty()) { >+ *profileName = ToNewUnicode(mCurrentProfileName); >+ } else { >+ gProfileDataAccess->GetCurrentProfile(profileName); >+ mCurrentProfileName = *profileName; >+ } So, as I understand it, this patch is correct, once you remove the line mCurrentProfileName = *profileName;
Attachment #177129 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Reporter | ||
Comment 28•19 years ago
|
||
Attachment #177464 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177464 -
Flags: review?(ccarlen)
Comment 29•19 years ago
|
||
Comment on attachment 177464 [details] [diff] [review] a revised patch >+ if (!mCurrentProfileName.IsEmpty()) >+ *profileName = ToNewUnicode(mCurrentProfileName); >+ > gProfileDataAccess->GetCurrentProfile(profileName); This is no good, you missed out the "else". sr=me if you put it back in.
Attachment #177464 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #177464 -
Attachment is obsolete: true
Attachment #177464 -
Flags: review?(ccarlen) → review-
Reporter | ||
Comment 30•19 years ago
|
||
sorry for the mistake
Attachment #177472 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177472 -
Flags: review?(ccarlen)
Updated•19 years ago
|
Attachment #177472 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Comment 31•19 years ago
|
||
Comment on attachment 177472 [details] [diff] [review] a new patch > >+ PRBool renamedIsCurrent = mCurrentProfileName.Equals(oldName); >+ > // Copy reg keys > rv = CopyRegKey(oldName, newName); > if (NS_FAILED(rv)) return rv; > > // Delete old profile entry > rv = DeleteProfile(oldName, PR_FALSE /* don't delete files */); >+ >+ // If the profile being renamed is the current profile, >+ // change the current profile name. >+ if (renamedIsCurrent) { >+ gProfileDataAccess->SetCurrentProfile(newName); >+ gProfileDataAccess->mForgetProfileCalled = PR_FALSE; >+ mCurrentProfileName.Assign(newName); >+ mCurrentProfileAvailable = PR_TRUE; >+ } >+ r=ccarlen if you get rid of the line: mCurrentProfileAvailable = PR_TRUE; or convince me why it's needed That member variable is all-important and should be set in the fewest places possible. It should be true anyway if mCurrentProfileName is not empty.
Attachment #177472 -
Flags: review?(ccarlen) → review+
Reporter | ||
Comment 32•19 years ago
|
||
The mCurrentProfileName is not empty. Because if user enters an empty string when renaming a profile. checkProfileName (in createProfileWizard.js) will catch this error and nsProfle::RenameProfile() will not be called. I.e. when nsProfile::RenameProfile() is called, newName (the second parameter) must not be empty. So as you said, if mCurrentProfileName is not empty, mCurrentProfileAvailable should be set to true.
Comment 33•19 years ago
|
||
Actually I thought it was because DeleteProfile can call ForgetCurrentProfile so when renaming the current profile we need to "unforget" the profile.
Reporter | ||
Comment 34•19 years ago
|
||
Can we checkin the patch now?
Comment 35•19 years ago
|
||
I checked the patch in.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 36•19 years ago
|
||
I just unstalled a Beast build with this fix incorporated. It appears to have also fixed... "Bugzilla Bug 279497 - Selected profile does not always appear focused" Mozilla/5.0 (Windows; U; Win95; en-US; rv:1.8b2) Gecko/20050321 Firefox/1.0+
Updated•19 years ago
|
Attachment #149667 -
Flags: review?(ccarlen)
Updated•19 years ago
|
Attachment #176856 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176856 -
Flags: review?(ccarlen)
Updated•19 years ago
|
Attachment #176980 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176980 -
Flags: review?(ccarlen)
Updated•19 years ago
|
Attachment #177121 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177121 -
Flags: review?(ccarlen)
Updated•19 years ago
|
Attachment #177129 -
Flags: review?(ccarlen)
Updated•16 years ago
|
Attachment #177129 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #177121 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #176980 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #176856 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•