590494 - Convert directory to frozen linkage

Status: RESOLVED FIXED

(Directory :: LDAP XPCOM SDK, defect)

Description

[Jan Horak [:jhorak]]

In order to compile Thunderbird with libxul we have to move directory module to frozen linkage.

Comment 1

[Mark Banner (:standard8)]

I guess this depends on bug 582943 and bug 583222 then.

Comment 2

[Jan Horak [:jhorak]]

Attachment: proposed patch 1

This patch fix Makefiles, replaces nsHashtable by nsDataHashtable, replaces nsString.h by nsStringGlue.h and adds nsLDAPUtils.h with helper functions and macros.
 
Test with --enable-incomplete-external-linkage build parameter.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 469055 [details] [diff] [review]
proposed patch 1

Actually, because this is the xpcom part of directory/ it effectively almost comes under comm-central rules - we don't use the cvs part of XPCOM these days. Therefore swapping review request.

I probably won't get round to reviewing this until next week though.

Comment 4

[Rich Megginson]

I don't know anything about the xpcom directory stuff - Dan Mosedale would be a better person to review this code.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

bienvenu has done a bunch of frozen linkage reviews already, and as such, is likely to be to do them much more efficiently.  Furthermore, he's graciously agreed to take this one.  Thanks, David!

Comment 6

[David :Bienvenu]

Comment on attachment 469055 [details] [diff] [review]
proposed patch 1

this seems to have bit-rotted - can you attach a diff against the trunk? thx!

Comment 7

[David :Bienvenu]

Comment on attachment 469055 [details] [diff] [review]
proposed patch 1

minusing in favor of a patch that applies...

Comment 8

[Jan Horak [:jhorak]]

Attachment: proposed patch 2

Unbitrotten patch. I can't build with --enable-incomplete-external-linkage to test it correctly but the last patch has contact search working. Please have a look.

Comment 9

[David :Bienvenu]

Comment on attachment 482543 [details] [diff] [review]
proposed patch 2

standard windows build fails:

.h -DMOZILLA_CLIENT /c/builds/tbirdhq/directory/xpcom/base/src/nsLDAPService.cpp

nsLDAPService.cpp
c:\builds\tbirdhq\objdir-tb\mozilla\dist\include\nsHashKeys.h(187) : warning C42
44: 'return' : conversion from 'const PRUint64' to 'PLDHashNumber', possible los
s of data
c:/builds/tbirdhq/directory/xpcom/base/src/nsLDAPService.cpp(1007) : error C3861
: 'strndup': identifier not found
make[7]: *** [nsLDAPService.obj] Error 2

other code uses PL_strndup.


can you put the comment on a separate line, to shorten up this line?

+    nsInterfaceHashtable<nsVoidPtrHashKey, nsILDAPOperation> *mPendingOperations; // keep these around for callbacks

Comment 10

[Jan Horak [:jhorak]]

Attachment: proposed patch 3

Thanks, could you look at this one?

Comment 11

[David :Bienvenu]

When I run with this, and do an ldap query, I hit this assertion in pldhash.c:

    NS_ASSERTION(op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0,
                 "op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0");
    INCREMENT_RECURSION_LEVEL(table);

op is PL_DHASH_ADD, which implies the recursion level > 0 (perhaps because an other thread is accessing the hash table?), and the full stack is:

 	xul.dll!NS_DebugBreak_P(unsigned int aSeverity=1, const char * aStr=0x5c9f1a3c, const char * aExpr=0x5c9f1a04, const char * aFile=0x5c9f19c8, int aLine=612)  Line 371 + 0xc bytes	C++
>	xul.dll!PL_DHashTableOperate(PLDHashTable * table=0x0e2dfb18, const void * key=0x00000002, PLDHashOperator op=PL_DHASH_ADD)  Line 612 + 0x4b bytes	C
 	xul.dll!nsTHashtable<nsBaseHashtableET<nsPtrHashKey<void const >,nsCOMPtr<nsILDAPOperation> > >::PutEntry(const void * aKey=0x00000002)  Line 189	C++
 	xul.dll!nsBaseHashtable<nsPtrHashKey<void const >,nsCOMPtr<nsILDAPOperation>,nsILDAPOperation *>::Put(const void * aKey=0x00000002, nsILDAPOperation * aData=0x109646a8)  Line 163 + 0xc bytes	C++
 	xul.dll!nsLDAPConnection::AddPendingOperation(nsILDAPOperation * aOperation=0x109646a8)  Line 392 + 0x13 bytes	C++
 	xul.dll!nsLDAPOperation::SearchExt(const nsACString_internal & aBaseDn={...}, int aScope=2, const nsACString_internal & aFilter={...}, unsigned int aAttrCount=68, const char * * aAttributes=0x10964728, unsigned int aTimeOut=0, int aSizeLimit=100)  Line 523 + 0x16 bytes	C++
 	xul.dll!nsAbQueryLDAPMessageListener::DoTask()  Line 283 + 0x5d bytes	C++
 	xul.dll!nsAbLDAPListenerBase::OnLDAPMessageBind(nsILDAPMessage * aMessage=0x1818bcd8)  Line 404	C++
 	xul.dll!nsAbQueryLDAPMessageListener::OnLDAPMessage(nsILDAPMessage * aMessage=0x1818bcd8)  Line 200 + 0xc bytes	C++
 	xul.dll!NS_InvokeByIndex_P(nsISupports * that=0x0e2dfa70, unsigned int methodIndex=3, unsigned int paramCount=1, nsXPTCVariant * params=0x1902ae68)  Line 103	C++
 	xul.dll!nsProxyObjectCallInfo::Run()  Line 181 + 0x2d bytes	C++
 	xul.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0042d64c)  Line 609 + 0x19 bytes	C++
 	xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00d45c10, int mayWait=1)  Line 250 + 0x16 bytes	C++
 	xul.dll!nsBaseAppShell::Run()  Line 184 + 0xb bytes	C++
 	xul.dll!nsAppShell::Run()  Line 243 + 0x9 bytes	C++
 	xul.dll!nsAppStartup::Run()  Line 191 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00d41190, const nsXREAppData * aAppData=0x00d41770)  Line 3682 + 0x25 bytes	C++


I also see a thread-safety assertion when the query is finished.

Neither of these assertions happen without the patch :-(

Comment 12

[David :Bienvenu]

Perhaps Neil or dmose can shed some light on the nature of the assertions...

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #11)
> I also see a thread-safety assertion when the query is finished.
> 
> Neither of these assertions happen without the patch :-(

Erm, afaik the thread-safety assertion has been around for quite a while.

Comment 14

[David :Bienvenu]

(In reply to comment #13)
> (In reply to comment #11)
> > I also see a thread-safety assertion when the query is finished.
> > 
> > Neither of these assertions happen without the patch :-(
> 
> Erm, afaik the thread-safety assertion has been around for quite a while.

You're right, I do sometimes see it w/o this patch as well. But the first assertion is new, and happens every time.

Comment 15

[David :Bienvenu]

Comment on attachment 487555 [details] [diff] [review]
proposed patch 3

minusing since I think we really need to understand the pldhash table assertion. Please let me know if you can't reproduce it. The rest of the patch looks OK, and ldap searches were working in general, so I think it's pretty close.

Comment 16

[Jan Horak [:jhorak]]

I can reproduce. It seems to be due to thread which loops in nsLDAPConnectionLoop make callback to main thread - nsAbLDAPListenerBase::OnLDAPMessageBind which calls nsLDAPConnection::AddPendingOperation. While both nsLDAPConnectionLoop (thru calling enumerate function CheckLDAPOperationResult) are trying to raise INCREMENT_RECURSION_LEVEL in PL_DHashTableEnumerate and PL_DHashTableOperate and PL_DHashTableOperate requires (by assert) RECURSION_LEVEL 0.

It seems to by synchronization problem and it may requires redesign :(.

Comment 17

[Mark Banner (:standard8)]

Would bug 343332 help this?

Comment 18

[Jan Horak [:jhorak]]

Looks like pretty much the problem I'm facing now :).

Comment 19

[Jan Horak [:jhorak]]

Attachment: ldap frozen 1 patch

While we're approaching to link thunderbird against libxul we still have to convert ldap code to frozen linkage. This is a patch which should be sufficient.

The patch doesn't solve install location of libldap60.so, libprldap60.so and libldif60.so which are put to dist/bin. However this location is not added to library patch during start of xulrunner-stub (which I'm using to start thunderbird linked against libxul) and results in libmozldap loading failure.

Comment 20

[Jan Horak [:jhorak]]

Adding Mark as long as this is most likely relevant to bug #452232.

Comment 21

[[e-mail]]

Comment on attachment 573213 [details] [diff] [review]
ldap frozen 1 patch

>+ifdef MOZ_INCOMPLETE_EXTERNAL_LINKAGE
>+INCLUDES += -I$(DEPTH)/ldap/sdks/c-sdk/ldap/include
>+endif
I meant to ask on the other bug, but I saw this one first... don't the headers end up in $(DIST)/include? Also, what's wrong with using LOCAL_INCLUDES? Also I think you should be using $(srcdir) somewhere.

>+  nsresult rv;
>   nsCOMPtr<nsIObserverService> obsServ =
>-      mozilla::services::GetObserverService();
>+      do_GetService("@mozilla.org/observer-service;1", &rv);
[rv is unused so you can omit it entirely]

>         nsDependentCString sValue(values[i]);
>-        if (IsUTF8(sValue))
>-            (*aValues)[i] = UTF8ToNewUnicode(sValue);
>-        else
>-            (*aValues)[i] = ToNewUnicode(sValue);
>+        (*aValues)[i] = ToNewUnicode(NS_ConvertUTF8toUTF16(sValue));
[Not sure why you changed this. ToNewUnicode on an nsDependentCString does an ASCII convertion, not a UTF8 conversion, which is why the UTF8 version exists.]

>-    PRUint32 numTokens = CountTokens(iter, iterEnd); 
>+    const char *iter = aValue.BeginReading();
>+    const char *iterEnd = aValue.EndReading();
>+    PRUint32 numTokens = CountTokens(aValue.BeginReading(), aValue.EndReading()); 
[Why not use iter and iterEnd?]

> char*
>-nsLDAPService::NextToken(nsReadingIterator<char> & aIter,
>-                         nsReadingIterator<char> & aIterEnd)
>+nsLDAPService::NextToken(const char *aIter,
>+                         const char *aIterEnd)
[This looks wrong, because the old version took references which it altered, whereas the new version takes copies.]

>-  nsCAutoString findAttribute(',');
>+  nsCAutoString findAttribute(",");
>   findAttribute.Append(aAttribute);
>-  findAttribute.Append(',');
>+  findAttribute.Append(",");
[I thought that the Append should still be OK]

>+#define MsgGetProxyForObject(aTarget, aIID, aObj, aProxyType, aProxyObject) \
>+        NS_GetProxyForObject(aTarget, aIID, aObj, aProxyType, aProxyObject)
[We're trying to get rid of NS_GetProxyForObject!]

Comment 22

[David :Bienvenu]

(In reply to [e-mail] from comment #21)
> >+#define MsgGetProxyForObject(aTarget, aIID, aObj, aProxyType, aProxyObject) \
> >+        NS_GetProxyForObject(aTarget, aIID, aObj, aProxyType, aProxyObject)
> [We're trying to get rid of NS_GetProxyForObject!]

Mailnews backend *has* gotten rid of all uses of NS_GetProxyForObject, because mozilla/gecko is going to get rid of it as well. See bug 675407 for more info.

Comment 23

[David :Bienvenu]

Comment on attachment 573213 [details] [diff] [review]
ldap frozen 1 patch

minusing based on getproxy issue.

Comment 24

[Jan Horak [:jhorak]]

Attachment: ldap frozen 2 patch

Sorry for keeping obsolete code with NS_GetProxyForObject. It is from previous version of patch and I forgot to remove it. Fixed issues which Neil mentioned (at least I hope) and most of linking libraries removed from ldap/xpcom/src/Makefile.in.

Comment 25

[Jan Horak [:jhorak]]

(In reply to [e-mail] from comment #21)
> Comment on attachment 573213 [details] [diff] [review] [diff] [details] [review]
> ldap frozen 1 patch
> 
> >+ifdef MOZ_INCOMPLETE_EXTERNAL_LINKAGE
> >+INCLUDES += -I$(DEPTH)/ldap/sdks/c-sdk/ldap/include
> >+endif
> I meant to ask on the other bug, but I saw this one first... don't the
> headers end up in $(DIST)/include? Also, what's wrong with using
> LOCAL_INCLUDES? Also I think you should be using $(srcdir) somewhere.

Yup, these files are installed to $(DIST)/include if make is called from  the ldap/c-sdk so my change isn't necessary. It is not done by default when building with incomplete linkages and this confused me.

Comment 26

[[e-mail]]

Comment on attachment 573808 [details] [diff] [review]
ldap frozen 2 patch

>+ifndef MOZILLA_INTERNAL_API
IMHO this is confusing. Could you change it to
ifdef MOZ_INCOMPLETE_EXTERNAL_LINKAGE

>   nsCOMPtr<nsIObserverService> obsServ =
>-      mozilla::services::GetObserverService();
>+      do_GetService("@mozilla.org/observer-service;1");
I don't know whether it's in scope, but if it is, I'd prefer
do_GetService(NS_OBSERVERSERVICE_CONTRACTID);

>-        mServers = new nsHashtable(16, PR_FALSE);
>+        mServers = new nsDataHashtable<nsStringHashKey, nsLDAPServiceEntry*>;
I don't think this is the right way to use nsDataHashtable. mServers should be a member, not a pointer, and then you should call its Init() method here, and error out if it fails. (Init defaults to 16.)

>+        nsLDAPServiceEntry *ldap_null;
>+        if (mServers->Get(key, &ldap_null)) {
If you just want to know whether the key exists, pass null as the second parameter. Or if we never put null, use the 1-parameter version.

>+    mServers->Get(nsDependentString(aKey), &entry);
>     if (entry) {
You get this right later on, by writing
if (mServers.Get(nsDependentString(aKey), &entry)) {

Comment 27

[[e-mail]]

My unpatched build crashes in LDAP code, so I can't really test this...

Comment 28

[Jan Horak [:jhorak]]

Attachment: ldap frozen 3 patch

Fixed mentioned issues.

Comment 29

[[e-mail]]

Comment on attachment 574241 [details] [diff] [review]
ldap frozen 3 patch

It's great to have 10-second rebuild times for C++ patches ;-)

> nsresult nsLDAPService::Init()
> {
>+    if (!mServers.Init()) {
>+      NS_ERROR("nsLDAPService::Init: out of memory ");
>+      return NS_ERROR_OUT_OF_MEMORY;
Nit: indentation mismatch (file style seems to be 4 spaces)

>     if (!mConnections) {
>-        mConnections = new nsHashtable(16, PR_FALSE);
>+        mConnections = new nsDataHashtable<nsVoidPtrHashKey, nsLDAPServiceEntry*>;
You've actually got two nsDataHashtables so this one needs to be updated in the same way as you did for mServers. r=me with that fixed.

Comment 30

[Jan Horak [:jhorak]]

Attachment: ldap frozen 4 patch

- fixed indentation
- change *mConnections  to mConnections.
Copying neil's review. Thanks for review.

Comment 31

[Jan Horak [:jhorak]]

Setting checking needed while patch has positive review. Thanks.

Comment 32

[[e-mail]]

Pushed changeset c016e832aabb to comm-central.