89019 - LDAP does not work on Mac OSX

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect, P1)

Description

[svbegg]

Trying to setup an LDAP directory for email address resolution will cause
Fizilla to terminate. This occurs with Mozilla 0.9.2 (Mozilla/5.0 (Macintosh; U;
PPC; en-US; rv:0.9.2) Gecko/20010702, Build ID: 0000000000), running on Mac OS X
10.0.4, PowerBook G3 (Lombard) 320 MBytes RAM, 12 Gbytes spare disk.

The problem is reproducible.

Go to the edit menu, select preferences. Go to 'Mail and Newsgroups', then to
addressing. Check the box for 'Directory Server', and click the button labled
'Edit Directories...'. Fill in the name of the directory and the host name.
Click the 'Advanced' tab, then click the button labeled 'OK'.

The window does not go away. Clicking the tabs in the window will swap the
views. Clicking the 'OK' button again will crash Mozilla.

The same thing happened with Mozilla 0.9.1 on the same platform.

Comment 1

[sairuh (rarely reading bugmail)]

->ldap. not sure if this is limited to Mac OS X...

Comment 2

[Chris Petersen]

I can't crash the application but OK button in the Directory Server Properties
dialod doesn't function. Checked under Mac OS X 10.0.4 and Fizzilla July 3rd
build. This problem doesn't occur under Mac OS 9 (2001070503) or Windows ME
(2001070503) branch builds.

Comment 3

[Srilatha Moturi]

yeah, i dont see the crash either. removing keyword crash. Changing summary.
over to sdagley since this is specific mac OSX.

Comment 4

[Steve Dagley]

The problem is that the LDAP SDK is not Carbonated and fails to load under Mac OS 
X.  Apple has supposedly done some work on the SDK so we're trying to get info 
from them before starting our own Carbon version.

Comment 5

[hcaley]

I'm running MacosX build 2001070515 on a Powerbook G3 (Pismo) and I can
duplicate both the inactive OK button and that hitting it twice will crash
Mozilla.  I would not take out the crash keyword just yet ;)

Comment 6

[lchiang]

sdagley - should you mark this OS X?  If LDAP won't be supported, we have to 
change the UI to remove LDAP.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

peterv might have some thoughts on this as well; adding him to the CC list.

Comment 8

[Steve Dagley]

I have found the changes Apple made to the LDAP code but they date back to 1999 
so I'm not certain if they're going to help with making it work on OS X (I 
haven't had a chance to test them yet).

Comment 9

[Srilatha Moturi]

We cannot remove LDAP UI because it is all in xul and js and we can ifdef the UI 
for OSX.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Srilatha: isn't there a separate bug to fix exactly this problem.. ie make
--disable-ldap builds do the right thing with the various UI stuff?

Comment 11

[Steve Dagley]

Attachment: Apple LDAP Changes (.sit file)

Comment 12

[Steve Dagley]

Found (and attached) the Apple changes but they seem to be based on a verison of 
the LDAK SDK somewhat different than what's in the current Mozilla tree.

Comment 13

[Mark Smith [:mcs]]

I was unable to determine exactly what date and/or branch the Apple changes were
based on, but they are not TOO far off the tarball from
ftp://ftp.mozilla.org/pub/directory/c-sdk/ldap/ldapsdk_12311998.tar.gz

Comment 14

[Mark Smith [:mcs]]

Attachment: Patch created from Apple changes

Comment 15

[Mark Smith [:mcs]]

The attachment I just created may be useful... but I don't see how these changes
will fix the MacOS X problem.  Does anyone know WHY libldap won't load on MacOS
X?   Are OpenTransport calls supported in the Carbon environment?

Comment 16

[Steve Dagley]

Thanks for the patch Mark, I'll try it was soon as I get a chance (that'd be 
Monday 7/30 since I'm going to be travelling for a few days).  As for what errors 
there are with the current libldap build - direct linkage to InterfaceLib and the 
OT libs rather than InterfaceStubs, use of Toolbox calls obsoleted in Carbon 
(SystemTask() for one) and possibly some other OT related issues.  I'll post more 
after I take a look Monday.

Comment 17

[Steve Dagley]

And no, the Apple changes won't solve our Carbon problem but their changes to 
avoid calling SystemTask() should be a good start

Comment 18

[Mark Smith [:mcs]]

I do not like the idea of introducing a Mac-specific option to the libldap API
which is on the standards track within the IETF (see Apple's
LDAP_OPT_MAC_YIELDTOTHREAD related changes in ldap.h, getoption.c, setoption.c,
).  I am a few years out of touch with the MacOS world, but can we always call
YieldToThread()?  Or always call it in the Carbonized version?

Also, it would be better to leave NET_SSL and LDAP_SSLIO_HOOKS defined because
Mozilla will needs those once LDAP over SSL support is added back into the
client (see Apple's changes in LDAPClientDebugDefs.h)

I am definitely not qualified to review the OT related changes on tcp.c, but
using asynch mode seems like a good thing.

Comment 19

[Steve Dagley]

Well I couldn't sleep on the flight so I did some hacking to get the LDAPClient 
project building under Carbon.  This involved excision of the MacTCP support as 
that doesn't compile under Carbon.  Since it's been at least 5 years since Apple 
shipped MacTCP with Mac OS I didn't think it was any great loss.  Note that this 
doesn't incluse the async mode change from Apple's changes.  I'll look at that 
next.  I'm attaching a patch with the current state of the changes and a .sit 
file with the revised project (linkage changed).

Comment 20

[Steve Dagley]

Attachment: First cut at changes to build Carbon compatible LDAPClient

Comment 21

[Steve Dagley]

Attachment: Revised LDAPClient.mcp to fix linkage issues w/Carbon

Comment 22

[Mark Smith [:mcs]]

Steve, you have my (module owner) permission to proceed with your changes (very 
nice clean up).  Another Mac expert should review though.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Steve: one thing worth noting is that in the relatively near future (next
several months, I think), the client will move to using the 5.0 or later version
of the LDAP C SDK, and the 4.0 branch that we're using now will be obsoleted. 
Where have you been doing your changes?

Comment 24

[Steve Dagley]

My changes are in the Mozilla tree and pretty specific to the Mac networking 
code.  I don't think there'll be any problem moving my changes over to the main 
LDAP SDK version as I don't think that code will be changing much.  I'm sure mcs 
will let us know if there's any problems there :-)

Comment 25

[Mark Smith [:mcs]]

I don't think any work has been done recently on the MacOS-specific LDAP C SDK 
code (except what Steve has done in the context of this bug).

Life should get better with the 5.0 or later LDAP C SDK code because when it is 
integrated into the Mozilla client we will very likely arrange at run time to 
ALWAYS use the NSPR networking code (which is better than maintaining two sets 
of code).  The 5.0 LDAP C SDK provides a prldap library that makes it easy to 
tie libldap to NSPR (also used to tie it to NSS for LDAP over SSL support).  
Note that the MacOS-specific networking code does still exist in the 5.0 SDK in 
case someone wants to use the SDK without NSPR, so Steve's changes will still be 
useful.

Comment 26

[Steve Dagley]

Attachment: Second pass at LDAP changes for OS X

Comment 27

[Steve Dagley]

Posted a more up to date patch that yields a version that works under Mac OS 9.1 
with the Carbon build.  Unfortunately it still doesn't work under OS X.  
srilatha, could you please try the patch on the non-Carbon builds and verify I 
haven't broken anything there (all my trees are Carbon)?  You don't need to 
replace the LDAPClient.mcp, just apply the patch I created on 08/03.  Thanks.

Comment 28

[Srilatha Moturi]

Steve, works fine in the non-carbon build with the patch

Comment 29

[smithp01]

I'm using Moz 0.93 and indeed LDAP does not work: as stated, you can't update
your servers, OK does not work.  Of course, Netscape 6.1 beta for OS X doesn't
work either.  

The other platform I care about is OpenVMS.  On the VMS version (build
2001080116,  I think) OK does work, but address completion does not work and the
addressbook does not allow you to lookup the directory you have defined.  It is
no help to define a directory server you can't access!!

<soapbox>
I read a long submission from a person whose company had "invested" heavily in
Netscape 4.7.7 as the preferred e-mail client.  We have too.  Despite its
drawbacks, the ability to use an LDAP directory, in an intuitive way, is
absolutely key and is the feature that caused us to choose the product.  The
lack of this feature in Netscape 6.0 caused us to instruct students and faculty
here NOT TO USE IT.

Please, getting LDAP directories working is make-or-break for us, and others too
you'll find.  It is a value-differentiator.
</soapbox>

Comment 30

[Steve Dagley]

I see I hadn't set the priority or milestone apparently leading some people to 
belive this wasn't a critical bug so adjusting accordingly.  Don't worry, we know 
LDAP support is critical.

Comment 31

[Steve Dagley]

Attachment: Patch for LDAP that works on 9.x and X

Comment 32

[Steve Dagley]

Looks like the 3rd try is the charm - new patch works on 9x and OS X.  Now to get 
it reviewed and checked in :-)

Comment 33

[Mark Smith [:mcs]]

I reviewed the patch, but my OT skills are rusty and my carbon knowledge is very 
limited.  It looks okay from the libldap point of view.  My only complaint is 
that the contextPtr that gets passed to the EventHandler() callback is sometimes 
a tcpstream * and sometimes an int * (which is confusing).  Maybe there should 
be a TCPEventHandler() and a DNREventHandler().

Comment 34

[Steve Dagley]

As Mark points out there's some bad assumptions in the EventHandler() completion 
proc regarding the contextPtr passed in.  Let me make another pass at this before 
going for a r/sr.

Comment 35

[Steve Dagley]

Oops, meant to add sfraser to the cc: list so he'd see my comment that I'll be 
submitting a different patch for r/sr

Comment 36

[Hong Kwon]

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

Comment 37

[Steve Dagley]

Attachment: Cleaned up patch incorporating suggestions from mcs and sfraser

Comment 38

[Steve Dagley]

Revised patch with different completion procs per mcs and WaitNextEvent() calls 
rather than YieldToThread() per sfraser

Comment 39

[Simon Fraser [no longer active]]

Comments:

 #include "IDE_Options.h"
 // #define NO_USERINTERFACE
 // #define LDAP_DEBUG
+
+/* Read generated build options. */
+#include "DefinesOptions.h"            // written at build time

Why aren't the LDAP projects using the standard config files, MacPrefix.h and 
MacPrefix_debug.h?

RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/macintosh/macos-ip.c,v 
+#include <Threads.h>

No longer needed.

RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/macintosh/tcp-univhdrs/
tcp.c,v +#include <Threads.h>

and again

RCS file: /cvsroot/mozilla/directory/c-sdk/ldap/libraries/macintosh/tcp-univhdrs/
tcp.c,v

+       OTClientContextPtr OTContext;
...
+#if TARGET_CARBON
+               gHaveOT = ( InitOpenTransportInContext(kInitOTForApplicationMask, 
&OTContext) == noErr );
+#else /* TARGET_CARBON */
...
+#if TARGET_CARBON
+               tsp->tcps_ep = OTOpenEndpointInContext( OTCreateConfiguration( 
kTCPName ), 0, &info, &err, NULL );
+#else /* TARGET_CARBON */

Shouldn't you pass the OTContext that you got from InitOpenTransportInContext() 
to OTOpenEndpointInContext() ?

+               GetDateTime( &time );
+               end_time = time + timeout;
+               while(( timeout <= 0 || end_time > time ) && !tsp->tcps_connected 
&&
+                               !tsp->tcps_terminated ) {
+                       GetDateTime( &time );
...

Why use GetDateTime() for timeouts? TickCount() is cheaper, and would do just as 
well. Same goes for the DNS loops.

+#if TARGET_CARBON
+               //YieldToAnyThread();
+               (void)WaitNextEvent(nullEvent, &dummyEvent, 1, NULL);
+#else /* TARGET_CARBON */
                Delay( 2L, &ticks );
                SystemTask();
+#endif /* TARGET_CARBON */
        }
This Delay() is pointless. It's better to just call SystemTask() in the loop, 
because then at least you're not wasting CPU cycles. If you take out the Delay(), 
don't forget to update 'ticks' using TickCount().

Why not WaitNextEvent here too? Or would that introduce too much latency?

The OT notifier routines probably need to deal with a few more event types, to 
deal with dropped connections, hosts that are full and disconnect etc. I haven't 
looked at the rest of the code to determine if we disconnect politely.

Comment 40

[Simon Fraser [no longer active]]

In the carbon case, this loop will never time out:

        while (( rc = tcpreadready( tsp )) == 0 && ( timeout == NULL || ticks < 
endticks )) {
+#if TARGET_CARBON
+               //YieldToAnyThread();
+               (void)WaitNextEvent(nullEvent, &dummyEvent, 1, NULL);
+#else /* TARGET_CARBON */
                Delay( 2L, &ticks );
                SystemTask();
+#endif /* TARGET_CARBON */
        }

sr with these changes

Comment 41

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

a=dbaron (on behalf of drivers), once you incorporate all of Simon's changes

Comment 42

[Steve Dagley]

Attachment: Updated patch to version sr='d by sfraser

Comment 43

[Steve Dagley]

Fix checked in

Comment 44

[yulian chang]

Verified with 20010830 trunk build on Mac OSX.

Comment 45

[Steve Dagley]

Comment on attachment 43664 [details] [diff] [review]
First cut at changes to build Carbon compatible LDAPClient

marking obsolete patches as such

Comment 46

[Steve Dagley]

Comment on attachment 44672 [details] [diff] [review]
Second pass at LDAP changes for OS X

marking obsolete patches as such

Comment 47

[Steve Dagley]

Comment on attachment 46816 [details] [diff] [review]
Patch for LDAP that works on 9.x and X

marking obsolete patches as such

Comment 48

[Steve Dagley]

Comment on attachment 47424 [details] [diff] [review]
Cleaned up patch incorporating suggestions from mcs and sfraser

marking obsolete patches as such

Comment 49

[Dan Mosedale (:dmosedale, :dmose)]

Reopening for landing on LDAP C SDK 5.0 branch.

Comment 50

[Dan Mosedale (:dmosedale, :dmose)]

Taking.

Comment 51

[Dan Mosedale (:dmosedale, :dmose)]

Checked in to ldapcsdk_branch_50.

Comment 52

[yulian chang]

Verified with 20020321 trunk build.