g_source_set_callback() and g_source_set_callback_indirect() are both
safe to call zero or more times on attached sources. The change in
callback will take effect the next time the source is dispatched, after
the set_callback() call returns (it could block due to locking).
https://gitlab.gnome.org/GNOME/glib/issues/827
It’s been de-facto deprecated for a long time, due to emitting a
critical warning when used in a non-internal context. Make that official
in the documentation and with a deprecation annotation.
Split the implementation into an internal helper and an external
wrapper, so the two remaining internal uses don’t emit deprecation
warnings.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://gitlab.gnome.org/GNOME/glib/issues/903
For the purposes of debugging, it is quite useful for every GSource to
have a name set. Ensure that any GSource we construct inside GLib has a
name set. For GSources which are then returned to the caller, this name
can then be overridden with something even more useful by the caller.
Since this data is only used for debugging, avoid doing any allocations
for it; just use static strings.
https://gitlab.gnome.org/GNOME/glib/issues/1175
It’s good to know *which* GMainContext is used to determine the ID, and
the preconditions for calling this method.
Using wording suggested by Emmanuele Bassi <ebassi@gmail.com>.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
All those logging functions already add a newline to any message they
print, so there’s no need to add a trailing newline in the message
passed to them.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
Reviewed-by: nobody
This reverts the following commits (but keeps the other recent changes
to gmain.c):
• e4ee3079c Do not wake up main loop if change is from same thread
• 208702404 main: Create a helper function for "owner wakeup" optimization
• 0c0469b56 gmain: Signal wakeups if context has never been acquired as well
• 9ba95e25b gmain: only signal GWakeup right before or during a blocking poll
Some combination of them is causing problems with LibreOffice and/or
WebKit, and the safest thing to do at the moment is revert them all
until we work out what’s going on. The previous revert (4976e8109) was
not sufficient (it fixed WebKit, but re-broken LibreOffice).
By reverting, we gain some spurious wakeups, but avoid dropping
necessary wakeups, which is presumably what’s causing problems in the
other modules.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://bugzilla.gnome.org/show_bug.cgi?id=761102
Commit 281e3010 narrowed the race between GCancellable::cancelled and
GCancellableSource's finalize(), but did not prevent it: there was
nothing to stop cancellation from occurring after the refcount drops
to 0, but before g_source_unref_internal() bumps it back up to 1 to
run finalize().
GCancellable cannot be expected to detect that situation, because the
only way it has to detect last-unref is finalize(), but in that
situation finalize() hasn't happened yet.
Instead of detecting last-unref, relax the precondition a little
to make it detect finalization: priv is only poisoned (set to NULL)
after the finalize() function has been called, so we can assume that
GCancellable has already seen finalize() by then.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=791754
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=884654
If we don't take the lock, then we don't have the necessary
"happens before" relationships to avoid this situation:
* source->priv->ready_time was equal to ready_time until recently
* another thread has set source->priv->ready_time to a different value
* that write hasn't become visible to this thread yet
* result: we should reset the ready_time, but we don't
Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=791754
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=884654
Rather than unreffing them _after_ finalising the GSource and freeing
its struct. This fixes the case where the GSourceCallbackFuncs data
contains a pointer to the GSource, and the unref() function operates on
that pointer, e.g. by calling g_source_destroy(). This happens when
using g_source_set_dummy_callback() on a GSource, as the generated
GClosure needs to destroy the GSource when it is invalidated, which
could happen (at latest) when the GSourceCallbackFuncs.unref() function
is called during finalisation of the GSource.
By moving the GSourceCallbackFuncs.unref() invocation higher up in
g_source_unref_internal(), it becomes re-entrancy-safe for GSource
methods.
https://bugzilla.gnome.org/show_bug.cgi?id=692034
Otherwise there is a race in finalising the GSourceCallback if one
thread is finishing off a g_main_dispatch() while another thread is
destroying the GSource which owns the GSourceCallback.
A helgrind log:
==21707== Possible data race during write of size 4 at 0x54EACB0 by
thread #12
==21707== Locks held: none
==21707== at 0x4ECC174: g_source_callback_unref (gmain.c:1528)
==21707== by 0x4ECD953: g_main_dispatch (gmain.c:3081)
==21707== by 0x4ECE667: g_main_context_dispatch (gmain.c:3673)
==21707== by 0x4ECE859: g_main_context_iterate (gmain.c:3744)
==21707== by 0x4ECEC7F: g_main_loop_run (gmain.c:3938)
==21707== by 0x41C197: some_thread (some-code.c:224)
==21707==
==21707== This conflicts with a previous write of size 4 by thread #5
==21707== Locks held: 1, at address 0x54CF320
==21707== at 0x4ECC174: g_source_callback_unref (gmain.c:1528)
==21707== by 0x4ECB86F: g_source_destroy_internal (gmain.c:1178)
==21707== by 0x4ECB9D4: g_source_destroy (gmain.c:1227)
==21707== by 0x41CF09: some_other_thread (some-other-code.c:410)
https://bugzilla.gnome.org/show_bug.cgi?id=737677
It's a very common pattern to see code that looks like this in
dispose() or finalize() implementations:
if (priv->source_id > 0)
{
g_source_remove (priv->source_id);
priv->source_id = 0;
}
This API allows to accomplish the same goal with a single line:
g_clear_handle_id (&priv->source_id, (GClearHandleFunc) g_source_remove);
Thanks to Emmanuele Bassi <ebassi@gnome.org> for making the patch
generic.
https://bugzilla.gnome.org/show_bug.cgi?id=788489
The warnings issued when dealing with waitpid() raising ECHILD are
somewhat misleading: there are lots of reasons why waitpid() might
fail in this way, and we can't tell which one has happened.
In particular, passing a non-child or a non-pid, waiting for the same
pid elsewhere, or creating a duplicate watch for the same pid would
all fail in the same way.
Consolidate the restrictions into one place, and change all the other
places they were (or should have been!) mentioned to point to
that one place.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Reviewed-by: Philip Withnall <withnall@endlessm.com>
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=723743
Prevent the situation where errno is set by function A, then function B
is called (which is typically _(), but could be anything else) and it
overwrites errno, then errno is checked by the caller.
errno is a horrific API, and we need to be careful to save its value as
soon as a function call (which might set it) returns. i.e. Follow the
pattern:
int errsv, ret;
ret = some_call_which_might_set_errno ();
errsv = errno;
if (ret < 0)
puts (strerror (errsv));
This patch implements that pattern throughout GLib. There might be a few
places in the test code which still use errno directly. They should be
ported as necessary. It doesn’t modify all the call sites like this:
if (some_call_which_might_set_errno () && errno == ESOMETHING)
since the refactoring involved is probably more harmful than beneficial
there. It does, however, refactor other call sites regardless of whether
they were originally buggy.
https://bugzilla.gnome.org/show_bug.cgi?id=785577
All glib/*.{c,h} files have been processed, as well as gtester-report.
12 of those files are not licensed under LGPL:
gbsearcharray.h
gconstructor.h
glibintl.h
gmirroringtable.h
gscripttable.h
gtranslit-data.h
gunibreak.h
gunichartables.h
gunicomp.h
gunidecomp.h
valgrind.h
win_iconv.c
Some of them are generated files, some are licensed under a BSD-style
license and win_iconv.c is in the public domain.
Sub-directories inside glib/:
deprecated/: processed in a previous commit
glib-mirroring-tab/: already LGPLv2.1+
gnulib/: not modified, the code is copied from gnulib
libcharset/: a copy
pcre/: a copy
tests/: processed in a previous commit
https://bugzilla.gnome.org/show_bug.cgi?id=776504
Temporarily increase the ref count of a GSource to 1 while calling its
finalize() callback, so that the finalize() implementation can call
GSource methods (like g_source_set_ready_time()) without causing
critical warnings. It’s safe to call those methods at this point, as the
source has been destroyed, but nothing has been freed.
This is an indirect way of fixing a race between GCancellable and
GCancellableSource, whereby the GCancellable::cancelled callback for the
GCancellableSource is not disconnected until the GCancellableSource’s
finalize() function is called. Previously, this meant there was a window
in which the GCancellableSource’s ref count was 0, but the ::cancelled
callback was still connected, and could legitimately be called as a
result of another thread calling g_cancellable_cancel() on the
GCancellable. The callback calls g_source_set_ready_time() on the
GSource, and there’s no thread-safe way of checking whether the GSource
has been destroyed. Instead, we have to change GSource so its ref count
is only decremented to 0 inside the locked section in
g_source_unref_internal() *after* the finalize() function has been
called, and hence after the GCancellable::cancelled callback has been
disconnected. The use of g_cancellable_disconnect() ensures that the
callback disconnection is thread safe.
Signed-off-by: Philip Withnall <withnall@endlessm.com>
https://bugzilla.gnome.org/show_bug.cgi?id=781601
Since commit e4ee307 ("Do not wake up main loop if change is from same
thread", bug 761102), GMainContext uses context->owner to decide if the
event loop is being run in the current thread. However, what really
matters is the phase in the prepare/query/poll/check/dispatch sequence.
Wakeups are only needed between the end of prepare and the end of poll,
and then only if prepare found that no sources were ready.
There is no need to take threads into account, because prepare, check
and all callers of conditional_wakeup all look at the new need_wakeup
flag inside LOCK_CONTEXT/UNLOCK_CONTEXT.
With this change, g_main_context_is_owner and g_main_context_wait are
the only functions for which acquire/release matters, just like before
commit e4ee307.
Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
The original patch really should have introduced a helper - among
other things it deserves a code comment. We're likely to make
further changes too, so it's obviously better to only do it in one
place.
See: https://bugzilla.gnome.org/show_bug.cgi?id=761102
If we have an input parameter (or return value) we need to use (nullable).
However, if it is an (inout) or (out) parameter, (optional) is sufficient.
It looks like (nullable) could be used for everything according to the
Annotation documentation, but (optional) is more specific.
revents is set in the same function after some lines. This check was
using revents from previous loop. This had the problem of causing two
poll execution for every changes to poll records.
Note that is not possible to move the code after revents is updated
as probably poll_changed is TRUE causing the function to exit.
Adapted from a patch by Frediano Ziglio,
https://bugzilla.gnome.org/show_bug.cgi?id=761102
This reduce the frequency the loop is waked up adding and removing
file descriptors or timeouts.
Considering that to support recursion events are removed from list and
added again this reduce iteration number a lot.
Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
https://bugzilla.gnome.org/show_bug.cgi?id=761102
Expand the set of available probes, and add a few more output parameters
to some of the existing ones to make them more useful. I do not know if
this breaks any existing stability guarantees for GLib’s SystemTap
tapset, as it is effectively just adding some more local variables in
the user’s probe.
https://bugzilla.gnome.org/show_bug.cgi?id=759813
Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.
Secondly, add various (not nullable) annotations as needed by the new
default in gobject-introspection of marking gpointers as (nullable). See
https://bugzilla.gnome.org/show_bug.cgi?id=729660.
This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.
https://bugzilla.gnome.org/show_bug.cgi?id=719966
It’s very common to see code where a timeout is scheduled using
g_timeout_add(), yet the owning object could be destroyed shortly
afterwards, before the timeout is fired, leading to use-after-free.
Try and prevent this happening with new code by documenting best
practices for memory management of user data for GSource callbacks.
https://bugzilla.gnome.org/show_bug.cgi?id=741779
Our signal handler calls write() on a pipe or an eventfd in order to
deliver the notification. It's unlikely, but this could fail, setting
errno. We even check the case that it fails with EINTR.
If it does set errno, then it has potentially blown away the value or
errno that the preempted code cared about (ie: if the signal arrived
shortly after a system call but before errno was checked).
Wrap the handler with code to save errno.
https://bugzilla.gnome.org/show_bug.cgi?id=741791
i.e. That calling g_timeout_add() from a thread other than the main one
probably doesn’t do what you want. Same for g_idle_add() and the
*_full() variants.
https://bugzilla.gnome.org/show_bug.cgi?id=741779
We intend to keep the list of poll records sorted by (integer) file
descriptor, but due to a typo we are actually keeping it sorted by
pointer address of the GPollFD.
Fix that.
https://bugzilla.gnome.org/show_bug.cgi?id=11059
If a given fd is being polled by multiple sources, we used to pass it
multiple times to g_poll(), which is technically illegal (and not
supported by the select()-based fallback implementation of poll() in
gpoll.c), and also made it more likely that we'd exceed the maximum
number of pollfds.
Fix it to merge together "duplicate" GPollFDs. The easiest way to do
this involves re-sorting context->poll_records into fd order rather
than priority order. This means we now have to walk the entire pollrec
list for every g_main_context_query() and g_main_context_poll(),
rather than only walking the list up to the current max_priority.
However, this will only have a noticeable effect if you have tons of
GPollFDs, and we're already too slow in that case anyway because of
other O(n) operations that happen too often. So this shouldn't change
much (and the new poll API will eventually let us be cleverer).
Remove some win32-specific code which did the same thing (but was
O(n^2)).
https://bugzilla.gnome.org/show_bug.cgi?id=11059
If a GSource is created, *not* attached to a GMainContext, and then has
child sources added, dropping the last reference to the parent GSource
will leak its references to its child sources. Currently, child sources
are only unreffed when g_source_destroy() is called on the parent.
https://bugzilla.gnome.org/show_bug.cgi?id=737338
