5132 - Commercial build must read netscape.cfg on startup

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect, P1)

Description

[mcmullen]

The prefs bootstrapping is not in place. This code needs to be written for
mozilla. Should the path to these prefs be done with chrome URLs? It seems likely
that they should, if this is the way we internationalize them. I need help on
finding documentation for this stuff, so I'm cc-ing hyatt and ftang.

Comment 1

[mcmullen]

platform=all

Comment 2

[mcmullen]

Changing plumbing bugs to M5 milestone, and accepting them.

Comment 3

[don]

Changed target milestone to M6.

Comment 4

[mcmullen]

*** Bug 5598 has been marked as a duplicate of this bug. ***

Comment 5

[mcmullen]

Fixed on Macintosh.

Comment 6

[mcmullen]

Changing to P1 the priority of bugs upon whose solution others are waiting.

Comment 7

[mcmullen]

Moving this to M7. I think this needs to be redone in the light of the to-be-
written new prefs architecture.

Also, there are still javascript parsing bugs in my way of getting this right,
and the windows prefs stuff is all over the floor.

Comment 8

[mcmullen]

*** Bug 5947 has been marked as a duplicate of this bug. ***

Comment 9

[mcmullen]

Please look at bug #5947 (link above) for Mac-specific information).

Comment 10

[selmer (gone)]

*** Bug 5139 has been marked as a duplicate of this bug. ***

Comment 11

[mcmullen]

I'm doing this as follows: The build will be changed so that the config files
will be exported to the components directory.

Next, the prefs manager will do this at startup:

1. Read initpref.js and all.js
2. Read any other .js files in the components directory.
3. Read the platform-specific files (the names will be hard-coded) such as
winpref.js, unixpref.js, and macprefs.js).

Comment 12

[David Hyatt]

You don't want to use different file names.  The chrome platform URLs are
designed so that you can have a single way of referring to a platform-specific
object... e.g., chrome://global/platform/platformPrefs.js could be used and it
would automatically point to either a Windows, Mac, or GTK file, etc. etc.

Preferences files should be retrieved using chrome URLs.

Comment 13

[David Hyatt]

Of course there may be ordering issues here... I don't know how early prefs are
read in.  As long as you know which profile you're using by the time you need
to read in the prefs files, then chrome URLs should function just fine.

Comment 14

[mcmullen]

Well actually, you don't. The default prefs are read in before the profile is
picked.

Comment 15

[mcmullen]

I checked in the fix as advertised above. Moving the netscape.cfg case to M9.

Comment 16

[mcmullen]

Changing summary from "Apprunner must read netscape.cfg on startup"

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Bulk reassigning [e-mail] bugs to [e-mail].

Comment 18

[don]

dp, can your folks handle this issue?

Comment 19

[Suresh Duddi (gone)]

Is this us or profile folks ?

Comment 20

[selmer (gone)]

DP, this bug is about having the pref backend recognize the netscape.cfg file
(and any other parts of the old prefs hierarchy that we want to bring forward.)

Profiles only determines where profile related stuff comes from.  Prefs makes it
happen.  In addition, the netscape.cfg file doesn't live in any profile - it's
application level.

Comment 21

[neeti]

moving target milestone to M11.

Comment 22

[neeti]

moving target milestone to M11.

Comment 23

[neeti]

moving to M15

Comment 24

[leger]

Moving all libPref component bugs to new Preferences: Backend component.  
libPref component will be deleted.

Comment 25

[Paul MacQuiddy]

spam: moving qa contact on some bugs from paulmac to [e-mail]

Comment 26

[neeti]

This feature is already checked in, but is currently disabled because, I could 
not check in the ns\modules\libpref\src\init\netscape.cfg file due to CVS 
problems. I have been trying to check in the file since 05/12 with Leaf's help, 
but the CVS problem was resolved only on 05/17 after the tree is closed. To 
enable the feature I will need to check in the makefile's in the commercial 
tree, and uncomment the line of which calls this feature. Also, I will need to 
update the package files. I have these changes ready to go in.If the feature is 
enabled, it would work for optimized windows and linux builds. I have put the 
feature only for optimized builds, since we do not have the psm.exe available in 
the seamonkey tree. Also, it is currently disabled for the Mac, because we 
currently do not have a psm.exe available for the Mac.

Neeti

Neeti

Comment 27

[leger]

Putting on [nsbeta2+][6/01] radar.  This work must be done by 06/01 or we may 
pull this for PR2.

Comment 28

[neeti]

Checked in the fix to enable this feature optimized windows and linux builds.

Neeti

Comment 29

[neeti]

Had to turn off this feature, because libpref is not able to find psm the the 
first time the browser runs on optimized builds( for cases where the 
component.reg has been deleted). It runs fine the next time. We can reproduce 
the problem if we delete the component.reg and start the browser. Works fine on 
debug builds. See bug 40581 for details.

Neeti

Comment 30

[neeti]

Attachment: Patch to turn off this feature

Comment 31

[neeti]

Reassigning libpref bug to mcafee

Comment 32

[don]

Move to M17 ...

Comment 33

[leger]

Due to slip in schedule, moving this bug from [6/01] to [Will be minus on 6/15] 
for fix deadline.

Comment 34

[don]

Move to M19 since this bug has a time limit ...

Comment 35

[Chris McAfee]

is this a netscape-only feature?  Why would mozilla need netscape.cfg?

Comment 36

[Chris McAfee]

varada says mozilla-side will have a generic.cfg file.
Can someone attach a sample .cfg file for testing?

Comment 37

[Chris McAfee]

marking netscape-only.

sample netscape.cfg is checked in, ns/modules/libpref/src/init/netscape.cfg

two functions to look at here are nsPref::useLockPrefFile(), 
pref_VerifyLockFile().

Feature only works in debug, it is currently commented out as a workaround.
Optimized bits: browser hangs.?  Neeti says we're blocked on another bug
but we're not sure what that is yet.

Comment 38

[Jim Roskind]

6/15 has passed... so I'm cleaning whiteboard by marking this beta2 minus.
This does not appear to be a "user visible" feature.  If I understand the bug, 
we are trying to set up a "bootstrap prefs" file to make it easy to configure.
If this is crucial, please nominate for beta3, or renominate for beta2 by 
deleting the beta2 minus.

Thanks,
Jim

Comment 39

[sairuh (rarely reading bugmail)]

added nsonly kw since this is netscape [commercial] only.

chris, should there be another bug filed for the mozilla case, ie, having a
generic.cfg?

Comment 40

[Claudius Gayle]

over to selmer

Comment 41

[Daniel Veditz [:dveditz]]

plussing based on a meeting with bijals and selmer. Steve is the wrong owner, 
though.

Comment 42

[selmer (gone)]

Neeti, can you take this bug?  You did the major work on this back in May.

Comment 43

[Gagan]

Steve: can we find another owner for this bug? neeti is very doomed with the
cache. Per Neeti, most of the work is done and works fine on debug builds and
someone just needs to investigate why it doesn't work in the release builds. You
can seek more details from her, but she won't be able to get to this bug.
Thanks!

Comment 44

[bijals (gone)]

Assigning this to Dan and removing "+" so that PDT reviews and understands the 
importance of this bug.

This bug has significant impact on CCK.  One of the most important 
customizations in CCK is the user agent string.  We not only tag the user agent 
string with a "-CCK" whenever someone uses CCK, but we also allow distributors 
to tag their bits.  

Without this tagging, we lose the ability to track our distributors.  Because of 
tracking, we know today that CCK brings in millions dollars in revenue.  This 
tagging is required by the Browser Distribution team to track their various 
distribution deals.

Comment 45

[Daniel Veditz [:dveditz]]

Readding plus while I investigate. Bijal: you need to remove the entire nsbeta3 
part, not just the plus.

Comment 46

[bijals (gone)]

Removed [nsbeta3+] from status whiteboard.

Comment 47

[Daniel Veditz [:dveditz]]

Sorry, am just swamped and will not be able to do this.

Comment 48

[Dan Mosedale (:dmosedale, :dmose)]

I know of at least one non-netscape contributor who is very interested in having 
this functionality working, and could perhaps even be talked into contributing 
code (no promises, of course :-).  If noone objects, I'd like to remove the 
"Netscape Confidential" bit from this bug.  Thoughts?

Comment 49

[selmer (gone)]

I don't see any secret stuff in this bug.  The netscape.cfg algorithm isn't
supposed to rely on secrecy anyway.  I'd want Bij and Dan to agree though.

Comment 50

[bijals (gone)]

Dan V. and I agreed that we would not get to this feature for Seamonkey 
timeframe.

Comment 51

[Daniel Veditz [:dveditz]]

I agree that this bug should not be secret, especially now that NSS is 
completely open. (I think the original instigation of secrecy was the fact that 
we called into the then non-public crypto libraries).

The code that does exist is pretty general, and uses the "vendor" name to form 
<vendor>.cfg, it's not Netscape specific and appears to all be in the Mozilla 
tree.

We seem to be in agreement, I've flipped the privacy bit

Comment 52

[Oliver Fels]

Dan is propably referring to us, so I´ll add my 2 cents.
We might be able to contribute with implementational help,
as we need a specific feature in the prefs bootstrap.
We require a hook for a module which lets us fetch (security related)
prefs from a third party server at startup, so we could also do some additional
things.
According to Dans proposal, I am setting myself on the CC list.

Comment 53

[Andreas Otte]

Is there a sample netscape.cfg (or any other vendor.cfg) around that can be used
to enable the feature and get to the observed hang/crash?

Comment 54

[selmer (gone)]

Varada, can you help Andreas with that?

Comment 55

[Andreas Otte]

Anyone with a netscape.cfg ... ?

Comment 56

[varada]

Attachment: Netscape.cfg file

Comment 57

[varada]

I have attached a netscape.cfg file to the bug. 

Comment 58

[Daniel Veditz [:dveditz]]

You can also get a netscape.cfg from www.ufaq.org, specifically from 
http://www.ufaq.org/downloads.html

Comment 59

[Daniel Veditz [:dveditz]]

reassigning to chipc

Comment 60

[Alec Flett]

adding myself to the CC - I've reviewed the latest patches from chip, and they
look ok.
chip, please attach the patches to the bug

Comment 61

[Chip Clark]

Attachment: diffs for MANIFEST_CFG and all-ns.js

Comment 62

[Chip Clark]

Attachment: diffs for all.js nspref.cpp nsipref.idl nsprofile.cpp

Comment 63

[Chip Clark]

Attachment: new cfg file

Comment 64

[Chip Clark]

Attachment: new cfg for Mozilla

Comment 65

[Chip Clark]

Attachment: MANIFEST_CFG for new mozilla.cfg file

Comment 66

[Chip Clark]

The new files (02/20/01) are as follows:

netscp6.cfg (ns/modules/libpref/src/init)
   This is the netscape.cfg renamed to follow the new naming convention
mozilla.cfg (mozilla/modules/libpref/src/init)
   This is the cfg for mozilla
Manifest_cfg (mozilla/modules/libpref/src/init)

The diff's should change the following files:
 ns/modules/libpref/src/init/all-ns.js
 ns/modules/libpref/src/init/Manifest_cfg
 mozilla/modules/libpref/src/init/all.js
 mozilla/modules/libpref/src/nsPref.cpp
 mozilla/modules/libpref/public/nsIPref.idl
 mozilla/profile/src/nsProfile.cpp
  
 
The .js files comment out the need for the lock file (OFF BY DEFAULT)
So, if you're building with these patchs... there should be no difference in 
functionality.  To turn the use of the cfg on...you need to edit the
all.js or all-ns.js file depending on whether you are using mozilla or netscp6

PSM is necessary to read the hashed file (at least at this point).  
 So, if you're testing cfg functionality you must build with PSM.

Comment 67

[Brian Nesse (gone)]

The current patches look good and work as advertised. r=bnesse

Comment 68

[Daniel Veditz [:dveditz]]

Since the only thing used in PSM is the MD5 hash (IIRC) couldn't we bring a 
copy of that out into some other place in the product? Firing off PSM at 
startup is not going to help our performance any. Adding perf keyword so we can 
track the impact of this.

Comment 69

[Brian Nesse (gone)]

We're trying to figure out if we can eliminate it completely... no answer on that 
yet. For the time being there will be no impact as long as it stays turned off.

Comment 70

[selmer (gone)]

After PSM 2.0, will we really be "firing up PSM" for this?

We explored getting just the md5 lib out of PSM and it was deemed feasible.  We
never got it, so maybe it was harder than advertised.  With PSM back in-process,
this should be trivial now...

Comment 71

[Alec Flett]

re: PSM2 - yes, if PSM2 is our only source of an MD5 implementation, then we'll
be loading the PSM2 dlls at startup, which still has noticable cost. I still
think that we need a small MD5 implementation in an existing library, to reduce
footprint, even if it exists in libpref itself. an md5 hash is a very small
routine (2-5k compiled) and I think it would be worth avoiding PSM to have a
duplicate MD5 hash sitting around somewhere, already loaded.

Comment 72

[Alec Flett]

ok, sr=alecf on the condition that
1) the standard mozilla builds, without PSM continue to run
2) the mozilla builds WITH PSM also run i.e. dont' require "netscape.cfg" or
anything

Comment 73

[Chip Clark]

This fix does not require the cfg for either mozilla or netscp6 with or without 
PSM.   However, if the associated all*.js file for each application is modified 
(removing the comments infront of 
  pref("general.config.filename"...
  pref("general.config.vendor"... )
Then PSM at startup is necessary for both mozilla AND netscp6.

Comment 74

[Brian Nesse (gone)]

setting milestone to 0.8.1

Comment 75

[Chip Clark]

The changes here are for the following reasons:

  1 - PSM is no longer required for the cfg
  2 - some variable name changes were made for consistency
  3 - the function getLockPrefFileInfo() - was removed (no longer used)
  4 - general.config.vendor is not checked until after the cfg if read
     This way it is possible to lock this pref and force the file name
  5 - pref_Alert(return_error); were replaced with 
      #ifdef NS_DEBUG
                      printf(return_error);
      #endif
      to avoid any use of GUI (as GUI is not initialized yet)


New diff for nsPref.cpp:

Index: mozilla/modules/libpref/src/nsPref.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/libpref/src/nsPref.cpp,v
retrieving revision 3.143
diff -u -r3.143 nsPref.cpp
--- mozilla/modules/libpref/src/nsPref.cpp	2001/03/20 14:34:53	3.143
+++ mozilla/modules/libpref/src/nsPref.cpp	2001/03/23 13:13:12
@@ -134,7 +134,6 @@
     nsresult useDefaultPrefFile();
     nsresult useUserPrefFile();
     nsresult useLockPrefFile();
-    nsresult getLockPrefFileInfo();
 
     nsresult unregisterObservers();
     
@@ -326,50 +325,6 @@
 } // nsPref::useUserPrefFile
 
 
//------------------------------------------------------------------------------
----------
-nsresult nsPref::getLockPrefFileInfo()
-//-----------------------------------------------------------------------------
-----------
-{
-    nsresult rv = NS_OK;
- 
-    gLockFileName = nsnull;
-    gLockVendor = nsnull;
- 
-    if (gLockInfoRead)
-        return rv;
-    
-    gLockInfoRead = PR_TRUE;
-
-    if (NS_SUCCEEDED(rv = CopyCharPref("general.config.filename",
-			&gLockFileName)) && (gLockFileName)) 
-    {
-#ifdef NS_DEBUG
-        printf("\ngLockFileName %s \n", gLockFileName);
-#endif
-        if (NS_SUCCEEDED(rv = CopyCharPref("general.config.vendor",
-			&gLockVendor)) && (gLockVendor)) 
-        {
-#ifdef NS_DEBUG
-            printf("\ngLockVendor %s \n", gLockVendor);
-#endif
-        }
-        else
-        {
-            /* No vendor name specified, hence cannot verify it */
-            rv = NS_ERROR_FAILURE;
-            return rv;
-        }
-    }
-    else
-    {
-        /* config file is not specified, hence no need to read it.*/
-        rv =  NS_OK;
-        return rv;
-    }
-    return rv;
-} // nsPref::getLockPrefFileInfo
-
-
-//-----------------------------------------------------------------------------
-----------
 nsresult nsPref::useLockPrefFile()
 
//------------------------------------------------------------------------------
----------
 {
@@ -400,122 +355,54 @@
 #endif
     }
 
-    if (NS_SUCCEEDED(rv = CopyCharPref("general.config.vendor",
-			getter_Copies(lockVendor)) && (lockVendor))) 
-    {
-#ifdef NS_DEBUG
-        printf("\nlockVendor %s \n", (const char *)lockVendor);
-#endif
-    }
-
-    if ((gLockFileName == nsnull) && (gLockVendor == nsnull))
-    {
-        if ((lockFileName == nsnull) && (lockVendor == nsnull)) 
-        {
-            return NS_OK;
-        }
-        if ((lockFileName == nsnull) || (lockVendor == nsnull))
-        {
-            return_error = PL_strdup("The required configuration filename or 
vendor name is incorrect.Please check your preferences.");
-            if (return_error) 
-            {
-                pref_Alert(return_error);
-                return NS_ERROR_FAILURE;
-            }
-        }
-
-        fileNameLen = PL_strlen(lockFileName);
-        vendorLen = PL_strlen(lockVendor);
-        if (PL_strncmp(lockFileName, lockVendor, fileNameLen -4) != 0)
-        {
-            return_error = PL_strdup("The required configuration filename and 
vendor name do not match.Please check your preferences.");
-            if (return_error) 
-            {
-                pref_Alert(return_error);
-                PR_FREEIF(return_error);
-                return NS_ERROR_FAILURE;
-            }            
-        }
-        else
-        {
-            configFile = nsXPIDLCString::Copy(lockFileName);
-            if(configFile == nsnull)
-                return NS_ERROR_FAILURE;
-        }
-    }
-    else if ((gLockFileName == nsnull) || (gLockVendor == nsnull))
-    {
-        return_error = PL_strdup("The required configuration filename or vendor 
name is incorrect.Please contact your administrator.");
-        if (return_error) 
-        {
-            pref_Alert(return_error);
-            PR_FREEIF(return_error);
-            return NS_ERROR_FAILURE;
-        }      
-    }
-    else
-    {
-        fileNameLen = PL_strlen(gLockFileName);
-        vendorLen = PL_strlen(gLockVendor);
-        if (PL_strncmp(gLockFileName, gLockVendor, fileNameLen -4) != 0)
-        {
-            return_error = PL_strdup("The required configuration filename and 
vendor name do not match.Please contact your administrator.");
-            if (return_error) 
-            {
-                pref_Alert(return_error);
-                PR_FREEIF(return_error);
-                return NS_ERROR_FAILURE;
-            }            
-        }
-        else
-        {
+	if (lockFileName == nsnull) 
+	{
+		return NS_OK;
+	}
+	else 
+	{
             configFile = nsXPIDLCString::Copy(lockFileName);
             if(configFile == nsnull)
                 return NS_ERROR_FAILURE;
-        }
     }
-
     
-    {
-        nsCOMPtr<nsIFile> aFile; 
-        rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, 
getter_AddRefs(aFile));
+    nsCOMPtr<nsIFile> aFile; 
+    rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, 
getter_AddRefs(aFile));
         
 #ifdef XP_MAC
         aFile->Append("Essential Files");
 #endif
         // TODO: Make the rest of this code use nsIFile and
         // avoid this conversion
-        rv = _nsIFileToFileSpec(aFile, getter_AddRefs(lockPrefFile));
+    rv = _nsIFileToFileSpec(aFile, getter_AddRefs(lockPrefFile));
         
-        if (NS_SUCCEEDED(rv) && lockPrefFile) 
+    if (NS_SUCCEEDED(rv) && lockPrefFile) 
+    {
+        if (NS_SUCCEEDED(lockPrefFile->AppendRelativeUnixPath(configFile)))
         {
-            if (NS_SUCCEEDED(lockPrefFile->AppendRelativeUnixPath(configFile)))
-            {
-                if (NS_FAILED(StartUp()))
-    	            return NS_ERROR_FAILURE;
+            if (NS_FAILED(StartUp()))
+	            return NS_ERROR_FAILURE;
 
-	            JS_BeginRequest(gMochaContext);
-                result = pref_OpenFileSpec(lockPrefFile, PR_TRUE, PR_TRUE, 
PR_FALSE, PR_FALSE);
-                if (result != PREF_NOERROR)
+            JS_BeginRequest(gMochaContext);
+            result = pref_OpenFileSpec(lockPrefFile, PR_TRUE, PR_TRUE, 
PR_FALSE, PR_FALSE);
+            if (result != PREF_NOERROR)
+            {
+                rv = NS_ERROR_FAILURE;
+                if (result != PREF_BAD_LOCKFILE)
                 {
-                    rv = NS_ERROR_FAILURE;
-                    if (result != PREF_BAD_LOCKFILE)
+					return_error = PL_strcpy(PL_strdup("The 
required configuration file "), lockFileName);
+					return_error = PL_strcpy(return_error, 
PL_strdup(" could not be found. Please reinstall the software or contact your 
administrator."));
+                    if (return_error) 
                     {
-                        return_error = PL_strdup("The required configuration 
file netscape.cfg could not be found. Please reinstall the software or contact 
your administrator.");
-                        if (return_error) 
-                        {
-                            pref_Alert(return_error);
-                            PR_FREEIF(return_error);
-                        }
+#ifdef NS_DEBUG
+						printf(return_error);
+#endif
+                        PR_FREEIF(return_error);
                     }
                 }
-                JS_EndRequest(gMochaContext);
             }
+            JS_EndRequest(gMochaContext);
         }
-#ifdef DEBUG_tao
-    GetLocalizedUnicharPref("browser.startup.homepage",getter_Copies(prefVal));
-    printf("\nStartup homepage %s \n", NS_ConvertUCS2toUTF8(prefVal).get());
-#endif
     }
     return rv;
 } // nsPref::useLockPrefFile
@@ -571,24 +458,28 @@
 {
     nsresult rv = StartUp(); // just to be sure
     if (NS_SUCCEEDED(rv)) {
-      rv = getLockPrefFileInfo();
 	  rv = useDefaultPrefFile();  // really should return a value...
     }
     if (NS_SUCCEEDED(rv))
 		  useUserPrefFile(); 
-/*
-#ifndef NS_DEBUG
-#ifndef XP_MAC
-    rv = useLockPrefFile();
-#endif
-#endif
-*/
 
     JS_MaybeGC(gMochaContext);
     return rv;
 }
 
 
//------------------------------------------------------------------------------
----------
+NS_IMETHODIMP nsPref::ReadCFGFile()
+//-----------------------------------------------------------------------------
-----------
+{
+	nsresult rv;
+//    if (NS_SUCCEEDED(rv)) {
+//      rv = getLockPrefFileInfo();
+//    }
+    rv = useLockPrefFile();
+	return rv;
+}
+
+//-----------------------------------------------------------------------------
-----------
 NS_IMETHODIMP nsPref::ReadUserPrefsFrom(nsIFileSpec* inFile)
 
//------------------------------------------------------------------------------
----------
 {
@@ -970,7 +861,7 @@
     printf("\n --> nsPref::GetLocalizedUnicharPref(%s) --", pref);
 #endif
     // if the user has set this pref, then just return the user value
-    if (PREF_HasUserPref(pref))
+    if (PREF_HasUserPref(pref) || PREF_PrefIsLocked(pref))
         return CopyUnicharPref(pref, return_buf);
 
     // user has not set the pref, so the default value
@@ -1510,8 +1401,10 @@
             return_error = PL_strdup("An error occurred reading the startup 
configuration file. Please contact your administrator.");
             if (return_error) 
             {
-                pref_Alert(return_error);
-                PR_Free(return_error);
+#ifdef NS_DEBUG
+						printf(return_error);
+#endif
+                        PR_FREEIF(return_error);
             }
             PR_Free(readBuf);
             return result;
@@ -1550,27 +1443,25 @@
 //   // xx xx xx xx xx xx xx xx xx xx xx xx xx xx xx xx
 //   where each 'xx' is a hex value. */
 
//------------------------------------------------------------------------------
----------
-PRBool pref_VerifyLockFileSpec(char* buf, long buflen)
+PRBool pref_VerifyLockFileSpec(char* readBuf, long fileLength)
 
//------------------------------------------------------------------------------
----------
 {
 
     PRBool success = PR_FALSE;
 	const int obscure_value = 7;
-	const long hash_length = 51;		/* len = 48 chars of MD5 + // + 
EOL */
-    unsigned char *digest;
-    char szHash[64];
+	//const long hash_length = 51;		/* len = 48 chars of MD5 + // + 
EOL */
+    //unsigned char *digest;
+    //char szHash[64];
    
 	/* Unobscure file by subtracting some value from every char. */
 	long i;
-	for (i = 0; i < buflen; i++)
-		buf[i] -= obscure_value;
+	for (i = 0; i < fileLength; i++)
+		readBuf[i] -= obscure_value;
 
-    if (buflen >= hash_length)
+/*    if (fileLength >= hash_length)
     {
-        const unsigned char magic_key[] = "VonGloda5652TX75235ISBN";
-   	    unsigned char *pStart = (unsigned char*) buf + hash_length;
-	    unsigned int len;
-       
+   	    unsigned char *pStart = (unsigned char*) readBuf + hash_length;
+
         nsresult rv;
 
         NS_WITH_SERVICE(nsISignatureVerifier, verifier, 
SIGNATURE_VERIFIER_CONTRACTID, &rv);
@@ -1581,15 +1472,17 @@
         rv = verifier->HashBegin(nsISignatureVerifier::MD5, &id);
         if (NS_FAILED(rv)) return success;
 
+        const unsigned char magic_key[] = "VonGloda5652TX75235ISBN";
         rv = verifier->HashUpdate(id, (const char*)magic_key, 
sizeof(magic_key));
         if (NS_FAILED(rv)) return success;
 
-        rv = verifier->HashUpdate(id, (const char*)pStart, (unsigned 
int)(buflen - hash_length));
+        rv = verifier->HashUpdate(id, (const char*)pStart, (unsigned 
int)(fileLength - hash_length));
         if (NS_FAILED(rv)) return success;
   
         digest = (unsigned char*)PR_MALLOC(nsISignatureVerifier::MD5_LENGTH);
         if (digest == nsnull) return success;
 
+	    unsigned int len;
         rv = verifier->HashEnd(id,&digest, &len, 
nsISignatureVerifier::MD5_LENGTH);
         if (NS_FAILED(rv)) { PR_FREEIF(digest); return success; }
             
@@ -1601,20 +1494,14 @@
 	        (int)digest[8],(int)digest[9],(int)digest[10],(int)digest[11],
 	        
(int)digest[12],(int)digest[13],(int)digest[14],(int)digest[15]);
 
-#ifdef DEBUG_neeti
-        printf("%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x 
%02x %02x %02x %02x",
-	        (int)digest[0],(int)digest[1],(int)digest[2],(int)digest[3],
-	        (int)digest[4],(int)digest[5],(int)digest[6],(int)digest[7],
-	        (int)digest[8],(int)digest[9],(int)digest[10],(int)digest[11],
-	        
(int)digest[12],(int)digest[13],(int)digest[14],(int)digest[15]); 
-#endif
-
    
-		success = ( PL_strncmp((const char*) buf + 3, szHash, 
(PRUint32)(hash_length - 4)) == 0 );
+		success = ( PL_strncmp((const char*) readBuf + 3, szHash, 
(PRUint32)(hash_length - 4)) == 0 );
 
         PR_FREEIF(digest);
  
-	}
+	}*/
+	if(readBuf)
+		success = PR_SUCCESS;
     return success;
 
 }
Index: mozilla/modules/libpref/src/nsPref.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/libpref/src/nsPref.cpp,v
retrieving revision 3.143
diff -u -r3.143 nsPref.cpp
--- mozilla/modules/libpref/src/nsPref.cpp	2001/03/20 14:34:53	3.143
+++ mozilla/modules/libpref/src/nsPref.cpp	2001/03/23 13:26:53
@@ -134,7 +134,6 @@
     nsresult useDefaultPrefFile();
     nsresult useUserPrefFile();
     nsresult useLockPrefFile();
-    nsresult getLockPrefFileInfo();
 
     nsresult unregisterObservers();
     
@@ -326,50 +325,6 @@
 } // nsPref::useUserPrefFile
 
 
//------------------------------------------------------------------------------
----------
-nsresult nsPref::getLockPrefFileInfo()
-//-----------------------------------------------------------------------------
-----------
-{
-    nsresult rv = NS_OK;
- 
-    gLockFileName = nsnull;
-    gLockVendor = nsnull;
- 
-    if (gLockInfoRead)
-        return rv;
-    
-    gLockInfoRead = PR_TRUE;
-
-    if (NS_SUCCEEDED(rv = CopyCharPref("general.config.filename",
-			&gLockFileName)) && (gLockFileName)) 
-    {
-#ifdef NS_DEBUG
-        printf("\ngLockFileName %s \n", gLockFileName);
-#endif
-        if (NS_SUCCEEDED(rv = CopyCharPref("general.config.vendor",
-			&gLockVendor)) && (gLockVendor)) 
-        {
-#ifdef NS_DEBUG
-            printf("\ngLockVendor %s \n", gLockVendor);
-#endif
-        }
-        else
-        {
-            /* No vendor name specified, hence cannot verify it */
-            rv = NS_ERROR_FAILURE;
-            return rv;
-        }
-    }
-    else
-    {
-        /* config file is not specified, hence no need to read it.*/
-        rv =  NS_OK;
-        return rv;
-    }
-    return rv;
-} // nsPref::getLockPrefFileInfo
-
-
-//-----------------------------------------------------------------------------
-----------
 nsresult nsPref::useLockPrefFile()
 
//------------------------------------------------------------------------------
----------
 {
@@ -400,122 +355,88 @@
 #endif
     }
 
-    if (NS_SUCCEEDED(rv = CopyCharPref("general.config.vendor",
-			getter_Copies(lockVendor)) && (lockVendor))) 
-    {
-#ifdef NS_DEBUG
-        printf("\nlockVendor %s \n", (const char *)lockVendor);
-#endif
-    }
-
-    if ((gLockFileName == nsnull) && (gLockVendor == nsnull))
-    {
-        if ((lockFileName == nsnull) && (lockVendor == nsnull)) 
-        {
-            return NS_OK;
-        }
-        if ((lockFileName == nsnull) || (lockVendor == nsnull))
-        {
-            return_error = PL_strdup("The required configuration filename or 
vendor name is incorrect.Please check your preferences.");
-            if (return_error) 
-            {
-                pref_Alert(return_error);
-                return NS_ERROR_FAILURE;
-            }
-        }
-
-        fileNameLen = PL_strlen(lockFileName);
-        vendorLen = PL_strlen(lockVendor);
-        if (PL_strncmp(lockFileName, lockVendor, fileNameLen -4) != 0)
-        {
-            return_error = PL_strdup("The required configuration filename and 
vendor name do not match.Please check your preferences.");
-            if (return_error) 
-            {
-                pref_Alert(return_error);
-                PR_FREEIF(return_error);
-                return NS_ERROR_FAILURE;
-            }            
-        }
-        else
-        {
-            configFile = nsXPIDLCString::Copy(lockFileName);
-            if(configFile == nsnull)
-                return NS_ERROR_FAILURE;
-        }
-    }
-    else if ((gLockFileName == nsnull) || (gLockVendor == nsnull))
-    {
-        return_error = PL_strdup("The required configuration filename or vendor 
name is incorrect.Please contact your administrator.");
-        if (return_error) 
-        {
-            pref_Alert(return_error);
-            PR_FREEIF(return_error);
-            return NS_ERROR_FAILURE;
-        }      
-    }
-    else
-    {
-        fileNameLen = PL_strlen(gLockFileName);
-        vendorLen = PL_strlen(gLockVendor);
-        if (PL_strncmp(gLockFileName, gLockVendor, fileNameLen -4) != 0)
-        {
-            return_error = PL_strdup("The required configuration filename and 
vendor name do not match.Please contact your administrator.");
-            if (return_error) 
-            {
-                pref_Alert(return_error);
-                PR_FREEIF(return_error);
-                return NS_ERROR_FAILURE;
-            }            
-        }
-        else
-        {
+	if (lockFileName == nsnull) 
+	{
+		return NS_OK;
+	}
+	else 
+	{
             configFile = nsXPIDLCString::Copy(lockFileName);
             if(configFile == nsnull)
                 return NS_ERROR_FAILURE;
-        }
     }
-
     
-    {
-        nsCOMPtr<nsIFile> aFile; 
-        rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, 
getter_AddRefs(aFile));
+    nsCOMPtr<nsIFile> aFile; 
+    rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, 
getter_AddRefs(aFile));
         
 #ifdef XP_MAC
         aFile->Append("Essential Files");
 #endif
         // TODO: Make the rest of this code use nsIFile and
         // avoid this conversion
-        rv = _nsIFileToFileSpec(aFile, getter_AddRefs(lockPrefFile));
+    rv = _nsIFileToFileSpec(aFile, getter_AddRefs(lockPrefFile));
         
-        if (NS_SUCCEEDED(rv) && lockPrefFile) 
+    if (NS_SUCCEEDED(rv) && lockPrefFile) 
+    {
+        if (NS_SUCCEEDED(lockPrefFile->AppendRelativeUnixPath(configFile)))
         {
-            if (NS_SUCCEEDED(lockPrefFile->AppendRelativeUnixPath(configFile)))
-            {
-                if (NS_FAILED(StartUp()))
-    	            return NS_ERROR_FAILURE;
+            if (NS_FAILED(StartUp()))
+	            return NS_ERROR_FAILURE;
 
-	            JS_BeginRequest(gMochaContext);
-                result = pref_OpenFileSpec(lockPrefFile, PR_TRUE, PR_TRUE, 
PR_FALSE, PR_FALSE);
-                if (result != PREF_NOERROR)
+            JS_BeginRequest(gMochaContext);
+            result = pref_OpenFileSpec(lockPrefFile, PR_TRUE, PR_TRUE, 
PR_FALSE, PR_FALSE);
+            if (result != PREF_NOERROR)
+            {
+                rv = NS_ERROR_FAILURE;
+                if (result != PREF_BAD_LOCKFILE)
                 {
-                    rv = NS_ERROR_FAILURE;
-                    if (result != PREF_BAD_LOCKFILE)
+					return_error = PL_strcpy(PL_strdup("The 
required configuration file "), lockFileName);
+					return_error = PL_strcpy(return_error, 
PL_strdup(" could not be found. Please reinstall the software or contact your 
administrator."));
+                    if (return_error) 
                     {
-                        return_error = PL_strdup("The required configuration 
file netscape.cfg could not be found. Please reinstall the software or contact 
your administrator.");
-                        if (return_error) 
-                        {
-                            pref_Alert(return_error);
-                            PR_FREEIF(return_error);
-                        }
+#ifdef NS_DEBUG
+						printf(return_error);
+#endif
+                        PR_FREEIF(return_error);
                     }
                 }
-                JS_EndRequest(gMochaContext);
             }
-        }
-#ifdef DEBUG_tao
-    GetLocalizedUnicharPref("browser.startup.homepage",getter_Copies(prefVal));
-    printf("\nStartup homepage %s \n", NS_ConvertUCS2toUTF8(prefVal).get());
+			if (NS_SUCCEEDED(rv = 
CopyCharPref("general.config.vendor",
+				getter_Copies(lockVendor)) && (lockVendor))) 
+			{
+#ifdef NS_DEBUG
+				 printf("\nlockVendor %s \n", (const char 
*)lockVendor);
+#endif
+			}
+			if (lockVendor == nsnull)
+			{
+				return_error = PL_strdup("The required vendor 
name is incorrect.Please check your preferences.");
+				if (return_error) 
+				{
+#ifdef NS_DEBUG
+				 printf(return_error);
+#endif
+                return NS_ERROR_FAILURE;
+				}
+			}
+
+			fileNameLen = PL_strlen(lockFileName);
+			vendorLen = PL_strlen(lockVendor);
+			if (PL_strncmp(lockFileName, lockVendor, fileNameLen -4) 
!= 0)
+			{
+				return_error = PL_strdup("The required 
configuration filename and vendor name do not match. Please contact your 
administrator.");
+				if (return_error) 
+				{
+#ifdef NS_DEBUG
+					 printf(return_error);
 #endif
+		            PR_FREEIF(return_error);
+				    return NS_ERROR_FAILURE;
+			    }            
+			}
+     
+            JS_EndRequest(gMochaContext);
+        }
     }
     return rv;
 } // nsPref::useLockPrefFile
@@ -571,24 +492,28 @@
 {
     nsresult rv = StartUp(); // just to be sure
     if (NS_SUCCEEDED(rv)) {
-      rv = getLockPrefFileInfo();
 	  rv = useDefaultPrefFile();  // really should return a value...
     }
     if (NS_SUCCEEDED(rv))
 		  useUserPrefFile(); 
-/*
-#ifndef NS_DEBUG
-#ifndef XP_MAC
-    rv = useLockPrefFile();
-#endif
-#endif
-*/
 
     JS_MaybeGC(gMochaContext);
     return rv;
 }
 
 
//------------------------------------------------------------------------------
----------
+NS_IMETHODIMP nsPref::ReadCFGFile()
+//-----------------------------------------------------------------------------
-----------
+{
+	nsresult rv;
+//    if (NS_SUCCEEDED(rv)) {
+//      rv = getLockPrefFileInfo();
+//    }
+    rv = useLockPrefFile();
+	return rv;
+}
+
+//-----------------------------------------------------------------------------
-----------
 NS_IMETHODIMP nsPref::ReadUserPrefsFrom(nsIFileSpec* inFile)
 
//------------------------------------------------------------------------------
----------
 {
@@ -970,7 +895,7 @@
     printf("\n --> nsPref::GetLocalizedUnicharPref(%s) --", pref);
 #endif
     // if the user has set this pref, then just return the user value
-    if (PREF_HasUserPref(pref))
+    if (PREF_HasUserPref(pref) || PREF_PrefIsLocked(pref))
         return CopyUnicharPref(pref, return_buf);
 
     // user has not set the pref, so the default value
@@ -1510,8 +1435,10 @@
             return_error = PL_strdup("An error occurred reading the startup 
configuration file. Please contact your administrator.");
             if (return_error) 
             {
-                pref_Alert(return_error);
-                PR_Free(return_error);
+#ifdef NS_DEBUG
+						printf(return_error);
+#endif
+                        PR_FREEIF(return_error);
             }
             PR_Free(readBuf);
             return result;
@@ -1550,27 +1477,25 @@
 //   // xx xx xx xx xx xx xx xx xx xx xx xx xx xx xx xx
 //   where each 'xx' is a hex value. */
 
//------------------------------------------------------------------------------
----------
-PRBool pref_VerifyLockFileSpec(char* buf, long buflen)
+PRBool pref_VerifyLockFileSpec(char* readBuf, long fileLength)
 
//------------------------------------------------------------------------------
----------
 {
 
     PRBool success = PR_FALSE;
 	const int obscure_value = 7;
-	const long hash_length = 51;		/* len = 48 chars of MD5 + // + 
EOL */
-    unsigned char *digest;
-    char szHash[64];
+	//const long hash_length = 51;		/* len = 48 chars of MD5 + // + 
EOL */
+    //unsigned char *digest;
+    //char szHash[64];
    
 	/* Unobscure file by subtracting some value from every char. */
 	long i;
-	for (i = 0; i < buflen; i++)
-		buf[i] -= obscure_value;
+	for (i = 0; i < fileLength; i++)
+		readBuf[i] -= obscure_value;
 
-    if (buflen >= hash_length)
+/*    if (fileLength >= hash_length)
     {
-        const unsigned char magic_key[] = "VonGloda5652TX75235ISBN";
-   	    unsigned char *pStart = (unsigned char*) buf + hash_length;
-	    unsigned int len;
-       
+   	    unsigned char *pStart = (unsigned char*) readBuf + hash_length;
+
         nsresult rv;
 
         NS_WITH_SERVICE(nsISignatureVerifier, verifier, 
SIGNATURE_VERIFIER_CONTRACTID, &rv);
@@ -1581,15 +1506,17 @@
         rv = verifier->HashBegin(nsISignatureVerifier::MD5, &id);
         if (NS_FAILED(rv)) return success;
 
+        const unsigned char magic_key[] = "VonGloda5652TX75235ISBN";
         rv = verifier->HashUpdate(id, (const char*)magic_key, 
sizeof(magic_key));
         if (NS_FAILED(rv)) return success;
 
-        rv = verifier->HashUpdate(id, (const char*)pStart, (unsigned 
int)(buflen - hash_length));
+        rv = verifier->HashUpdate(id, (const char*)pStart, (unsigned 
int)(fileLength - hash_length));
         if (NS_FAILED(rv)) return success;
   
         digest = (unsigned char*)PR_MALLOC(nsISignatureVerifier::MD5_LENGTH);
         if (digest == nsnull) return success;
 
+	    unsigned int len;
         rv = verifier->HashEnd(id,&digest, &len, 
nsISignatureVerifier::MD5_LENGTH);
         if (NS_FAILED(rv)) { PR_FREEIF(digest); return success; }
             
@@ -1601,20 +1528,14 @@
 	        (int)digest[8],(int)digest[9],(int)digest[10],(int)digest[11],
 	        
(int)digest[12],(int)digest[13],(int)digest[14],(int)digest[15]);
 
-#ifdef DEBUG_neeti
-        printf("%02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x 
%02x %02x %02x %02x",
-	        (int)digest[0],(int)digest[1],(int)digest[2],(int)digest[3],
-	        (int)digest[4],(int)digest[5],(int)digest[6],(int)digest[7],
-	        (int)digest[8],(int)digest[9],(int)digest[10],(int)digest[11],
-	        
(int)digest[12],(int)digest[13],(int)digest[14],(int)digest[15]); 
-#endif
-
    
-		success = ( PL_strncmp((const char*) buf + 3, szHash, 
(PRUint32)(hash_length - 4)) == 0 );
+		success = ( PL_strncmp((const char*) readBuf + 3, szHash, 
(PRUint32)(hash_length - 4)) == 0 );
 
         PR_FREEIF(digest);
  
-	}
+	}*/
+	if(readBuf)
+		success = PR_SUCCESS;
     return success;
 
 }



Awaiting review (and sr) for check-in  

Comment 76

[Daniel Veditz [:dveditz]]

Ay carumba! please use attachments for patches. Not only does it make the 
actual comments in the bug easier to read and print, it makes it much easier to 
download and test the patch, and saves re-review time by making it possible to 
diff against previously reviewed patches.

Comment 77

[Chip Clark]

Attachment: new diffs - cfg without PSM

Comment 78

[Chip Clark]

Attachment: changes to the makefiles to include new files

Comment 79

[Chip Clark]

Attachment: diff's for the ns makefiles

Comment 80

[Chip Clark]

To clarify the new attachments:
  03/23/01 15:36 - diffs for the mozilla tree
     nsPref.cpp, nsProfile.cpp, all.js, nsIPref.idl
  03/23/01 15:41 - diffs for the makefiles (mozilla tree)
  03/23/01 15:45 - diffs for the makefiels (ns tree)

old attachments still valid:
  02/20/01 15:23 - MANIFEST_CFG
  02/20/01 15:22 - mozilla.cfg
  02/20/01 15:21 - netscp6.cfg
  02/20/01 15:16 - diffs for MANIFEST_CFG and all-ns.js

Comment 81

[Brian Nesse (gone)]

Patch thoughts:
I would move the #ifdef NS_DEBUG to encompass the creation of return_error, not 
just the printf. No point in creating it if you're not displaying it. You appear 
to be leaking at least one instance of return_error (in the if (lockVendor == 
nsnull) check.)

Why have ReadCFGFile call useLockPrefFile. Can't you just move the remaining code 
from useLockPrefFile into ReadCFGFile?

Comment 82

[Chip Clark]

Thanks 
- made changes to #ifdef NS_DEBUG to include the creating of return_error.  
(diff on the way)

However... the reason for keeping useLockPrefFile separate is:
useLockPrefFile returns nsresult... which, although I am not using it for 
anything in ReadCFGFile at present - there might be uses for it in the future 
(error messages relating to problems with the cfg file) that can be handled in 
the prefs framework... and not in the profile initialization area (which is 
what calls ReadCFGFile).

Comment 83

[Chip Clark]

Attachment: diff adding return_error creation to #ifdef

Comment 84

[Alec Flett]

argh, trying to review this bug again, bugzilla keeps losing my comments on THIS
bug for some reason.
anyway,
- return_error contains non-localized strings, which makes me think that all
uses of it should be confined to NS_DEBUG... which means that the variable
declaration ITSELF should be inside a NS_DEBUG to ensure that it is never used
in release builds. Also, why are you randomly PL_strdup'ing static strings? It
makes no sense. why not just say return_error = "foo" instead of return_error =
PL_strdup("foo");
Also, I don't understand this line:
+ return_error = PL_strcpy(return_error, PL_strdup(" could not be found. Please
reinstall the software or contact your administrator."));

you're losing the value of the 2nd strdup - maybe you meant strcat? but if you
meant strcat, you would be writing over random memory. Why don't you just get
rid of return_error entirely, and just put #ifdef'ed printf()s in the places
where you get an error?
- similarly, this code makes no sense in a non-debug environment. This code is
twice as complicated and hard to read as it needs to be:

+
		if (NS_SUCCEEDED(rv = CopyCharPref("general.config.vendor",
+
			getter_Copies(lockVendor)) && (lockVendor))) 
+
		{
+#ifdef NS_DEBUG
+
			 printf("\nlockVendor %s \n", (const char *)lockVendor);
+#endif
+
		}


You should just write
+ rv = CopyCharPref("general.config.vendor", getter_Copies(lockVendor));
+ NS_ENSURE_SUCCESS(rv, rv);

I think you really just need to learn to stop using printf() and start using the
debugger to determine if your code is working correctly. all develoepers (i.e.
those running with NS_DEBUG) don't need to see a million printfs() at startup.

- please don't unnecessarily abbreviate function or variable names.
ReadCFGFile() should be ReadConfigFile
- in pref_VerifyLockFileSpec please don't use C comments to remove blocks of
code - either use #if 0, or remove the code entirely. Please read section 2,
item 6 of http://www.mozilla.org/hacking/portable-cpp.html
(and read that whole document if you haven't already!)

Comment 85

[Chip Clark]

Attachment: corrections based on alexf comments (and changes to nsProfile.cpp)

Comment 86

[Alec Flett]

Please fix your indenting..stop using tabs in source code...

Also, you're using JS_BeginRequest() and JS_EndRequest(), but you allow for
early exit from between these two! This is bad - you need to make sure
JS_EndRequest is called.

Comment 87

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

+
if (lockFileName == nsnull) 
+
{
+
	return NS_OK;
+
}
+
else 

else-after-return is a non-sequitur.

+    nsCOMPtr<nsIFile> aFile; 
+    rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
getter_AddRefs(aFile));
         

You never check the result code there -- will _nsIFileToFileSpec (spit) handle
that, or crash on a null pointer?  Also, why not convert over to nsIFile, while
you're whacking this code anyway?

     gInstance->StartUp();
-    }
+
}
     return gInstance;

Looks like random tab-damage.  Please make sure that there are no tabs in the
lines you check in, and that indentation conforms to the dominant (hopefully
universal) style for this file -- which should be reflected in the modeline.

+NS_IMETHODIMP nsPref::ReadConfigFile()
+//----------------------------------------------------------------------------------------
+{
+    nsresult rv; 
+
rv = useLockPrefFile(); 
+    return rv;
+}

Why not just |return useLockPrefFile();|?

-
const long hash_length = 51;		/* len = 48 chars of MD5 + // + EOL */
-    unsigned char *digest;
-    char szHash[64];
+
//const long hash_length = 51;		/* len = 48 chars of MD5 + // + EOL */
+    //unsigned char *digest;
+    //char szHash[64];

Why not delete these, rather than just commenting them out?

+
//This segment was left here as a reference - just in case anyone decides to
reimplement it!

People can use CVS history for that, I think.  Let's try and keep the dead code
to a minimum.

+
-I$(PUBLIC)/raptor      \

Do we actually have a "raptor" directory?  Do you still need all those include
directories?

+install::
+
$(INSTALL) -m 444 $(srcdir)/mozilla.cfg $(DIST)/bin

You put it in $(DIST)/bin, but you don't seem to be packaging it anywhere.  Is
this a necessary file, or not?  (If not, should it be unconditionally installed
into $(DIST)/bin?)

Comment 88

[Chip Clark]

Question:
If the cfg if off by default and the only reason to turn it on would be because 
the cfg actually has data internal (meaning cfg was created) is there any reason 
to have a cfg as part of the build?

If not... I will turn off the installing of both cfg files in the makefiles

Comment 89

[Alec Flett]

if that is how it works, then yeah, leave it out of the build...

Also, I'm not real keen on the .cfg file being in the same directory as the
binary, now that I think of it - we have enough junk in there already... we
should probably find a better spot. how about something like defaults\eclient or
something? (I welcome better names)

Comment 90

[Chip Clark]

how about default/prefs.. the directory already exists on all platforms 
(initialization of the .js files) and this is a pref file.

Comment 91

[Chip Clark]

Attachment: corrected diff

Comment 92

[Chip Clark]

Latest diff has the following changed (beyond corrections)

    1 - The makefiles are not changed as it has been decided to NOT include 
        the cfg in the build... but it should be added to the source tree
    2 - conversion to nsIFile not done as part of this project

Comment 93

[selmer (gone)]

Chip, how does the enterprise IT department deploy a browser that REQUIRES the
cfg file?  The bits they deploy should refuse to work if the user deletes or
corrupts the cfg file.  Does that happen somewhere else?

Comment 94

[Brian Nesse (gone)]

The activation of the requirement of the config file is done via enabling the
 pref("general.config.filename", "netscp6.cfg");
preference in all-ns.js. This activates the code which contains the functionality 
you seek. Removal of or tampering with the will cause an error to be thrown at 
this point.

Comment 95

[Alec Flett]

selmer: the commercial build should require this by default, via the prefs that
chip listed. all-ns.js would should be stored in a read-only location.

Comment 96

[Chip Clark]

As Brian mentioned... the general.config.vendor pref can be set and locked in 
the cfg file. (it can also be set in the all.js or all-ns.js files)

This value is checked after the cfg is read (so the pref can be set within the 
cfg file).... if the value doesn't match the name of the file it fails.  

So... yes, it would be possible for a use to take the cfg file and alter an 
external all-ns.js file to match the names and function.  But, the security of 
the file is greater than that with 4.x - and with the complaints about having 
PSM startup this early in the initialization phaze...  I opted for this "small" 
bit of security (which, again, is more than 4.x) but doesn't require PSM.

IF that level of security is required:
 1 - I see that as a different bug
 2 - I suggest we implement it as part of the profile management
       Determine if the profile needs PSM - and if so... check the cfg
       at that point (which is much later in the startup process and
       would not be a requirement for all profiles).

Comment 97

[Alec Flett]

Comments on this patch:
- indenting is still screwed up, please stop using tabs. just look at the diff,
it's VERY obvious.
- you've got to comment code to explain what is going on... for instance:
  - it was not obvious to me why you were checking general.config.vendor AFTER
loading the config file - then I realized that the config file would have that
pref in it.
  - what is this magic number "-4" you're subtracting from fileNameLen - _I_ now
realize it refers to the length of ".cfg" but that's because I'm intimately
familiar with this code.
  - this patch deserves at least 20 lines of comments sprinkled througout it
explaining what you are doing. comments cost nothing.
- you're returning PREF_ERROR from a method that is supposed to return an
nsresult - they are not the same thing. you should be returning an error that's
prefixed with NS_ERROR_* - like NS_ERROR_FAILURE
- I thought we agreed to get rid of return_error - I still see it declared in
this patch, even though I don't see it used. please remove it so nobody
(including yourself) is tempted to use it again. Are you looking at compiler
warnings? are there other variables that are unused?
- please don't extend the PREF_ERROR system - it should eventually go away and
be replaced with nsresults, but in the mean time all new code should use nsresult.

Comment 98

[Chip Clark]

Attachment: -tabs -PREF_ERROR +comments +nsresults

Comment 99

[Alec Flett]

Almost there....

some of your changes are just adding unnecessary clutter and making it hard to
read. why say:

if (foo)
{
    rv = NS_ERROR_FAILURE;
    return rv;
}

when you could say:

if (foo)
    return NS_ERROR_FAILURE;

Also, never compare directly against NS_OK, instead use the NS_SUCCEEDED()
macro, like the rest of the codebase does.

Comment 100

[Chip Clark]

Attachment: comments added

Comment 101

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

+    if (NS_SUCCEEDED(rv = CopyCharPref("general.config.filename",
+
		getter_Copies(lockFileName)) && (lockFileName)))
+    {
+#ifdef NS_DEBUG
+        printf("\nlockFile %s \n", (const char *)lockFileName);
+#endif
+    }
+    if (lockFileName == nsnull)
+        return rv;

1) We don't need more debug spew.  Make it DEBUG_chipc if you _must_ leave it
in, but best is to remove it and use a debugger if it starts to fail.

2) You check NS_SUCCEEDED before your console message, but then check
lockFileName against nsnull (explicitly, unlike in the previous check!) to
determine whether or not you're going to bail early.  Can CopyCharPref hand back
a null string and a success code?  I'd think not, so just check NS_FAILED(rv)
and then bail with rv if so.

+    nsCOMPtr<nsIFile> aFile; 
+    rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
getter_AddRefs(aFile));
+    if (NS_SUCCEEDED(rv))
+
{

Here, and in the code quoted above, you diverge from the dominant brace
placement style (cuddled with if).  Please fix.

+        // TODO: Make the rest of this code use nsIFile and
+        // avoid this conversion
+        rv = _nsIFileToFileSpec(aFile, getter_AddRefs(lockPrefFile));

1) _nsIFileToFileSpec is a non-ANSI name (compilers own everything in the global
namespace that begins with an underscore)
2) I asked about this in my previous comments, and didn't get an answer, and the
code remains: why not just convert to nsIFile now, while you're in there?

+                if (NS_FAILED(StartUp()))
+                    return NS_ERROR_FAILURE;
+                // failure here means hash table is not started (more serious
problem exists)

Does that mean that you should be propagating the error code (such as
NS_ERROR_OUT_OF_MEMORY) back to the caller, for better diagnostics?

+    return rv;

Do you really want to return NS_OK, or whatever success code CopyCharPref
happens to return?

     NS_IF_RELEASE(mFileSpec);

Not changed, but: use nsCOMPtr?

+
if (NS_FAILED(fileSpec->ResolveSymlink()))
+        return NS_ERROR_FAILURE;
+    
     if (!Exists(fileSpec))
-        return PREF_ERROR;
+        return NS_ERROR_FAILURE;
     
     char* readBuf;
     if (NS_FAILED(fileSpec->GetFileContents(&readBuf)))
-    	return PREF_ERROR;
+        return NS_ERROR_FAILURE;
+

1) You can unify all of these to improve readability:

if (NS_FAILED(fileSpec->ResolveSymlink()) ||
    !Exists(fileSpec) ||
    NS_FAILED(fileSpec->GetFileContents(&readBuf)))
    return NS_ERROR_FAILURE;

2) Could you use nsXPIDLCString here, so that you don't have to free the readBuf
by hand, and you use the shared allocator correctly?

+    /* Unobscure file by subtracting some value from every char. */
+
long i;
+
for (i = 0; i < fileLength; i++)
+
	readBuf[i] -= obscure_value;
 
-   
-
	success = ( PL_strncmp((const char*) buf + 3, szHash, (PRUint32)(hash_length -
4)) == 0 );
+
if(!readBuf)
+
	return NS_ERROR_FAILURE;

Unless I'm _totally_ misreading this diff, you're:
  1) checking for a null readBuf _after_ you dereference it, and 
  2) never comparing it to anything
What's the purpose of VerifyLockFileSpec?

-    	    PR_FALSE) == PREF_NOERROR);
+    	    PR_FALSE) == NS_OK);

alecf asked you to use NS_SUCCEEDED rather than comparison with NS_OK, I think.

Comment 102

[Chip Clark]

the reason for not moving to nsIFile is because after speaking with Brian Nesse 
(who is converting nsPref.cpp to nsPrefBranch.cpp and nsPrefService.cpp) to 
change it would mean changing it in several other functions, functions that are 
called by practically everyone calling into the prefs framework - which would be 
a major task... and out of the scope of this fix.

1) We don't need more debug spew.  
 - done

2) You check NS_SUCCEEDED before your console message
 - done

Here, and in the code quoted above, you diverge from the dominant brace
 - done

1) _nsIFileToFileSpec is a non-ANSI name 
 - see above
2) I asked about this in my previous comments
 - see above

Does that mean that you should be propagating the error code
   - there are a host of reasons (not just memory) that could case a failure

Do you really want to return NS_OK
 - Yes... at this point

NS_IF_RELEASE(mFileSpec);
Not changed, but: use nsCOMPtr?
 - Brian Nesse is doing this as part of his work with nsPrefBranch/nsPrefService

1) You can unify all of these to improve readability:
 - done

2) Could you use nsXPIDLCString here
 - no because I couldn't figure out a way to get nsXPIDLCString to allow
   me a way to edit the string readBuf (get() returns a const char *)

What's the purpose of VerifyLockFileSpec?
 - to unhash the readBuf (comment exists)

alecf asked you to use NS_SUCCEEDED rather than comparison with NS_OK
 - done

diff's on their way (when I am done with testing)

Comment 103

[Chip Clark]

Attachment: adjustments based on comments of April 3, 2001

Comment 104

[Alec Flett]

this makes no sense:
+    if(NS_FAILED(rv =
CopyCharPref("general.config.filename",getter_Copies(lockFileName)) &&
(lockFileName)))
+        return rv;
+

and is totally unreadable.
try:
+ rv = CopyCharPref("general.config.filename", getter_Copies(lockFileName));
+ if (NS_FAILED(rv) || !lockFileName) return NS_OK;


you should follow the same pattern for the "general.config.vendor" pref as well,
and avoid the indented if and invalid success value if lockVendor is null.

you're still returning PREF_ERROR from pref_OpenFileSpec, which you changed to
be an nsresult.

while you're touching that code, can you remove the XP_WIN code which pops up a
MessageBox?

Also, your tabs are STILL messed up on some lines, and you're removing the
broken GetVersionNumber() foo that rickg checked in yesterday.  This makes me
think that you are editing the file and then whacking it back down on top of
what's already in CVS...and worries me that there are other bugs that you're
backing out.

do you read your own diffs? You should ALWAYS read over the whole diff before
attaching it.

Comment 105

[Chip Clark]

Attachment: diff to latest pull

Comment 106

[Rodney Velasco]

eclient issue...adding trix and I to Cc.

Comment 107

[Chip Clark]

waiting for sr & r  

Comment 108

[Chip Clark]

Attachment: diff to work with changes made in bug 46863

Comment 109

[Chip Clark]

Attachment: removed dead function; diffs for nsPrefService.cpp

Comment 110

[Chip Clark]

Attachment: nsPrefService/all.js/nsAppRunner - implementing ReadConfigFile()

Comment 111

[Chip Clark]

Attachment: removed check for null of nsxpidlstring

Comment 112

[Brian Nesse (gone)]

r=bnesse

Comment 113

[Alec Flett]

sr=alecf with a few minor comments:
- do NOT check in all.js - there is no reason to add extra comments to that
file. There should be documentation which supports it instead
- the comment that refers to mozilla vs. 'netscp6' - please just explain that if
the pref exists, then read the config file - there may be other clients besides
netscape 6 (or 6.5, or 7.0, etc) which use this file
- regarding the actual name of the .cfg file - let's call it something better
than "netscp6" - netscape 6 has already been released - Netscape is already
working on Netscape 6.5, and what are you going to do when it's called "Netscape 7"?
- At some point, we really need to move this stuff out of nsAppRunner.cpp into
an app-startup system or profile-startup mechanism.. not now obviously, but it's
something to plan for.

Comment 114

[Brendan Eich [:brendan]]

Given alecf's conditions, a new patch should be attached before checkin.

/be

Comment 115

[Jussi-Pekka Mantere]

In 4.x we had netscape.exe and netscape.cfg. For 6.x we have netscp6.exe and
propose netscp6.cfg as the config file name. Logic being: the config file has
the same name as the exe file, but with cfg extension.

Are we changing the Windows exe name for 6.5?

Comment 116

[Chip Clark]

Attachment: updated diff

Comment 117

[Alec Flett]

I doubt we're changing the .exe name, but just because someone made a bad choice
there doesn't mean we have to continue mirroring it... 

the only reason the old netscape.exe matched the old netscape.cfg is that both
were produced by a company called "Netscape" :)

we don't go calling the prefs file netscape.js/netscp6.js nor do we call the
history file netscape.dat/netscp6.dat - let's break that habit right now..

Comment 118

[Chip Clark]

Actually, I'm not sure this discussion of the "naming of the cfg file" isn't moot. 
Since we don't include the pref in the all.js (or all-ns.js) file, and the
preference can be named anything the e-customer wants to.... (and everyone else
won't bother even having one)....   Let e-customers figure out for themselves
what to name it.

 Fix was landed by mitesh yesterday and the tree is green.

Marking FIXED.

Comment 119

[Rodney Velasco]

over to trix for verification...adding sarah to Cc.

Comment 120

[Trix Supremo]

verified on 20010515 builds on win32,mac, & linux_glibc