Closed
Bug 75955
Opened 24 years ago
Closed 23 years ago
Implement a mechanism for getLDAPAttributes() functionality
Categories
(Directory :: LDAP XPCOM SDK, enhancement, P3)
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)
(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),
application/octet-stream
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details |
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.
Reporter | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P3
Reporter | ||
Updated•24 years ago
|
Assignee: dmose → leif
Status: ASSIGNED → NEW
Reporter | ||
Comment 1•24 years ago
|
||
Hmmm, this is my bug, give it back
Assignee | ||
Comment 2•24 years ago
|
||
I thought I created the bug 75229 for this. Anyway, will go ahead and mark it
as a duplicate
Reporter | ||
Updated•24 years ago
|
Reporter | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
Updated•24 years ago
|
Target Milestone: --- → mozilla0.9.2
Updated•23 years ago
|
Target Milestone: mozilla0.9.2 → mozilla1.0
Updated•23 years ago
|
Assignee | ||
Comment 5•23 years ago
|
||
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.
Assignee | ||
Comment 6•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
OS: Linux → All
Assignee | ||
Comment 7•23 years ago
|
||
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
Comment 8•23 years ago
|
||
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.
Assignee | ||
Comment 9•23 years ago
|
||
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
Assignee | ||
Comment 10•23 years ago
|
||
Comment 11•23 years ago
|
||
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?
Assignee | ||
Comment 12•23 years ago
|
||
- 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•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 15•23 years ago
|
||
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.
Assignee | ||
Comment 16•23 years ago
|
||
Comment 17•23 years ago
|
||
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.
Assignee | ||
Comment 18•23 years ago
|
||
Adding Seth for Super Review
Assignee | ||
Updated•23 years ago
|
Keywords: nsenterprise
Comment 19•23 years ago
|
||
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•23 years ago
|
||
* 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•23 years ago
|
||
+ 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•23 years ago
|
||
> 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.
Assignee | ||
Comment 23•23 years ago
|
||
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
Assignee | ||
Comment 24•23 years ago
|
||
Comment 25•23 years ago
|
||
Please #ifdef the inclusion of nsPrefLDAP.h in nsPrefsFactory. With that change
r=bnesse on the Prefs stuff.
Comment 26•23 years ago
|
||
I think you are going to leak str/argv in nsPrefLDAP::CallJSFunction().
Assignee | ||
Comment 27•23 years ago
|
||
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•23 years ago
|
||
> > 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•23 years ago
|
||
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•23 years ago
|
||
>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.
Assignee | ||
Comment 31•23 years ago
|
||
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.
Assignee | ||
Comment 32•23 years ago
|
||
Comment 33•23 years ago
|
||
r=bnesse for tbe prefs changes in the 8/8 patch.
Assignee | ||
Comment 34•23 years ago
|
||
Assignee | ||
Comment 35•23 years ago
|
||
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.
Assignee | ||
Comment 37•23 years ago
|
||
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.
Assignee | ||
Comment 39•23 years ago
|
||
- 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•23 years ago
|
||
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.)
Assignee | ||
Comment 42•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Whiteboard: Requesting r & sr → Requesting sr
Comment 44•23 years ago
|
||
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.
Comment 46•23 years ago
|
||
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•23 years ago
|
||
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.
Updated•23 years ago
|
Keywords: nsenterprise → nsenterprise+
Assignee | ||
Comment 48•23 years ago
|
||
Assignee | ||
Comment 49•23 years ago
|
||
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.
Assignee | ||
Comment 50•23 years ago
|
||
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.
Comment 52•23 years ago
|
||
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 54•23 years ago
|
||
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.
Assignee | ||
Comment 56•23 years ago
|
||
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.
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 57•23 years ago
|
||
go ahead and check this into the branch. Should be open Tuesday. Marking PDT+.
Whiteboard: Requesting sr → Requesting sr [PDT+}
Comment 58•23 years ago
|
||
I mispoke in my previous post. Branch should be open tomorrow (Friday). Check
in then. Thanks!
Comment 59•23 years ago
|
||
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
Comment 60•23 years ago
|
||
alec/jud, are the current patches "safe" for embedding?
Comment 61•23 years ago
|
||
this is chofmann using the pitcam system on the last two comments.
Comment 62•23 years ago
|
||
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.
Assignee | ||
Comment 63•23 years ago
|
||
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•23 years ago
|
||
Comment 65•23 years ago
|
||
Comment 66•23 years ago
|
||
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+
Comment 67•23 years ago
|
||
Mitesh, can you spin a build for QA without LDAP? Thanks -- Jussi-Pekka
Whiteboard: pdt+ → Requesting sr
Comment 68•23 years ago
|
||
sr=sfraser on revised mac build changes which bnesse will soon attach.
Comment 69•23 years ago
|
||
Assignee | ||
Comment 70•23 years ago
|
||
Assignee | ||
Comment 71•23 years ago
|
||
Moved #ifdef so non-ldap compiles correctly.
changed eventloop to use PushThreadEventQueue instead of GetThreadEventQueue as
per the bug 99515
Assignee | ||
Comment 72•23 years ago
|
||
As per Luke, non-ldap works fine. Requesting PDT+
Whiteboard: Requesting sr → Requesting PDT approval
Comment 73•23 years ago
|
||
Comment on attachment 50004 [details] [diff] [review]
latest changes
excellent. sr=alecf
Comment 74•23 years ago
|
||
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.)
Assignee | ||
Comment 75•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 78•23 years ago
|
||
Code checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 79•23 years ago
|
||
Actually, the code is checked into the branch only, trunk solution still being
worked on.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Assignee | ||
Comment 80•23 years ago
|
||
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+
Assignee | ||
Comment 81•23 years ago
|
||
Comment 82•23 years ago
|
||
Has this been checkin to the 094 branch? If yes, pls update the Status Whiteboard.
Whiteboard: [FIXED on branch?]
Assignee | ||
Updated•23 years ago
|
Whiteboard: [FIXED on branch?] → Checked into the branch - the bug is open for trunk solution
Comment 83•23 years ago
|
||
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
Updated•23 years ago
|
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
Comment 84•23 years ago
|
||
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•23 years ago
|
||
What were the old and new versions of glibc? We should release note that once
this lands..
Comment 86•23 years ago
|
||
Let's mark this fixed and create a bug for the trunk landing.
Comment 87•23 years ago
|
||
seems reasonable to me, especially given that this should be landing on the
trunk in extensions/pref/ldap or something similar.
Comment 88•23 years ago
|
||
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•23 years ago
|
||
damn. It would be ideal if we could figure out the latest version of glibc that
this fails on...
Assignee | ||
Comment 90•23 years ago
|
||
Created a new bug 104927 for the trunk solution. Marking this fixed.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 91•23 years ago
|
||
Marking this bug as verified fixed, I finished testing it on friday.
Comment 92•23 years ago
|
||
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.
Description
•