Created attachment 182217 [details] [review]
Adds a sink before the signal gets emitted.

I think instead of adding an extra ref, you can just drop the UNref after the signal emission.

I don't believe so because the marshaller will behave differently depending if someone is registered for the signal or not.

Hmm. Tried a testcase, and you're right. I guess the docs for G_TYPE_VARIANT should mention that somewhere; I certainly didn't know that when I *wrote* that code in bug 610863.

(In reply to comment #4)
> Hmm. Tried a testcase, and you're right. I guess the docs for G_TYPE_VARIANT
> should mention that somewhere; I certainly didn't know that when I *wrote* that
> code in bug 610863.

Sounds like a bug then. The semantics really need to be that g_signal_emits() consumes the GVariant if it is floating. No matter how many connections there are. Otherwise everything is going to break....

It's because of this check in g_signal_emit_valist, before collecting the varargs:

  /* optimize NOP emissions */
  if (signal_check_skip_emission (node, instance, detail))
    {
      /* nothing to do to emit this signal */
      SIGNAL_UNLOCK ();
      /* g_printerr ("omitting emission of \"%s\"\n", node->name); */
      return;
    }

Created attachment 182259 [details] [review]
Make sure signals with VARIANT params always collect their arguments

Bug #643624.

I would think that this same check should also be done for G_TYPE_OBJECT for the cases of objects that inherit from GInitiallyUnowned.

GInitiallyUnowned is different in that the value collection uses g_object_ref, not ref_sink.

Special-casing variant in signal-emission like this seems like a totally broken hack to me.

from an API standpoint: the principle of least surprise dictates that we ought to expect that g_signal_emit() on a floating GVariant consumes the variant and frees it when it's done.

From the standpoint of maintaining compatibility with code we already have, things are a little bit more complicated.  For example, David's existing GDBus code would break since he is doing the unref() after the signal emit.

Only cases where you emit a floating GVariant would be affected, of course, and we could argue that this was merely "ill defined" before...  I'm not sure of the impact on code in the wild, but my gut tells me that it wouldn't be *too* substantial.

In any case, I think it would be worth the pain to get this right.  I'm surprised it wasn't already working...

Christian is right, unfortunately.

If no handlers are connected then the signal emission is skipped, including collecting the varargs from the emit call.

The problem is that we really need to sink that floating variant.

In order to do that, we need to collect the pointer.

In order to do that, we _can't_ skip the emission.

The only other option is to change the definition of what it means to skip an emission to including collecting (and discarding) the argument list.  I don't like this idea for its performance implications in the non-GVariant case.

Matthias suggests a solution here that is somewhat more elegant than scanning the argument type list: add a G_SIGNAL_MUST_COLLECT flag that disables the no-emit optimisation for that signal.

I like that option.

If we don't do it, though, the only thing we can really do is add docs telling people that they can't rely on their pointers being collected, so they better sink the values for themselves.  In that case, Ted's original patch is clearly correct.

Created attachment 182500 [details] [review]
Add G_SIGNAL_MUST_COLLECT

In some cases, signal arguments have to be collected, even if there are i
no signal handlers connected (e.g. for GVariant parameters, where collection
consumes a floating variant).

Bug #643624.

Created attachment 182501 [details] [review]
Use G_SIGNAL_MUST_COLLECT for VARIANT signals

Bug #643624.

Review of attachment 182500 [details] [review]:

::: gobject/gsignal.h
@@ +113,2 @@
  * 
  * The signal flags are used to specify a signal's behaviour, the overall

You MUST ALWAYS use the same name :-)

Review of attachment 182500 [details] [review]:

::: gobject/gsignal.c
@@ +1539,3 @@
+   */
+  for (i = 0; i < n_params; ++i)
+    g_return_val_if_fail (param_types[i] != G_TYPE_VARIANT || (signal_flags & G_SIGNAL_MUST_COLLECT), 0);

I disagree with this check.  Not everyone needs the must-collect behaviour with GVariant if they know, for example, that they will never be dealing in floating values.

Think particularly about language bindings that sink everything immediately and don't want to have to worry about this.

@@ +2944,3 @@
   /* optimize NOP emissions */
+  skip_emission = signal_check_skip_emission (node, instance, detail);
+  if (skip_emission && (node->flags & G_SIGNAL_MUST_COLLECT) == 0)

I think this is too complicated.

I'd prefer just having a separate check in the skip_emission() code (or better still -- another check in g_signal_new to avoid setting the offset, thereby preventing skipping from being a possibility).

Consider that the existing code that prevents skipping when a return value needs to be set goes through the full signal emission stuff anyway.  I don't think it's that expensive...

On the balance of it, I actually suspect that your complicating the logic here is a bigger net loss to signal emission (99.99% of which won't be using this flag at all) than is saved by skipping the actual emission.

::: gobject/gsignal.h
@@ +109,3 @@
  *  third-party code.
  * @G_SIGNAL_NO_HOOKS: No emissions hooks are supported for this signal.
+ * @G_SIGNAL_ALWAYS_COLLECT: Varargs signal emission will always collect the

The context of Matthias's comment was slightly off.  He was referring to this. :)

Created attachment 182512 [details] [review]
my changes to the patch

Here's what I had in mind, just to make sure we're all on the same page.

Two things remain: do we backport it?

-and-

We talked on IRC about adding a check that would do the following: when collecting GVariant parameters for signals that do not have the MUST_COLLECT flag and finding one of them floating, emit a very detailed warning message along the lines of:

"
You passed a floating GVariant instance to g_signal_emit() for the "frobbed" signal of the class GtkFooBar.  g_signal_emit() typically consumes floating references, but in the case of a skipped emission (due to no handlers being connected), the reference will not be collected at all.  Consider the use of the  G_SIGNAL_MUST_COLLECT flag to avoid this problem.
"

Patch looks good to me. Thanks for adding a testcase.

As for, do we backport it: I think we should

And for the warning: I think it is a good idea

Looks like we never finished fixing this bug as MUST_COLLECT is not yet used in signals using G_TYPE_VARIANT in gio yet... that's just an oversight, yes?

Yes. Attachment 182501 [details] is a patch for that; looks like it was just forgotten while discussing the main problem...

Note: MUST_COLLECT is not required for all signals with GVariant parameters.  You only need it if you intend to pass floating variants at some point.

Comment on attachment 182501 [details] [review]
Use G_SIGNAL_MUST_COLLECT for VARIANT signals

Looks good to me - at least for the GDBusProxy parts where this is needed (what the bug was originally about).

Review of attachment 182512 [details] [review]:

This was committed a while ago

Review of attachment 182501 [details] [review]:

Committed

Review of attachment 182501 [details] [review]:

Committed

Review of attachment 182501 [details] [review]:

Committed