89137 - Move netscape.cfg and autoconfig our of libpref module/ separate centralized pref management module

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Mitesh Shah]

Right now, prefs are being provided through local files and  stored back to
the files. There is a need that prefs should be provided/accessed
through HTTP, LDAP, ACAP, etc. servers. To accommodate other providers,
this new interface is being created. The new design will also be useful
in the future when prefs, along with other data stored on a remote
server for roaming access.

Comment 1

[Mitesh Shah]

Right now reading of netscape.cfg and autoconfig are parts of libpref module. We
need more functionality such as getldap, autoupdate, etc. under Centralized
Preferences Management for enterprise customers. These will unnecessary add up
code and dependencies in libpref. So we are thinking of moving cetralized
preference management out of preferences and have a separate module. 

Netscape.cfg and autoconfig both make use of the fact that preferences can be
get/set/lock through JS function calls defined in Prefs JS Context. All these
callbacks are defined in prefapi.c. So there are some dependencies on the
libpref code.

There are two ways of solving this:

(1) Expose the prefs JS context and the pref object through nsPrefService. The
centralized pref management will add additional functions to it and/or make it
XPCONNECT enabled, write a security manager etc. Essentially, two functions to
expose from prefs context: PREF_GetConfigContext and PREF_GetConfigObject. 
This approach has a disadvantage of playing with the prefs context and changing it.

(2) the other approach would be to create a new JS context for centralized pref
management and have all those things happen in the new context. This approach
invloves extra memory and some duplication of code.

So my question is what would be a better way? It would be nice if we can reslove
this before I go ahead and implement in one way.
Any other way besides these two?

Of course, in both ways, we have to find out a way to create a centralized pref
management object and call it at the right time. Best place would be to call it
when Pref Service is started/restarted. Any other suggestion?

Comment 2

[Mitesh Shah]

As per the talk with mstoltz and dveditz, having a separate JS context for
autoconfig makes sense if the prefs are not going to use JS and prefs context
going away in the future. Any suggestion?

Comment 3

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

I don't know anything about plans to move prefs away from JS, so I can't really
offer much advice on that front.  Is there a bug or newsgroup posting that can
give me some context there?

Comment 4

[Brian Nesse (gone)]

There was a "pie in the sky" plan to remove the JS dependencies from prefs
because prefs weren't really JS and launch time was negatively impacted by
having to start up JS. Unfortunately I discovered that in the initialization of
prefs some of the files (specifically "initpref.js" and <platform>.js) *are*
executing JS. Thus there would be issues to overcome before we could really
break this dependence.

Comment 5

[Alec Flett]

ok, I finally filed a bug on the pie in the sky: bug 98533

Comment 6

[Alec Flett]

Ok, here's the plan as discussed with mitesh, dmose, and others back in late
june/early july:
- move prefs away from JavaScript, since the basic pref functionality does not
require JavaScript
- start providing a mechanism for "pref-providers" - these are modules that
provide pref values to the prefs module. Two examples of such pref providers are
autoconfig and LDAP. ACAP, HTTP, etc could eventually come down the pipeline.
- the autoconfig pref-provider would have a JavaScriptable mechanism for
defining pref values, and that is what mitesh is working on. Initially I don't
see anything wrong with just migrating the terrible JSAPI-based system that's in
libpref, but I'm sure others will disagree.
- Eventually, the autoconfig module would just use an xpconnect-based JS context
and expose the existing user_pref/etc functions via helper functions.

Basically this prefs from js, AND provides a pluggable mechanism for adding
preferences from external modules.

Comment 7

[Mitesh Shah]

Attachment: Changes required to move AutoConfig out of libpref

Comment 8

[Alec Flett]

cool! I'll try to carefully review these in the next day or so..

Comment 9

[Mitesh Shah]

This is a first set of changes required to move AutoConfig out of libpref. We
have created a new directory called 'libeclient' under mozilla/modules/ . 

ReadConfig is now an observer to the nsPrefService::ReadUserPrefs() and it gets
notified when ReadUserPref() happens. So there is no readconfig/autoconfig
dependency on prefs and also ReadConfig is created as part of nsAppStartUp
category. (Thanks Brian)

Requesting reviews.

Comment 10

[Brendan Eich [:brendan]]

I'm sorry, but new source tree structure needs to get past [e-mail],
and modules is not the place to put new code.  What's more, "eclient" is a poor
name choice for autoconfig.  Can we please consider making autoconfig an
extension (mozilla/extensions/autoconfig)?

/be

Comment 11

[Mitesh Shah]

That's fine. The naming choice was not finalized anyway, I can change it
/mozilla/extensions/autoconfig and also will get a permission from [e-mail]

Comment 12

[Gervase Markham [:gerv]]

It this being architected with roaming profiles in mind?

Even if it is, I have serious issues with the roaming model in Netscape 4.x. If
this feature's architecture is oriented towards replicating that in Mozilla then
we need to talk. ;-)

Gerv

Comment 13

[Mitesh Shah]

There is no new architecture here as such, this is a first step in separating
existing readconfig and autoconfig code out of libpref, second step would be to
create a new JS context and not use the pref JS context. Later on, other
Enterprise features such as getLDAPAttributes() (bug 75955) and autoupdate (bug
88958) will be added to this module. As far as I know, this has nothing to do
with roaming, this is more of a centralized preference management for enterprise
environment.  Roaming issues are discussed in bug 17048. 

Regarding the name 'autoconfig', we probably need a better name. This modules
contains other features than AutoConfig. I think 'enterprise' would be a good
name for this module, Any other suggestion?

Comment 14

[Brendan Eich [:brendan]]

What other features?  "Enterprise" is the new Star Trek show, it's catchy, but
far from descriptive.  Can we try to separate things so modules are cleanly
factored and composable?  I.e., why would we not want an autoconfig extension
that does just automated, remote prefs provisioning?

/be

Comment 15

[Mitesh Shah]

As I mentioned in my previous note, other features are : 'LDAP access to the
preferences (this happens during the executing autoconfig.jsc file) as we
discussed in bug 75955  and also AutoUpdating the software (this also happens
through autoconfig.jsc file). They both are tightly integrated with autoconifg
so there is no point in creating separate modules for it. 

so 'autoconfig' is not a bad choice, I just thought we are including other
functionality and we can have a more general name. How about we group the
enterprise related features together in /mozilla/extensions/enterprise/<dirs>
and autoconfig will be the first one to go as
/mozilla/extensions/enterprise/autoconfig/ ? 
Otherwise, /mozilla/extensions/autoconfig is fine with me.

Comment 16

[Alec Flett]

"enterprise" presumes a whole host of options.. does that library also include
LDAP support? roaming? Automated distribution? mcd? calendar? The list of
"enterprise" features goes on...

That said, I don't think we should be grouping these features specifically as
enterprise features. They are all features that various vendors will want to
bundle in different combinations to meet different needs. Netscape will want to
combine many of these features and make an enterprise client.

To those listening (brendan, etc) - the features mitesh just listed are ways
that we can extend libpref, and probably should be bound pretty tightly
together. AutoConfig is one implementation of such an extension.
How about mozilla/extensions/pref
and then we can have
mozilla/extensions/pref/autoconfig
mozilla/extensions/pref/ldap
etc..

Comment 17

[Brendan Eich [:brendan]]

alecf: I like it, the "pref" keyword was what I was calling autoconfig, but (as
I should have seen from mitesh's comments earlier -- sorry) if LDAP etc. should
be split out as subdirectories, then an extensions/pref container makes sense.

/be

Comment 18

[Mitchell Stoltz (not reading bugmail)]

How about "remoteadmin?" That would cover all of Mitesh's features, wouldn't it?

Comment 19

[Mitesh Shah]

The new set of changes contain a nsJSConfigTriggers.cpp file which creates a new 
XPCONNECT enabled JS context for autoconfig. Right now, the security manager for 
thsi context doesn't allow any access but we can add extra details once we 
implement getldap and autoupdate in this context. I have moved the directory 
structure to /mozilla/extensions/pref/autoconfig. 'remoteadmin' also sounds good 
to me.

Comment 20

[Mitesh Shah]

Attachment: changes with new JS context for AutoConfig

Comment 21

[Mitesh Shah]

Attachment: latest patch with inclusion of getldap.js

Comment 22

[Mitesh Shah]

The latest patch has a new file getldap.js which is a JS version of 
getLDAPAttributes() (bug 75955) and also the mechanism to execute it in the 
autoconfig JS context. (It will loaded based on the value of pref 
(autoadmin.getldap.enabled))

Comment 23

[Mitesh Shah]

Attachment: latest patch with prefcalls.js and getldap.js

Comment 24

[Mitesh Shah]

I have replaced native C++ calls with JS cals for lockpref/pref etc. in a file 
prefcalls.js. It will be installed in defaults/autoconfig along with getldap.js

There are still few things to be done for security manager but I have posted the 
patch for initial reviews and feedback.

Comment 25

[Mitesh Shah]

As per the talk with mstoltz, we probably don't need Security Manager if we are 
going to allow autoupdate through autoconfig in the future. For the time being, 
he suggested to use Principals to distinguish between trusted and untrusted 
scripts if that can be done. Mitch, please confirm.

Comment 26

[Mitesh Shah]

Attachment: few improvments over the prev patch

Comment 27

[Mitesh Shah]

Few changes included in the latest patch:

- nsAutoConfig now supports weakReference because we no longer need a strong 
reference throug Observer list, nsReadConfig is holding a strong reference to 
nsAutoConfig

- Removed security manager for autoconfig JS context.

- removed lockPref and unlockPref from prefapi.c/prefs context

- removed removObserver from nsReadConfig destructor - it was useless to call 
removeObserver at the end while there is no observerservice is available.

Comment 28

[Brian Nesse (gone)]

Comment on attachment 55996 [details] [diff] [review]
few improvments over the prev patch

nsPrefService.cpp
You should probably ignore any error returned from notifyObservers(). Generally we really shouldn't bail if the notification fails.

As per previous discussion with chipc, NS_PREFSERVICE_READ_TOPIC_ID notification should happen *after* loading of user files, not before.

Please send NS_PREFSERVICE_RESET_TOPIC_ID notification in ResetPrefs before calling Pref_CleanupPrefs()

All of your makefiles have the old license boilerplate.

General JS file comments:
Please be consistent. I'm seeing if ( this == that ), if (this == that), if (this == that ). Also (param1,param2,param3) vs. (param1, param2, param3). Same goes for switch statements, use a consisent indentation scheme, getldap.js and prefcalls.js do it two different ways.

in function getLDAPAttributes:
    var url = Components.classes["@mozilla.org/network/ldap-url;1"].
createInstance(Components.interfaces.nsILDAPURL);
    url.spec = "ldap://" + host + "/" + base + "?" + attribs 
+ "?sub?" +  filter;
is much more readable as:
    var url = Components.classes["@mozilla.org/network/ldap-url;1"]
                        .createInstance(Components.interfaces.nsILDAPURL);
    url.spec = "ldap://" + host + "/" + base + "?" + attribs 
               + "?sub?" +  filter;

nsReadConfig.cpp
+    nsReadConfig::nsReadConfig()
+{
should be
+nsReadConfig::nsReadConfig()
+{

You are saving the AutoConfig object into mAutoConfig (presumably so it doesn't get created twice based on our discussions). You are not releasing it before calling readConfigFile again, however, thus causing a leak in the situation where prefs are reset.


nsReadConfig.cpp
Please remove this stuff
+        // Commenting out the RemoveObserver code, it is causing 
+        // nsIObserverService to skip the next element.  bug 94349
as this is not going to be fixed.

Comment 29

[Mitesh Shah]

Attachment: improved version

Comment 30

[Mitesh Shah]

I think license boiler plates in the makefiles have not been changed. All other 
makefiles have the same old license plates.

Regarding leaking nsAutoConfig, I think when you reassing a XPCOM component, it 
automatically deletes the previous value. But anyway, I have added the code to 
check for its existence and removal.

All other comments are addressed in the latest patch. Also, There are some 
additional changes in configure, configure.in and allmakefiles.sh to build the 
new directory on Unix.

Requesting review and superreview

Comment 31

[Alec Flett]

Regarding that mAutoConfig thing, yes nsCOMPtrs do automatically release their
previous value.. those extra lines of code are unnecessary and should not go in... 

...continuing to review..

Comment 32

[Alec Flett]

Comment on attachment 56772 [details] [diff] [review]
improved version

Ok, this seems reasonable, pending r=bnesse of course. 

it doesn't matter if we haven't gotten around to updating the other license boilerplates, new files always get the new license - sorry!

so with the new license, and the unnecessary if (mAutoConfig) lines removed, sr=alecf

Comment 33

[Brian Nesse (gone)]

Preferences module minor nits:
Please remove the added extra blank lines

nsPrefService.cpp
@@ -174,11 +166,14 @@
   nsresult rv;
 
   if (nsnull == aFile) {
+
     rv = useDefaultPrefFile();  // really should return a value...

nsPrefService.h
@@ -64,9 +64,10 @@
   nsresult Init();
                            
 protected:
+  nsresult notifyObservers(const char * aSubject);
   nsresult useDefaultPrefFile();
   nsresult useUserPrefFile();
-  nsresult readConfigFile();
+

Also, please remove the space between the * and aSubject in the notifyObservers 
declaration... it looks odd not connected to anything (IMO).

Autoconfig module.
nsJSConfigTriggers.cpp also has old license boilerplate.

Comment above AutoConfigSecMan says:
// Security Manager for new XPCONNECT enabled JS Context
// Right now it doesn't allow any access to XPCOM

but every function return NS_OK, this appears to be the opposite of the 
documentation.

CentralizedAdminPrefManagerInit() can potentially be called more than once 
without calling CentralizedAdminPrefManagerFinish(). This will cause you to leak 
|autoconfig_cx|. Also, is |autoconfig_glob| automatically disposed by virtue of 
being installed into |autoconfig_cx|? If not, you'll leak it too. PREF_Init() 
protects itself from multiple init calls, maybe you should too.

It looks like you've left some debugging code in nsReadConfig.cpp
+    printf("file's URL version: %s \n", fileURL.get()); 

You don't need to check for a null buffer in decodeHashFile(), you've already 
insured it can't be null in openAndEvaluateJSFile().

Again, consistency please:
const char * js_buffer
const char* filename
const char *aTopic

Also, white space around operators please (I realize you didn't write [all of] 
it, please fix anyway :)):
...fileNameLen -4...
PRUint32 amt=0;
...PR_Malloc(fs*sizeof(char)) ;
...isEncoded? PR_TRUE:PR_FALSE

Suggestion:
You could cut a bit of weight from prefcalls.js by adding a helper function to 
get the prefBranch...

Comment 34

[Alec Flett]

Comment on attachment 56772 [details] [diff] [review]
improved version

ack, I did a sloppy review. nice job bnesse...:)

Comment 35

[Mitesh Shah]

Attachment: latest patch with review comments addressed

-As per discussions with Alecf and Danm replaced getldap.js with
nsPrefLDAP.cpp, the wrapper function is still in JS and inside the file
prefcalls.js. 

- Js_DestroyContext will take care of cleaning the global/root object, all
other review comments addressed in the latest patch

Thanks for the previous reviews.
Requesting review and superreview again.

Comment 36

[Mitesh Shah]

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

Comment 37

[Alec Flett]

Comment on attachment 57051 [details] [diff] [review]
latest patch with review comments addressed

So regarding nsPrefLDAP/nsIPrefLDAP - in these two particular 
files, you're never actually referencing anything that has to do 
with prefs, so it seems a little strange to have "Pref" in the name.

In fact, it really looks like this should be nsILDAPSynchronousRequest 
or something, and put into the core LDAP component.

Finally, regarding the interface, it seems like there are too many attributes
just to do a simple query.

Why not have one of these:
1) a completely synchronous API, with one method:
wstring getQueryResult(in nsILDAPURL url);
2) a sort of pseudo-synchronous API - one method that kicks off
the query, and one method that blocks until it completes:
void startQuery(in nsILDAPURL url);
wstring getResults();

I'm not sure that "results" should be an attribute.. most of the
time I would say make methods with no parameters into attributes
but in this case, because we block to wait on the net, we should make it a
method

Overall, I do like this though.

Comment 38

[Brian Nesse (gone)]

Makefiles general:
You have mixture of tabs and spaces scattered throughout your makefiles

nsJSConfigTriggers:
Your class declaration for AutoConfigSecMan is using 2 space indention, the rest 
of the file uses 4 spaces.

You should probably insure that autoconfig_cx is released in the event of an 
error in CentralizedAdminPrefManagerInit(). Here's why...

In CentralizedAdminPrefManagerInit() I'm a little concerned about
  if (autoconfig_cx) 
      return NS_OK;

Assume readConfigFile() calls CentralizedAdminPrefManagerInit(), and the 
autoconfig_cx is created, but something farther down fails (XPConnect service, 
SecurityManager, autoconfig_glob, or JS class initialization). This will return 
an error to readConfigFile() which will cause mRead to remain set to PR_FALSE. if 
readConfigFile() gets called again, CentralizedAdminPrefManagerInit() will return 
NS_OK even though it isn't properly initialized.

Other than that... and whatever you and alecf work out with nsPref/LDAP... it 
looks pretty good.

Comment 39

[Mitesh Shah]

Attachment: latest patch with review comments addressed

Renamed prefLDAP to LDAPSyncQuery but haven't moved nsILDAPSyncQuery to
directory/XPCOM - this is not actually an API for sync query. Dmose, what's yr
suggestion? Should I move this to directory/XPCOM/base/{public,source} ?

Other comments are addressed in this patch

Comment 40

[Mitesh Shah]

modules/libpref/src/init/all.js changes in the previous attachment are not pat
of the patch.

Comment 41

[Mitesh Shah]

Attachment: The corrected version

This is a correct version

Comment 42

[Brian Nesse (gone)]

cc'ing sfraser who requested that we try to find a way to not load the
autoconfig dll if the .cfg file does not exist. We discussed, but were unable to
come up with a scenario that did not involve adding pref dependencies on the
autoconfig module.

Comment 43

[Brian Nesse (gone)]

Attachment: Cleaned up version of mitesh's previous patch

Comment 44

[Brian Nesse (gone)]

I guess I own this one now... :(

Comment 45

[Brian Nesse (gone)]

Attachment: New and improved patch

This patch addresses alecf's recent tree changes (thanks alot alec!), addresses
sfrasers desire that the library not be loaded unnecessarily, assigns a new
contract ID to the autoconfig object, and adds a manifest file so the mac can
actually find the prefcalls.js file.

Requesting input/reviews please.

Comment 46

[Alec Flett]

Comment on attachment 59895 [details] [diff] [review]
New and improved patch

looking over this, there are a number of things we could fix, but at this
point, I'm really glad just to see this land. 

sr=alecf

Comment 47

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 59895 [details] [diff] [review]
New and improved patch

One nit: prefcalls.js has a bunch of constants defined at the beginning whose
names end with |CID|.  They should actually end with |ContractID|, because
they're not CIDs.  Fix that, and you've got r=[e-mail] for the
LDAP-related stuff in this patch (I'm not really qualified to review the rest).

Comment 48

[Brian Nesse (gone)]

Ooof. Due to massive merge conflicts with other patches alec and I landed,
coupled with the Mac project landing from yesterday... this is not going to make
it in before the freeze.

Alec, if you'd like to detail some of the "things we could fix" I'll see if I
can address them in the next patch

Comment 49

[Alec Flett]

I'd really rather not go any more revs on this.. the only one I remember off
hand is the one I mentioned above (about double-copying the buffer)

Comment 50

[Brian Nesse (gone)]

Attachment: Updated to reflect the current state of the tree and comments from dmose.

Comment 51

[Brian Nesse (gone)]

Attachment: Patch with appropriate linux makefile changes.

It seems none of mitesh's patchs ever addressed the necessary linux makefile
changes in /modules/libpref. I have made the appropriate changes.

Alec, as I don't have a linux box, could you please verify these for me?

Comment 52

[Brian Nesse (gone)]

Fix checked in.