Created attachment 257920 [details] [review]
gmain: Warn when g_source_remove() fails

Trying to remove a non-existent source should really be
a programming error, as the programmer could be trying to
use the wrong function to remove a callback, as seen when
GtkScrolledWindow tried to remove ID from another function
using g_source_remove().

See: https://bugzilla.gnome.org/show_bug.cgi?id=710666#c12

Review of attachment 257920 [details] [review]:

::: glib/gmain.c
@@ +2182,3 @@
     g_source_destroy (source);
+  else
+    g_warning ("Source ID %i was not found when attempting to remove it", tag);

g_critical() for programmer error please.

also: I think you want %u here because tags are unsigned ints.

Created attachment 257931 [details] [review]
gmain: Warn when g_source_remove() fails

Trying to remove a non-existent source should really be
a programming error, as the programmer could be trying to
use the wrong function to remove a callback, as seen when
GtkScrolledWindow tried to remove ID from another function
using g_source_remove().

See https://bugzilla.gnome.org/show_bug.cgi?id=710666#c12

I'm no longer totally happy to commit this patch based on the fact that it regresses our own testsuite.  protocol.c does this:

      g_assert (!g_source_remove (child_source));


Which now throws a critical...


I'm not sure that there is a valid case for wanting to throw an ID that you may not own against g_source_remove() to see if it fails so maybe we should document this function as always returning TRUE except in case of programmer error and fix the tests.

I do agree that people are unlikely to check the return value here and that the critical is helpful...

Created attachment 257933 [details] [review]
gmain: Warn when g_source_remove() fails

Trying to remove a non-existent source should really be
a programming error, as the programmer could be trying to
use the wrong function to remove a callback, as seen when
GtkScrolledWindow tried to remove ID from another function
using g_source_remove().

See https://bugzilla.gnome.org/show_bug.cgi?id=710666#c12

Created attachment 257934 [details] [review]
gmain: test g_source_remove() with invalid ID

Make sure we get the proper critical displayed.

Created attachment 257935 [details] [review]
tests: expect critical on failure to remove source

We've added a g_critical() on failure to remove sources, so make sure we
expect to see that (instead of failing the test due to the unexpected
message).

This got slightly non-trivial so I'd appreciate a signoff by another maintainer.

(In reply to comment #4)
> I'm not sure that there is a valid case for wanting to throw an ID that you may
> not own against g_source_remove() to see if it fails

But there's probably code out there that carelessly removes the same source more than once. Eg, it attaches a source, and then maybe the source is triggered and returns FALSE, and gets destroyed, but then later on the code that originally added the source calls g_source_remove() unconditionally.

OTOH, g_signal_handler_disconnect() warns when you pass it a bad ID, so it would make sense for g_source_remove() to do so too. And on that note, I remember seeing a bug recently somewhere where someone was trying to remove a signal handler with g_source_remove()...

It's early in the release cycle. Try it and see... we can revert it later.

Attachment 257933 [details] pushed as a919be3 - gmain: Warn when g_source_remove() fails
Attachment 257934 [details] pushed as b9de6f0 - gmain: test g_source_remove() with invalid ID
Attachment 257935 [details] pushed as f7beb90 - tests: expect critical on failure to remove source

Created attachment 257949 [details] [review]
gtester: only remove source if not already dead

Don't attempt to g_source_remove() a source for which we already returned FALSE
from the handler.

Note, the new critical caused a bit of fallout in the PyGI test suite, logged as bug 710978. The first problem is simply because we test removal of a source twice and verify the second time source.remove returns False. We simply need to silence the critical so the test suite doesn't fail.

The second failure is indeed an unnecessary call to source.remove in our MainLoop override code.

Are the docs accurate?
"Return value: For historical reasons, this function always returns %TRUE"

In addition to the critical, I'm still seeing a FALSE return, otherwise this would be an API break? Or does "It is a programmer error to attempt to remove a non-existent source." imply a FALSE return?

(In reply to comment #12)
> In addition to the critical, I'm still seeing a FALSE return, otherwise this
> would be an API break? Or does "It is a programmer error to attempt to remove a
> non-existent source." imply a FALSE return?

Indeed.  This is the usual case of "violate preconditions and we violate documented behaviour".