75955 - Implement a mechanism for getLDAPAttributes() functionality

Status: VERIFIED FIXED

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

Description

[Leif Hedstrom]

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.

Comment 1

[Leif Hedstrom]

Hmmm, this is my bug, give it back

Comment 2

[Mitesh Shah]

I thought I created the bug 75229 for this. Anyway, will go ahead and mark it 
as a duplicate

Comment 3

[Mitesh Shah]

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

Comment 4

[yulian chang]

reassign to Olga as QA contact

Comment 5

[Mitesh Shah]

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.

Comment 6

[Mitesh Shah]

Attachment: patch to support getLDAPAttribute through javascript

Comment 7

[Mitesh Shah]

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

Comment 8

[Alec Flett]

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.

Comment 9

[Mitesh Shah]

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. 

Comment 10

[Mitesh Shah]

Attachment: patch using XPCOM LDAP

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 12

[Mitesh Shah]

- 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?

Comment 13

[Brian Nesse (gone)]

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.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 15

[Mitesh Shah]

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.

Comment 16

[Mitesh Shah]

Attachment: updated patch

Comment 17

[Brian Nesse (gone)]

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.

Comment 18

[Mitesh Shah]

Adding Seth for Super Review

Comment 19

[Brian Nesse (gone)]

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

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

* 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/

Comment 21

[jag (Peter Annema)]

+            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]);

Comment 22

[Brian Nesse (gone)]

> 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.

Comment 23

[Mitesh Shah]

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.

Comment 24

[Mitesh Shah]

Attachment: latest patch

Comment 25

[Brian Nesse (gone)]

Please #ifdef the inclusion of nsPrefLDAP.h in nsPrefsFactory. With that change
r=bnesse on the Prefs stuff.

Comment 26

[Brian Nesse (gone)]

I think you are going to leak str/argv in nsPrefLDAP::CallJSFunction().

Comment 27

[Mitesh Shah]

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.

Comment 28

[jag (Peter Annema)]

> > 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.

Comment 29

[jag (Peter Annema)]

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.

Comment 30

[Brian Nesse (gone)]

>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.

Comment 31

[Mitesh Shah]

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.

Comment 32

[Mitesh Shah]

Attachment: latest patch

Comment 33

[Brian Nesse (gone)]

r=bnesse for tbe prefs changes in the 8/8 patch.

Comment 34

[Mitesh Shah]

Attachment: updated patch, moving pref_get_ldap_attributes() to nsAutoConfig.cpp

Comment 35

[Mitesh Shah]

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.

Comment 36

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

+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.

Comment 37

[Mitesh Shah]

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. 

Comment 38

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

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.

Comment 39

[Mitesh Shah]

- 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

Comment 40

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 41

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

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.)

Comment 42

[Mitesh Shah]

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. 

Comment 43

[jag (Peter Annema)]

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.

Comment 44

[Mitchell Stoltz (not reading bugmail)]

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.

Comment 45

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

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 [e-mail], and you can petition drivers@
to hold 0.9.4 for a (super-reviewed, if not by me) fix to this bug.

Comment 46

[Brendan Eich [:brendan]]

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

Comment 47

[jag (Peter Annema)]

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.

Comment 48

[Mitesh Shah]

Attachment: latest patch with sr changes

Comment 49

[Mitesh Shah]

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.

Comment 50

[Mitesh Shah]

The all.js changes in the patch is not part of the changes for this bug

Comment 51

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

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.

Comment 52

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 53

[Rodney Velasco]

QA over to luke.

Comment 54

[Brendan Eich [:brendan]]

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

Comment 55

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

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.

Comment 56

[Mitesh Shah]

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.

Comment 57

[grega]

go ahead and check this into the branch.  Should be open Tuesday.  Marking PDT+.

Comment 58

[grega]

I mispoke in my previous post.  Branch should be open tomorrow (Friday).  Check
in then.  Thanks!

Comment 59

[Jaime Rodriguez, Jr.]

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.

Comment 60

[Jaime Rodriguez, Jr.]

alec/jud,  are the current patches "safe" for embedding?

Comment 61

[Jaime Rodriguez, Jr.]

this is chofmann using the pitcam system on the last two comments.

Comment 62

[Alec Flett]

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.

Comment 63

[Mitesh Shah]

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.

Comment 64

[Brian Nesse (gone)]

Attachment: Mac project file changes

Comment 65

[Brian Nesse (gone)]

Attachment: (Mac) MozillaBuildList.pm diff

Comment 66

[grega]

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=.

Comment 67

[Jussi-Pekka Mantere]

Mitesh, can you spin a build for QA without LDAP? Thanks -- Jussi-Pekka

Comment 68

[Simon Fraser [no longer active]]

sr=sfraser on revised mac build changes which bnesse will soon attach.

Comment 69

[Brian Nesse (gone)]

Attachment: Mac buildlist changes per sfrasers comments

Comment 70

[Mitesh Shah]

Attachment: latest changes

Comment 71

[Mitesh Shah]

Moved #ifdef so non-ldap compiles correctly.
changed eventloop to use PushThreadEventQueue instead of GetThreadEventQueue as 
per the bug 99515

Comment 72

[Mitesh Shah]

As per Luke, non-ldap works fine. Requesting PDT+

Comment 73

[Alec Flett]

Comment on attachment 50004 [details] [diff] [review]
latest changes

excellent. sr=alecf

Comment 74

[Brian Nesse (gone)]

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.)

Comment 75

[Mitesh Shah]

Sure, I won't check in all.js. It is just for the test purpose and I forogot to
remove from the patch.

Comment 76

[Brian Nesse (gone)]

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.

Comment 77

[grega]

check it in marking pdt+

Comment 78

[Mitesh Shah]

Code checked in.

Comment 79

[Mitesh Shah]

Actually, the code is checked into the branch only, trunk solution still being 
worked on.

Comment 80

[Mitesh Shah]

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

Comment 81

[Mitesh Shah]

Attachment: getLDAPAttributes in JS

Comment 82

[Jaime Rodriguez, Jr.]

Has this been checkin to the 094 branch? If yes, pls update the Status Whiteboard.

Comment 83

[Jaime Rodriguez, Jr.]

PDT+ in status whiteboard per grega's comment on 09.21.

Comment 84

[lrg]

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.

Comment 85

[Alec Flett]

What were the old and new versions of glibc? We should release note that once
this lands..

Comment 86

[Hong Kwon]

Let's mark this fixed and create a bug for the trunk landing.

Comment 87

[Alec Flett]

seems reasonable to me, especially given that this should be landing on the
trunk in extensions/pref/ldap or something similar.

Comment 88

[lrg]

I didn't write it down at the time, and as I said I upgraded; but iirc it was
2.2.18-helix.

Comment 89

[Alec Flett]

damn. It would be ideal if we could figure out the latest version of glibc that
this fails on...

Comment 90

[Mitesh Shah]

Created a new bug 104927 for the trunk solution. Marking this fixed.

Comment 91

[lrg]

Marking this bug as verified fixed, I finished testing it on friday.

Comment 92

[lchiang]

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.