Previously:
1. old_val = atomic_add(&object->ref_count);
2. if (old_val == 1 && OBJECT_HAS_TOGGLE_REF (object)) { toggle_notify() }
As old_val was 1, you might think that no other thread can have a valid
reference to object. However, that's not the case. For one, GWeakRef can
be used to create another strong reference. More easily, the single
reference can be shared between multiple threads (as long as the code
takes care that the object lives long enough).
That means, another thread can easily add and drop references (including
toggle references). All between step 1 and 2.
A race here might be hard to hit, and the effect might not be obviously
bad. However, consider old_val is 1 due to a normal reference, and
another thread adds a toggle ref between step 1. and 2. Then we would
notify a toggle from 1->2, although a newly added toggle ref is expected
to always start with a normal and a toggle reference. The first toggle
notification is expected to notify about the loss of other references, not
about getting a second reference.
To handle this properly, when we increase the reference count from 1 to
2, we must do so under a lock and check for the toggle notification.
As we now correctly track the toggle behavior, we can also assert in
toggle_refs_get_notify_unlocked() that n_toggle_refs agrees with the
number of references, that is, that the user did always match
g_object_add_toggle_ref() with g_object_remove_toggle_ref().
The downside is here too, that there is now a case (when increasing the
reference count from 1 to 2) where we need to take the global lock.
That performance problem should be addresses by using per-object locks
instead of a global lock.
The previous g_object_unref() was racy. There were three places where we
decremented the ref count, but still accessed the object afterwards
(while assuming that somebody else might still hold a reference). For
example:
if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1,
&old_ref))
continue;
TRACE (GOBJECT_OBJECT_UNREF (object, G_TYPE_FROM_INSTANCE (object), old_ref));
/* if we went from 2->1 we need to notify toggle refs if any */
if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object))
{
/* The last ref being held in this case is owned by the toggle_ref */
toggle_refs_notify (object, TRUE);
}
After we decrement the reference count (and gave up our reference), we
are only allowed to access object if we know we have the only possible
reference to it. In particular, if old_ref is larger than 1, then
somebody else holds references and races against destroying object.
The object might be a dangling pointer already.
This is slightly complicated due to toggle references and clearing of
weak-locations.
For toggle references, we must take a lock on the mutex. Luckily, that
is only necessary, when the current reference count is exactly 2.
Note that we emit the TRACE() after the ref count was already decreased.
If another thread unrefs the object, inside the TRACE() we might have a
dangling pointer. That would only be fixable, by emitting the TRACE()
before the actual unref (which has its own problems). This problem
already existed previously.
The change to the test is necessary and correct. Before this patch,
g_object_unref() would call dispose() and decrement the reference count
right after.
In the test case at gobject/tests/reference.c:1108, the reference count
after dispose and decrement is 1. Then it thaws the queue notification,
which emits a property changed signal. The test then proceeds to
reference the object again and notifying the toggle reference.
Previously, the toggle reference was notified 3 times.
After this change, the property changed signal is emitted before
decreasing the reference count. Taking a reference then does not cause
an additional toggle on+off, so in total only one toggle happens.
That accounts for the change in the test. The new behavior is
correct.
The indentation level in g_object_unref() is wrong. Fix it by reformatting
the function with clang-format. That makes follow up patches easier to adhere
a consistent style.
No other changes.
Toggle refs are seldom used, and when they are, it makes mostly sense that
there is only one of them. Thus, when removing the last toggle ref, also
remove the associated data.
The library shipped by gobject-introspection.git was
`libgirepository-1.0.so`, but for some reason (accident?), it was
accompanied by `GIRepository-2.0.gir`. That’s been the case for the last
6 years.
In moving libgirepository to glib.git, we’ve bumped the version to
`libgirepository-2.0.so`, and have changed the API.
In order to avoid a collision between the new `GIRepository-2.0.gir` and
the old `GIRepository-2.0.gir`, we can either:
* Rename the basename of the library (confusing).
* Re-version the whole thing to 3.0 (would mean it’s completely out of
sync with the rest of glib.git, and would lead to build system
misery).
* Re-version only the GIR file (a bit confusing, but hopefully less
confusing).
So I’ve done the final option: glib.git now ships
`libgirepository-2.0.so` and `GIRepository-3.0.gir`. This avoids
collisions with what’s shipped by gobject-introspection.git, while
hopefully still making some sense.
We considered using version number 2.1 rather than 3.0, but decided
against it because that makes it look like it’s compatible with version
2.0, which it isn’t.
Note that none of these changes touch the
`${prefix}/lib/girepository-1.0` and `${prefix}/share/gir-1.0`
directories. The version numbers in those refer to the versions of the
GIR and typelib file formats, which have not changed.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3155
They are now installed to (e.g.)
`${prefix}/share/doc/glib-2.0/{glib,gmodule,gobject,gio}/index.html`.
We might want to drop one level of nesting out of that, but for the
moment I thought I’d keep it in so we can disambiguate by installed
major version.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3037
Previously, `-Dman=false` was the default, because the generated man
pages were shipped in the distribution tarball already, so the option
actually mostly controlled whether to *re*build them.
The generated pages are no longer shipped in the tarball (and probably
haven’t been since the port to Meson, though I haven’t checked), so it
makes sense to change the default to encourage building the man pages if
the right tooling (`rst2man`) is available.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
So they are consistent with the way we’re building man pages in other
projects, and because some people are allergic to XML.
This changes the build-time dependencies from `xsltproc` to `rst2man`,
and also takes the opportunity to change the `-Dman` Meson option from a
boolean to a feature (so you should use `-Dman-pages={enabled,disabled}`
now, rather than `-Dman={true,false}`).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3037
We used to store the search paths into a GSList but this is not
efficient for various reasons, so replace this with an array so that we
can replace return just a GStrv in the public API.
Previously:
- if the object is currently not frozen, we called
g_object_notify_queue_freeze() once. Afterwards dispatch the event
directly. This is probably the common case, and requires one
notify_lock lock.
- if the object is currently frozen, we call
g_object_notify_queue_freeze(), g_object_notify_queue_add().
g_object_notify_queue_thaw().
This required taking the notify_lock three times.
- if the object is currently not frozen and in_init, then we called
g_object_notify_queue_freeze(), g_object_notify_queue_freeze(),
g_object_notify_queue_add(). This also required to take
the lock three times. There is another thaw at the end of
object initialization.
That was because we first call g_object_notify_queue_freeze() to see
whether we are frozen. And depending on that, queue the event (and thaw
again).
Instead, g_object_notify_queue_add() can do the check and queueing in
one step. There is no need to call a freeze() to (conditionally) enqueue
a notification. Now only one lock is taken in all cases.
Also, g_object_notify_queue_freeze() and g_object_notify_queue_thaw()
both call g_datalist_id_get_data() (which also take a bit lock). As the
thaw is no longer necessary, the second lock is also saved.
Before dispatching signals (and calling out to user code), we want to
take a reference and ensure that the object stays alive.
However, a thaw may not decrease the freeze_count to zero, or there may
be no properties to notify. Avoid taking a reference in those cases.
This was done since the beginning (commit e773d7dba6 ('fixed dealing
with collection/lcopy of NULL values.'). But it's not clear, why we
would need to take a reference on the calling object.
Freeze does not emit any signals/callbacks and does not call back to the
user. It just sets up some internal state.
This doesn't require to take a reference. The caller must hold a valid
reference to being with, but if that's given, there is no need to
acquire another reference.
g_atomic_int_get() returns a signed int. While we don't expect this to be ever
negative, a negative value would also indicate a bug. Adjust the check to assert
against negative ref-count too.
Without this when compiling GLib with address sanitizer enabled, we'd
end up failing with this error:
==375535==ASan runtime does not come first in initial library list;
you should either link runtime to your application or manually preload
it with LD_PRELOAD.
Now, given that addressing the fix implies doing more radical changes,
it's just fine here to ignore ASAN to work on tools we use for building
gir files.
Generating gir and typelib files has inter-dependencies that may depend
on other elements.
For example, glib requires gobject and gdump generated files require
gmodule, so we've a cyclic dependency because gmodule requires gobject,
that requires glib.
To prevent this, let's just generate the introspection files at once in
a different meson file so that we don't have to deal with this.
As per this we could even revert commit fa37ab6d0 since gio is now
compiled before the gir files.
GLib gir requires glib-types.h that also includes gobject-visibility.h
that needs to be generated in order to be able to generate the GLib gir,
so explicitly add it to the sources.
Also, in order to link the typelib we need the definitions of the *_get_type()
functions that are defined in the gboxed.c file, so this should be part
of the gir sources or we'll have linking issues.
The code for stripping the query and fragment from file:// URIs was
wrong, as it would not properly strip a query if there was a fragment.
Fortunately, that code was actually useless, as the "stripped URI" was
passed to g_filename_from_uri() that does proper stripping itself.
So simply drop this extra unnecessary stripping logic from GLocalVFS's
get_file_for_uri() and let g_filename_from_uri() do all the work.
