1672902 - Remove dead code in LDAP

Status: RESOLVED FIXED

(Thunderbird :: Address Book, task)

Description

[Ben Campbell]

There seems to be a goodly chunk of LDAP-related code that isn't actually used. I don't know if it works or not, but I'd be inclined to remove it to avoid any confusion.

There are two main ones which stand out:

Firstly, nsLDAPSyncQuery isn't used anywhere in the C-C codebase.
The moz.build file includes it conditionally via if CONFIG['MOZ_PREF_EXTENSIONS']:, which suggests it might be used by extensions?

Secondly, there's nsILDAPService and nsILDAPServer. Originally it seems the idea was that nsILDAPService would have a list of LDAP server details and cache LDAP connections. But that doesn't seem to be used at all any more.
The LDAP server list is now (I think) just addressbook directories of type nsIAbLDAPDirectory.

I think the only nsILDAPService usage is in AbLDAPAutoCompleteSearch, which uses nsILDAPService.createFilter().
There are a bunch of historical bugs related to nsILDAPService which could probably be closed here too: Bug 75954, Bug 75989, Bug 75999, Bug 76755, Bug 76887

Comment 1

[Ben Campbell]

Geoff: Magnus mentioned you had some ideas on LDAP stuff which could be trimmed or reimplemented in JS?
Not sure if any of that falls into the "dead code" bag, but if so, here's a bug to hang it off :-)

Comment 2

[Geoff Lankow (:darktrojan)]

What I want to do is get rid of the C++ LDAP stuff in mailnews/addrbook and replace it with a JS equivalent. It only really does two things: searches the remote address book (so the same as what the autocomplete does but with more arbitrary parameters), and fetches all of the contacts from the server and stores them locally for offline use. If there's anything else I've not come across it.

Ultimately we should be able to replace all of the LDAP C++ code and replace it with something much simpler. For now though I agree, let's throw away anything we're not using.

Comment 3

[Ben Campbell]

(In reply to Geoff Lankow (:darktrojan) [away until 16/11] from comment #2)

What I want to do is get rid of the C++ LDAP stuff in mailnews/addrbook and replace it with a JS equivalent. It only really does two things: searches the remote address book (so the same as what the autocomplete does but with more arbitrary parameters), and fetches all of the contacts from the server and stores them locally for offline use. If there's anything else I've not come across it.

(just musing here - not expecting any response :-)
Maybe a good approach would be to define the JS interface you'd like to see for LDAP functionality. We could implement that on top of what's there already, then replace it at leisure, with new C++, JS or (most likely) a combination. Like so many of the protocols there's enough byte-fiddling that I'm inclined to think C++ is a better fit down there (I've tried doing some stuff with JS byte arrays and found them to be a pretty blunt tool). But as soon as you bootstrap up to something workable, JS really comes into it's own.
(and at the very least, I'd like to merge the libprldap and nsLDAPSecurityGlue layers. Lots of extra obfuscation there.)

Comment 4

[Ben Campbell]

Attachment: 1672902-remove-nsLDAPSyncQuery-1.patch

Nothing currently uses nsLDAPSyncQuery, and I don't even know if it works.

But I don't know what the situation is with extensions. I don't see it show up in bugzilla, so it's either not being used by any extensions, or it just works perfectly and everyone is really happy with it :-)

try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9759f0151b0dcc1e5660c9bdde0dcbfc11458594

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9184155 [details] [diff] [review]
1672902-remove-nsLDAPSyncQuery-1.patch

Review of attachment 9184155 [details] [diff] [review]:
-----------------------------------------------------------------

It is actually used, https://searchfox.org/mozilla-central/rev/c409dd9235c133ab41eba635f906aa16e050c197/extensions/pref/autoconfig/src/prefcalls.js#148,157
... but apparently broken (for ssl only?). Bug 1662433.

"Extensions" in this code I believe refer back basically to the old mozilla application suite (now seamonkey) where you would optionally enable only certain parts, i.e. disable LDAP, disable Chatzilla, disable mail, disable editor and so on.

Comment 6

[Ben Campbell]

Ahh, it didn't even occur to me that any M-C code might rely on C-C code...
Shifting attention to Bug 1662433 instead.

Comment 7

[Ben Campbell]

Attachment: 1672902-remove-unused-nsILDAPService-methods-1.patch

This removes all the connection management in nsILDAPService, which isn't used as far as I can tell.
createFilter() is the only one left. It is only used in AbLDAPAutoCompleteSearch, and could probably be reimplemented easily enough in javascript - only a small subset of the filter creation syntax is required. But for now, since the rest of the patch is a simple set of cuts, I'll leave this C++ implementation in place.

Doubt there's any test coverage, but there's a try run here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=f45e1cddeaf160e4d82d4f43a45d8c17812dd616

The try run isn't this exact patch. After I submitted it I snipped out a bunch of now-redundant #includes (which shouldn't affect anything - it still compiles fine).

Comment 8

[Ben Campbell]

(In reply to Ben Campbell from comment #7)

The try run isn't this exact patch. After I submitted it I snipped out a bunch of now-redundant #includes (which shouldn't affect anything - it still compiles fine).

Errm. And I removed a couple of unused vars which the compiler warned me about locally but cause the try build to fail :-)
Let's have another go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=46d8deba291069c5e551cd703feda81fe17861c6

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9189907 [details] [diff] [review]
1672902-remove-unused-nsILDAPService-methods-1.patch

Review of attachment 9189907 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=mkmelin

Comment 10

[Magnus Melin [:mkmelin]]

Test failures looks unrelated.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/75d7bbd322cc
Remove unused nsILDAPService methods. r=mkmelin