168433 - mozilla -CreateProfile doesn't create prefs.js for subsequent profiles

Status: VERIFIED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[hacker formerly known as [e-mail]]

When creating new profiles using ./mozilla -CreateProfile <profilename>, the new
profile doesn't contain the prefs.js file.  At first, I thought that this was
just a bug in the static build but I see the same problem in my non-static
builds.  Since the tinderbox will fail the runtime tests if prefs.js isn't
present, I would expect more tinderboxes to show bustage but it appears to be
just select builds that don't work.  If I run ./mozilla -P <profilename>, then
the files show up but the tinderbox script immediately checks for the prefs file
after creating the profile.

Comment 1

[Chris McAfee]

adding bnesse, I seem to remember that prefs.js isn't always created
with a new profile ? until it is used?  I don't remember.  Brian?

Comment 2

[Conrad Carlen (not reading bugmail)]

It's not copied from the defaults. When a new profile is made, the contents of
<bin>/defaults/profile are copied to the new profile dir. prefs.js is not part
of the defaults. If prefs.js has been showing up in new profiles, it's probably
because something is setting a pref and calling SavePrefFile() early in the
startup process? Anyway, creation of prefs.js is not directly a part of making a
new profile.

Comment 3

[Brian Nesse (gone)]

Yes, this caused me no end of grief on the Tinderboxen which run tests when I
tried to land the safe save code. The -CreateProfile option follows a codepath
which either 1) does not instantiate a PrefService or 2) never loads/saves the
preferences. This makes sense as neither of these things should be needed to
create an empty profile.

The (empty) prefs.js file will be created the first time the application is run
using the created profile... on exit it will write the current preferences state
(which if you simply launch and quit will be approximately 8 prefs).

Comment 4

[hacker formerly known as [e-mail]]

I briefly traced the callpath and eventually we wind up in
nsProfile::StartupWithArgs which will return NS_ERROR_ABORT if there's no
current profile (which implies -CreateProfile is being used).   This return
value gets passed back up and eventually winds up as the return value of main1
which causes mozilla to skip the SavePrefFile call on shutdown.

The only thing that pops in mind is that GetCurrentProfile isn't returning 0 but
I'll have to hack a tinderbox with a working test to see what it says.

Comment 5

[hacker formerly known as [e-mail]]

This is weird.  I stepped through a -CreateProfile run on myotonic with profile,
appshell & apprunner recompiled with debug info.  When I run -CreateProfile
manually, remembering to reset $HOME to the tinderbox dir, it doesn't create the
prefs.js file.  When the command is run from the script, it does. 

AH HA!  For giggles, I removed the tinderbox's entire .mozilla directory.  Now,
when I run -CreateProfile, it creates a prefs.js file for the *first* profile
that gets created.  

Eventually, nsPrefService::ReadUserPrefs gets called with a non-null arg via
nsProfile::LoadNewProfilePrefs.  This causes SavePrefFile to be called from
nsPrefService::UseDefaultPrefFile.  I still don't understand why it's not
creating a prefs.js file for each profile.  

Comment 6

[hacker formerly known as [e-mail]]

A printf in ReadUserPrefs() shows that UseDefaultProfile() is returning the path
to the prefs.js file of the first profile rather than the current profile.  It
looks like there's a bug in NS_GetSpecialDirectory(). 

Comment 7

[Chris McAfee]

seawood: any idea why this is only happening on the static builds?  I looked at
the tinderbox scripts for a while and ? I am a bit confused also.

Comment 8

[hacker formerly known as [e-mail]]

It's not just the static builds.  Myotonic is not doing a static build.  

I think we are seeing a different problem with the tinderbox static builds where
the profile isn't being created at all.  Note that my local static build creates
the profile (sans prefs.js since I already have a primary profile) just fine.

Comment 9

[hacker formerly known as [e-mail]]

For whatever reason, the static build on balsa is failing in main1() trying to
create an instance of the startupNotifier.  It never even gets to processing the
cmdline args.

Comment 10

[Conrad Carlen (not reading bugmail)]

It's not a bug in NS_GetSpecialDirectory(). When LoadNewProfilePrefs() is called
in creating a new profile, nsProfile::SetCurrentProfile() has not been called so
the profile-relative file locations returned by NS_GetSpecialDirectory() are not
for the profile which was just created. The question is: why is
LoadNewProfilePrefs() called in creating a new profile?

Comment 11

[hacker formerly known as [e-mail]]

According to lxr, that's the only place that LoadNewProfilePrefs is called and
it's been there awhile.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/profile/src/nsProfile.cpp#882

Comment 12

[hacker formerly known as [e-mail]]

Attachment: Set the current profile before loading prefs

Calling nsProfile::SetCurrentProfile fixes the problem of prefs.js only being
created for the first profile.

Comment 13

[Conrad Carlen (not reading bugmail)]

-CreateProfile is supposed to simply create a new profile and exit.

> Since the tinderbox will fail the runtime tests if prefs.js isn't present

Why is that? What is being tested for? I'm a little leary of changing this for
the sake of the tinderbox scripts. Can we instead have the scripts pass an arg
which explicitly does whatever is needed (create a new profile, make it current,
save some prefs to it) instead of changing the behavior of -CreateProfile?

Comment 14

[hacker formerly known as [e-mail]]

> Why is that? What is being tested for? 

The tinderbox script looks for prefs.js to find the actual location of the
profile (obsfucated due to the .slt) and then it adds a handful of preferences
to that file.

> I'm a little leary of changing this for the sake of the tinderbox scripts. 

Well, the current behavior is inconsistent at best.  Due to the inconsistency of
  the prefs.js generation, I thought that this would break the tinderboxes. 
Since most of the tinderboxes do not use multiple profiles, they should be
unaffected by this bug.

> Can we instead have the scripts pass an arg which explicitly does whatever is 
> needed (create a new profile, make it current, save some prefs to it) instead 
> of changing the behavior of -CreateProfile?

If the actual behavior of -CreateProfile changed our from underneath the
tinderbox scripts, then the tb scripts will have to adjust accordingly. 
However, it doesn't sound like the behavior has changed from when the tb scripts
were written.  I think that adding a new runtime option just for the sake of
tinderbox is overkill.  And it definitely shouldn't be done until after we
determine what the correct behavior for -CreateProfile (to generate prefs.js or
not) should be.

Comment 15

[Conrad Carlen (not reading bugmail)]

Comment on attachment 99221 [details] [diff] [review]
Set the current profile before loading prefs

>Index: profile/src/nsProfile.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/profile/src/nsProfile.cpp,v
>retrieving revision 1.267
>diff -u -r1.267 nsProfile.cpp
>--- nsProfile.cpp	2002/09/07 17:12:07	1.267
>+++ nsProfile.cpp	2002/09/14 22:05:01
>@@ -882,6 +882,7 @@
>             if (NS_SUCCEEDED(rv)) {
>                 *profileDirSet = PR_TRUE;
>                 mCurrentProfileAvailable = PR_TRUE;

Since you're calling SetCurrentProfile(),  mCurrentProfileAvailable = PR_TRUE
can come out. SetCurrentProfile() sets that.

>+                gProfileDataAccess->SetCurrentProfile(currProfileName.get());
> 
>                 // Need to load new profile prefs.
>                 rv = LoadNewProfilePrefs();

Comment 16

[hacker formerly known as [e-mail]]

Attachment: Updated with conrad's change

Comment 17

[Conrad Carlen (not reading bugmail)]

Comment on attachment 99618 [details] [diff] [review]
Updated with conrad's change

r=ccarlen

Comment 18

[hacker formerly known as [e-mail]]

Comment on attachment 99618 [details] [diff] [review]
Updated with conrad's change

Transferring r=

Comment 19

[Alec Flett]

Comment on attachment 99618 [details] [diff] [review]
Updated with conrad's change

sr=alecf

Comment 20

[hacker formerly known as [e-mail]]

Patch has been checked in.

Comment 21

[hacker formerly known as [e-mail]]

I had to revert back to the first patch as removing the mCurrentProfileAvailable
= PR_TRUE caused the prefs.js file to not be created at all....which broke the
tinderboxes.

Comment 22

[Conrad Carlen (not reading bugmail)]

> removing the mCurrentProfileAvailable
= PR_TRUE caused the prefs.js file to not be created at all

Ack - I need to see why.

Comment 23

[Grace Bush]

verified Linux 2003032505