Re: [PATCH 2/5] youtube: Make browse() a cancellable operation




On Tue, 19 Apr 2011 07:34:48 +0000, Iago Toral <itoral igalia com> wrote:
On Fri, 15 Apr 2011 09:55:12 +0200, "Juan A. Suarez Romero"
<jasuarez igalia com> wrote:
@@ -1046,6 +1045,8 @@ search_cb (GObject *object, GAsyncResult
*result, OperationSpec *os)
     GRL_DEBUG ("Search operation has been cancelled");
     os->callback (os->source, os->operation_id, NULL, 0,
os->user_data, NULL);
     operation_spec_unref (os);
+    /* needs an extra unref */
+    operation_spec_unref (os);
     return;
   }

We have to be much more descriptive with this kind of things. "Needs
double unref" does not say anything, I already know that when i see 2
unrefs in a row, the thing to explain here is *why* it needs the
double unref.

A simple:
/* Look for OPERATION_SPEC_REF_RATIONALE for details on the reason
for this double-unref */
or
/* Since we are not freeing the spec in the emission callback we have
to do it here */

would do the trick.

The rest of the patches look good to me, but please test this well, since bugs dealing with cancellation are usually crashers and difficult to track once the code is committed.

Iago


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]