Created attachment 233393 [details] [review]
Fix leak of GtkSourceCompletionContext.

The new context would not be stored if it was different from the previous one.

Review of attachment 233393 [details] [review]:

(Since I'm working on the completion code, I make this review, but take this with a grain of salt as I'm not a maintainer ;)

The fix seems correct. If the new context is not updated, it can cause more serious problems than a simple memory leak I think (crashes in some corner cases…). So it's nice that you found it!

But the code can be simplified then:
- rename context -> new_context, so it's clearer;
- reverse the if, remove the else, and update priv->context only at the end:

if (completion->priv->context != NULL)
{
        /* bla bla */
}

completion->priv->context = new_context;

That code is tricky and Sébastien is definitely the most familiar with it right now, so whenever he says that it can go in, I am ok with it.


That said, I think you may have uncovered a bigger refcounting bug: I think logically the code should be:

if (completion->priv->context != NULL)
{
        unref (completion->priv->context)
}

completion->priv->context = new_context;

if (completion->priv->context != NULL)
{
        ref (completion->priv->context)
}

So that we acquire a ref to the new context... not sure where it would be released

(In reply to comment #2)
> Review of attachment 233393 [details] [review]:
> 
> (Since I'm working on the completion code, I make this review, but take this
> with a grain of salt as I'm not a maintainer ;)
> 
> The fix seems correct. If the new context is not updated, it can cause more
> serious problems than a simple memory leak I think (crashes in some corner
> cases…). So it's nice that you found it!
> 
> But the code can be simplified then:
> - rename context -> new_context, so it's clearer;
> - reverse the if, remove the else, and update priv->context only at the end:
> 
> if (completion->priv->context != NULL)
> {
>         /* bla bla */
> }
> 
> completion->priv->context = new_context;

The problem with this solution is that cancel_completion() via update_completion() may be called with completion->priv->context as the context parameter. The ref we own to the context will then be removed.

Perhaps the calls to update_completion() should take a new ref to completion->priv->context in that case? I'll attach a new patch that does that.

Created attachment 233404 [details] [review]
Cleanup the updating of the current context.

update_completion() and cancel_completion() now always expect that they own a
ref on the passed in context. This means that cancel_completion() always can unref
the previous active context even if the new context passed in is the same.

This should fix a potential problem where the active context wasn't updated properly
leading to the context being leaked and the active context being NULL even though
it shouldn't.

Review of attachment 233404 [details] [review]:

By looking in more details, it appears that the context parameter of cancel_completion() has no reasons to be there.

What I propose is to remove this parameter, and ensure that priv->context is NULL after the call to cancel_completion().
The single place where the context parameter was not NULL, i.e. in update_completion(), we can update priv->context after the call to cancel_completion(), like it is done for priv->running_providers.

So cancel_completion() can be simplified, and the g_object_ref() on the priv->context before calling update_completion() can be kept.

sounds good

Created attachment 233527 [details] [review]
Cleanup cancellation and updating of the current context.

Remove the context parameter from cancel_completion() and change
it so that it only unrefs and sets the active context to NULL.

update_completion() now always expect that it owns a
ref on the passed in context. This means that cancel_completion() always can unref
the previous active context even if the new context passed in is the same.

This should fix a potential problem where the active context wasn't updated properly
leading to the context being leaked and the active context being NULL even though
it shouldn't.

Review of attachment 233527 [details] [review]:

Thank you, for me it's good.

Review of attachment 233527 [details] [review]:

::: gtksourceview/gtksourcecompletion.c
@@ -1728,3 +1728,3 @@
 		update_completion (completion,
 				   completion->priv->active_providers,
-				   completion->priv->context);
+				   g_object_ref (completion->priv->context));

To be honest I still do not like this... I have not looked closely so so feel free to "override" my review, but I still do not understand why we cannot do this from within the function

if (new_context != priv_context)
{
   if priv_context != NULL
       unref (priv_context)

   priv_context = new_context

   if priv_context != NULL
       ref (priv_context)
}

(In reply to comment #10)
> Review of attachment 233527 [details] [review]:
> 
> ::: gtksourceview/gtksourcecompletion.c
> @@ -1728,3 +1728,3 @@
>          update_completion (completion,
>                     completion->priv->active_providers,
> -                   completion->priv->context);
> +                   g_object_ref (completion->priv->context));
> 
> To be honest I still do not like this... I have not looked closely so so feel
> free to "override" my review, but I still do not understand why we cannot do
> this from within the function
> 
> if (new_context != priv_context)
> {
>    if priv_context != NULL
>        unref (priv_context)
> 
>    priv_context = new_context
> 
>    if priv_context != NULL
>        ref (priv_context)
> }

Something like that would probably be possible, I don't know if it really is cleaner though. It would also require that we keep the context parameter to cancel_completion()

cancel_completion() was doing two different things: cancel the completion, and updating priv->context in case there is a new context. I think it's better for a function to do only one thing (but do it well): the thing implied by the function's name.

Now the ref() of the context can be moved inside update_completion(), where we assign the new value to priv->context. It would maybe be more natural. But in this case we must ref() and unref() at other places, it's a bit more complicated.

The problem with the Completion class is that it needs a big cleanup/refactoring (see bug #679419). So when fixing a bug, why not at the same time cleaning a bit the code around. But the "around" must have a reasonable limit, else we are starting to rewrite half of the file ;)

I've pushed the commit. Since I'm working on the Completion class too, I prefer to avoid patch conflicts in the future.