245135 - The using profile can be deleted after renaming

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Boying Lu]

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 1

[Boying Lu]

Attachment: patch v0

Comment 2

[Boying Lu]

Comment on attachment 149667 [details] [diff] [review]
patch v0

Can you give r?
Thanks

Comment 3

[Conrad Carlen (not reading bugmail)]

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.

Comment 4

[Brian Duffy]

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

Comment 5

[Boying Lu]

Attachment: patch v1

Comment 6

[WADA:World Anti-bad-Duping Agency]

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

[Conrad Carlen (not reading bugmail)]

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.

Comment 8

[[e-mail]]

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).

Comment 9

[Boying Lu]

Attachment: the patch is made by adding bug 225264's patch

this patch contains the patch
https://bugzilla.mozilla.org/attachment.cgi?id=143219&action=edit

Comment 10

[Boying Lu]

Attachment: a new proposal

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.

Comment 11

[[e-mail]]

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

[[e-mail]]

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.

Comment 13

[Boying Lu]

(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.

Comment 14

[Boying Lu]

Attachment: add setting gProfileDataAccess->mForgetProfileCalled to PR_FALSE code

Comment 15

[[e-mail]]

(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.

Comment 16

[Boying Lu]

(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

[[e-mail]]

(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.

Comment 18

[Boying Lu]

(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

[[e-mail]]

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

[Conrad Carlen (not reading bugmail)]

(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. 

Comment 21

[Boying Lu]

Attachment: a new patch

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

Comment 22

[[e-mail]]

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.

Comment 23

[Boying Lu]

Attachment: patch

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.

Comment 24

[Boying Lu]

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

[[e-mail]]

(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.

Comment 26

[Boying Lu]

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

[[e-mail]]

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;

Comment 28

[Boying Lu]

Attachment: a revised patch

Comment 29

[[e-mail]]

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.

Comment 30

[Boying Lu]

Attachment: a new patch

sorry for the mistake

Comment 31

[Conrad Carlen (not reading bugmail)]

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.

Comment 32

[Boying Lu]

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

[[e-mail]]

Actually I thought it was because DeleteProfile can call ForgetCurrentProfile so
when renaming the current profile we need to "unforget" the profile.

Comment 34

[Boying Lu]

Can we checkin the patch now?

Comment 35

[[e-mail]]

I checked the patch in.

Comment 36

[Fred Collier]

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+