Closed Bug 245135 Opened 20 years ago Closed 19 years ago

The using profile can be deleted after renaming

Categories

(SeaMonkey :: Startup & Profiles, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: eagle.lu, Unassigned)

References

Details

Attachments

(1 file, 8 obsolete files)

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
Attached patch patch v0 (obsolete) — Splinter Review
Comment on attachment 149667 [details] [diff] [review]
patch v0

Can you give r?
Thanks
Attachment #149667 - Flags: review?(ccarlen)
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.
Product: Browser → Seamonkey
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
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #176562 - Flags: review?(ccarlen)
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 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 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
Attachment #176722 - Flags: review?(ccarlen)
Attachment #176722 - Attachment is obsolete: true
Attachment #176722 - Flags: review?(ccarlen) → review-
Attached patch a new proposal (obsolete) — Splinter Review
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 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.
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.
(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.
Attachment #176980 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176980 - Flags: review?(ccarlen)
(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.
(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.
(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.
(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?
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.
(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. 

Attached patch a new patch (obsolete) — Splinter Review
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 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.
Attached patch patch (obsolete) — Splinter Review
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)
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.
(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.
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 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+
Attached patch a revised patch (obsolete) — Splinter Review
Attachment #177464 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177464 - Flags: review?(ccarlen)
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-
Attached patch a new patchSplinter Review
sorry for the mistake
Attachment #177472 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177472 - Flags: review?(ccarlen)
Attachment #177472 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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+
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.
Actually I thought it was because DeleteProfile can call ForgetCurrentProfile so
when renaming the current profile we need to "unforget" the profile.
Can we checkin the patch now?
I checked the patch in.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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+	
Attachment #149667 - Flags: review?(ccarlen)
Attachment #176856 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176856 - Flags: review?(ccarlen)
Attachment #176980 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #176980 - Flags: review?(ccarlen)
Attachment #177121 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #177121 - Flags: review?(ccarlen)
Attachment #177129 - Flags: review?(ccarlen)
Attachment #177129 - Attachment is obsolete: true
Attachment #177121 - Attachment is obsolete: true
Attachment #176980 - Attachment is obsolete: true
Attachment #176856 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: