If the GDBusObjectManagerClient doesn't get a name owner during its lifetime,
`on_control_proxy_g_signal' will never be connected to any signal, so we
shouldn't dump any warning in that case.
Fixes https://bugzilla.gnome.org/show_bug.cgi?id=662858
Strictly speaking, neither of the two uses that aren't under the lock
*needs* to be atomic, but it seems better to be obviously correct (and
we save another 4 bytes of struct).
One of these uses is in g_dbus_connection_is_closed(), any use of which
is inherently a race condition anyway.
The other is g_dbus_connection_flush_sync, which as far as I can tell
just needs a best-effort check, to not waste effort on a connection that
has been closed for a while (but I could be wrong).
I removed the check for the closed flag altogether in
g_dbus_connection_send_message_with_reply_unlocked, because it turns out
to be redundant with one in g_dbus_connection_send_message_unlocked,
which is called immediately after.
g_dbus_connection_close_sync held the lock to check the closed flag,
which is no longer needed.
As far as I can tell, the only reason why the lock is still desirable
when setting the closed flag is so that remove_match_rule can't fail
by racing with close notification from the worker thread - but
on_worker_closed needs to hold the lock anyway, to deal with other
data structures, so there's no point in trying to eliminate the
requirement to hold the lock.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
Also, a few that don't need to be.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
This isn't strictly necessary, because in every location where it's
checked, if the reading thread misses an update from another thread,
it's indistinguishable from the reading thread having been scheduled
before the writing thread, which is an unavoidable race condition that
callers need to cope with anyway. On the other hand, merging exit_on_close
into atomic_flags gives the least astonishing semantics to library users
and saves 4 bytes of struct, and if you're accessing exit-on-close often
enough for it to be a performance concern, you're probably doing it wrong.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
The thread shared between all GDBusWorker instances was variously called
the "worker thread" or "message handler thread", which I mostly changed to
"the GDBusWorker thread" to avoid ambiguity.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
As part of the deserialisation process of a zero-length array in the
DBus wire format, parse_value_from_blob() recursively calls itself with
the expectation of failing (as can be seen by the assert immediately
following).
It passes &local_error to this always-failing call and then fails to
free it (indeed, to use it at all). The result is that the GError is
leaked.
Fix it by passing in NULL instead, so that the GError is never created
in the first place.
https://bugzilla.gnome.org/show_bug.cgi?id=662411
The only exceptions are those of the trivial getters/setters that don't
already need the initialization check for its secondary role as a memory
barrier (this is consistent with GSocket, where trivial getters/setters
don't check):
* g_dbus_connection_set_exit_on_close
* g_dbus_connection_get_exit_on_close
* g_dbus_connection_is_closed
g_dbus_connection_set_exit_on_close needs to be safe for
use before initialization anyway, so it can be set at construct-time.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661689
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
Also document which fields require such a check in order to have correct
threading semantics.
This usage doesn't matches the GInitable documentation, which suggests
use of a GError - but using an uninitialized GDBusConnection is
programming error, and not usefully recoverable. (The GInitable
documentation may have been a mistake - GNOME#662208.) Also, not all of
the places where we need it can raise a GError.
The check serves a dual purpose: it turns a non-deterministic crash into
a deterministic critical warning, and is also a memory barrier for
thread-safety. All of these functions dereference or return fields that
are meant to be protected by FLAG_INITIALIZED, so they could crash or
return an undefined value to their caller without this, if called from a
thread that isn't the one that called initable_init() (although I can't
think of any way to do that without encountering a memory barrier,
undefined behaviour, or a race condition that leads to undefined
behaviour if the non-initializing thread wins the race).
One exception is that initable_init() itself makes a synchronous call.
We deal with that by passing new internal flags up the call stack, to
reassure g_dbus_connection_send_message_unlocked() that it can go ahead.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661689
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
The comment implied that even failed initialization would set
is_initialized = TRUE, but this wasn't the case - failed initialization
would only set initialization_error, and it was necessary to check both.
It turns out the documented semantics are nicer than the implemented
semantics, since this lets us use atomic operations, which are also
memory barriers, to avoid needing separate memory barriers or locks
for initialization_error (and other members that are read-only after
construction).
I expect to need more than one atomically-accessed flag to fix thread
safety, so instead of a minimal implementation I've turned is_initialized
into a flags word.
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661689
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661992
Reviewed-by: David Zeuthen <davidz@redhat.com>
We didn't previously test anything except the implicit default of TRUE.
Now we test implicit TRUE, explicit TRUE, explicit FALSE, and
disconnecting at the local end (which regressed while fixing Bug #651268).
Also avoid some questionable use of a main context, which fell foul of
Bug #658999 and caused this test to be disabled in master.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=662100
Bug-NB: NB#287088
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
This was a regression caused by my previous work on GDBusWorker thread-safety
(Bug #651268). The symptom is that if you disconnect a GDBusConnection
locally, the default implementation of GDBusConnection::closed
terminates your process, even though it shouldn't do that for
locally-closed connections; this is because GDBusWorker didn't think a
cancelled read was a local close.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=662100
Bug-NB: NB#287088
Signed-off-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: David Zeuthen <davidz@redhat.com>
GDBusConnection sets the closed flag in the worker thread, then adds an
idle callback (which refs the Connection) to signal this in the main
thread. The tests session_bus_down doesn't spin the mainloop, so the
"closed" signal will always fire if iterating the mainloop later (and
drops the ref when doing so). But _is_closed can return TRUE even before
signalling this, in which case the "closed" signal isn't fired and the
ref isn't dropped, causing the test to fail.
Instead simply always wait for the closed signal, which is a good thing
to check anyway and ensures the ref is closed.
Bug: https://bugzilla.gnome.org/show_bug.cgi?id=661896
Reviewed-by: Matthias Clasen <mclasen@redhat.com>
This was used as an optimisation for the macro hackery that used to live
in gthread.h. If a particular library or program knew that it could
rely on thread support being enabled, it would allow for static
evaluation of conditionals in some of those macros.
Since the macros are dead and thread support is now always-on, we can
get rid of this bit of legacy.
Add functions for manipulating the environment under which a
GAppLaunchContext will launch its children, to avoid thread-related
bugs with using setenv() directly.
FIXME: win32 side isn't implemented yet
https://bugzilla.gnome.org/show_bug.cgi?id=659326
With search gaining traction as being the preferred way to locate
applications, the existing .desktop file fields meant for browsing
often produce insufficient results.
gnome-control-center introduced a custom X-GNOME-Keywords field for
that purpose, which we plan to support in gnome-shell as well.
https://bugzilla.gnome.org/show_bug.cgi?id=661763
And remove the 'joinable' argument from g_thread_new() and
g_thread_new_full().
Change the wording in the docs. Clarify expectations for
(deprecated) g_thread_create().
It is possible for _g_io_module_get_default() to be called recursively
(eg, if a module of one type is loaded that tries to look up gsettings
from its init() method and ends up causing the gsettings module to be
loaded). So use a recursive mutex.
If the connection to the bus is lost while a method call is ongoing,
the method call does not get cancelled. Instead it just sits around
until it times out.
This is visible here on XO laptops when stopping the display manager
during shutdown. imsettings starts sending a sync message to give up
its bus name (via g_bus_unown_name()), then systemd terminates the
session bus at approximately the same time. imsettings then hangs for
about 20 seconds before timing out the message.
http://lists.freedesktop.org/archives/dbus/2011-September/014717.html
imsettings behaviour could be improved as described in that thread,
but I think this is a glib bug. I've also come up with the attached
patch which fixes it.
Credits for the bug-fix goes to Daniel Drake <dsd@laptop.org>. The test
case was written by David Zeuthen <zeuthen@gmail.com>.
https://bugzilla.gnome.org/show_bug.cgi?id=660637
Signed-off-by: David Zeuthen <davidz@redhat.com>
Add g_main_context_ref_thread_default(), which always returns a
reffed GMainContext, rather than sometimes returning a (non-reffed)
GMainContext, and sometimes returning NULL. This simplifies the
bookkeeping in any code that needs to keep a reference to the
thread-default context for a while.
https://bugzilla.gnome.org/show_bug.cgi?id=660994
