Closed Bug 70348 Opened 19 years ago Closed 19 years ago

Implement an AutoConfig Feature

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: mitesh, Assigned: mitesh)

References

Details

(Whiteboard: Checked in)

Attachments

(30 files)

15.19 KB, patch
Details | Diff | Splinter Review
19.13 KB, patch
Details | Diff | Splinter Review
1.23 KB, text/plain
Details
1.41 KB, text/plain
Details
9.15 KB, text/plain
Details
3.73 KB, patch
Details | Diff | Splinter Review
1.33 KB, patch
Details | Diff | Splinter Review
5.41 KB, patch
Details | Diff | Splinter Review
14.66 KB, patch
Details | Diff | Splinter Review
6.91 KB, patch
Details | Diff | Splinter Review
16.07 KB, patch
Details | Diff | Splinter Review
3.81 KB, text/plain
Details
24.76 KB, patch
Details | Diff | Splinter Review
25.41 KB, patch
Details | Diff | Splinter Review
23.44 KB, patch
Details | Diff | Splinter Review
27.75 KB, patch
Details | Diff | Splinter Review
15.98 KB, patch
Details | Diff | Splinter Review
26.32 KB, patch
Details | Diff | Splinter Review
26.37 KB, patch
Details | Diff | Splinter Review
16.27 KB, text/plain
Details
27.33 KB, patch
Details | Diff | Splinter Review
27.13 KB, patch
Details | Diff | Splinter Review
27.13 KB, patch
Details | Diff | Splinter Review
6.01 KB, patch
Details | Diff | Splinter Review
6.37 KB, patch
Details | Diff | Splinter Review
12.69 KB, patch
Details | Diff | Splinter Review
15.58 KB, patch
Details | Diff | Splinter Review
17.84 KB, patch
Details | Diff | Splinter Review
19.47 KB, patch
Details | Diff | Splinter Review
21.51 KB, patch
Details | Diff | Splinter Review
AutoConfig downloads the preferences from a http:// or file:// URL and helps 
system administrator to keep the enterprise specific preferences at a central 
location.
Status: NEW → ASSIGNED
Summary: Implement an AutoConfig Feature → Implement an AutoConfig Feature
should this behave like PAC support?
I think PAC is for AutoProxy configuration. This is a facility to system 
administrator to put the prefernces specific to the enterprise on a central 
location and enforce it through netscape.cfg file. 
The autoconfig file simple text JS file.
Here with I am attaching code diffs for initial review. Please send me your 
feedback.
There is only one file I changed: modules/libpref/src/nsPref.cpp
I have added two methods: dowloadAutoCfgFile() and readOfflineFailoverJSC() to 
nsPref class. Also nsPref is now also inheriting from nsITimerCallBack and 
nsIStreamListener. 
yes, proxy autoconfig is a js file, just like this thing which appears to be 
browser autoconfig. it looks like you're duplicating code. please examine the 
pac support in bug 53080.

also, your style is switching between 2 and 4 space indentation, please pick 
one.

NS_ERROR("Only http:/ or file:/ URLs are vaild for autoconfig");
that's nice, is there a reason? what's wrong w/ ftp, gopher, https, imap, or 
news? [for that matter, do you really object to someone using data, javascript, 
resource, chrome, or about?].
The new patch file is created with 'cvs diff -u' for unified context diff.
mitesh, please coordinate with bnesse, who is in the process of revamping the
prefs interface.
It seems like PAC is more of a necko thing, no?
In the next revision, I will stick to one coding style.

Before started working on AutoConfig Module, I looked into PAC support for 
AutoProxyConfig and found it different. I will again look into that bug and see 
if we can leverage the code from there.

Regarding ftp://, telnet:// and other URLs, we are actually working on the 
feature what 4.x enterprise client supports. AutoConfig documentation says that 
it doesn't support other URLs than http:// and file://. Link to the 
documentation: 
http://blues/users/chipc/publish/MCD_DOC/dg/dgp14.html#1025739

comments on the patch as it stands now (I still think this belongs somewhere in
necko or something, I don't think nsPref should be burdened with this)

- pref_Alert is going away, not to mention that backend code should not have any
XUL-oriented frontend dependancies. people might want to use libpref in an
environment that does not have a frontend
- Why the JS_BeingRequest/JS_EndRequest? We tried to rip these OUT of nsPref a
while back (and now actually I see that some of the lock file stuff has it..
doh!) - remove them, they are not necessary.
- even if we were to be using FE code, why are there non-localizable strings in
this code? (like JS_BeginRequest/EndRequest, the presence of existing
non-localizable strings does not justify adding new ones)
- even if we were to be popping up dialogs from JS, we do not parent them off of
the hidden window.. yes, there are places in the code that do that, but they are
broken right now.
- the user's e-mail is not stored in mail.identity.usermail, it's stored using
some mailnews stuff. However, nobody outside of the mozilla/mailnews directory
should be depending on any mail code. I'm not sure about how to resolve that
dependancy just yet.
- in the gigantic function readOfflineFailoverJSC, you have an nsXPIDLCString
that is used for nothing other than copying the static string "failover.jsc" -
you should either just use the static string "failover.js", #define it somewhere
in the file, or use nsLiteralCString("failover.js") if you need a
nsAReadableString (which you don't seem to)
- Why are you appending "Essential Files" to the filename for mac? Are you sure
there isn't a pre-defined way of getting to that directory via
NS_GetSpecialDirectory (especially so we don't have to have XP_MAC code in this
mostly XP file!)
- you defined "aFile" as a local variable - the "a" prefix in a variable name
refers to the variable being an argument to the current function - which this is
not.

these are fairly minor nits though

The biggest issue I see right now is that this is rolled into nsPref. I guess
what I'd really like to see is this put into a totally seperate component, and
then fix up the nsIPref/nsIPrefService interface to be able to read vendor and
PAC files that are given to it, either as a buffer or a stream of some kind.
this component could live in the pref dll if it can't live in necko, but it
belongs in a seperate object.
Is that internal link copywritten in any way? if not, we should post it to
www.mozilla.org, it seems to have some good design info
Thanks Alecf for the feedback. I will work with Brian to find out a better place 
for AutoConfig Module if nsPref.cpp is not the right one. I will correct the 
other issues in the next revision. 
As you mention about not having any XUL or frontend code in libpref, so how 
should I go by if I want to bring up a confirm dialog box?

I don't think you want to be ripping out JS_BeginRequest/JS_EndRequest,
actually.  I just got assigned 39373 (JS on threads other than "main" must run
within requests), which has the opposite intent.
well, talk to brendan - he ripped them all out months ago
I prolly ripped those out well before fixing bug 54743.  Here's the current deal
on requests:

- If all JS objects that might be handled by a script are single-threaded,
accessible only by one thread, *and* that thread is the UI/main thread where GC
must run (because of single-threaded DOM finalizers), then you do not need
requests around batches of JS API and non-blocking native code.

- If your thread might create an object that is accessible by other threads, you
need requests.

- If your thread might use an object that was created by another thread, then
you need requests.

So, does any other thread use pref JS objects?  If so, we need requests (again).
Sorry for ripping them out before -- then, without bug 54743's fix, the only
reason to have them was to keep the GC at bay, and it seemed like the prefs code
was running on the same thread as the GC, viz, the main thread.  So there was no
point in them.

/be
right. All pref JS objects are completely private to libpref, and all of libpref
works only on the main thread.
Here is the new set of code changes. I have created a separate component under 
Preferences module. 
- Removed frontend related code.
- Removed JS_BeginRequest and JS_EndRequest code. (as per the last discussion, 
it is no longer needed since all JS objects are private to libpref)
- Removed the mailnews dependency. 
- There doesn't seem to be a solution to "Essential Files" folder yet so keeping 
it as it is.
The component contains three files:
nsIAutoConfig.idl, nsAutoConfig.h, nsAutoConfig.cpp
and changes made to nsPref.cpp and other build related files.
 
Attached file Class Implementation
looking much better! a few issues:
- use nsCOMPtr and do_CreateInstance, don't ever use
nsComponentManager::CreateInstance except under special circumstances
- you should never EVER use NS_DEFINE_xID() to define an IID.. even if you were
using nsComponentManager::CreateInstance(), you should be using NS_GET_IID. What
ancient code are you cutting and pasting this from, so I can go whack it with a
big stick?
- there is something really wierd going on with your cvs diff - it's missing
line numbers, so I can't tell where you're doing the autoconfig download stuff
- can we try to keep the autoconfig stuff as isolated from prefs as possible?
define the generic factory constructor in nsAutoConfig.cpp
- I'm not a huge fan of the "backdoor" of PREF_EvaluateConfigScript but I think
it's good for now - my hope is that we'll eventually switch away from using JS
to read in the user's prefs, but we'll use JS for things like the autoconfig
scripts... so it would be nice if nsAutoConfig were the place where all the JS
stuff lived, and nsPref just dealt with straight preferences, no JS. but THAT
issue is blue sky for now and shouldn't affect this patch.

Alecf,
Thanks for the feedback.

- As you can see this is the first time I am playing with mozilla code so by 
mistake I have copied from the old code. I think it's not even in the current 
tree. But mozilla lxr's search returned it for some search. I will change it to 
use do_CreateInstance and nsCOMPtr<>

- Regarding CVS diff, I have applied this changes on top of Chip's changes and 
Chip's changes are not checked in the tree yet. (Bug 5132) The AutoConfig stuff 
is actually in method: ReadLockPrefs (Which is not in the tree yet) Once those 
changes are in the tree, diffs will be more clear.
- If I define the generic factory constructor in nsAutoConfig.cpp, nsPref.cpp 
complains abt missing identifier 'nsAutoConfigConstructor' during the components  
registration. How can I solve that issue? Can we just declare the constructor in 
the nsAutoConfig.h?

There are still issues with user's email address and frontend dialog boxes but 
for now I have removed from this patch untill we resolve them.
don't worry about the offline icon.
I will be handling that automatically as a part of bug 65704
In addition to the changes to the utilityOverlay.js file, there is some 
additional code to nsAutoConfig.cpp:readOfflineFailoverJSC() function:

 
+        // lock the "network.online" prference so user cannot toggle back to
+        // online mode.
+        rv = prefs->SetBoolPref("network.online", PR_FALSE);
+        if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
+        pref_LockPref("network.online");
+

The file is not yet checked in to CVS so this changes are in addition to the 
previous attachments.

Basically, it locks the pref "network.online" and utilityOverlay.js changes will 
disable the network icon and menu item (under File) if the pref is locked.
I think this will changes will be in addition to the changes you will be making 
for bug 65704
oh, I see what you're saying. you're right. Thought at a later point I will move
your code into the XBL widget that I'm creating.
Attaching latest set of diffs, requesting code review

- These changes are on top of Chipc's changes for bug 5132 
- The creation of nsAutoConfig object has been moved nsApprunner.cpp along with 
netscape.cfg reading.
- For the unique identity problem, first we are using the email address if it is 
available but if this is a client without messenger and mail address is not 
available,  we are using the profile name as an unique identifier. This needs to 
be documented for IS people.

Thanks.
nope! there should be no reason to touch nsAppRunner.cpp.

you should add yourself to the "app-startup" category, which will make you get
called at startup... 

http://lxr.mozilla.org/seamonkey/search?string=APPSTARTUP_CATEGORY

look at the xml extras example.
The other problem is that that you're using the 4.x pref for the user's email
address... see:

http://lxr.mozilla.org/seamonkey/search?string=mail.identity.useremail

Note that it is not used anywhere in the tree except to migrate from 4.x to mozilla!

I can think of two possible solutions to this problem:
1) come up with a mail-independant way of determining the user's email address
(because the tree should compile with or without entering the mailnews
directory) - one possibility is to go through the nsIUserInfo service:
http://lxr.mozilla.org/seamonkey/source/xpfe/appshell/public/nsIUserInfo.idl

this will not use the user's preferences though, but will instead go right
through to the respective system's information about the user.

2) get the mail client to set the old 4.x pref whenever the default account is
set - i.e. in nsMsgAccountManager::SetDefaultAccount(), make it so that we set
mail.identity.useremail from the identity that's in that account.
It seems that nsUserInfo::GetEmailAddress is not implemented for WIN32 platform 
and on Unix it is created with username@domainname. I think this won't be the 
same email address as mainews will be using. In that scenario setting the 
preference through mailnews would be a better soltuion.
assigning QA to Luke for eClient purposes, adding sarah to Cc.
QA Contact: sairuh → lrg
I have added nsAutoConfig to the APP_STARTUP category and it works fine but 
there are two problems: 
- nsAutoConfig has a dependency on netscape.cfg bug 5132. Actually the value of 
the autoconfig URL is set in netscape.cfg so AutoConfig has to be called after 
netscape.cfg read.
- nsAutoConfig is also using a current profile name in case where there is no 
messenger. So it has to be called after the current profile name is set.

is there a way that we can call the method from AutoConfig module later on even 
if we instatiate it with the app-startup category?

Depends on: 5132
Posting latest set of diffs including following changes:

- Email address is now retrived from the set of perferences (by first finding 
the default account and then using it's id)
- Mac related change for the filesize in function evaluateLocalFile(). It's 64 
bit nunber and I was just casting it to unsigned int. In fact, it's a structure 
so using a macro to retrive the 32 bit value.
- The initialization is still under nsAppRunner.cpp, there is no better place. 
app-startup category doesn't work, since autoconfig has to be after netscape.cfg 
and profile name selection.
(changes are on top of the changes from bug 5132 as before)

Requesting r and sr, 

Thanks,
mitesh: I still object to adding this to nsAppRunner.cpp, so I'm going to
suggest the following:
- use the app-startup mechanism to initialize your object
- when your object is initialized, add yourself as an observer to the
profile-startup (I don't remember the actual topic name) topic.. from there you
can do stuff like block a profile from starting, etc.
you can make diffs of new files.
just 
cvs add <filename>

and then it will be included in your diff if you say
cvs diff -u -N <directory or file>
looks like NS_PROFILE_STARTUP_CATEGORY is the perfect place since the profile 
name has been decided at that stage. I have added the nsAutoConfig in that 
category. The only problem is that the code in nsProfile.cpp need some change or 
I am missing something. 

In following part of the code is not retriving the contractid correctly. It 
picks up the second argument from the RegisterAutoConfig function. while it 
needs to pick up the third argument. (second argument is the name of the module 
while the third argument is the contract id string) I looked up the 
APPSTARTUP_CATEGORY implementation and it does it differently. I am attaching 
both code snippets. When I put the same contract id for second and third 
argument to the Register function, the code worked fine. Also, attaching 
ResgisterAutoConfig function.

Let me know what should we do.


Attached file code snippets
Attaching latest set of diffs. (New files are included in the diffs now)
Changes:
- nsAutoConfig is instatiaed through APPSTARTUP category
- it is being called through observerservice with topic = "profile-after-change"

Latest set of diffs include following additions compare to the last time:

- It was supposed to write a cache copy of autoconfig.jsc to failover.jsc file. 
I have added a method writeFailoverFile() to write the downloaded preferences to  
failover.jsc file

- Removed checking for "autoadmin.failover_answer" preference in 
readOfflineFailoverJSC() method. It was redundant. Created a documentation bug 
75647 to document these changes.

Changing milestone to 0.9.1. Doesn't look like this will get r & sr by the 
checkin deadline. Will try again after the tree reopens for 0.9.1 checkins.
Target Milestone: --- → mozilla0.9.1
talked with mitesh over IRC - basically I have issues with the protocol-specific
nature of this patch - it should not matter if we're using http:, file:, or
gopher: to retrieve the jsc file - we should be using the same technique to suck
down the data, and behaving the same, no matter what the error is. (i.e. no need
to check for http error values, etc)

Also, the more I've though about this, the more I've decided I don't like the
direct calls into the pref library using PREF_* and pref_* - if you cannot get
to these routines through the pref service interface and friends, then we need
to fix those interfaces, and coordinate with bnesse on that.

I could eventually see this object in a seperate DLL along with other eclient
features, and in that case we MUST call across interface boundaries..
I'm not sure I agree with Alec's assessment of this feature.

I consider this like the cfg file.
This has (or should have) the same functionality (and purpose) as the cfg file.
As such.... why shouldn't it be part of the preference network?
Well chip, you are right.. this is very  much like the .cfg file stuff..and that
only further proves my point.

eventually I would like to see all JS-related configuration happening in an
entirely seperate object than prefs... many clients (i.e. embedding clients for
one) do not need e-client features like that.
Alec, while I do agree that this is the right approach in the long run, I'm not
sure if we'd want to hold off checking in any of the AutoConfig or netscape.cfg
for the new design.

Probably the best approach would be to have the current implementation land, get
it into the builds and general testing by QA, then open a separate bug for
redoing all JS prefs work, including core prefs.

The high level bug would be "Remove core preferences dependencies on
JavaScript", then have bugs filed for breaking out AutoConfig and netscape.cfg
as part of that work.
I never said anything about holding up this checkin for a design change...

what I did say is that I want them to use the nsIPref* interfaces instead of the
PREF_* calls.. that's just part of my review.

moving out the JS stuff is a whole different beast, I was explaining my
reasoning for requesting the removal of the PREF* right now.

So what is holding up this checkin right now is:
1) protocol-specific handling of the config files
2) use of PREF_*/pref_* calls
Latest set of diffs include following changes:

- Added unRegisterAutoConfig method in nsPrefsFactory.cpp

- Removed protocol specific downloading of autoconfig.jsc. AsyncOpen is now 
being called on any URI and the result expected is javascript code. If the 
result cannot be parsed correctly, the module go into failover mode.

- Added the event queue code in DownloadAutoCfg() to stall the thread till the 
downloading is completed(success/failure). 

- Added two methods getEmailAddr() and reportError() to the module for clarity. 
The mLoaded needs to be set to true at all possible paths to avoid deadlock.

- pref_lock is now being called through nsPrefBranch::LockPref(). Still calling 
PREF_EvaluateConfigFile() directly from prefapi.c. Talking with Brian, if we can 
provide different interface.
+    rv = inStr->Read(buf, fs, &amt);
+    if (NS_FAILED(rv)) 
+        NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv), 
+                     "The stream should never block.");
+    

I assume you want to bail out here too... maybe you meant
rv = inStr->Read(buf, fs, &amt);
NS_ASSERTION(rv != NS_BASE_STREAM_WOULD_BLOCK, 
            "The stream should never block.");
if (NS_FAILED(rv)) return rv;

Regarding the use of eventQueues, I think you need to make sure to
PopThreadEventQueue whenever you return, even on an error - otherwise you'll
stop processing events. I think the string bundle code has this same problem,
I'll have to fix that... sorry to direct you at a shabby example...

Also, please don't put "\n" in your assertions (more cut and paste from the
string bundles I see)


Here:

+    rv = prefBranch->GetCharPref("autoadmin.global_config_url",&t);
+    if(NS_FAILED(rv)) return NS_OK; //Return ok if there is no config url set.
+    
+    // Converting the char * to nsCString class for efficient string
manuplation
+    if (t) {
+        cfgUrl = t;
+        PR_Free(t);
+        t = nsnull;
+    } 
+    else return NS_OK; // There is no cfg URL set so no need to go further.

You're doing a lot more work than you need to. use nsXPIDLCString, and
getter_Copies(); - and what kind of a variable name is "t" - something more
descriptive, please :)

also, you have a giant
+ if (cfgUrl) {
+ ...
+ }
block followed by return NS_OK;

Normally, I would say turn this into

> if (cfgUrl.IsEmpty()) return NS_OK;

to avoid unnecessary indentation

but the thing is, you've already checked if the string has a value... so this
if() check will always succeed.

also, cfgUrl is a nsCString - seems like you should be able to use
nsLiteralString here, if you use nsXPIDLCString.
i.e.

> rv = prefBranch->GetCharPref("autoadmin.global_config_url",getter_Copies(t));
> if(NS_FAILED(rv)) return NS_OK; //Return ok if there is no config url set.
> 
> nsLiteralCString cfgUrl(t);


emailAddr is a nsCStrings, it should be an nsCAutoString (or is it
nsAutoCString? I can't remember) - local variables should always use the "Auto"
variety.


in OnDataAvailable:
+    while (aLength) {
+        
+        rv = aIStream->Read(buf, aLength, &amt);
+        if (NS_FAILED(rv)) {
+            NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv), 
+                         "The stream should never block.");
+            PR_FREEIF(buf);
+            return readOfflineFile();
+        }

do you really mean to read the offline file every time a read fails? seems like
you should record the error, and read the offline file after you've exited your
event loop... or maybe you're already doing that in OnStopRequest() - need to
straighten that out.

Also in OnDataAvailable, you're allocating a big buffer every time, and then
looping through calling Read() - why not use a fixed size buffer that's
allocated on the stack? 

the use of mBytesRead - isn't this ALWAYS equivalent to mBuf.Length()?

this:
+    // Send the autoconfig.jsc to javascript engine.
+    if (PREF_EvaluateConfigScript(mBuf.get(),
+                                  mBytesRead,
+                                  nsnull,
+                                  PR_FALSE,
+                                  PR_TRUE,
+                                  PR_FALSE)) {
+        rv = writeFailoverFile(); // Write the autoconfig.jsc to failover.jsc
(cached copy)
+        if(NS_FAILED(rv)) 
+            NS_WARNING("Error writing failover.jsc file\n");
+    }

is confusing

instead of

> if (some_really_long_call(with, 
>                           lots, 
>                           of, 
>                           parameters)) {
> 
> }

use a local temporary variable - the compiler will optimize it out and it will
be easier to read:

> PRBool success = some_really_long_call(with, lots, of, parameters)
> if (success) {
> 
> }

etc
- Is it ok to use a label and bunch of goto statements to make sure 
PopThreadEventQueue being executed in all error cases?

- Regarding string manuplations, what would be the best way? I tried 
nsLiteralCString but it doesn't allow Append on it. The need is to pass it as a 
char* to the pref functions to get it initialized and should be able to append 
otehr strings to it. nsXPIDLCString also doesn't allow append.

Other corrections will be in the next diffs.
new version of nsAutoConfig.cpp

- Used a label and goto statments to make sure PopThreadEventQueue gets called 
in all cases

- changed char * to nsXPIDLCString class but didn't change nsCString to 
nsLiteralCString. Later doesn't allow string manipulations.
you're right about the nsLiteralCString stuff - I forgot that you were modifying
the string.


Everything looks good - the one problem I see is that if you fail with
PREF_EvaluateConfigScript, you'll never set mLoaded to true, and thus never get
out of your event loop! So if you somehow tget a bad config file that isn't
valid JS, it will just hang on startup. There may be similar pitfalls that I
haven't noticed...
There are two places PREF_Evaluate can fail,
First from onStopRequest, it will call readOfflineFile(), that will take care of 
mLoaded and second from evaluateLocalFile(), in that case when it 
returns to readOfflineFile(), reportError() will take care of setting mLoaded to 
true.
I understand that if mLoaded will not set to true in any of the possible path, 
the code will just hang on startup. I have tried my best to set it through all 
possible code paths.

Thanks for the review.
My thoughts:
Both NS_WITH_SERVICE and AddObserver can return an error. You should probably be
doing these in an Init function (i.e. use NS_GENERIC_FACTORY_CONSTRUCTOR_INIT)
rather than in the constructor.

While you're in there, I would also get the prefBranch, once, into a member
variable. Also use prefService->GetBranch(nsnull) rather than doing the QI this
will get you a real branch reference rather than a re-directed one.

In your destructor I believe you should be calling RemoveObserver() and/or
deriving yourself from nsSupportsWeakReference.
Posting a latest set of changes including:

- Moved the initialization stuff from constructor to Init() and changed the 
generic factory constructor macro to include init
- Initialized prefs branch in a member variable 
- Added RemoveObserver() to the destructor.
- Not deriving from nsSupportsWeakReference. It is causing the object to get 
deleted at the first Observe() call. The other way would be to add a "service" 
word to the module name in the AddCategory() method in nsPrefsFactory.cpp. 
- Removed Push/Pop from the event loop code. It was casuing the Linux build to 
hang and for this event loop it should be fine (talked to Danm about it)
Attached patch latest diffsSplinter Review
r=bnesse.
Wow. Big bug. I just thought I'd also mention that NS_WITH_SERVICE has long been 
deprecated in favour of nsCOMPtr...do_GetService. But they're both basically the 
same thing, so I don't know why I'm complaining.
NS_WITH_SERVICE is depricated? Where was the memo? Oh well, I guess I have some
code to clean up...
don't worry about it - it used to be a complicated macro, but it's been
redefined just to be do_GetService()
New version of nsAutoConfig with following minor changes:

- Added an extra check for httpStatus since this is primarily going to be used 
as http request, might as well add an extra check, instead sending the erroneous  
data to javascript.
- Moved the event code under the first_time case, We actually, need to stall the 
thread only on the startup, later on when the timer kicks in, it's fine if it's 
asynchronus.

Requesting r & sr

Thanks,
Opps. Please fix utilityOverlay.js
(mozilla/xpfe/communicator/resources/content/) to use nsIPrefService and
nsIPrefBranch rather than nsIPref. And, I guess, change the NS_WITH_SERVICE
macros to do_CreateInstance. No point in checking in depricated code.

Changes as per Brian's review

- Removed NS_WITH_SERVICE macros, replaced with do_GetService

- changed utilityOverlay.js to use nsPrefService and nsIPrefBranch

Requesting r & sr ,

Thanks.
Attached patch latest diffsSplinter Review
r=bnesse
Jumping in with some review, more later:

+    nsIInputStream *inStr;

Use an nsCOMPtr here...

+    
+    rv = NS_NewLocalFileInputStream(&inStr, file);
+    if (NS_FAILED(rv)) return rv;
+        
+    PRInt64 fileSize;
+    PRUint32 fs, amt=0;
+    file->GetFileSize(&fileSize);
+    LL_L2UI(fs, fileSize); // Converting 64 bit structure to unsigned int
+    char*  buf = (char *) PR_Malloc(fs*sizeof(char)) ;
+    if(!buf) 
+        return NS_ERROR_OUT_OF_MEMORY;

... or you'll leak if you return this error here...

+    
+    rv = inStr->Read(buf, fs, &amt);
+    if (NS_FAILED(rv)) 
+        NS_ASSERTION((NS_BASE_STREAM_WOULD_BLOCK != rv), 
+                     "The stream should never block.");
+    
+    if (!PREF_EvaluateConfigScript(buf,fs,nsnull, PR_FALSE, PR_TRUE, PR_FALSE))
+        return NS_ERROR_FAILURE;

... or here -- AND you leak buf here, don't forget to free it! ...

+    inStr->Close();
+    PR_FREEIF(buf);
+    return NS_OK;

No NS_RELEASE(inStr), so you always leak that.

+    nsIOutputStream *outStr;

nsCOMPtr please...

+    PRUint32 amt;
+
+    rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, 
+                                getter_AddRefs(failoverFile));
+    if (NS_FAILED(rv)) return rv;
+#ifdef XP_MAC
+    failoverFile->Append("Essential Files");
+#endif
+    
+    failoverFile->Append("failover.jsc");
+    
+    rv = NS_NewLocalFileOutputStream(&outStr, failoverFile);
+    if (NS_FAILED(rv)) return rv;
+    rv = outStr->Write(mBuf.get(),mBuf.Length(),&amt);
+    if (NS_FAILED(rv)) return rv;

...else you leak on the early return after Write failure.

+    outStr->Close();

... and then you return without NS_RELEASE(outStr), so it's always leaked.

Should you close even if the Write fails?  If so, just remove the rv failure
check and early return one-liner after the Write call.

+    return NS_OK;

In general, don't use nsCAutoString as a formal parameter type in XPCOM-like
methods.  I know, getEmailAddr is protected, but this is a pattern to avoid.  Is
it useful here to declare the narrowest type?  There's only one call that I can
see, so it "doesn't hurt" -- but could there be more calls in the future?

+nsresult nsAutoConfig::getEmailAddr(nsCAutoString & emailAddr)
+{
+
+    nsresult rv;
+    nsXPIDLCString prefValue;
+    nsCAutoString prefStr;
+    
+    /* Getting an email address through set of three preferences:
+       First getting a default account with 
+       "mail.accountmanager.defaultaccount"
+       second getting an associated id with the default account
+       Third getting an email address with id
+    */
+    
+    rv = mPrefBranch->GetCharPref("mail.accountmanager.defaultaccount", 
+                                 getter_Copies(prefValue));
+    // Checking prefValue and its length, too. Since by default the preference 
+    // is set to nothing
+    if (NS_SUCCEEDED(rv) && prefValue && nsCRT::strlen(prefValue) > 0) {

Where did this pattern of checking both NS_SUCCEEDED(rv) && prefValue arise?  It
should not be necessary for string (Char) prefs, and it only adds cost and doubt
(at the limit, covering up a bug in GetCharPref where an allocation fails but
NS_ERROR_OUT_OF_MEMORY is not returned).

+        prefStr = NS_LITERAL_CSTRING("mail.account.") + 
+            nsLiteralCString(prefValue) + NS_LITERAL_CSTRING(".identities");

Good use of nsLiteralCString -- stylistically, it would be a little better to
use nsLocalCString(prefValue).

+        rv = mPrefBranch->GetCharPref(prefStr,getter_Copies(prefValue));
+        if (NS_SUCCEEDED(rv) && prefValue && nsCRT::strlen(prefValue) > 0) {
+            prefStr = NS_LITERAL_CSTRING("mail.identity.") + 
+                nsLiteralCString(prefValue) + NS_LITERAL_CSTRING(".useremail");

A-indenting we will go.  Why not just return rv; early if (NS_FAILED(rv) ||
*prefValue == '\0'), for both these cases?

+            rv = mPrefBranch->GetCharPref(prefStr,getter_Copies(prefValue));
+            if (NS_SUCCEEDED(rv) && prefValue && nsCRT::strlen(prefValue) > 0)
{
+                prefStr = nsLiteralCString(prefValue);

nsLocalCString.

+            }
+            else 
+                return rv;
+        }
+        else 
+            return rv;
+    }

else-return clauses are a sign that you should consider inverting the sense of
the if condition and return early in the revised if's then clause.

+    else {
+        if (mCurrProfile) {
+            prefStr=mCurrProfile;

How about a space on both sides of that =?

+        }
+    }
+    
+    if(prefStr) emailAddr = prefStr;

So (reversing myself slightly to allow for the nsCAutoString& parameter) why
didn't you just compute the result in emailAddr, and avoid another nsCAutoString
and a byte-copy here?

+    
+    return NS_OK;
+}

Use the proper modeline parameters here:

+++ nsAutoConfig.h      Thu May  3 15:56:29 2001
@@ -0,0 +1,63 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

to wit: tab-width: 2 and c-basic-offset: 2 -- or else change the class's
indentation to use 4 space basic offset.  Of course, the existing
modules/libpref/src .cpp files use a basic offset of 2, so When In Rome argues
for that in the new files you're adding -- unless you want to move those to a
separate directory?

In utilityOverlay.js, is all the if-testing necessary?  Why not just let the
hard case of a service manager error cause an exception to be thrown?  Testing
prefService, prefBranch, etc. seems unnecessary to me.

+  if (!offlineLocked ) {
+      if (aToggleFlag)
+          ioService.offline = !ioService.offline;
+  }
+  else {
+            broadcaster.setAttribute("disabled","true");
+  }

Indentation is whacked in the else clause.

/be
Might want to withdraw that r=bnesse!

/be
Made changes according to Brendan's suggestions. Alecf gave a sr over AIM
Still need to be tested on Linux and Mac before checking in.
Attached patch latests diffsSplinter Review
Modeline should use 4, not 2, for c-basic-offset at least:

+++ nsAutoConfig.cpp    Thu May  3 18:59:00 2001
@@ -0,0 +1,493 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-

tab-width doesn't matter in light of indent-tabs-mode unless there are still
tabs in the file that stand for 4-space indentation units, in which case it too
should be 4.

Why is buf 1025 bytes long?

+    char buf[1025];

else after return is a non-sequitur, and incongruous in light of early returns
above it in the same function:

+        return NS_OK;
+    }
+    else {
+        NS_WARNING("Error reading autoconfig.jsc from the network, reading the
offline version");

What happened to the indentation/brace style here?

+NS_IMETHODIMP nsAutoConfig::Observe(nsISupports *aSubject, const PRUnichar
*aTopic, const PRUnichar *someData)
+{
+    nsresult rv = NS_OK;
+    if (!nsCRT::strcmp(aTopic,
NS_LITERAL_STRING("profile-after-change").get()))
+        {
+            // Getting the current profile name since we already have the 

The rest of the file seems to put open braces on the same line as their if,
while, etc. keywords.  Anyway, the substance of the then clause (starting with
the comment // Getting the current profile name...) should not be indented yet
again.  Also, there's another else after return just below:

+            return DownloadAutoCfg();
+        }  
+    else if (!nsCRT::strcmp(aTopic, NS_LITERAL_STRING("app-startup").get())) 

Just above this (above the 'return DownloadAutoCfg();') is this:

+                if (NS_SUCCEEDED(rv)) 
+                    // setting the member variable to the current profile name
+                    mCurrProfile.AssignWithConversion(profileName); 
+                else 
+                    return rv;
+            }

The if should test NS_FAILED(rv) and return early, not NS_SUCCEEDED -- that way,
you don't overindent the mCurrProfile.AssignWithConversion(profileName); and it
flows into the subsequent return DownloadAutoCfg(); more clearly.

Also, why is mCurrProfile a nsCString, while the only source of it,
nsProfileAccess::GetCurrentProfile, returns an nsString?  Use the right type or
non-ISO-Latin-1 characters will be lost (or if GetCurrentProfile is abusing the
nsString type, file a bug on it).

In DownloadAutoCfg, you test firstTime early, presumably to avoid hanging if
there's some unlikely fatal error getting the event queue service or queue:

+    if (firstTime) {
+        nsCOMPtr<nsIEventQueueService> service = 
+            do_GetService(NS_EVENTQUEUESERVICE_CONTRACTID, &rv);
+        if (NS_FAILED(rv)) return rv;
+        rv =
service->GetThreadEventQueue(NS_CURRENT_THREAD,getter_AddRefs(currentThreadQ));
+        NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+    }
+    mLoaded = PR_FALSE;

That's ok, but why then set mLoaded to PR_FALSE unconditionally?  It was already
set false in Init, so it shouldn't be necessary to clear it again if firstTime.
And if !firstTime, then what will set it true?  Is DownloadAutoCfg meant to be
callable at any time?

In getEmailAddr, I suggested testing *prefValue == '\0' after NS_FAILED(rv) ||
in order to throw out bad string (Char) pref values, but you kept the
nsCRT::strlen() calls -- not a big deal, except you mean == 0, not < 0, here and
later:

+        if (NS_FAILED(rv) || nsCRT::strlen(prefValue) < 0) return rv;

In utilityOverlay.js, why try/catch?

+  } catch(e) {
+  }

This suppresses any exception printing, which would be a useful clue as to why
things went wrong in the rare case that you couldn't get the service or the null
pref branch, or whatever.  Let the chips fall where they may, get rid of all try
with empty catch clause instances.  Or is there a bug where you sometimes get an
XPCOM failure result (that turns into a JS exception)?

/be
Closing this as RESOLVED. 
(This is basically for the Feature Development)

Open new bugs for the improvement.

Thanks, Mohan.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Mohan, I asked mitesh some questions (non-trivial ones included those concerning
the decimation from nsString to nsCString, the utterly broken testing of strlen
< 0, and the DownloadAutoCfg non-firstTime logic).  He hasn't answered, and may
not if the bug stays closed.  It was assigned to him, I think he should answer
my questions, whether here or in new bug(s).  Your closing the bug when it is
not assigned to you, and when it has open code review questions pending, is bad
form.

I don't think this code got a proper code review.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Brenden,

I would be working on AutoConfig code for any escalations, while Mitesh is on
vacation till mid June.  

Thanks, Mohan.
Mohan: Since this bug has useful information still alive in it, rather than try
to copy it to a new bug, can you please reassign to yourself?

/be
Assigning myself. -mohan.
Status: REOPENED → ASSIGNED
Assigning myself. -mohan.
Assignee: mitesh → mohanb
Status: ASSIGNED → NEW
I will correct the tab width and indentation in the next revision of the file. 

Regarding the size of the character buffer, I copied it from an example in a 
test directory in netwerk module. What should be the right one then? 

I will make the code consistent to have "if(fail) return rv" format. 

Regarding mCurrProfile being nsCString, I think all other string operation are 
of C format string so I kept it nsCString and when I got the profileName in 
nsString format I converted it to nsCString. I need nsCString to append it to 
another nsCString. I will see what's better and efficient way but I think one 
conversion will sure be needed. 

setting of mLoaded to PR_FALSE is redundant, will remove it. mLoaded is not 
needed  after the first time. Also, mLoaded is being set to true through all 
possible code paths to avoid any dead lock(mainly from readOfflineFile() and
OnStopRequest()) yes, DowLoadAutoConfig is callable at any time, the reason why 
the event queue is implemented first time is because it's only necessary to 
enforce sync read during the startup, all other times, it's ok to have
asyncread and firstTime is already available for Timer implementation so no 
extra cost. 

yes, strlen(prefValue) < 0 testing is wrong, it should be ==0 or the better way 
would be to test it again a null character. I will change it in the next 
revision. 

Thanks for the review. 
  
>Regarding the size of the character buffer, I copied it from an example in a 
>test directory in netwerk module. What should be the right one then? 

1024 is a power of two and (more imp., no big deal still) is 0 mod 4. 1025
wastes a few bytes of stack on alignment padding.  Why not be traditional and
use 1024?  Nit, minor, still worth doing.  Maybe the example you copied from
wanted to put a '\0' terminator after reading at most 1024 bytes, so
overallocated?  Better style in that event would be to use 1024+1 or MAXLEN+1
with a manifest constant, and a comment.

>Regarding mCurrProfile being nsCString, I think all other string operation are
>of C format string so I kept it nsCString and when I got the profileName in
>nsString format I converted it to nsCString. I need nsCString to append it to 
>another nsCString. I will see what's better and efficient way but I think one 
>conversion will sure be needed. 

The profile name comes back in a PRUnichar* (wrapped in an nsXPIDLString), so
you should assume it can contain non-ASCII, non-ISO-Latin-1 characters for some
users, and never chop PRUnichars down to chars via AssignWithConversion -- don't
do it unless you know that GetProfileName uses too wide a string type (if you
know that, file a bug on that method).  There's no great efficiency difference,
and all string operations available on chars are available on PRUnichars via
mozilla/string/* facilities.

/be
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Priority: -- → P2
reassigning to mitesh;
Assignee: mohanb → mitesh
I have made few improvements  in the next patch as per Brendan's suggestions. 
mCurrProfile is still nsCString. I want to add the profilename as a part of the
URL. So even if I use nsString, it will be converted to a CString somewhere in
order to add it to the URL. 
Whiteboard: Requesting r and sr
Attached patch few improvementsSplinter Review
>mCurrProfile is still nsCString. I want to add the profilename as a part of the
>URL. So even if I use nsString, it will be converted to a CString somewhere in
>order to add it to the URL. 

Yes, but *how* will the PRUnichar string be converted to a char string?  Will it
be converted to UTF-8?  AssignWithConversion does not do that -- it just chops
16-bit chars down to 8-bit chars mercilessly.

/be
Oh! I thought AssignWithConversion will take care of it.  Ok I will use
NS_ConvertUCS2toUTF8() 
Status: NEW → ASSIGNED
Changes at line  @@ -280,7 +275,7 @@  in the previous patch is actually for bug
84559


Requesting r & sr for the new patch.

Thanks,

In the methods:
Init()
Observe()
DownloadAutoCfg()

you are initializing rv to NS_OK. This is unnecessary as rv is always set before 
being tested.

In Observe() the else case 
    else if (!nsCRT::strcmp(aTopic, NS_LITERAL_STRING("app-startup").get())) {
      // This is the object instantiation, do nothing and return NS_OK;
      return NS_OK;
    }
serves no apparent purpose as NS_OK is returned anyway.

I would prefer to see mLoaded set to PR_TRUE once at the top of 
readOfflineFile(), rather than at each NS_OK return point and in reportError(). 
It's safer and easier to follow.

In evaluateLocalFile() you probably shouldn't pass "buf" to 
Pref_EvaluateConfigScript() if the call to Read() failed. Also the PR_FREEIF 
macro is overkill for a buffer you know a) is valid, and b) won't outlive the 
function. Also, you don't need to check NS_FAILED(rv) before the NS_ASSERTION... 
it won't fire unless it failed anyway.

minor nits:
Please stick to one comment style... don't alternate between C & C++ styles.
Please be consistent in your use of spaces [i.e. "if(" and "//Comment" vs. "if (" 
and "// Comment"]

This just makes the code easier to read.
PDT+ per 6/12 mtg.
Whiteboard: Requesting r and sr → [PDT+] Requesting r and sr
Made changes according to the review.

- Removed reportError() and put the mLoaded=PR_TRUE in the beginning of 
readOfflineFile(). Actually, at this point we really don't need to block the 
main thread so allowing it to start execution while we are reading the offline 
file.

- Removed rv=NS_OK since it was unnecessary.

- Removed "app-startup" category checking from Observe().

- Replaced PR_FREEIF with PR_Free.

- Improved comment & coding style.
Attached patch updated patch.Splinter Review
In Init()
+    // member initializers and constructor code */
Creating a new comment style? :)

+    nsCOMPtr<nsIObserverService> observerService =
+        do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv);
+    if (NS_FAILED(rv)) return rv;
    
+    if (observerService) {
+       rv = observerService->AddObserver(this, 

if rv was NS_OK, you shouldn't need to check if (observerService) here.

You have some wierd indentation in OnStopRequest and Observe. Did you "ignore 
white space" on this diff? (I know you mentioned cleaning up indentation)

In evaluateLocalFile() you're going to leak buf if the Read fails. In order to 
eliminate yet another PR_Free call, I'd recommend:

rv = inStr->Read(buf, fs, &amt);
if (NS_SUCCEEDED(rv)) {
    if (!PREF_EvaluateConfigScript(buf, fs, nsnull, PR_FALSE, PR_TRUE, PR_FALSE))
        rv = NS_ERROR_FAILURE;
}
inStr->Close();
PR_Free(buf);
return rv;
Oops, forgot to remove '*/'

yes, I ignored the white space for this diff. 

I think, using NS_SUCCEEDED for evaluateLocalFile seems a good idea. 
 
posting new diffs without '-w' option. 
Attached patch updated diffsSplinter Review
the use of rv in 
+NS_IMETHODIMP nsAutoConfig::Observe(nsISupports *aSubject, 
+                                    const PRUnichar *aTopic, 
+                                    const PRUnichar *someData)
+{
+    nsresult rv;
+    if (!nsCRT::strcmp(aTopic, 
+                       NS_LITERAL_STRING("profile-after-change").get())) {
...
+        return DownloadAutoCfg();
+    }  
     return NS_OK;
 }
appears a bit unusual, if you aren't going to use rv outside the |if| then i'd 
suggest moving it inside, however i'm sure no one would like that solution.

oh, an alternative which you might consider to the preceding code (this does 
not address my complaint):
    nsresult rv;
    if (nsCRT::strcmp(aTopic, 
                      NS_LITERAL_STRING("profile-after-change").get()))
        return NS_OK;
    /*... body of older if block*/
    return DownloadAutoCfg();

you use this syntax a lot:
if (NS_FAILED(rv)) return ...;
unfortunately, it makes it hard to place a breakpoint on |return ...;| would 
your module owner consider allowing you to split them?
I should note that code you've reindented uses the multiline format.

converting from /* long descriptive multiline comment block */ to // for each 
line in a long comment block is rather unappealing.  

I'm guessing this was in response to Comments From Brian Nesse 2001-06-12 16:58 
I think his comments were misinterpretted.

-    // failCache = true 
+    // failCache == true 
since this isn't a code, could you use words?
Personally, I'm not a big fan of:

if (NS_FAILED(rv)) return ...;

I won't complain at all if it changes.

And, yes,
/*
 * comment block
 */
 is ok. I was really complaining about the inline use.
if you're not a fan, you can always use
if (NS_FAILED(rv))
    return rv;

then it's possible to break on the second line

if it's something you really want to debug every time it happens, you can use
NS_ENSURE_SUCCESS(rv, rv);

which will throw an assertion if rv is a failure - you can catch it in the
debugger.. in release builds, it'll just return rv on failure.
Ok, replaced if (NS_FAILED(rv)) return rv; with
if (NS_FAILED(rv)
    return rv;
for easy debugging.

- reverted back the comment blocks to /*..*/ style. I misunderstood the comment
by bneese.

- Improved comment for 'failcache == true' case.
ok, time for me to sound totally ignorant :-(

// Releasing the deadlock
what does that mean??  In normal terms once you have deadlock you're sunk and 
you can't possibly do anything to fix it.

i still don't like (again no one cares):
+        return DownloadAutoCfg();
+    }  
     return NS_OK;
because you could easily have done:
+    nsresult rv=NS_OK;
     if (...){
...
+        rv=DownloadAutoCfg();
+    }  
-    return NS_OK;
+    return rv;

which saves a return and makes consistent use of rv.
Seems reasonable to me. I would also make the same case at the bottom of Init()

 rv = prefs->GetBranch(nsnull,getter_AddRefs(mPrefBranch));
+    if (NS_FAILED(rv)) 
+        return rv;
     return NS_OK;

could just be:
rv = prefs->GetBranch(nsnull,getter_AddRefs(mPrefBranch));
return rv;

The "deadlock" reference is caused in DownloadAutoConfig where it loops while
!mLoaded. Perhaps the comment should refer to releasing from that loop instead?
Points noted. will post the updated patch later on at once. 
any more suggestion? 
this needs to be checked in before Tuesday. (0.9.2 deadline)
timeless: "return" is not expensive - there's no reason to "save" them, and the
compiler will optimize out unused local variables like rv..(or optimize them out
once the function isn't using it any more) this is really a personal preference
issue - mitesh, do whatever you prefer. 
re: return whatever, consistency is nice and bnesse seemed to agree.

i think i'm done. sorry to delay. however i'm not sure i like your sense of 
urgency in 'this needs to be checked in before Tuesday. (0.9.2 deadline)'

but that's because i'm jaded by dealing w/ bug 77914 which has more status, 
keywords, dupes, ccs, and votes than I can shake a stick at. Again i'm really 
sorry to delay your patch (wow this bug is 27 pages while that one is only 17).
Attached patch latest patchSplinter Review
For consistency, please change the 2
  if (NS_FAILED(rv) || (len = nsCRT::strlen(prefValue)) == 0) return rv;
references in getEmailAddr() to be multi line.

With that change r=bnesse.
sr=alecf with brian's comments
Whiteboard: [PDT+] Requesting r and sr → [PDT+] , r and sr done, requesting drivers' approval
Blocks: 83989
adding nsenterprise keyword
Keywords: nsenterprise
a=dbaron for 0.9.2 checkin of attachment 39249 [details] [diff] [review] (on behalf of drivers).  Although
this isn't normally the type of thing we're looking for in a stabilization
period before a release, you seem to want to get it in, the only changes to
things other than handling of error conditions seem to be minor ones (to when
you set mLoaded and under what conditions you call PREF_EvaluateConfigScript),
and almost all the changes seem to be only in the codepath of a feature that's
not enabled by default.

However, please note that drivers@mozilla.org has the power not to approve bugs
(even when developers and their managers repeatedly send email requesting such
approval) -- otherwise there would be no point to the approval process.

Also, if you'd said that the entire patch was code cleanup in your initial
message to drivers, I wouldn't have instinctively replied to your one-line
request asking for more details after I saw the 600 line patch.  (Under the
pressure of large numbers of approval requests, members of drivers@mozilla.org
don't have the time to read the entire bug for lengthy bugs such as this one,
especially at 17:30 on the last day before the milestone freeze.)
Thanks for your help dbaron!

Mitesh, please checkin asap.
Whiteboard: [PDT+] , r and sr done, requesting drivers' approval → [PDT+] ready for checkin
Fix checked in to the trunk and the branch
Status: ASSIGNED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] ready for checkin → Checked in
marking it fixed
Verified this has been fixed in 0626 trunk builds on MacOS, Win32, and Linux.
Status: RESOLVED → VERIFIED
I have also verified that this functionality is present on the 0625 branch 
build.
You need to log in before you can comment on or make changes to this bug.