The use case for exposing this field is GTK wanting reproducible
encoding output across different OSes.
I decided to expose the OS as an integer because zlib uses an int
in its header and does not make its OS codes available as a custom
type in its API.
I also decided to limit it to values between 0 and 255 because zlib
encodes the OS as a single byte.
Test included.
Fixes: #3663
Like the GWeakRef's in GObject, there is a global lock that is consulted
whenever g_main_context_unref() or g_source_destroy() or
g_source_unref() is called to retrieve a reference to the associated
GMainContext.
There are a number of actual races that are fixed by this change.
1. Racing GSource destruction with g_main_context_unref() is solved
by holding the global source_weak_locations lock while setting
source->context = NULL and while g_source_destroy() attempts to
retrieve source->context;
2. Same race as 1. but inside g_source_unref()
3. Theoretical race of double freeing the contents of
context->pending_dispatches if both g_source_destroy() and
g_main_context_unref() both free resources inside g_main_context_unref().
A couple of implementation notes:
1. Unlocking source_weak_locations too early in g_main_context_unref()
(before g_source_destroy_internal() is called) may have a race of the
G_HOOK_FLAG_ACTIVE state of the source and cause a leak of the source.
This is why source_weak_locations is also held over the calls to
g_source_destroy_internal() in g_main_context_unref(). So that
either g_main_context_unref() or g_source_destroy() (but only 1) has
the chance to free the resources associated with the GMainContext.
2. g_main_context_unref() now needs to be more of a dispose()
implementation as it can be called multiple times with losing the
last ref.
Fixes: https://gitlab.gnome.org/GNOME/glib/-/issues/803
These show up in various places in docgen-based docs, including in
results for fairly common searches (such as ‘alloc’). Whilst people
should have been scared off by these being undocumented, the red
deprecated tags make it clear these shouldn't be touched.
dispose() would previously set the "handlers" pointer to NULL. But
dispose() also calls g_signal_group_gc_handlers(), which requires this
pointer to be not NULL.
This means, dispose() could not be called multiple times. Which is a
good practice to allow, because g_object_run_dispose() and object
resurrection both requires that dispose() can be called more than once
per object.
Fix that problem, by leaving the array until finalize().
Fixes: dd43471f60 ('gobject: add GSignalGroup')
We call g_object_weak_release_all() at two places.
Once right before finalize(). At this point, the object is definitely
going to be destroyed, and the user must no longer resurrect it or
subscribe new weak notifications. In that case, we really want to
notify/release all weak notifications.
However, we also call it from g_object_real_dispose(). During dispose,
the API allows the user to resurrect an object. Granted, that is
probably not something anybody should do, but GObject makes a reasonable
attempt to support that.
A possible place to resurrect (and subscribe new weak notifications) is
when GObject calls g_object_real_dispose().
static void
g_object_real_dispose (GObject *object)
{
g_signal_handlers_destroy (object);
/* GWeakNotify and GClosure can call into user code */
g_object_weak_release_all (object);
closure_array_destroy_all (object);
}
But previously, g_object_weak_release_all() would continue iterating
until there are no more weak notifications left. So while the user can
take a strong reference and resurrect the object, their attempts to
register new weak notifications are thwarted.
Instead, when the loop in g_object_weak_release_all() starts, remember
the initial number of weak notifications, and don't release more than
that. Note that WeakRefStack preserves the order of entries, so by
maintaining the "remaining_to_notify" counter we know when to stop.
Note that this brings also an earlier behavior back, where we would call
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
This would take out the entire WeakRefStack at once and notify the weak
notifications registered at the time. But subsequent registrations would
not be released/notified yet.
It seems bad style, to use a naive realloc() +1 increment each time a new
element gets added. Instead, remember the allocation size and double the
buffer size on buffer grow. This way we get linear amortized runtime
complexity for buffer growth.
Well, WeakRefStack uses a flat array for tracking the entires. We anyway
need to search and memmove() the entries and are thus O(n) anyway. We do
that, because it allows for relatively simple code while being memory
efficient. Also, we do expect only a reasonably small number of weak
notifications in the first place.
I still think it makes sense to avoid the O(n) number of realloc() calls
on top of that. Note that we do this while holding the (per-object)
lock. It's one thing to do a linear search or a memmove(). It's another
to do a (more expensive) realloc().
Also, shrink the buffer during g_object_weak_unref() to get rid of
excess memory.
Also, note that the initial allocation only allocates space for the
first item. I think that makes sense, because I expect that many objects
will only get a single weak notification registered. So this allocation
should not yet have excess memory allocated.
Also, note that the "flexible" array member WeakRefStack.weak_refs has a
length of 1. Maybe we should use C99 flexible array members ([]) or the
pre-C99 workaround ([0]). Anyway. Previously, we would always allocate
space for that extra one tuple, but never use it. Fix that too.
Previously, at two places (in g_object_real_dispose() and shortly before
finalize()), we would call
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
This clears @quark_weak_notifies at once and then invokes all
notifications.
This means, if you were inside a notification callback and called
g_object_weak_unref() on the object for *another* weak-reference, then
an exception failed:
GLib-GObject-FATAL-CRITICAL: g_object_weak_unref_cb: couldn't find weak ref 0x401320(0x16b9fe0)
Granted, maybe inside a GWeakNotify you shouldn't call much of anything
on where_the_object_was. However, unregistering things (like calling
g_object_weak_unref()) should still reasonably work.
Instead, now remove each weak notification one by one and invoke it.
As we now invoke the callbacks in a loop, if a callee registers a new
callback, then that one gets unregistered right away too. Previously,
we would during g_object_real_dispose() only notify the notifications
that were present when the loop starts. This is similar to what happens
in closure_array_destroy_all(). This is a change in behavior, but it
will be fixed in a separate follow-up commit.
https://gitlab.gnome.org/GNOME/glib/-/issues/1002
g_object_weak_unref() would have done a fast-removal of the entry, which
messes up the order of the weak notifications.
During destruction of the object we emit the weak notifications. They
are emitted in the order in which they were registered (FIFO). Except,
when a g_object_weak_unref() messes up the order. Avoid that and
preserve the order.
Now, do a memmove(), which is O(n). But note that we already track weak
references in a flat array that requires a O(n) linear search. Thus,
g_object_weak_unref() was already O(n) and that didn't change. More
importantly, users are well advised to limit themselves to a reasonably
small number of weak notifications. And for small n, the linear search
and the memmove() is an efficient solution.
From `man 3 ctime`:
According to POSIX.1, localtime() is required to behave as though tzset(3)
was called, while localtime_r() does not have this requirement.
For portable code, tzset(3) should be called before localtime_r().
At the time when this code was added ([1]), the code and the comment was
correct. g_object_run_dispose() did not clear GWeakRef.
That was later adjusted to clear them ([2]), but at various times it was
not ensured that the GWeakRef was cleared *before* the weak notification
is emitted.
This is now fixed, and the checks for "where_the_object_was" are no
longer necessary. Drop them.
I considered to keep the checks just to be extra safe. But we need to
rely on how g_object_run_dispose() works in detail. By now there is a
test that checks GWeakRef are cleared before emitting the notifications,
so we should not accidentally mess this up and the code is no longer
needed.
[1] commit e82eb490fe ('Handle the case of g_object_run_dispose() in GBinding')
[2] commit a7262d6357 ('gobject: Cleanup weak locations data as part of dispose')
This changes behavior from commit [1] most similar to what was before.
The point of g_object_run_dispose() is to break reference cycles to
bring down an object. We don't expect the object to take new references
to keep it alive for longer. We probably also don't expect it to
register new weak references. We also don't expect the dispose() callees
to check g_weak_ref_get() for the object. In that case, this change
makes not difference.
Note that during g_object_run_dispose() the ref count does not yet go to
zero, still we clear GWeakRef. As such, GWeakRef rather tracks when
objects get disposed, instead of when the ref count really goes to zero.
That is intentional (e.g. issue [2]).
But compare to g_object_unref(), where we also clear GWeakRef *before*
calling dispose. That makes more sense, because inside dispose() (and
for example during weak notifications), we probably want to see that
g_weak_ref_get() indicates the object is already disposed. For that
reason, it seems more correct to clear out the GWeakRef before calling
dispose().
Also, the dispose() callees (e.g. the weak notifications) might refuse to
let the object die by intentionally keeping strong references around.
Not sure why they would do that, it is similar to resurrecting an object
during dispose(). But if they do, they might also want to register new
GWeakRef. In that case, we wouldn't want to unset those newly set
GWeakRef unconditionally right after.
In most cases, it shouldn't make a difference. In the case where it
does, this is the more sensible order of doing things.
[1] commit 2952cfd7a7 ('gobject: drop clearing quark_weak_locations from g_object_real_dispose()')
[2] https://gitlab.gnome.org/GNOME/glib/-/issues/2266
During object initialization, we may want to freeze the notifications,
but only do so once (and once unfreeze at the end).
Rework how that was done. We can avoid an additional GData lookup.
By now, GObjectNotifyQueue gets reallocated. So quite possibly if we
keep the queue, it is a dangling pointer.
That is error prone, but it's also unnecessary. All we need to know is
whether we bumped the freeze count and need to unfreeze. The queue
itself was not useful, because we anyway must take a lock (via
g_datalist_id_update_atomic()) to do anything with it.
Instead, use a nqueue_is_frozen boolean variable.
GSList is almost in all use cases a bad choice. It's bad for locality
and requires a heap allocation per entry.
Instead, use an array, and grow the buffer exponentially via realloc().
Now, that we use g_datalist_id_update_atomic(), it is also easy to
update the pointer. Hence, the GObjectNotifyQueue struct does not point
to an array of pspecs. Instead the entire GObjectNotifyQueue itself gets
reallocated, thus saving one heap allocation for the separate head
structure.
The members of `struct segment_command` appear to have type `uint32_t`,
so definitely need casting to the machine’s integer pointer type before
doing pointer arithmetic on them.
See https://developer.apple.com/documentation/kernel/segment_command
Tested only on macOS CI as I don’t have access to a macOS machine.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Because of the generic nature of `GError`, `g_set_error()` has to take
an `int`, but `g_file_error_from_errno()` returns a `GFileError`. The
macOS CI runner decides that’s a good reason to emit
`-Wsign-conversion`.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
This struct is only ever heap allocated, and enums are always the same
size as an int (or unsigned int), so it won’t change size.
The struct doesn’t correspond to any mmapped structure from a
typelib file.
This should fix some `-Wsign-conversion` warnings (curiously only seen
on the macOS CI runner) by using the most specific type.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Parsing scripts using g_shell_parse_argv() line by line, it's useful to
have empty comments, so one can write pragraphs etc, e.g.
# This is a comment with multiple paragraphs.
#
# It's useful to split things up like this at times.
#
# The empty comment characters makes it clear the paragraphs form a
# single comment.
