Closed
Bug 590494
Opened 14 years ago
Closed 13 years ago
Convert directory to frozen linkage
Categories
(Directory :: LDAP XPCOM SDK, defect)
Directory
LDAP XPCOM SDK
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jhorak, Assigned: jhorak)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
jhorak
:
review+
|
Details | Diff | Splinter Review |
In order to compile Thunderbird with libxul we have to move directory module to frozen linkage.
Comment 1•14 years ago
|
||
I guess this depends on bug 582943 and bug 583222 then.
Assignee | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
||
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.
Attachment #469055 -
Flags: review?(richm) → review?(bugzilla)
Comment 4•14 years ago
|
||
I don't know anything about the xpcom directory stuff - Dan Mosedale would be a better person to review this code.
Updated•14 years ago
|
Blocks: mailnews-libxul
Assignee | ||
Updated•14 years ago
|
Attachment #469055 -
Flags: review?(bugzilla) → review?(dmose)
Comment 5•14 years ago
|
||
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!
Updated•14 years ago
|
Attachment #469055 -
Flags: review?(dmose) → review?(bienvenu)
Comment 6•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 469055 [details] [diff] [review]
proposed patch 1
minusing in favor of a patch that applies...
Attachment #469055 -
Flags: review?(bienvenu) → review-
Assignee | ||
Comment 8•14 years ago
|
||
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.
Attachment #469055 -
Attachment is obsolete: true
Attachment #482543 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #482543 -
Flags: review? → review?(bienvenu)
Comment 9•14 years ago
|
||
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
Attachment #482543 -
Flags: review?(bienvenu) → review-
Assignee | ||
Comment 10•14 years ago
|
||
Thanks, could you look at this one?
Attachment #482543 -
Attachment is obsolete: true
Attachment #487555 -
Flags: review?(bienvenu)
Comment 11•14 years ago
|
||
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•14 years ago
|
||
Perhaps Neil or dmose can shed some light on the nature of the assertions...
Comment 13•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
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.
Attachment #487555 -
Flags: review?(bienvenu) → review-
Assignee | ||
Comment 16•14 years ago
|
||
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•14 years ago
|
||
Would bug 343332 help this?
Assignee | ||
Comment 18•14 years ago
|
||
Looks like pretty much the problem I'm facing now :).
Assignee | ||
Comment 19•13 years ago
|
||
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.
Attachment #487555 -
Attachment is obsolete: true
Attachment #573213 -
Flags: review?(dbienvenu)
Assignee | ||
Comment 20•13 years ago
|
||
Adding Mark as long as this is most likely relevant to bug #452232.
Depends on: 452232
Comment 21•13 years ago
|
||
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•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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•13 years ago
|
||
Comment on attachment 573213 [details] [diff] [review]
ldap frozen 1 patch
minusing based on getproxy issue.
Attachment #573213 -
Flags: review?(dbienvenu) → review-
Assignee | ||
Comment 24•13 years ago
|
||
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.
Attachment #573213 -
Attachment is obsolete: true
Attachment #573808 -
Flags: review?(neil)
Assignee | ||
Comment 25•13 years ago
|
||
(In reply to neil@parkwaycc.co.uk 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•13 years ago
|
||
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•13 years ago
|
||
My unpatched build crashes in LDAP code, so I can't really test this...
Assignee | ||
Comment 28•13 years ago
|
||
Fixed mentioned issues.
Attachment #573808 -
Attachment is obsolete: true
Attachment #574241 -
Flags: review?(neil)
Attachment #573808 -
Flags: review?(neil)
Comment 29•13 years ago
|
||
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.
Attachment #574241 -
Flags: review?(neil) → review+
Assignee | ||
Comment 30•13 years ago
|
||
- fixed indentation
- change *mConnections to mConnections.
Copying neil's review. Thanks for review.
Attachment #574241 -
Attachment is obsolete: true
Attachment #574284 -
Flags: review+
Assignee | ||
Comment 31•13 years ago
|
||
Setting checking needed while patch has positive review. Thanks.
Keywords: checkin-needed
Comment 32•13 years ago
|
||
Pushed changeset c016e832aabb to comm-central.
You need to log in
before you can comment on or make changes to this bug.
Description
•