I can't reproduce it with 2.43.90, but it is problem with g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error), not with message serialization.

Created attachment 302081 [details] [review]
gdbus-serialization test: run our own session bus

Most of the GDBus regression tests don't rely on running in an
environment that has a session bus already up, but this one was
missed because it has an extra dependency (libdbus-1 headers)
which often aren't installed on minimal build servers.

---

This specific failure mode is caused by the Debian packaging, which sets DBUS_SESSION_BUS_ADDRESS="this-should-not-be-used-and-will-fail:" in debian/rules as a way to catch anything that is (implicitly) relying on already having a session bus. Debian autobuilders are unaffected, because they build in a minimal chroot that does not have libdbus-1-dev, but manual builds can be affected.

The fact that this one test relies on having a session bus does seem like something that's worth fixing upstream.

Created attachment 302084 [details] [review]
gdbus-serialization test: run our own session bus

Most of the GDBus regression tests don't rely on running in an
environment that has a session bus already up, but this one was
missed because it has an extra dependency (libdbus-1 headers)
which often aren't installed on minimal build servers.

---

I'll try that again, this time without uncommitted changes...

Review of attachment 302084 [details] [review]:

Wouldn’t it be better to change the test to use check_serialization() instead of setting up a fake session bus? Or have I misunderstood exactly what’s being tested? (Seems it’s testing GVariant-serialisation-of-ad against libdbus-serialisation-of-ad, when it could be testing the former against known-good-const-serialisation.)

From the comment in the new test:

   * Some versions of glib encoded arrays of doubles wrong. Here we send such
   * a message and check that we didn't get bumped from the connection.

It looks as though the intention of that test is that we are specifically talking to dbus-daemon, to verify that what we produce is considered to be syntactically valid by dbus-daemon.

This does seem like overkill: we could equally well have serialized the message from GVariant to D-Bus blob, then demarshalled the blob with libdbus, which is what check_serialization() does? That would mean this test doesn't have to connect to a bus at all.

(However, the perfect is the enemy of the good, and all that.)

Created attachment 302095 [details] [review]
[different fix] gdbus-serialization: use check_serialization() instead of  dbus-daemon

This test originally did not connect to the bus, which meant it was
omitted from commits like 415a8d81 that made sure none of GLib tests
rely on the presence of an existing session bus. (In particular,
Debian autobuilders don't have a session bus.)

When test_double_array() was added, environments like the Debian
autobuilders didn't catch the fact that this test relied on having a
session bus, because it is often skipped in minimal environments
due to its libdbus-1 dependency.

We don't actually need to connect to a dbus-daemon here: it's enough
to convert the message from GVariant to D-Bus serialization, and
back into an in-memory representation through libdbus. That's what
check_serialization() does, and I've verified that when I re-introduce
bug #732754 by reverting commits 627b49b and 2268628 locally, this
test still fails.

---

If people would prefer this to be fixed by not connecting to the bus at all as Philip suggested, here's that implementation. I think this is a better approach, so I'll go with this one if the GDBus maintainers are equally happy with either.

(In reply to Philip Withnall from comment #4)
> Wouldn’t it be better to change the test to use check_serialization()
> instead of setting up a fake session bus?

You're right, it would be better, and now I have.

Review of attachment 302095 [details] [review]:

::: gio/tests/gdbus-serialization.c
@@ +1068,3 @@
+      "    double: 8.000000\n"
+      "    double: 22.000000\n"
+      "    double: 0.000000\n");

Without having checked the correct serialisation format for (ad), this looks good to me, apart from the fact that it leaks @body.

I’m about to push a fix for a few other leaks in this test, so we might as well get this right.

(In reply to Philip Withnall from comment #8)
> Review of attachment 302095 [details] [review] [review]:
> 
> *snip* apart from the fact that it leaks @body.

That’s a lie. g_variant_new() returns a floating reference, and check_serialization() cunningly consumes any floating reference on @value, so this code shouldn’t leak at all.

(In reply to Philip Withnall from comment #9)
> g_variant_new() returns a floating reference, and
> check_serialization() cunningly consumes any floating reference on @value,
> so this code shouldn’t leak at all.

Yeah, I was about to say the same.

Created attachment 302456 [details] [review]
gdbus-serialization: use check_serialization() instead of  dbus-daemon

[same long commit message as Attachment #302095 [details]]

---

Rebased onto Philip's recent memory leak fixes; no real changes, it just deletes different code. I believe this is still non-leaky because check_serialization() consumes a floating reference.

Is this OK for merge? It fixes test failures when "make check" is run in an environment without a working session bus, which I think is a requirement that's worth avoiding.

Review of attachment 302456 [details] [review]:

:+1: from me.

Comment on attachment 302456 [details] [review]
gdbus-serialization: use check_serialization() instead of  dbus-daemon

Fixed in git for 2.45.2. f42d2c1b54541cf8d4e399f5c6d27d3ac40fc1a9