Writing here for some patches we’ve been using, as advised: “report new issues, please post them in the Forum”
We’ve been using smack for our jabber support in Jitsi, for IM, audio and video - calls and conferences. Here are some patches that we have been adding from smack version 3.1. Those patches are against current trunk.
Patch 1 - discovery-info-escape-nodes.patch
For this one we found a problem where the nodes strings in the discovery info packets are not escaped as in the other packets.
Patch 2 - memory-leak.patch
This one was recently discovered. Improper use of WeakHashMap leads to memory leak as XMPPConnections stay in memory and all their listeners.
Patch 3 - xmppconnection.patch
These changes to XMPPConnection are some fixes and features we’ve added and we are currently using:
Setting a custom trust manager, so we can control from outside the certificates, like asking the users with UI dialog do they trust this particular certificate and remember their choice.
Replace a call getInetAddress().getHostName() with getInetAddress().getHostAddress() to avoid unnecessary DNS requests.
hey folks, we were wondering if there’s a problem with this patch or if it simply is of no interest to you … or has it just slipped through the cracks?
Sorry I’m getting into this late, but this is a feature we are really needing now. If it’s not too late to get a “dib” in, I’d like to also request a custom trust manager to act as a filter rather than a replacement. The ServerTrustManager is a package private class, so can’t be inherited by a class outside, and I don’t see any other way to get a filter in (so we don’t replicate the functionality of the original trust manager, just want to do some other checking). If anyone knows of another way to do this, that would be great. Thanks (and thanks for the patches too).
What about this patch. The custom trust manager now is plugged as custom one in the private ServerTrustManager and if set (not null), it is used as filter one.
Only had a doubt in checkServerTrusted whether the filtering to be before or after the current checks that are available in ServerTrustManager.
Excellent, thank you. I think the typical scenario for use of the custom trust manager is to act on behalf of the user, especially in the case of private, self-signed certificates. Since that may involve prompting the user to accept, it would be nice to not do that if the certificate is expired, or otherwise fails the standard checks. So that would imply to call the custom one after.
Maybe that’s not the typical scenario, though. I have read others that want to just trust everyone (more on the client trust than server, but I guess it would be possible that way too, especially in a private network). So might be nice to have the option for the custom manager to have first dibs, and then set something as to whether or not to continue and do the filter or make it a replacement. Or maybe always call the custom manager, even in the case when the certificate fails, to let them accept anyway.
About the patch for SMACK-374 “Improper use of WeakHashMap…”: I am not sure but ChatStateManager becoming a WeakReference as the value of the WeahHashMap, means ChatStateManager instances will gc’ed if the application does not hold a strong reference for the ChatStateManager instance.
But I am also not 100% sure about this either, since ChatStateManagers init() does add two packet interceptors which are inner classes of ChatStateManager to Connection, which means there is still a strong reference from Connection to ChatStateManager.
the problem we spotted is that with WeakHashMap the value must not reference the key or it will never be gc’ed. And ChatStateManagers has direct reference to Connection and its interceptors are also held in WeakHashMap, does it make sense?
The following is taken from the java api:
Implementation note: The value objects in a WeakHashMap are held by ordinary strong references. Thus care should be taken to ensure that value objects do not strongly refer to their own keys, either directly or indirectly, since that will prevent the keys from being discarded. Note that a value object may refer indirectly to its key via the WeakHashMap itself; that is, a value object may strongly refer to some other key object whose associated value object, in turn, strongly refers to the key of the first value object. One way to deal with this is to wrap values themselves within WeakReferences before inserting, as in: m.put(key, new WeakReference(value)), and then unwrapping upon each get.
I think I understand the problem you are trying to solve. In fact that is a common problem in Smack that instances (espically from “Manager” classes) are not gc’ed and therefore leak memory. The impact is especially problematic in aSmack.
Anyway, the problem is that there is at least one more strong reference to Connection from ChatStateManager:
The connection member variable of ChatStateManager
I have a feeling that this variable should either be dropped or become also a weak reference.
Or could you verify that this particular patch sovled the issue for you?
Or could you verify that this particular patch sovled the issue for you?
It was an year ago, but as I remember I found the problem examing the heap dumps looking for other memory leak and confirmed it again with exaimg heap dumps, so yes it is solved.