Commercial build must read netscape.cfg on startup

VERIFIED FIXED in mozilla0.9.1

Status

()

P1
normal
VERIFIED FIXED
20 years ago
17 years ago

People

(Reporter: mcmullen, Assigned: chipc)

Tracking

({perf})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(22 attachments)

730 bytes, patch
Details | Diff | Splinter Review
52 bytes, application/octet-stream
Details
1.25 KB, patch
Details | Diff | Splinter Review
4.29 KB, patch
Details | Diff | Splinter Review
51 bytes, text/plain
Details
52 bytes, text/plain
Details
13 bytes, text/plain
Details
15.62 KB, patch
Details | Diff | Splinter Review
2.32 KB, patch
Details | Diff | Splinter Review
1.81 KB, patch
Details | Diff | Splinter Review
13.76 KB, patch
Details | Diff | Splinter Review
20.59 KB, patch
Details | Diff | Splinter Review
18.35 KB, patch
Details | Diff | Splinter Review
21.49 KB, patch
Details | Diff | Splinter Review
19.87 KB, patch
Details | Diff | Splinter Review
22.37 KB, patch
Details | Diff | Splinter Review
22.86 KB, patch
Details | Diff | Splinter Review
3.88 KB, patch
Details | Diff | Splinter Review
4.36 KB, patch
Details | Diff | Splinter Review
8.16 KB, patch
Details | Diff | Splinter Review
8.12 KB, patch
Details | Diff | Splinter Review
7.33 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

20 years ago
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.
(Reporter)

Updated

20 years ago
Assignee: hshaw → mcmullen
(Reporter)

Updated

20 years ago
Hardware: Macintosh → All
(Reporter)

Comment 1

20 years ago
platform=all
(Reporter)

Comment 2

20 years ago
Changing plumbing bugs to M5 milestone, and accepting them.

Updated

20 years ago
Target Milestone: M5 → M6

Comment 3

20 years ago
Changed target milestone to M6.
(Reporter)

Comment 4

20 years ago
*** Bug 5598 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

20 years ago
Whiteboard: Fixed on Macintosh, Unix.
(Reporter)

Comment 5

20 years ago
Fixed on Macintosh.
(Reporter)

Comment 6

20 years ago
Changing to P1 the priority of bugs upon whose solution others are waiting.
(Reporter)

Updated

20 years ago
Whiteboard: Fixed on Macintosh, Unix. → I don't think I can do this for M6.
Target Milestone: M6 → M7
(Reporter)

Comment 7

20 years ago
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.
(Reporter)

Comment 8

20 years ago
*** Bug 5947 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 9

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

Comment 10

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

Updated

20 years ago
Blocks: 4998
(Reporter)

Comment 11

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

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

20 years ago
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.
(Reporter)

Comment 14

20 years ago
Well actually, you don't. The default prefs are read in before the profile is
picked.
(Reporter)

Updated

20 years ago
OS: Mac System 8.5 → All
Whiteboard: I don't think I can do this for M6. → netscape.cfg is the only remaining case (and a can of worms)
Target Milestone: M7 → M9
(Reporter)

Comment 15

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

Updated

20 years ago
Blocks: 7579
(Reporter)

Comment 16

20 years ago
Changing summary from "Apprunner must read netscape.cfg on startup"
Bulk reassigning mcmullen@netscape.com bugs to don@netscape.com.

Updated

19 years ago
Assignee: don → dp
Target Milestone: M9

Comment 18

19 years ago
dp, can your folks handle this issue?

Updated

19 years ago
Assignee: dp → neeti

Comment 19

19 years ago
Is this us or profile folks ?

Comment 20

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

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M12

Updated

19 years ago
Target Milestone: M12 → M10

Updated

19 years ago
Summary: Apprunner must read netscape.cfg and all.js on startup → [FEATURE]Apprunner must read netscape.cfg and all.js on startup

Updated

19 years ago
Blocks: 11408

Updated

19 years ago
Target Milestone: M10 → M11

Comment 21

19 years ago
moving target milestone to M11.

Comment 22

19 years ago
moving target milestone to M11.

Updated

19 years ago
Target Milestone: M11 → M15

Comment 23

19 years ago
moving to M15

Comment 24

19 years ago
Moving all libPref component bugs to new Preferences: Backend component.  
libPref component will be deleted.
Component: libPref → Preferences: Backend

Updated

19 years ago
Target Milestone: M15 → M16

Comment 25

19 years ago
spam: moving qa contact on some bugs from paulmac to sairuh@netscape.com
QA Contact: paulmac → sairuh

Comment 26

19 years ago
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
Keywords: nsbeta2

Comment 27

19 years ago
Putting on [nsbeta2+][6/01] radar.  This work must be done by 06/01 or we may 
pull this for PR2.
Whiteboard: netscape.cfg is the only remaining case (and a can of worms) → [nsbeta2+][6/01]netscape.cfg is the only remaining case (and a can of worms)

Comment 28

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

Neeti
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED

Comment 29

19 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 30

19 years ago
Created attachment 9219 [details] [diff] [review]
Patch to turn off this feature

Comment 31

19 years ago
Reassigning libpref bug to mcafee
Assignee: neeti → mcafee
Status: REOPENED → NEW

Comment 32

19 years ago
Move to M17 ...
Target Milestone: M16 → M17

Comment 33

19 years ago
Due to slip in schedule, moving this bug from [6/01] to [Will be minus on 6/15] 
for fix deadline.
Whiteboard: [nsbeta2+][6/01]netscape.cfg is the only remaining case (and a can of worms) → [nsbeta2+][Will be minus on 6/15]netscape.cfg is the only remaining case (and a can of worms)

Comment 34

19 years ago
Move to M19 since this bug has a time limit ...
Target Milestone: M17 → M19

Comment 35

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

Comment 36

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

Comment 37

18 years ago
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.
Group: netscapeconfidential?
Summary: [FEATURE]Apprunner must read netscape.cfg and all.js on startup → [FEATURE]Apprunner must read netscape.cfg on startup
Whiteboard: [nsbeta2+][Will be minus on 6/15]netscape.cfg is the only remaining case (and a can of worms) → [nsbeta2+][Will be minus on 6/15]

Updated

18 years ago
Summary: [FEATURE]Apprunner must read netscape.cfg on startup → [FEATURE] Commercial build must read netscape.cfg on startup

Comment 38

18 years ago
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
Whiteboard: [nsbeta2+][Will be minus on 6/15] → [nsbeta2-]
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?
Keywords: nsonly

Comment 40

18 years ago
over to selmer
Assignee: mcafee → selmer
plussing based on a meeting with bijals and selmer. Steve is the wrong owner, 
though.
Keywords: nsbeta3
Summary: [FEATURE] Commercial build must read netscape.cfg on startup → Commercial build must read netscape.cfg on startup
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3+]

Comment 42

18 years ago
Neeti, can you take this bug?  You did the major work on this back in May.
Assignee: selmer → neeti

Comment 43

18 years ago
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!
Assignee: neeti → selmer

Comment 44

18 years ago
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.
Assignee: selmer → dveditz
Whiteboard: [nsbeta2-][nsbeta3+] → [nsbeta2-][nsbeta3]
Readding plus while I investigate. Bijal: you need to remove the entire nsbeta3 
part, not just the plus.
Whiteboard: [nsbeta2-][nsbeta3] → [nsbeta2-][nsbeta3+] investigating 8/24

Comment 46

18 years ago
Removed [nsbeta3+] from status whiteboard.
Whiteboard: [nsbeta2-][nsbeta3+] investigating 8/24 → [nsbeta2-]investigating 8/24

Updated

18 years ago
Blocks: 6117
Sorry, am just swamped and will not be able to do this.
Whiteboard: [nsbeta2-]investigating 8/24 → [nsbeta2-][nsbeta3-]
Target Milestone: M19 → Future
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

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

18 years ago
Dan V. and I agreed that we would not get to this feature for Seamonkey 
timeframe.
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
Group: netscapeconfidential?

Comment 52

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

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

18 years ago
Varada, can you help Andreas with that?

Comment 55

18 years ago
Anyone with a netscape.cfg ... ?

Comment 56

18 years ago
Created attachment 17749 [details]
Netscape.cfg file

Comment 57

18 years ago
I have attached a netscape.cfg file to the bug. 
You can also get a netscape.cfg from www.ufaq.org, specifically from 
http://www.ufaq.org/downloads.html
reassigning to chipc
Assignee: dveditz → chipc
Keywords: nsbeta2, nsbeta3 → nsbeta1
Whiteboard: [nsbeta2-][nsbeta3-]

Comment 60

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

Comment 61

18 years ago
Created attachment 25732 [details] [diff] [review]
diffs for MANIFEST_CFG and all-ns.js
(Assignee)

Comment 62

18 years ago
Created attachment 25733 [details] [diff] [review]
diffs for all.js nspref.cpp nsipref.idl nsprofile.cpp
(Assignee)

Comment 63

18 years ago
Created attachment 25734 [details]
new cfg file
(Assignee)

Comment 64

18 years ago
Created attachment 25735 [details]
new cfg for Mozilla
(Assignee)

Comment 65

18 years ago
Created attachment 25736 [details]
MANIFEST_CFG for new mozilla.cfg file
(Assignee)

Comment 66

18 years ago
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.
Status: NEW → ASSIGNED

Comment 67

18 years ago
The current patches look good and work as advertised. r=bnesse
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.
Keywords: perf

Comment 69

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

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

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

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

Comment 73

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

18 years ago
setting milestone to 0.8.1

Target Milestone: Future → mozilla0.8.1
(Assignee)

Updated

18 years ago
Target Milestone: mozilla0.8.1 → mozilla0.9
(Assignee)

Comment 75

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

Comment 77

18 years ago
Created attachment 28645 [details] [diff] [review]
new diffs - cfg without PSM
(Assignee)

Comment 78

18 years ago
Created attachment 28647 [details] [diff] [review]
changes to the makefiles to include new files
(Assignee)

Comment 79

18 years ago
Created attachment 28649 [details] [diff] [review]
diff's for the ns makefiles
(Assignee)

Comment 80

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

18 years ago
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?
(Assignee)

Comment 82

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

Comment 83

18 years ago
Created attachment 28810 [details] [diff] [review]
diff adding return_error creation to #ifdef

Comment 84

18 years ago
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!)
(Assignee)

Comment 85

18 years ago
Created attachment 28925 [details] [diff] [review]
corrections based on alexf comments (and changes to nsProfile.cpp)

Comment 86

18 years ago
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.
+
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?)

(Assignee)

Comment 88

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

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

Comment 90

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

(Assignee)

Comment 91

18 years ago
Created attachment 29112 [details] [diff] [review]
corrected diff
(Assignee)

Comment 92

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

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

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

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

Comment 96

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

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

Comment 98

18 years ago
Created attachment 29497 [details] [diff] [review]
-tabs -PREF_ERROR +comments +nsresults

Comment 99

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

Comment 100

18 years ago
Created attachment 29600 [details] [diff] [review]
comments added
+    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.
(Assignee)

Comment 102

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

Comment 103

18 years ago
Created attachment 29712 [details] [diff] [review]
adjustments based on comments of April 3, 2001

Comment 104

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

Comment 105

18 years ago
Created attachment 29879 [details] [diff] [review]
diff to latest pull

Comment 106

18 years ago
eclient issue...adding trix and I to Cc.
(Assignee)

Comment 107

18 years ago
waiting for sr & r  

Updated

18 years ago
Blocks: 70348
(Assignee)

Comment 108

18 years ago
Created attachment 30632 [details] [diff] [review]
diff to work with changes made in bug 46863
(Assignee)

Comment 109

18 years ago
Created attachment 30935 [details] [diff] [review]
removed dead function; diffs for nsPrefService.cpp
(Assignee)

Updated

18 years ago
Depends on: 46863
(Assignee)

Updated

18 years ago
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Comment 110

18 years ago
Created attachment 32896 [details] [diff] [review]
nsPrefService/all.js/nsAppRunner - implementing ReadConfigFile()
(Assignee)

Comment 111

18 years ago
Created attachment 32900 [details] [diff] [review]
removed check for null of nsxpidlstring

Comment 112

18 years ago
r=bnesse

Comment 113

18 years ago
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.
Given alecf's conditions, a new patch should be attached before checkin.

/be

Comment 115

18 years ago
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?
(Assignee)

Comment 116

18 years ago
Created attachment 32989 [details] [diff] [review]
updated diff

Comment 117

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

Comment 118

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

Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago18 years ago
Resolution: --- → FIXED

Comment 119

18 years ago
over to trix for verification...adding sarah to Cc.
QA Contact: sairuh → trix

Comment 120

18 years ago
verified on 20010515 builds on win32,mac, & linux_glibc
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.