92447 - PREF: "general.config.filename" is not read after profile migration from 4.x

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect)

Description

[Trix Supremo]

DESC:
If a profile is migrated during the launch of netscape, netscape fails to read
the pref "general.config.filename" from the ALL-NS.JS which in turn fails to
read the netscape.cfg at first startup.

STEPS:
1. Assume 4.x installed with user profile previously created.
2. Install 6.x.
3. copy activ_01.cfg into application directory.(browser startup page is locked
and set to be blank)
4. In all-ns.js change value of pref ("general.config.filename","netscape") to
("general.config.filename","activ_01.cfg").
5. launch 6.x and goto Edit|Preferences

RESULT: 
Browser Startup page is not setup to be blank nor is it locked.

EXPECTED:
CFG to be read and startup page to be set and locked.

NOTE:
Verified that prefs are being read in the ALL-NS.JS after profile migration. The
problem seems to be isolated to the inability to read that pref the first time
after profile migration.

Comment 1

[Rodney Velasco]

This is going to be a problem for our eClient config and deployment tool beta.
How?
1. IT administrator hashes a CFG with customized browser prefs.
2. Uses install builder to build new client.
3. After building N6Setup.exe he/she installs on computer.

After launching customized version of browser, no customized prefs will be
viewed in browser at initial launch.  You have to File -> Exit and start again
for the CFG to be read by browser.

Comment 2

[Rodney Velasco]

adding chip to Cc since he developed the CFG activation in the browser.

Comment 3

[Hong Kwon]

Changing severity to blocker -- this must be fixed for Netscape6.Fall RTM.

Comment 4

[Mitch Green]

Hong, I concur.  It's a blocker.  

Requirement:  The new netscap6.cfg must override any previous netscape.cfg.  I
think we need to simply ignore netscape.cfg and lay down a new netscap6.cfg
instead, then scoop the rest of the prefs.js file and act appropriately.  

One reason we purposefully broke backward compatibility between netscape.cfg and
netscap6.cfg was to ensure that enterprises rolled out brand new customized
preset/locked config files.  We want to override/ignore old settings and rely
only upon new settings.  Does this make the problem resolution easier?

Comment 5

[Rodney Velasco]

Currently we do not break backward compatibility (on the branch and trunk).  A
bug for this has been filed (bug 80789) but was not checked into the 6.1 RTM
release.  The current hash tool defaults it's output to read netscp6.cfg, but
the entry in the all-ns.js file does not reflect that change (but can easily be
changed manually via any text editor).  Luckily chip designed CFG activation in
a way that a hashed file with any name can be used to activate the customizations.

SideNote:
This issue does not happen if profile migration does not occur (i.e. fresh N6.1
install on a system that has no 4.x build on it)

Comment 6

[Brian Nesse (gone)]

I'd be willing to bet that this has nothing to do with netscape.cfg. It is
probably related to Bug 91175 (and all of the bugs it references). I'll look
into it, but you might try this with another preference...maybe something like:
user_pref("font.size.fixed.x-western", 18);

Comment 7

[Brian Nesse (gone)]

There are two things going on here. After the profile migration you aren't
seeing the startup page... you are seeing the "override" url.

To paraphrase chipc from bug 91175... "The reason for this pref is so that
generic downloads of the product will go to home.netscape.com...the first time
the browser loads..." So, in essence this *is* doing what it has been told to
do. When you launch the broswer the second time, the regular processing kicks in
and it does what the .cfg file tells it to.

The fact that the UI isn't locked however is interesting. Regardless of the
state of the url override flag, it should obey the lock state. This will require
further investigation.

Comment 8

[Brian Nesse (gone)]

Bummer. Well, this is both good and bad I guess. It turns out that the reason
this does not work as expected is because the profile migration process is
calling ResetPrefs(). ResetPrefs() basically empties out the hash table and
starts over.

Since the config file is only read in at startup in nsAppRunner, it gets flushed
but not reloaded... (this is the bad part :)). On the up side this will force me
to make some changes I've been considering anyway.

What we probably need to do is move the reading of the cfg file out of
nsAppRunner, and into nsPrefService. This way we can control it if and reload it
if ResetPrefs() is called.

Comment 9

[Conrad Carlen (not reading bugmail)]

What migration is doing:

(1) resetting all prefs
(2) reading old prefs into freshly cleared pref service
(3) writing the new contents of the pref service to the new prefs file
(4) resetting all prefs

is bad news. Prefs is a service and this code is treating it as if it were a
component of which it had a private instance for this task.

Brian, can the prefs object really only exist as a singleton (the prefs
service)? I think I remember when prefs were being reworked wanting to be able
to either (1) create independent pref instances or (2) be able to merge pref
branches. Yep, it was for this problem.

Comment 10

[Brian Nesse (gone)]

Yes, the pref service is a singleton object. As it stands, there is no simple
way to make it work otherwise. The underlying prefapi.c code which handles the
hash table support is fairly globals based and not prone to modification.

For the problem at hand, I believe what I suggest above will do the trick. Are
there other objects that you believe are susceptible to problems because of this
process? It would have to be a preference that is only created/set between
launch and the selection of the profile... anything after that point should be
reset properly.

Comment 11

[Brian Nesse (gone)]

Attachment: Patch to make reading of config file internal to preferences.

Comment 12

[Brian Nesse (gone)]

The attached patch removes the ReadConfigFile() from nsAppRunner and makes it 
internal to the preferences service. This does fix the problem with the 
preferences panel not being locked. It does not however change the fact that the 
startup page appears on first launch as per my comments of 2001-08-03.

Comment 13

[Chip Clark]

r=chipc

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

+  nsresult rv = NS_ERROR_FAILURE;
 
   if (PREF_Init(nsnull) == PR_TRUE) {
+    rv = readConfigFile();
+    if (NS_FAILED(rv))
+      return rv;

...
-  } else {
-    rv = NS_ERROR_FAILURE;
   }
   return(rv);

This is really convoluted.  Why compare explicitly against PR_TRUE?  Why not 
just return early from a failed PREF_Init?

if (!PREF_Init(nsnull))
    return NS_ERROR_FAILURE;

nsresult rv = readConfigFile();
...

(I notice that ResetPrefs was, previously, using PREF_Init's PRBool return as 
an nsresult.  Yikes.)

+  rv = mRootBranch->GetCharPref("general.config.filename", getter_Copies
(lockFileName));
+  if(NS_FAILED(rv))
+    return NS_OK;
+    // If the lockFileName is NULL return ok, because no lockFile will be used

The comment implies that you're checking for a null lockFileName, but that's 
not what the code is doing.  Are all errors ignorable there (out of memory, for 
example?), or just the absence of general.config.filename?  (And are we still 
doing this crazy vendor/lockfile name duplication thing?)

Also, the dominant style in that file is space-before-opening-paren for if 
conditions, but you've (sometimes) omitted it.  Please make it consistent.

+  //  lockVendor and lockFileName should be the same with the addtion of .cfg 
to the filename
+  //  by checking this post reading of the cfg file this value can be set 
within the cfg file
+  //  adding a level of security.
+  if (PL_strncmp(lockFileName, lockVendor, fileNameLen -4) != 0)
+    return NS_ERROR_FAILURE;

I asked Mitesh about this in another bug, and he deferred to you.  So now I'm 
asking you: if you require lockFileName to be lockVendor.cfg, why aren't you 
checking for that?  The code in question will let a lockVendor of "netscape" 
and a lockFileName of "netscape4you" pass.  But let's discuss why we're 
requiring the filename to match the vendor in the first place; I didn't get a 
very satisfactory response in Mitesh's bug, but you said that Mitch Green might 
be able to explain better.  Mitch?

I'd also like to see a review by Mitesh, or someone else with more prefs 
experience (I'd hold out for alecf, but he's on sabbatical) before I stamp this 
to go in.

Comment 15

[Brian Nesse (gone)]

> Why compare explicitly against PR_TRUE?
Mainly to visually indicate thatPref_Init() returns a jsbool rather than an
nsresult.

>(I notice that ResetPrefs was, previously, using PREF_Init's PRBool return as 
>an nsresult.  Yikes.)
Yeah, fortunately no one was actually checking that return value.

Regarding the various code in readConfigFile:
Actually I believe mitesh deferred to chipc who, having brought it over from
4.x,  is most familiar with this code.

I am pulling mitesh's changes for bug 87661 which, apparenlty, have some of this
code cleaned up. I will generate a new patch using that as a starting point.

Comment 16

[Mitesh Shah]

I have opened a new bug 96897 to discuss about the netscape.cfg reading so we
can separate this one and bug 87661 from it.

Comment 17

[Brian Nesse (gone)]

Attachment: New patch addressing sr comments

Comment 18

[Brian Nesse (gone)]

I have attached a new patch. I cleaned up some of the functionality in 
readConfigFile and I tried to address some things via comments in the code.

To the best of my knowledge we do not *require* the lockFileName to be 
lockVendor.cfg. We allow the vendor the opportunity to do it that way as an 
additional security level. Admittedly this is weak security, but it is simply an 
additional deterrent to keep people from trying to hack the .cfg file.

Comment 19

[Mitesh Shah]

This may create a duplicate AutoConfig object when the
ResetPrefs()->ReadConfig() happens but I think I will open a separate bug to
address the issue of how to release and restart AutoConfig (That invloves
removing from the Observer List (bug 86688, bug 94349, bug 94636) and removing
circular timer reference (bug  95719) )

r=mitesh for the latest patch. 

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 47068 [details] [diff] [review]
New patch addressing sr comments

A diff -u would have been nice. Comments:
+    if (rv == NS_ERROR_UNEXPECTED)
+       return NS_OK;
+     return rv;

would be cleaner as:

+    if (rv == NS_ERROR_UNEXPECTED)
+       rv = NS_OK;
+     return rv;

+     if (NS_SUCCEEDED(lockPrefFile->Append(lockFileName))) {
+       if (NS_FAILED(openPrefFile(lockPrefFile, PR_FALSE, PR_TRUE, 
+                                  PR_FALSE, PR_TRUE)))
+         return NS_ERROR_FAILURE;
+       // failure here means problem within the config file script
+     }
+   }
Rather confusing. Is it OK for lockPrefFile->Append(lockFileName) to fail?
A comment would be good.

+     nsCOMPtr<nsIAutoConfig> autocfg;
+     autocfg = do_CreateInstance(NS_AUTOCONFIG_CONTRACTID,&rv);

Collapse onto one line.

Fix these minor quibbles, and sr=sfraser

Comment 21

[Brian Nesse (gone)]

Hmm, I thought I had done a -u... guess not.

>+    if (rv == NS_ERROR_UNEXPECTED)
>+       rv = NS_OK;
>+     return rv;

I thought about doing that... wasn't sure what the reaction would be :)... I'll
do it now.

> Is it OK for lockPrefFile->Append(lockFileName) to fail?

That is, actually, a very good question. Realistically Append() will always
return NS_OK unless the pathname has been badly formed (mac and windows anyway)
no validation checking is done. Functionally, I would expect that an error
condition at this state means something bad and should return failure. I will
change it to do so. 

Comment 22

[Brian Nesse (gone)]

Attachment: New patch -u addressing sfraser's comments

Comment 23

[Brian Nesse (gone)]

Fix checked in on trunk. Waiting to land on branch until after Netscape takes
control as per [e-mail].

Comment 24

[Mitesh Shah]

This bug causes the nsPrefService object to be created multiple times. Because
we request the nsPrefService through nsAutoConfig->Init() and this happens
during nsPrefService->Init()->ReadConfig(). Since the nsPrefService has not
finished registering itself with the component manager, it creates the duplicate
nsPrefService obeject which will again call nsAutoConfig->Init(). So the browser
keeps creating both objects until the process runs out of memory. 

In fact, we don't need to request nsPrefService during nsAutoConfig->Init() so I
am moving the request of nsPrefService at appropriate time. The new patch is
attached. 

Comment 25

[Mitesh Shah]

Attachment: Fix for the regression

Comment 26

[Brian Nesse (gone)]

Comment on attachment 48471 [details] [diff] [review]
Fix for the regression

<sigh>. Good catch mitesh. r=bnesse.

Comment 27

[Alec Flett]

Comment on attachment 48471 [details] [diff] [review]
Fix for the regression

ok, after a few go-rounds to determine that mPrefBranch is 
not going to be null, sr=alecf

Comment 28

[Mitesh Shah]

Fix checked in to the trunk. 

Checking in nsAutoConfig.cpp;
/cvsroot/mozilla/modules/libpref/src/nsAutoConfig.cpp,v  <--  nsAutoConfig.cpp
new revision: 3.14; previous revision: 3.13

Comment 29

[Mitesh Shah]

Requesting PDT approval for branch check in

Comment 30

[Brian Nesse (gone)]

Marking PDT+ as per jpm & hong for grega.

Comment 31

[Brian Nesse (gone)]

... except that I can't add any tabs to my sidebar. The OK button in the sidebar
window doesn't seem to work.

Comment 32

[Brian Nesse (gone)]

Arrgh... wrong bug. sorry for the spam.

Comment 33

[Rodney Velasco]

talked to brian, this was checked in at 11:47 a.m., marking fixed and changing
QA contact to me.

Comment 34

[Rodney Velasco]

great news for factory tool, this works. verified on branch
Win2K: 20010927
Linux: 20010927
MacOS 9.1: 20010926
MacOS X: 20010927