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

VERIFIED FIXED in mozilla0.9.5

Status

()

Core
Preferences: Backend
--
blocker
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Trix Supremo, Assigned: Brian Nesse (gone))

Tracking

Trunk
mozilla0.9.5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PDT+, URL)

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
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.

Updated

17 years ago
Keywords: nsenterprise
QA Contact: sairuh → trix

Comment 1

17 years ago
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

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

Comment 3

17 years ago
Changing severity to blocker -- this must be fixed for Netscape6.Fall RTM.
Severity: normal → blocker

Comment 4

17 years ago
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

17 years ago
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)

Updated

17 years ago
Keywords: nsenterprise → nsenterprise+
(Assignee)

Comment 6

17 years ago
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);
(Assignee)

Comment 7

17 years ago
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.
(Assignee)

Comment 8

17 years ago
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.
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 10

17 years ago
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.
(Assignee)

Comment 11

17 years ago
Created attachment 45834 [details] [diff] [review]
Patch to make reading of config file internal to preferences.
(Assignee)

Comment 12

17 years ago
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.
(Assignee)

Updated

17 years ago
OS: Windows 2000 → All
Whiteboard: [patch]
Target Milestone: --- → mozilla0.9.4

Comment 13

17 years ago
r=chipc
+  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.
(Assignee)

Comment 15

17 years ago
> 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

17 years ago
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.
(Assignee)

Comment 17

17 years ago
Created attachment 47068 [details] [diff] [review]
New patch addressing sr comments
(Assignee)

Comment 18

17 years ago
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

17 years ago
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. 

Updated

17 years ago
Blocks: 96897

Updated

17 years ago
Blocks: 97182

Comment 20

17 years ago
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
Attachment #47068 - Flags: superreview+
(Assignee)

Comment 21

17 years ago
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. 
(Assignee)

Comment 22

17 years ago
Created attachment 47856 [details] [diff] [review]
New patch -u addressing sfraser's comments
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Comment 23

17 years ago
Fix checked in on trunk. Waiting to land on branch until after Netscape takes
control as per drivers@mozilla.org.

Comment 24

17 years ago
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

17 years ago
Created attachment 48471 [details] [diff] [review]
Fix for the regression
(Assignee)

Comment 26

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

<sigh>. Good catch mitesh. r=bnesse.
Attachment #48471 - Flags: review+

Comment 27

17 years ago
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
Attachment #48471 - Flags: superreview+

Comment 28

17 years ago
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

17 years ago
Requesting PDT approval for branch check in
Whiteboard: [patch] → Requesting PDT approval
(Assignee)

Updated

17 years ago
Keywords: nsbranch
(Assignee)

Comment 30

17 years ago
Marking PDT+ as per jpm & hong for grega.
Keywords: nsbranch → nsbranch+
Whiteboard: Requesting PDT approval → PDT+
(Assignee)

Comment 31

17 years ago
... except that I can't add any tabs to my sidebar. The OK button in the sidebar
window doesn't seem to work.
(Assignee)

Comment 32

17 years ago
Arrgh... wrong bug. sorry for the spam.

Comment 33

17 years ago
talked to brian, this was checked in at 11:47 a.m., marking fixed and changing
QA contact to me.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
QA Contact: trix → rvelasco
Resolution: --- → FIXED

Comment 34

17 years ago
great news for factory tool, this works. verified on branch
Win2K: 20010927
Linux: 20010927
MacOS 9.1: 20010926
MacOS X: 20010927
Status: RESOLVED → VERIFIED
Keywords: vtrunk
You need to log in before you can comment on or make changes to this bug.