606575 - Profile local data dir (ProfLD / NS_APP_USER_PROFILE_LOCAL_50_DIR) is incorrect for a newly created profile

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Michael Smith]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.11) Gecko/20101013 Ubuntu/9.10 (karmic) Firefox/3.6.11
Build Identifier: 4.0b8pre

Getting "ProfLD" from the directory service returns the wrong directory for a profile newly created from the profile manager. On subsequent app runs, it's correct.

This is because the profile service's CreateProfile method is passed a null local profile directory by the create-profile wizard, and the service interprets this as "use the same directory as the root profile directory".

A patch is attached.

Reproducible: Always

Steps to Reproduce:
To reproduce, paste this line into the error console:
 alert(Components.classes["@mozilla.org/file/directory_service;1"].createInstance().QueryInterface(Components.interfaces.nsIProperties).get("ProfLD", Components.interfaces.nsIFile, null).path)
Actual Results:  
The first run with a freshly created profile, I get:
  C:\Documents and Settings\msmith\Application Data\Mozilla\Firefox\Profiles\wiiuioi1.ff4test

on subsequent runs, the correct/expected data is reported.

Expected Results:  
   C:\Documents and Settings\msmith\Local Settings\Application Data\Mozilla\Firefox\Profiles\wiiuioi1.ff4test

See null second parameter here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/content/createProfileWizard.js#235

This could alternatively be fixed in createProfileWizard.js by passing the directory here - not sure which is preferred.

Comment 1

[Michael Smith]

Attachment: Don't default local profile root to the same as the profile root when creating a new profile with a specified profile root but null local directory

Comment 2

[Marco Bonardo [:mak]]

Looks like this was ignored, but it's still a valid bug.
I can reproduce it by creating a new profile and checking the path, as soon as the profile is created it points to the roaming folder, not the local ones.

Comment 3

[Marco Bonardo [:mak]]

updated links to code:
http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/content/createProfileWizard.js#198
http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#751

Comment 4

[Marco Bonardo [:mak]]

There are 2 possible fixes imo:
1. the wizard distinguishes the case where the user just names a profile in the default root dir or he chooses a completely different path. In the former case passes null to both rootDir and tempDir, in the latter case passes the path to both.
2. instead of checking if aRootDir is defined the service may check if aRootDir != GetUserProfilesRootDir, and only in that case use it as temp dir
That said, the createProfile API allowing different behaviors based on null positions is quite error-prone, probably the only allowed cases should be both defined or both null.

Comment 5

[[e-mail]]

When you save a profile, it only saves the root path, not the local path. When you then start using that profile, if the root path was relative to the roaming profile folder, it uses the local path relative to the local appdata profile folder. Otherwise it uses the root path as the local path.

So either we should start saving the local path in profiles.ini, or creating a profile shouldn't be passed a local path, instead it should assume the local path based on the root path being relative or not.

Comment 6

[Marco Bonardo [:mak]]

(In reply to [e-mail] from comment #5)
> So either we should start saving the local path in profiles.ini, or creating
> a profile shouldn't be passed a local path, instead it should assume the
> local path based on the root path being relative or not.

Creating a profile (from the wizard) we don't pass a local path, the problem is that the service then looks that we passed a root path, and uses the root path as the local path. So my suggestion 2 would work here.
But I still think there a bit too much magic hidden by createProfile this way. IF instead we force the caller to either provide both or none, isn't clearer what will happen?

Comment 7

[[e-mail]]

There's no point bikeshedding over a parameter that doesn't get saved...

Comment 8

[Marco Bonardo [:mak]]

ok, I misinterpreted your comment, I agree.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

How hard is it to fix this? Can someone provide pointers on how to address this if someone wants to pick it up?

Comment 10

[[e-mail]]

(In reply to Tim Taubert from comment #9)
> How hard is it to fix this? Can someone provide pointers on how to address
> this if someone wants to pick it up?

The problem manifests when the profile created by CreateProfile is launched immediately. In this case the specified root and local profile paths are honoured by the startup code. However nsToolkitProfileService::Flush doesn't save the local profile path, so on the next restart it the local profile path is assumed based on whether the profile lives under the appdata folder.

The quick fix is to remove the local profile path parameter from CreateProfile and make it assume the local path using the same algorithm as Flush.

An alternative is to remove all knowledge of local profiles from startup code instead making the directory provider assume the local profile path.

Comment 11

[Bellindira Castillo [:bellindira]]

Attachment: Patch v1

Basically, the patch compares the root Dir and GetUserProfilesRootDir, if both are the same, local Dir is updated using GetUserProfilesLocalDir instead of rootDir. If their values are different, local Dir is equaled to root Dir.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 641737 [details] [diff] [review]
Patch v1

Sorry, I'm not the right one for this. Let's wait for Neil's feedback.

Comment 13

[[e-mail]]

Comment on attachment 641737 [details] [diff] [review]
Patch v1

Having the local dir equal to the root dir isn't always a problem.

In particular, if your root dir is outside of your default profile root then the local dir is expected to equal the root dir. (See Flush for how to tell whether the root dir is inside or outside your default profile root.)

You also make assumptions as to what dir you will be passed in, so it will only work if you create the profile in certain dirs. I guess the one you tested with is the default one that the create profile wizard offers you.

Comment 14

[Bellindira Castillo [:bellindira]]

In particular, if your root dir is outside of your default profile root then the local dir is expected to equal the root dir. (See Flush for how to tell whether the root dir is inside or outside your default profile root.)

Currently, at the patch, if the local dir is outside of the default profile root, the local dir is set to the root dir (line 773). If this is not the case, could you please clarify? I checked the Flush and it gives me the same result.

You also make assumptions as to what dir you will be passed in, so it will only work if you create the profile in certain dirs. I guess the one you tested with is the default one that the create profile wizard offers you.

I tested the default dir and "Choose Folder..." using the wizard (I chose a folder on the Desktop). So, what dirs would be a good test to make sure this works in all dirs? 

Thanks!

Comment 15

[Bellindira Castillo [:bellindira]]

> In particular, if your root dir is outside of your default profile root then
> the local dir is expected to equal the root dir. (See Flush for how to tell
> whether the root dir is inside or outside your default profile root.)

Currently, at the patch, if the local dir is outside of the default profile root, the local dir is set to the root dir (line 773). If this is not the case, could you please clarify? I checked the Flush and it gives me the same result.

> You also make assumptions as to what dir you will be passed in, so it will
> only work if you create the profile in certain dirs. I guess the one you
> tested with is the default one that the create profile wizard offers you.

I tested the default dir and "Choose Folder..." using the wizard (I chose a folder on the Desktop). So, what dirs would be a good test to make sure this works in all dirs? 

Thanks!

Comment 16

[[e-mail]]

(In reply to Bellindira Castillo from comment #15)
> > In particular, if your root dir is outside of your default profile root then
> > the local dir is expected to equal the root dir. (See Flush for how to tell
> > whether the root dir is inside or outside your default profile root.)
> Currently, at the patch, if the local dir is outside of the default profile
> root, the local dir is set to the root dir (line 773). If this is not the
> case, could you please clarify? I checked the Flush and it gives me the same
> result.
Ah, maybe I misread the patch. Sorry about that. I'll read it again.

> > You also make assumptions as to what dir you will be passed in, so it will
> > only work if you create the profile in certain dirs. I guess the one you
> > tested with is the default one that the create profile wizard offers you.
> I tested the default dir and "Choose Folder..." using the wizard (I chose a
> folder on the Desktop). So, what dirs would be a good test to make sure this
> works in all dirs?
The easiest way would seem to be to create your profile in the parent of the default Profiles folder. (This of course assumes that you're on the Mac or Windows, but then again Linux doesn't have a separate local profile root.)

Comment 17

[[e-mail]]

Comment on attachment 641737 [details] [diff] [review]
Patch v1

>     nsCOMPtr<nsIFile> localDir (aLocalDir);
> 
>     if (!localDir) {
>+      bool getLocalDir = false;
>+
>+      if (aRootDir) {
>+        nsAutoString rootDirPath, localDirPath;
>+        nsCOMPtr<nsIFile> tempParentDir;
>+
>+        rv = gDirServiceProvider->GetUserProfilesRootDir(getter_AddRefs(localDir),
>+                                                           aProfileName, aAppName,
>+                                                           aVendorName);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        rv = rootDir->GetParent(getter_AddRefs(tempParentDir));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        rv = tempParentDir->GetPath(rootDirPath);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        rv = localDir->GetPath(localDirPath);
>+        NS_ENSURE_SUCCESS(rv, rv);
Having looked again, I think this is still a logic bug, but for a different reason. Notice the !localDir test above your changes? This means that you're modifying the part of the code that works, and it will in fact crash here.

Comment 18

[Bellindira Castillo [:bellindira]]

(In reply to [e-mail] from comment #16)

> > > You also make assumptions as to what dir you will be passed in, so it will
> > > only work if you create the profile in certain dirs. I guess the one you
> > > tested with is the default one that the create profile wizard offers you.
> > I tested the default dir and "Choose Folder..." using the wizard (I chose a
> > folder on the Desktop). So, what dirs would be a good test to make sure this
> > works in all dirs?
> The easiest way would seem to be to create your profile in the parent of the
> default Profiles folder. (This of course assumes that you're on the Mac or
> Windows, but then again Linux doesn't have a separate local profile root.)
I tested on the parent of the default profiles folder and it works fine. As Linux doesn't have a separate local profile root, it uses the root dir which I think it is right.

(In reply to [e-mail] from comment #17)
> Comment on attachment 641737 [details] [diff] [review]
> Patch v1
> 
> >     nsCOMPtr<nsIFile> localDir (aLocalDir);
> > 
> >     if (!localDir) {
> >+      bool getLocalDir = false;
> >+
> >+      if (aRootDir) {
> >+        nsAutoString rootDirPath, localDirPath;
> >+        nsCOMPtr<nsIFile> tempParentDir;
> >+
> >+        rv = gDirServiceProvider->GetUserProfilesRootDir(getter_AddRefs(localDir),
> >+                                                           aProfileName, aAppName,
> >+                                                           aVendorName);
> >+        NS_ENSURE_SUCCESS(rv, rv);
> >+        rv = rootDir->GetParent(getter_AddRefs(tempParentDir));
> >+        NS_ENSURE_SUCCESS(rv, rv);
> >+        rv = tempParentDir->GetPath(rootDirPath);
> >+        NS_ENSURE_SUCCESS(rv, rv);
> >+        rv = localDir->GetPath(localDirPath);
> >+        NS_ENSURE_SUCCESS(rv, rv);
> Having looked again, I think this is still a logic bug, but for a different
> reason. Notice the !localDir test above your changes? This means that you're
> modifying the part of the code that works, and it will in fact crash here.

I cannot make it crash (How did you make it crash?).

This part of the code does the following:
GetUserProfilesRootDir: Saves in localDir the user profile according to aProfileName.
Then, asks for the path of localDir (localDir->GetPath(localDirPath). So, it saves the path of the user profile on localDirPath.
Also, it gets the path of rootDir (the parent Dir) and it saves it on rootDirPath.
Then, it compares the two values.

If the localDirPath is equal to rootDirPath, it means that it should get the GetUserProfiles*Local*Dir instead of GetUserProfiles*Root*Dir.

Please, let me know if you have any question about it.

BTW, it is not possible to use the same algorithm as Flush because if we do: cur = mFirst, and then iterate on it (cur = cur->mNext) cur will never reach the profile we are just creating.

Thanks.

Comment 19

[[e-mail]]

(In reply to Bellindira Castillo from comment #18)
> (In reply to comment #16)
> > The easiest way would seem to be to create your profile in the parent of the
> > default Profiles folder. (This of course assumes that you're on the Mac or
> > Windows, but then again Linux doesn't have a separate local profile root.)
> I tested on the parent of the default profiles folder and it works fine. As
> Linux doesn't have a separate local profile root, it uses the root dir which
> I think it is right.
I'm sorry if I wasn't clear earlier, but this bug cannot be reproduced on Linux, because the local profile directory is always the same. On Windows, and possibly the Mac, the local profile directory is expected to be different if the profile directory is a subdirectory of the default profile root.

Comment 20

[Hector Zhao [:hectorz]]

Attachment: bug-606575-createprofile.patch

Comment 21

[[e-mail]]

Comment on attachment 831964 [details] [diff] [review]
bug-606575-createprofile.patch

This fixes the bug as described, so f+ for you. Thanks!

Anyone who reviews this is going to at least want a test. I suggest modifying test_create_profile.xul to check that the created profile has different local and root paths. You should write the test both with and without your patch so that you can be sure that your patch fixes the bug and the test.

The other change that I would appreciate is the removal of the now unused aLocalDir parameter to CreateProfileInternal and CreateProfile. I found the following references to CreateProfile:
DebuggerProcess.jsm, line 127
createProfileWizard.js, line 198
test_create_profile.xul, line 79
nsIToolkitProfileService.idl, line 51
nsToolkitProfileService.cpp, line 700
nsAppRunner.cpp, lines 2189;2192;2276
ProfileReset.cpp, line 42

Comment 22

[Hector Zhao [:hectorz]]

(In reply to [e-mail] from comment #21)
> Anyone who reviews this is going to at least want a test. I suggest
> modifying test_create_profile.xul to check that the created profile has
> different local and root paths.
But they are not necessarily different, maybe I should check that they are repectively in DefProfRt/DefProfLRt? (as aRootDir is always null in the existing test)

> 
> The other change that I would appreciate is the removal of the now unused
> aLocalDir parameter to CreateProfileInternal and CreateProfile.
Thanks for the feedback! I'll attach an updated patch next week.

Comment 23

[[e-mail]]

What you could do is to create the profile, get the root dir, delete the profile, create the profile again using the root dir, check the local dir is different.

Comment 24

[Hector Zhao [:hectorz]]

Attachment: bug-606575-createprofile.patch

Patch updated.

(In reply to [e-mail] from comment #21)
> Anyone who reviews this is going to at least want a test. I suggest
> modifying test_create_profile.xul to check that the created profile has
> different local and root paths. You should write the test both with and
> without your patch so that you can be sure that your patch fixes the bug and
> the test.
The test will fail on Windows w/o my patch.

> 
> The other change that I would appreciate is the removal of the now unused
> aLocalDir parameter to CreateProfileInternal and CreateProfile. I found the
> following references to CreateProfile:
> ...
Done.

(In reply to [e-mail] from comment #23)
> What you could do is to create the profile, get the root dir, delete the
> profile, create the profile again using the root dir, check the local dir is
> different.

Thanks for the advice.

Comment 25

[[e-mail]]

Comment on attachment 8334415 [details] [diff] [review]
bug-606575-createprofile.patch

Heh, I had forgotten that I was one of the preferred reviewers for profile.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e2cc0ab65871

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2cc0ab65871

Comment 28

[Hector Zhao [:hectorz]]

Thanks Neil and Ryan!