Closed Bug 70348 Opened 24 years ago Closed 23 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)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), text/plain
Details
(deleted), text/plain
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), text/plain
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), 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.
Attached patch AutoConfig Module changes (deleted) — Splinter Review
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?].
Attached patch Unified context diff. (deleted) — Splinter Review
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 nsAutoConfig.h (class declaration) (deleted) —
Attached file Class Implementation (deleted) —
Attached patch Changes to other files (deleted) — Splinter Review
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.
Attached patch Changes to disable network icon (deleted) — Splinter Review
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.
Attached patch Changes to the existing files (deleted) — Splinter Review
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 (deleted) —
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"
Attached patch latest set of diffs (deleted) — Splinter Review
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.
Attached patch diffs with the latest tree (deleted) — Splinter 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 diffs (deleted) — Splinter 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,
Attached file new version of nsAutoConfig.cpp (deleted) —
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 diffs (deleted) — Splinter 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 diffs (deleted) — Splinter 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: 24 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 improvements (deleted) — Splinter 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. (deleted) — 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 diffs (deleted) — Splinter 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.
Attached patch new patch with improvements (deleted) — Splinter Review
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 patch (deleted) — Splinter 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
Attached patch patch with suggested changes (deleted) — Splinter Review
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: 24 years ago23 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.

Attachment

General

Creator:
Created:
Updated:
Size: