GNOME Bugzilla – Bug 741257
GUPnPNetworkManager is never deallocated due to internal circular reference
Last modified: 2015-06-17 10:18:11 UTC
This commit https://bugzilla.gnome.org/show_bug.cgi?id=694454#c5 introduces a circular reference bug. You can't make one thing reference another, and have that other thing reference the original thing. As it stands now, creating a context manager object and then unref-ing it does not actually destroy the object. See https://github.com/01org/dleyna-renderer/issues/149 for how this was discovered. You should test your code with valgrind for leaks like this one, by the way.
I think there are two approaches to fix this: 1) (the better option) the objects that hold a pointer to GUPnPNetworkManager should not reference it, and their lifecycle should be changed so that they're destroyed before GUPnPNetworkManager is. or 2) those objects should hold a *weak* reference, which is essentially a notification mechanism for when GUPnPNetworkManager is deallocated. When that happens those objects should no longer access that pointer in any way.
What GUPnPNetworkManager is doing does not really amount to a leak. It takes a reference to stay alive across asynchronous and idle calls, which is a pretty standard thing to do. Otherwise you might be accessing invalid memory if the object got destroyed while the calls were pending. It is only a 'leak' if the process dies before all the pending calls have had a chance to complete, but in that case we are not leaking any resources because the process is gone. Ideally, it would have implemented the GAsyncInitable interface to make it obvious to its users that asynchronous operations are involved during object construction. Since that might require intrusive changes, we can use the private GCancellable to cancel the pending operations from dispose. Instead of taking references to stay alive across async operations, we would check the GError to find out if the operation was cancelled and only proceed if it was not. The idle operation is OK from this perspective because the idle source gets removed during destruction so removing the reference is not an issue.
Maybe we can make it nice and shiny and proper when doing Bug 743224 - Port NetworkManager context manager to libnm
Created attachment 295026 [details] [review] Make the ref / unref pairs more obvious
Created attachment 295027 [details] [review] Use the GCancellable to cancel pending operations during destruction This is what I had in mind.
From a first look it seems ok to me, but I have not tested it yet. Olivier, could you have a look as well?
Debarshi - Did the above propose resolve this issue, was it released, and which version is this fixed. I was following on this issue in dleyna-renderer https://github.com/01org/dleyna-renderer/issues/149 and was there any fix required in dleyna-renderer. I am wanting to close See https://github.com/01org/dleyna-renderer/issues/149 if this resolved the issue.
(In reply to Rick Bell from comment #7) > Debarshi - Did the above propose resolve this issue, was it released, and > which version is this fixed. I was following on this issue in > dleyna-renderer https://github.com/01org/dleyna-renderer/issues/149 and was > there any fix required in dleyna-renderer. I am wanting to close See > https://github.com/01org/dleyna-renderer/issues/149 if this resolved the > issue. The patch is still pending review / testing. Presumably Jens is waiting for Olivier's opinion. It would be good to know if the patch works for dleyna or not.
Created attachment 305414 [details] [review] gupnp-network-manager: Use void when declaring function Sorry for the slow reply, patches look good to me and the gupnp-igd tests pass. Also attaching a further patch to make the NetworkManager support compile on F22.
Review of attachment 305414 [details] [review]: Thanks
Review of attachment 295026 [details] [review]: +1
Review of attachment 295027 [details] [review]: +1
I pushed the patches to master. Thanks for the reviews.