Closed Bug 75955 Opened 24 years ago Closed 23 years ago

Implement a mechanism for getLDAPAttributes() functionality

Categories

(Directory :: LDAP XPCOM SDK, enhancement, P3)

x86
All
enhancement

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: leif, Assigned: mitesh)

References

()

Details

(Whiteboard: [PDT+] Checked into the branch - the bug is open for trunk solution)

Attachments

(12 files)

We need to support the getLDAPAttributes() javascript function, as used in autoconfig.jsc (in v4.x). Like value = getLDAPAttributes(host, base, filter, attrib); I'd also like to add value = getLDAPAttributesURL(url); which is both cleaner, and more efficient. In order to do this, we'd also fix the bug in nsLDAPURL, where it doesn't implement the attributes handling properly.
Status: NEW → ASSIGNED
Priority: -- → P3
Assignee: dmose → leif
Status: ASSIGNED → NEW
Hmmm, this is my bug, give it back
I thought I created the bug 75229 for this. Anyway, will go ahead and mark it as a duplicate
*** Bug 75229 has been marked as a duplicate of this bug. ***
Depends on: 70422, 70611
Status: NEW → ASSIGNED
Blocks: 17880
Target Milestone: --- → mozilla0.9.2
reassign to Olga as QA contact
QA Contact: yulian → olgac
Target Milestone: mozilla0.9.2 → mozilla1.0
There are two issues with LDAP - The new LDAP XPCOM is async in nature and we want this function to be sync so that we have the preferences before we start the browser. - The second issue is related to prefs JS Context. Right now, autoconfig.jsc is executed in prefs JS context and it is not XPCOM enabled. (i.e. no Components interface available). We probably need a new context or prefs JS context has to initialize XPCOM interface. untill we solve above issues, I am proposing a workaround using C-sdk bypassing LDAP XPCOM wrapper which is basically going back to 4.x way. patch in the next post.
OS: Linux → All
This is a required feature for eClient deployment (parity feature with 4.x) so needs to be checked into trunk. May be a candidate for limbo build. Requesting review and super review. The XPCOM version doesn't have a sync interface and the event queue stalling is not working (we are already in the event queue - stalling for autoconfig.) so I have requested a Sync version of LDAP XPCOM
Assignee: leif → mitesh
Status: ASSIGNED → NEW
Whiteboard: Requesting r & sr
ok, I talked with mitesh on this: here's the deal. The patch, as it stands, cannot be checked into the trunk - it introduces a link-time and runtime dependancy that will break the embedding builds. This means that this patch, as it stands, cannot be checked into the trunk under any circumstances. the only option we have right now to land this on the trunk is to expose synchronous LDAP in the LDAP interfaces, and then go through XPCOM to get to LDAP - this ensures that the LDAP dll is not required to start the browser or the embedding client. Mitesh will look into exposing this since dmose is on vacation, that way we can check into the trunk.
The new patch uses XPCOM LDAP to get preferences. JS to C translation is done in prefapi.c, so prefs conext is not XPCONNECT enabled. EventQ is added to make the LDAP operation act synchronously. Requesting a review.
Status: NEW → ASSIGNED
Attached patch patch using XPCOM LDAP (deleted) — Splinter Review
I've reviewed as best I can, but I don't have much experience with the prefs directly nor with JSAPI, so you should probably also get review from someone who does. Comments: Does the stuff in prefldap.h really need to be exported at all? The only place I see it referenced outside of there at all is http://lxr.mozilla.org/mozilla/source/lib/mac/AutoAdmin/AutoAdmin.exp#2 Is that AutoAdmin stuff still used or useful? If not, should the decl from prefldap.h instead be moved to some existing file under src and prefldap.h be |cvs remove|d from public? For brand new files, how about wrapping them at column 78 or so for readability? As far as I can tell, every instance of the word "lock" in the patch refers to a variable or param which is not a lock (ie PR_Lock or nsAutoLock). Can you rename those variables and params for better readability? The builds need to continue to work if MOZ_LDAP_XPCOM is not defined (Unix), DISABLE_LDAP is defined (Windows), or 'options ldap' is not set (Mac), in part because an embedder might want prefs but not LDAP. See the LDAP autcomplete stuff in xpfe/components/{autocomplete,build} for an example of how to do this. nsIPrefLDAP.idl --------------- Please add doxygen-style comments to the interface, methods, and attributes in nsIPrefLDAP. Examples of these can be found in the same place. Rather than making methods that have a single out param and returning void on IDL methods, how about turning getResults, and isDone into IDL readonly attrs, something like this: readonly attribute string results; readonly attribute boolean isDone; Any particular reason the nsIPrefLDAP interface isn't marked as scriptable? pref_get_ldap_attributes ------------------------ It looks like |errorMesg| is being declared on the stack, and then returned. I _think_ this means that by the time the caller reads the string, the string may have been thrown away and overwritten. Making it a |const char *| _might_ fix this problem, but I'm not sure. What's the ownership model of the string returned by pref_get_ldap_attributes anyway? Is the caller going to try and free() it? I'm surprised that aResult is a char ** and UTF8 encoded. Since it's ultimately being returned back to JS, I was expecting PRUnichar ** and UCS2 encoding. I assume I'm missing something?
- Initially, I removed the prefldap.h and moved the declaration to prefapi.c but somehow later thought not to change the prefapi.c. But removing prefldap.h makes more sense since no one else is using it except prefapi.c - Will correct the wraping of new files to 78 and change the variable name from lock to something more meaningful. - I saw the MOZ_LDAP_XPCOM ifdefs but didn't know why they were there, will change it in the next patch. - nsIPrefLDAP isn't marked scriptable because I didn't see a need for it right now. - Regarding pref_get_ldap_ttributes, yes, caller will free the strings as you can see in prefapi.c. Also, aResults are char * becaue I haven't changed the old implementation of perf_get_ldap_attributes in prefapi.c. Should we change it? what do you suggest? Thanks for the review. Brian, could you please review it from the prefs point of view?
In general: I believe that pref_get_ldap_attributes() should be changed to not return a string. As dmose pointed out, you're returning bogus memory anyway. The only purpose for this string is so that it can be displayed by Pref_Alert(), which you shouldn't be using anyway. I don't see the need for the inclusion of nspr.h in nsPrefLDAP.cpp, it isn't needed (it's also included in nsLDAP.h and isn't needed there either... but that's not your fault.) Optimally I'd prefer to see the (errCode != nsILDAPErrors::SUCCESS) become a (errCode != NS_OK) check so you could remove the inclusion of nsLDAP.h also. Specific to prefs: It seems that code you have removed allows for the removal of more... pref_NativeGetLDAPAttr() is the only consumer of both |gAutoAdminLib| and pref_LoadAutoAdminLib(). This also allows the removal of the #include "prlink.h". Even better would be if you could get pref_NativeGetLDAPAttr() out of prefapi.c altogether.
bnesse: so if pref_Alert is bad news, what should be used to provide error feedback to the user? As far as nsILDAPErrors::SUCCESS goes, that needs to stay, because these error codes are not nsresults. Instead of including nsLDAP.h, though, nsILDAPErrors.h could be included directly. However, I think you're onto something w.r.t. nsLDAP.h. After thinking about it a bit, I no longer think there's a real need for nsLDAP.h at all. I've filed bug 92650 on that.
Updated patch. - The event queue mechanism was failing once the browser starts so added a callback to execute a function in JS to process LDAP values. Still need to do a security review with Mstoltz - Removed prefldap.h and other autoadmin code from prefapi.c, prefapi.h and macpref.cp - included other changes based on the reviews.
Attached patch updated patch (deleted) — Splinter Review
Well, that's kind of the problem...it's sort of undefined. All of the prefs stuff has been written to percolate the error back up to the caller... thus making it callers responsibility to do this. The theory is that some XUL based method should be used rather than the platform specific pref_Alert(). alecf (on sabatical) was the driving force behind this.
Adding Seth for Super Review
Keywords: nsenterprise
I had a long discussion with mitesh on this... to condense: - remove the unused definition of ldap_func above pref_NativeGetLDAPAttr(). - use the accessors to get gMochaContext and gMochaPrefObject instead of adding PREF_CallJSFunction() to prefapi.c
* there's probably a way to append faster than just calling mResults.Append four times in a row, since this could conceivably cause more than one realloc() to happen. Also the ToNewUnicode isn't necessary, I don't think, because appending is going to copy anyway. I'm thinking of something along the lines of: mResults += NS_LITERAL_STRING('\n') + NS_ConvertASCIItoUCS2(mAttrs[i]) + NS_LITERAL_STRING("= ") + nsDependantString(vals[j]); I'm adding jag to the CC on this bug, as he can confirm, deny, or improve this suggestion. :-) * doxygen comments need to start like this: /** * in order to get picked up by doxygen. The IDL as written is missing the second star on the first line. * any particular reason you're doing the addref, release, and interface map stuff by hand, rather than just using NS_IMPL_THREADSAFE_ISUPPORTS2(nsPrefLDAP, nsIPrefLDAP, nsILDAPMessageListener) * is there any circumstance in which the first call to pref_get_ldap_attributes might happen later than startup? I assume the answer is no, but I just wanted to verify that assumption. * nsPrefsFactory.cpp (and possibly the build machinery that compiles it) needs to be conditionalized based on MOZ_LDAP_XPCOM and friends. An example of this can be found in mozilla/xpfe/components/build/
+ mResults.Append(NS_LITERAL_STRING("\n")); + mResults.Append(NS_ConvertASCIItoUCS2(mAttrs[i]).ToNewUnicode()); + mResults.Append(NS_LITERAL_STRING("= ")); + mResults.Append(vals[j]); The ToNewUnicode() here allocates memory which isn't freed, so this is even a leak. NS_ConvertASCIItoUCS2 gives you a string object (it's actually a constructor of a class that directly inherits from nsAutoString), so Append will do the right thing. I'd follow dmose's advice and use: mResults += NS_LITERAL_STRING('\n') + NS_ConvertASCIItoUCS2(mAttrs[i]) + NS_LITERAL_STRING("= ") + nsDependantString(vals[j]);
> any particular reason you're doing the addref, release, and > interface map stuff by hand, rather than just using Based on my input in a different bug. This is, as I understand it, the "new" way of declaring things (personally I like it better). See nsISupportsUtils.h line #414.
There is a very remote possibility of calling pref_get_ldap_attributes first time other than startup. If the IS changes the autoconfig.jsc file to include pref_get_ldap_attributes which was not there before and from the client side the timer kicks in to download autoconfig.jsc, the browser will hang in the eventloop. This is a very remote possiblity but I will look into it and have some kind of check to see if the brwoser is up and running. Please let me know if any of you knows something like that. - moved JS-CallFunctionName() to nsPrefLDAP.cpp from prefapi.c Posting new patch with other suggested changes.
Target Milestone: mozilla1.0 → mozilla0.9.4
Attached patch latest patch (deleted) — Splinter Review
Please #ifdef the inclusion of nsPrefLDAP.h in nsPrefsFactory. With that change r=bnesse on the Prefs stuff.
I think you are going to leak str/argv in nsPrefLDAP::CallJSFunction().
I thought JS GC will take care of it but in any case I will add JS_free(prefContext, str) aftet the call to the callback function.
> > any particular reason you're doing the addref, release, and > > interface map stuff by hand, rather than just using > > NS_IMPL_THREADSAFE_ISUPPORTS2(nsPrefLDAP, nsIPrefLDAP, > > nsILDAPMessageListener) > Based on my input in a different bug. This is, as I understand it, the > "new" way of declaring things (personally I like it better). See > nsISupportsUtils.h line #414. Who gave you that input? If you look at the macros at: http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsISupportsUtils.h#1081 and follow them out, you'll see you'll end up with what you call ``the "new" way''. Better yet, in the debug case there will be additional debugging code for the "threadsafe" case that you're currently missing. As far as I know, the NS_IMPL_(THREADSAFE_)ISUPPORTSn macros are preferred over manually putting in the ADDREF, RELEASE and MAPPING macros, unless you need to do something special, which is a very uncommon case.
The ownership model of the char* in onLDAPSearchResult/CallJSFunction is rather funny. Instead of freeing it in CallJSFunction, why not free it in onLDAPSearchResult itself after you've called CallJSFunction? Or document that CallJSFunction will free any char* passed into it. I think you're right in that JS will take care of freeing the JSString when it's no longer needed, but I'd prefer a confirmation by someone who knows the js code better.
>Who gave you that input? It's been a while...I don't recall for certain. >As far as I know, the NS_IMPL_(THREADSAFE_)ISUPPORTSn macros are preferred... I stand corrected. Mitesh, please change accordingly. Thanks.
OK, changed the nsISupport Implementation to one Macro. Since JS converts the string back to Unicode, it was unnecessary to convert a Unicode to C string and back to Unicode. So I changed the results to be PRUnichar * type instead char *. Regarding ownership, now I am using JS_NewUCString() which takes the ownsership of the string passed to it and we don't have to worry abt freeing it. Although, we need to free it if it fails.
Attached patch latest patch (deleted) — Splinter Review
r=bnesse for tbe prefs changes in the 8/8 patch.
I am moving pref_get_ldap_attributes() to nsAutoConfig.cpp out of nsPrefLDAP.cpp. AutoConfig is already using firstTime static variable to determine if it's the startup time versues timer notification. I am making it a file static and using the same variable to control pref_get_ldap_attributes()'s event loop. Talked to dmose and he approved the change. r=dmose Requesting sr.
+ifdef MOZ_LDAP_XPCOM +REQUIRES += mozldap +XPIDLSRCS += nsIPrefLDAP.idl +endif While I applaud not making prefs depend on LDAP, I don't like conditional provision of interfaces. People shouldn't have to rebuild their tree to plop LDAP support in, and moreover there are people who are going to want to plug in ACAP and other remote-pref systems. This interface seems like it could be made to be a little more general, by making the serverURL a regular old nsIURL, and specifying the format of results expected back, and then we could call it nsIRemotePrefProvider.idl or some such. That way I don't get my purity hackles up about having the conditional compilation, and we (hopefully) don't have to reinvent the whole wheel when ACAP support comes on-line. (OK, OK, _if_ ACAP support comes on line.) I'll look at the patch in greater detail tomorrow, after more sleep, but I wanted to throw that out ASAP.
Ok, I will remove the conditional compilation of nsIPrefLDAP.idl. Regarding, the roaming, there is a separate design and discussion going on the prefs message board.
I would greatly prefer making the Components object visible to the prefs context, rather than hand-writing JSAPI code. Why did we decide not to XPConnect-ize the prefs context? If I read the description and summary correctly, you could then just stick a suitable getLDAPAttributes JS function in the autoconfig.jsc file's scope and avoid writing a pile of C++ code. (And nsIPrefLDAP should be scriptable. Unless you have a very good reason otherwise, all interfaces should be marked scriptable.) The JS function could fail gracefully in the absence of LDAP components, as well, which would make embedders happy. Further to my no-LDAP-dependency comment above, I would love to see this stuff decoupled a little better, perhaps such that you'd have an entry in your prefs.js (or from the autoconfig.jsc) like: Further to my no-LDAP-dependency comment above, I would love to see this stuff decoupled a little better, perhaps such that you'd have an entry in your prefs.js (or from the autoconfig.jsc) like: load_remote_pref("ldap", "ldap://host/base?attributes?one/sub?filter"); or load_remote_pref("http", "http://www.prefserver.com/username/prefs.js"); and then we would instantiate |"@mozilla.org/prefs/remote-pref?type=" + type| and ask it to get our prefs. Maybe that's a little beyond what we're doing here, though I'd like to see some bug filage to make sure that these issues stay on the radar. Back to the patch: + if (aResults) { + JSString * str = JS_NewUCString(prefContext, aResults, + nsCRT::strlen(aResults)); + if (str) { + argv = STRING_TO_JSVAL(str); + } else { + nsMemory::Free(aResults); + } First, it's bad XPCOM practice to free a caller-provided string. The caller should be responsible for memory management of |in| arguments (what if it's a string literal, and not allocated on the heap? what if it's on the stack, from an nsAutoString or similar). You should then use JS_NewUCStringCopyZ(prefContext, aResults) to let JS make a copy of the string, stopping at NUL-termination. Secondly, if JS_New*String* returns NULL, it has reported an out of memory error via the JS error reporter service, and you should return NS_ERROR_OUT_OF_MEMORY immediately. (Or maybe call the memory flusher and retry, but that seems like overkill.) Let's talk more about why this is in C++, rather than making the Components object visible to the pref context.
- Actually, we started implementing it in JS but it seems that making prefs context XPCONNECT-ize is not secure. According to Mitch Stoltz, it's not safe enabling XCONNECT in prefs context. Adding mstoltz on the bug so he can comment why making prefs context XPCONNECT enalbled is not safe. - There is no reason why nsIPrefLDAP is no scriptable, I will change it to scriptable. - The current design is for 4.x parity. I will file a new bug to change the instantiation through a preference instead of a JS call. - Ok I will change back to JS_NewUCStringCopyZ instead JS_NewUCString call. That way the string ownership model will remain straight
Mike, this autoconfig feature makes me vaguely nervous, securitywise. I have expressed this to Mitesh et al. I see a lot of potential for abuse. Because of this, I would like the functionality of the pref context to be no greater than necessary. Putting the Components object into the pref JS context allows the remote config file complete access to the user's machine. This is much more access than is needed. I would prefer to put only the functions needed by AutoConfig into the pref context, not the whole shebang.
Forgive me if I'm missing something, but isn't the pref context -- the ability to change the user's security and other settings -- really the whole shebang? Can't I grant UniversalXPConnect to myself from the pref and just go nuts? (It's not the Components object that's the risk here, of course, it's the behaviour of the security manager poked into the context. We already expose the Components object to content, such that things with UniversalXPConnect can work their magic. So let's put a security manager into the prefs context that only allows a small number of operations, perhaps via a security-checked-component, or perhaps just through explicitly listing the LDAP interfaces. That approach will scale better than this one in the face of additional compatibility requirements and future enhancements, I think.)
I think there has been enough efforts put in to make this work in C++ so it would be better if we can check this in as C++ code and file a new bug to make it better by implementing it in JS.
I don't think "I put so much effort into it, now you have to take it" is a valid argument. What could be a valid argument is that we have something that works now, there's a direct and immediate need for that functionality, and this option won't force us into backward compatibility support when we switch to the preferred implementation.
Whiteboard: Requesting r & sr → Requesting sr
Mike, there's a pretty low-level security check in prefs that prevents unprivileged code from accessing security policy prefs (the ones by which a script could give itself UniversalXPConnect), but come to think of it, even the low-level check doesn't apply to prefs set via the pref context. So you're right, autoconfig pretty much has full system access already. This may change in the future, as I'd like to store security policy somewhere other than prefs, eventually. Perhaps the best way to do this is to put Components into the pref context and write a security manager for the context that allows autoconfig to do only what it needs and no more. This would be a lot of work, with a lot of potential for error, and while it may be a worthwhile thing to do, I understand that the LDAP feature of autoconfig needs to be completed soon, and writing a new security manager doesn't sound feasible in the 0.9.4 time frame. I'm still worried about adding the Components object to the pref context, because it would provide an easy way for well-intentioned autoconfig users to do a lot of harm by careless or buggy scripting. For now, and considering the time pressure, I'd rather we go with the C++/jsapi solution we already have.
I don't think we can have it both ways, Mitch. Either there's a substantial concern about exposing new functionality to the prefs context (where a malicious attacker can already do their worst), in which case I don't think we should be rushing it in for 0.9.4, or there is no incremental security risk here -- which I believe, and you seem to agree with -- and the right thing is to lose the compile-time dependency on LDAP, write less C++, and use our established-and-tested security mechanisms to protect people from themselves. I have seen nothing in this bug to indicate that this code shouldn't be written in JS, where the sharp edges of the JSAPI and XPCOM calling conventions are helpfully hidden away, and where we -- or an embedding client that doesn't like our prefs-context-is-omnipotent model -- can use our existing security manager code to determine what's available to the pref context, rather than having to hack this out by hand. And where we don't have compile-time dependencies on the LDAP code. And where we have general interfaces for general requirements, like retrieving remote pref values. As it stands, I do not want to put my sr=shaver stamp on this patch, and I don't think it needs to hold 0.9.4's freeze, branch or release. You can feel free to appeal the review to brendan or staff@mozilla.org, and you can petition drivers@ to hold 0.9.4 for a (super-reviewed, if not by me) fix to this bug.
1. More JSAPI-based code should not be written, given XPConnect. There is no security degradation, as mstoltz concedes now, in exposing Components to the pref context, although: 2. We should have an autoconfig security manager. 3. The build-time dependency on LDAP is wrong. Make an autoconfig service available via XPCOM, or just extending the prefs service, with an interface that LDAP can implement and pass in as a callback-set. The LDAP part is a detail that should not show up in prefs code. The essential idea is "remote prefs provider", and that's the interface autoconfig should publish for LDAP, ACAP, etc. to implement. That's easy to implement, easier than all this JS API and XPCOM-violating in-param string freeing jazz. 4. Saying we're in a hurry is no justification. We've made PGP wait. We made PAC wait. At the most, a new patch for this bug with the fixes shaver called for could be landed on a branch. It should not go into the trunk. /be /be
jag wrote: > I don't think "I put so much effort into it, now you have to take it" is > a valid argument. What could be a valid argument is that we have something > that works now, there's a direct and immediate need for that functionality, > and this option won't force us into backward compatibility support when we > switch to the preferred implementation. Actually, the latter isn't a valid argument either for landing this in the Mozilla tree. This can and should wait till there is an implementation that addresses the concerns raised here by shaver and brendan.
Attached patch latest patch with sr changes (deleted) — Splinter Review
The new patch is with the suggested sr changes (except the rewrite in JS). I will start rewriting in JS but need an sr on this patch if it is required to be landed on the branch.
The all.js changes in the patch is not part of the changes for this bug
I leave the is-this-suitable-for-the-0.9.4-branch question to drivers; I don't think Mozilla needs it for 0.9.4, and I _know_ I don't want to approve it going on the trunk. mitesh: you should mail drivers@ to get approval to land this on the branch; my comments on the patch and approach are largely complete.
shaver: my understanding is that mitesh wants to land this on the branch _after_ 0.9.4 ships and the branch is handed over to netscape control. So he's looking for sr on the code as it stands to make sure there are no obvious bugs in it, since afaik even after netscape gets control of the branch they still want sr= marks.
QA over to luke.
QA Contact: olgac → lrg
dmose: see shaver's 8/21 10:53 comments. Sounds like Netscape can ok this for the branch after 0.9.4 releases. I'm keen to see the trunk solution, as is shaver (he's said so here and on IRC). /be
My understanding, from talking to mitesh on IRC today, is that he wants to land it on the 0.9.4 branch as soon as such a branch is cut. I'm not sure what you want me to do with regard to the post-0.9.4 Netscape branch: if they want super-review, I've provided super-review commentary on how I believe this feature, as implemented, interacts with Mozilla-in-the-large. Isn't that the super-reviewer's job? If Netscape wants to honour my super-review for this patch on their branch, then they probably shouldn't check it in. If they think that my super-review concerns are not sufficient to keep it off their branch, then they should check it in without (my) super-review; I will not be offended.
Thanks Shaver and Brendan for reviews. I think at this point I will continue working with the trunk solution and in the mean time, if Netscape decides to add this in post 0.9.4-netscape branch, the latest patch is available on the bug. For the trunk solution we need to resolve the JS context issue (bug 89137) first. Any input on that would be appreciated.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
go ahead and check this into the branch. Should be open Tuesday. Marking PDT+.
Whiteboard: Requesting sr → Requesting sr [PDT+}
I mispoke in my previous post. Branch should be open tomorrow (Friday). Check in then. Thanks!
I'm taking the PDT+ off this until we can review these changes and the current state of building and testing before this lands on the 0.9.4 branch. let's set up a meeting early in the week of 9/17 to discuss.
Whiteboard: Requesting sr [PDT+} → Requesting sr
alec/jud, are the current patches "safe" for embedding?
this is chofmann using the pitcam system on the last two comments.
Comment on attachment 47248 [details] [diff] [review] latest patch with sr changes this is fine as long as we've tested a non-ldap build as well (as people have pointed out, this is embedding-related, and embedding should not require ldap. with that requirement met, sr=alecf for the branch.
There is a small change required for it to build without LDAP. In prefapi.c, the function definition for pref_NativeGetLDAPAttr() is required in no-ldap case. I have moved #ifdefs within the function so it will compile fine in no-ldap case.
Attached file Mac project file changes (deleted) —
Keywords: nsbranch+
as soon as you have tested a non-ldap build and things look good, go ahead and check into the branch. Need to make sure we do not hose embedding. Make sure Marek and chofmann are cc'd on the test results. Marking pdt=.
Whiteboard: Requesting sr → pdt+
Mitesh, can you spin a build for QA without LDAP? Thanks -- Jussi-Pekka
Whiteboard: pdt+ → Requesting sr
sr=sfraser on revised mac build changes which bnesse will soon attach.
Attached patch latest changes (deleted) — Splinter Review
Moved #ifdef so non-ldap compiles correctly. changed eventloop to use PushThreadEventQueue instead of GetThreadEventQueue as per the bug 99515
As per Luke, non-ldap works fine. Requesting PDT+
Whiteboard: Requesting sr → Requesting PDT approval
Comment on attachment 50004 [details] [diff] [review] latest changes excellent. sr=alecf
Mitesh, make sure you don't check in all.js with the general.config.filename preference set (it's currently part of your patch file.)
Sure, I won't check in all.js. It is just for the test purpose and I forogot to remove from the patch.
In nsPrefLDAP.cpp, at line 249 + JSString * str = JS_NewUCStringCopyZ(prefContext, aResults); The usage of JS_NewUCStringCopyZ needs a (const jschar *) typecast added to the aResults parameter to compile on the Mac. This is because jschar is defined as a uint16 and PR_Unichar is defined as a wchar_t, which the compiler considers different.
check it in marking pdt+
Whiteboard: Requesting PDT approval → pdt+
Code checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Actually, the code is checked into the branch only, trunk solution still being worked on.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla0.9.5 → mozilla0.9.6
I am attaching a JS version of the getLDAPAttributes. The file will be placed in defaults/prefs as getldap.js and will be read into the new AutoConfig JS context 9bug 89137) if the pref (autoadmin.getldap.enabled) is true. Reviews requested
Whiteboard: pdt+
Attached file getLDAPAttributes in JS (deleted) —
Has this been checkin to the 094 branch? If yes, pls update the Status Whiteboard.
Whiteboard: [FIXED on branch?]
Whiteboard: [FIXED on branch?] → Checked into the branch - the bug is open for trunk solution
PDT+ in status whiteboard per grega's comment on 09.21.
Summary: Implement a mechanism for getLDAPAttributes() functionality → [PDT+] Implement a mechanism for getLDAPAttributes() functionality
Summary: [PDT+] Implement a mechanism for getLDAPAttributes() functionality → Implement a mechanism for getLDAPAttributes() functionality
Whiteboard: Checked into the branch - the bug is open for trunk solution → [PDT+] Checked into the branch - the bug is open for trunk solution
I have completed testing of this in the branch, and it works fine for in MacOS (9 and 10.x) Windows and Linux. One note, it seems that if one doesn't have the supported glibc files (i previously didn't) It will cause a hang during startup. This could affect some people who haven't updated their systems recently (as was the situation with me, running an old version of ximian, and having a glibc with a .helix attribute in it). Aside from that I would normally close this as verified, but can't pending a trunk solution.
What were the old and new versions of glibc? We should release note that once this lands..
Let's mark this fixed and create a bug for the trunk landing.
seems reasonable to me, especially given that this should be landing on the trunk in extensions/pref/ldap or something similar.
I didn't write it down at the time, and as I said I upgraded; but iirc it was 2.2.18-helix.
damn. It would be ideal if we could figure out the latest version of glibc that this fails on...
Created a new bug 104927 for the trunk solution. Marking this fixed.
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Marking this bug as verified fixed, I finished testing it on friday.
I'm going to mark this bug verified as per the comments. There is a new bug which has been filed for the trunk changes/landing.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: